[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dc64c60c-aecf-f24f-f423-03499b55e19e@linaro.org>
Date: Thu, 10 Dec 2020 16:12:18 -0300
From: Adhemerval Zanella <adhemerval.zanella@...aro.org>
To: Szabolcs Nagy <szabolcs.nagy@....com>, libc-alpha@...rceware.org
Cc: Mark Rutland <mark.rutland@....com>, kernel-hardening@...ts.openwall.com,
Catalin Marinas <catalin.marinas@....com>, linux-kernel@...r.kernel.org,
Jeremy Linton <jeremy.linton@....com>, Mark Brown <broonie@...nel.org>,
Topi Miettinen <toiwoton@...il.com>, Will Deacon <will@...nel.org>,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/2] aarch64: Use mmap to add PROT_BTI instead of
mprotect [BZ #26831]
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@...aro.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.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
