[v3,2/2] aarch64: Use mmap to add PROT_BTI instead of mprotect [BZ #26831]
Commit Message
Re-mmap executable segments if possible instead of using mprotect
to add PROT_BTI. This allows using BTI protection with security
policies that prevent mprotect with PROT_EXEC.
If the fd of the ELF module is not available because it was kernel
mapped then mprotect is used and failures are ignored. To protect
the main executable even when mprotect is filtered the linux kernel
will have to be changed to add PROT_BTI to it.
The delayed failure reporting is mainly needed because currently
_dl_process_gnu_properties does not propagate failures such that
the required cleanups happen. Using the link_map_machine struct for
error propagation is not ideal, but this seemed to be the least
intrusive solution.
Fixes bug 26831.
---
v3:
- split the last patch in two.
- pushed to nsz/btifix-v3 branch.
sysdeps/aarch64/dl-bti.c | 54 ++++++++++++++++++++++++++-------------
sysdeps/aarch64/dl-prop.h | 8 +++++-
sysdeps/aarch64/linkmap.h | 2 +-
3 files changed, 44 insertions(+), 20 deletions(-)
Comments
On 02/12/2020 05:55, Szabolcs Nagy via Libc-alpha wrote:
> Re-mmap executable segments if possible instead of using mprotect
> to add PROT_BTI. This allows using BTI protection with security
> policies that prevent mprotect with PROT_EXEC.
>
> If the fd of the ELF module is not available because it was kernel
> mapped then mprotect is used and failures are ignored. To protect
> the main executable even when mprotect is filtered the linux kernel
> will have to be changed to add PROT_BTI to it.
>
> The delayed failure reporting is mainly needed because currently
> _dl_process_gnu_properties does not propagate failures such that
> the required cleanups happen. Using the link_map_machine struct for
> error propagation is not ideal, but this seemed to be the least
> intrusive solution.
>
> Fixes bug 26831.
LGTM, thanks.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> v3:
> - split the last patch in two.
> - pushed to nsz/btifix-v3 branch.
>
> sysdeps/aarch64/dl-bti.c | 54 ++++++++++++++++++++++++++-------------
> sysdeps/aarch64/dl-prop.h | 8 +++++-
> sysdeps/aarch64/linkmap.h | 2 +-
> 3 files changed, 44 insertions(+), 20 deletions(-)
>
> diff --git a/sysdeps/aarch64/dl-bti.c b/sysdeps/aarch64/dl-bti.c
> index 67d63c8a73..ff26c98ccf 100644
> --- a/sysdeps/aarch64/dl-bti.c
> +++ b/sysdeps/aarch64/dl-bti.c
> @@ -19,9 +19,17 @@
> #include <errno.h>
> #include <libintl.h>
> #include <ldsodefs.h>
> +#include <sys/mman.h>
>
> -static void
> -enable_bti (struct link_map *map, const char *program)
> +/* See elf/dl-load.h. */
> +#ifndef MAP_COPY
> +# define MAP_COPY (MAP_PRIVATE | MAP_DENYWRITE)
> +#endif
> +
> +/* Enable BTI protection for MAP. */
> +
> +void
> +_dl_bti_protect (struct link_map *map, int fd)
> {
> const size_t pagesz = GLRO(dl_pagesize);
> const ElfW(Phdr) *phdr;
> @@ -41,19 +49,31 @@ enable_bti (struct link_map *map, const char *program)
> if (phdr->p_flags & PF_W)
> prot |= PROT_WRITE;
>
> - if (__mprotect (start, len, prot) < 0)
> - {
> - if (program)
> - _dl_fatal_printf ("%s: mprotect failed to turn on BTI\n",
> - map->l_name);
> - else
> - _dl_signal_error (errno, map->l_name, "dlopen",
> - N_("mprotect failed to turn on BTI"));
> - }
> + if (fd == -1)
> + /* Ignore failures for kernel mapped binaries. */
> + __mprotect (start, len, prot);
> + else
> + map->l_mach.bti_fail = __mmap (start, len, prot,
> + MAP_FIXED|MAP_COPY|MAP_FILE,
> + fd, off) == MAP_FAILED;
> }
> }
>
OK. I am not very found of ignore the mprotect failure here, but it
has been discussed in multiple threads that we need kernel support
to proper handle it.
> -/* Enable BTI for MAP and its dependencies. */
> +
> +static void
> +bti_failed (struct link_map *l, const char *program)
> +{
> + if (program)
No implicit checks.
> + _dl_fatal_printf ("%s: %s: failed to turn on BTI protection\n",
> + program, l->l_name);
> + else
> + /* Note: the errno value is not available any more. */
> + _dl_signal_error (0, l->l_name, "dlopen",
> + N_("failed to turn on BTI protection"));
> +}
> +
> +
> +/* Report BTI protection failures for MAP and its dependencies. */
>
Ok.
> void
> _dl_bti_check (struct link_map *map, const char *program)
> @@ -61,16 +81,14 @@ _dl_bti_check (struct link_map *map, const char *program)
> if (!GLRO(dl_aarch64_cpu_features).bti)
> return;
>
> - if (map->l_mach.bti)
> - enable_bti (map, program);
> + if (map->l_mach.bti_fail)
> + bti_failed (map, program);
>
> unsigned int i = map->l_searchlist.r_nlist;
> while (i-- > 0)
> {
> struct link_map *l = map->l_initfini[i];
> - if (l->l_init_called)
> - continue;
> - if (l->l_mach.bti)
> - enable_bti (l, program);
> + if (l->l_mach.bti_fail)
> + bti_failed (l, program);
> }
> }
Ok.
> diff --git a/sysdeps/aarch64/dl-prop.h b/sysdeps/aarch64/dl-prop.h
> index 2016d1472e..e926e54984 100644
> --- a/sysdeps/aarch64/dl-prop.h
> +++ b/sysdeps/aarch64/dl-prop.h
> @@ -19,6 +19,8 @@
> #ifndef _DL_PROP_H
> #define _DL_PROP_H
>
> +extern void _dl_bti_protect (struct link_map *, int) attribute_hidden;
> +
> extern void _dl_bti_check (struct link_map *, const char *)
> attribute_hidden;
>
> @@ -43,6 +45,10 @@ static inline int
> _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
> uint32_t datasz, void *data)
> {
> + if (!GLRO(dl_aarch64_cpu_features).bti)
> + /* Skip note processing. */
> + return 0;
> +
> if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND)
> {
> /* Stop if the property note is ill-formed. */
> @@ -51,7 +57,7 @@ _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
>
> unsigned int feature_1 = *(unsigned int *) data;
> if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
> - l->l_mach.bti = true;
> + _dl_bti_protect (l, fd);
>
> /* Stop if we processed the property note. */
> return 0;
Ok.
> diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
> index 847a03ace2..b3f7663b07 100644
> --- a/sysdeps/aarch64/linkmap.h
> +++ b/sysdeps/aarch64/linkmap.h
> @@ -22,5 +22,5 @@ struct link_map_machine
> {
> ElfW(Addr) plt; /* Address of .plt */
> void *tlsdesc_table; /* Address of TLS descriptor hash table. */
> - bool bti; /* Branch Target Identification is enabled. */
> + bool bti_fail; /* Failed to enable Branch Target Identification. */
> };
>
Ok.
@@ -19,9 +19,17 @@
#include <errno.h>
#include <libintl.h>
#include <ldsodefs.h>
+#include <sys/mman.h>
-static void
-enable_bti (struct link_map *map, const char *program)
+/* See elf/dl-load.h. */
+#ifndef MAP_COPY
+# define MAP_COPY (MAP_PRIVATE | MAP_DENYWRITE)
+#endif
+
+/* Enable BTI protection for MAP. */
+
+void
+_dl_bti_protect (struct link_map *map, int fd)
{
const size_t pagesz = GLRO(dl_pagesize);
const ElfW(Phdr) *phdr;
@@ -41,19 +49,31 @@ enable_bti (struct link_map *map, const char *program)
if (phdr->p_flags & PF_W)
prot |= PROT_WRITE;
- if (__mprotect (start, len, prot) < 0)
- {
- if (program)
- _dl_fatal_printf ("%s: mprotect failed to turn on BTI\n",
- map->l_name);
- else
- _dl_signal_error (errno, map->l_name, "dlopen",
- N_("mprotect failed to turn on BTI"));
- }
+ if (fd == -1)
+ /* Ignore failures for kernel mapped binaries. */
+ __mprotect (start, len, prot);
+ else
+ map->l_mach.bti_fail = __mmap (start, len, prot,
+ MAP_FIXED|MAP_COPY|MAP_FILE,
+ fd, off) == MAP_FAILED;
}
}
-/* Enable BTI for MAP and its dependencies. */
+
+static void
+bti_failed (struct link_map *l, const char *program)
+{
+ if (program)
+ _dl_fatal_printf ("%s: %s: failed to turn on BTI protection\n",
+ program, l->l_name);
+ else
+ /* Note: the errno value is not available any more. */
+ _dl_signal_error (0, l->l_name, "dlopen",
+ N_("failed to turn on BTI protection"));
+}
+
+
+/* Report BTI protection failures for MAP and its dependencies. */
void
_dl_bti_check (struct link_map *map, const char *program)
@@ -61,16 +81,14 @@ _dl_bti_check (struct link_map *map, const char *program)
if (!GLRO(dl_aarch64_cpu_features).bti)
return;
- if (map->l_mach.bti)
- enable_bti (map, program);
+ if (map->l_mach.bti_fail)
+ bti_failed (map, program);
unsigned int i = map->l_searchlist.r_nlist;
while (i-- > 0)
{
struct link_map *l = map->l_initfini[i];
- if (l->l_init_called)
- continue;
- if (l->l_mach.bti)
- enable_bti (l, program);
+ if (l->l_mach.bti_fail)
+ bti_failed (l, program);
}
}
@@ -19,6 +19,8 @@
#ifndef _DL_PROP_H
#define _DL_PROP_H
+extern void _dl_bti_protect (struct link_map *, int) attribute_hidden;
+
extern void _dl_bti_check (struct link_map *, const char *)
attribute_hidden;
@@ -43,6 +45,10 @@ static inline int
_dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
uint32_t datasz, void *data)
{
+ if (!GLRO(dl_aarch64_cpu_features).bti)
+ /* Skip note processing. */
+ return 0;
+
if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND)
{
/* Stop if the property note is ill-formed. */
@@ -51,7 +57,7 @@ _dl_process_gnu_property (struct link_map *l, int fd, uint32_t type,
unsigned int feature_1 = *(unsigned int *) data;
if (feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)
- l->l_mach.bti = true;
+ _dl_bti_protect (l, fd);
/* Stop if we processed the property note. */
return 0;
@@ -22,5 +22,5 @@ struct link_map_machine
{
ElfW(Addr) plt; /* Address of .plt */
void *tlsdesc_table; /* Address of TLS descriptor hash table. */
- bool bti; /* Branch Target Identification is enabled. */
+ bool bti_fail; /* Failed to enable Branch Target Identification. */
};