From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Szabolcs Nagy <szabolcs.nagy@arm.com>, libc-alpha@sourceware.org
Cc: Mark Rutland <mark.rutland@arm.com>,
kernel-hardening@lists.openwall.com,
Catalin Marinas <catalin.marinas@arm.com>,
linux-kernel@vger.kernel.org,
Jeremy Linton <jeremy.linton@arm.com>,
Mark Brown <broonie@kernel.org>,
Topi Miettinen <toiwoton@gmail.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3 2/2] aarch64: Use mmap to add PROT_BTI instead of mprotect [BZ #26831]
Date: Thu, 10 Dec 2020 16:12:18 -0300 [thread overview]
Message-ID: <dc64c60c-aecf-f24f-f423-03499b55e19e@linaro.org> (raw)
In-Reply-To: <7e800caa0485fec67b21ebab1e27bb23b2f0d971.1606898457.git.szabolcs.nagy@arm.com>
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.
next prev parent reply other threads:[~2020-12-10 19:13 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-27 13:19 [PATCH v2 0/6] aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) [BZ #26831] Szabolcs Nagy
2020-11-27 13:19 ` [PATCH v2 1/6] aarch64: Fix missing BTI protection from dependencies [BZ #26926] Szabolcs Nagy
2020-12-10 17:51 ` Adhemerval Zanella
2020-12-11 15:33 ` Szabolcs Nagy
2020-11-27 13:20 ` [PATCH v2 2/6] elf: lose is closely tied to _dl_map_object_from_fd Szabolcs Nagy
2020-11-27 13:20 ` [PATCH v2 3/6] elf: Fix failure handling in _dl_map_object_from_fd Szabolcs Nagy
2020-12-10 18:25 ` Adhemerval Zanella
2020-12-11 9:32 ` Szabolcs Nagy
2020-11-27 13:20 ` [PATCH v2 4/6] elf: Move note processing after l_phdr is updated Szabolcs Nagy
2020-11-27 13:21 ` [PATCH v2 5/6] elf: Pass the fd to note processing Szabolcs Nagy
2020-12-10 18:35 ` Adhemerval Zanella
2020-11-27 13:21 ` [PATCH v2 6/6] aarch64: Use mmap to add PROT_BTI instead of mprotect [BZ #26831] Szabolcs Nagy
2020-12-02 8:55 ` [PATCH v3 1/2] aarch64: align address for BTI protection [BZ #26988] Szabolcs Nagy
2020-12-10 18:49 ` Adhemerval Zanella
2020-12-02 8:55 ` [PATCH v3 2/2] aarch64: Use mmap to add PROT_BTI instead of mprotect [BZ #26831] Szabolcs Nagy
2020-12-10 19:12 ` Adhemerval Zanella [this message]
2020-11-30 15:56 ` [PATCH v2 0/6] aarch64: avoid mprotect(PROT_BTI|PROT_EXEC) " Szabolcs Nagy
2020-12-03 17:30 ` Catalin Marinas
2020-12-07 20:03 ` Szabolcs Nagy
2020-12-11 17:46 ` Catalin Marinas
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc64c60c-aecf-f24f-f423-03499b55e19e@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=jeremy.linton@arm.com \
--cc=kernel-hardening@lists.openwall.com \
--cc=libc-alpha@sourceware.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=szabolcs.nagy@arm.com \
--cc=toiwoton@gmail.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).