From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Szabolcs Nagy <szabolcs.nagy@arm.com>, libc-alpha@sourceware.org
Cc: Sudakshina Das <Sudi.Das@arm.com>
Subject: Re: [PATCH 09/12] aarch64: support BTI enabled binaries
Date: Thu, 7 May 2020 18:07:28 -0300 [thread overview]
Message-ID: <6ceb0ca3-2d71-907c-0a91-0b28d20b94fa@linaro.org> (raw)
In-Reply-To: <20200430174407.GE29015@arm.com>
On 30/04/2020 14:44, Szabolcs Nagy wrote:
> From 45c6bce5a691ecec9bba52785bd1f3a4cbc76fd4 Mon Sep 17 00:00:00 2001
> From: Sudakshina Das <sudi.das@arm.com>
> Date: Tue, 17 Mar 2020 15:54:12 +0000
> Subject: [PATCH 09/12] aarch64: support BTI enabled binaries
>
> Binaries can opt-in to using BTI via an ELF property marking.
> The dynamic linker has to then mprotect the executable segments
> with PROT_BTI. In case of static linked executables or in case
> of the dynamic linker itself, PROT_BTI protection is done by the
> operating system.
>
> On AArch64 glibc uses PT_GNU_PROPERTY instead of PT_NOTE to check
> the properties of a binary because PT_NOTE can be unreliable with
> old linkers.
>
> Co-authored-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
> ---
> elf/dl-load.c | 2 +
> elf/rtld.c | 2 +
> sysdeps/aarch64/Makefile | 4 +
> sysdeps/aarch64/dl-bti.c | 54 ++++++
> sysdeps/aarch64/dl-prop.h | 170 ++++++++++++++++++
> sysdeps/aarch64/linkmap.h | 1 +
> sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h | 1 +
> sysdeps/unix/sysv/linux/aarch64/bits/mman.h | 31 ++++
> .../unix/sysv/linux/aarch64/cpu-features.c | 3 +
> .../unix/sysv/linux/aarch64/cpu-features.h | 1 +
> 10 files changed, 269 insertions(+)
> create mode 100644 sysdeps/aarch64/dl-bti.c
> create mode 100644 sysdeps/aarch64/dl-prop.h
> create mode 100644 sysdeps/unix/sysv/linux/aarch64/bits/mman.h
>
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index a6b80f9395..0930250619 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1145,6 +1145,8 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
> l->l_relro_size = ph->p_memsz;
> break;
>
> + case PT_GNU_PROPERTY:
> + /* Fall through. PT_GNU_PROPERTY holds property notes. */
> case PT_NOTE:
> if (_dl_process_pt_note (l, ph, fd, fbp))
> {
This will print the same error message for a failure in _dl_process_pt_note
("cannot process note segment"). Wouldn't be better to use a more specific
error message, like "cannot process GNU property segment"?
> diff --git a/elf/rtld.c b/elf/rtld.c
> index b2ea21c98b..88b8e74de0 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1505,6 +1505,8 @@ of this helper program; chances are you did not intend to run this program.\n\
> main_map->l_relro_size = ph->p_memsz;
> break;
>
> + case PT_GNU_PROPERTY:
> + /* Fall through. PT_GNU_PROPERTY holds property notes. */
> case PT_NOTE:
> if (_rtld_process_pt_note (main_map, ph))
> _dl_error_printf ("\
As before.
> diff --git a/sysdeps/aarch64/Makefile b/sysdeps/aarch64/Makefile
> index 9cb141004d..5ae8b082b0 100644
> --- a/sysdeps/aarch64/Makefile
> +++ b/sysdeps/aarch64/Makefile
> @@ -1,5 +1,9 @@
> long-double-fcts = yes
>
> +ifeq ($(subdir),elf)
> +sysdep-dl-routines += dl-bti
> +endif
> +
> ifeq ($(subdir),elf)
> sysdep-dl-routines += tlsdesc dl-tlsdesc
> gen-as-const-headers += dl-link.sym
Ok.
> diff --git a/sysdeps/aarch64/dl-bti.c b/sysdeps/aarch64/dl-bti.c
> new file mode 100644
> index 0000000000..9ce697527d
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-bti.c
> @@ -0,0 +1,54 @@
> +/* AArch64 BTI initializers function.
'functions' maybe?
> + Copyright (C) 2020 Free Software Foundation, Inc.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#include <unistd.h>
> +#include <errno.h>
> +#include <libintl.h>
> +#include <ldsodefs.h>
> +
> +static int
> +enable_bti (struct link_map *map, const char *program)
> +{
> + const ElfW(Phdr) *phdr;
> + unsigned prot = PROT_READ | PROT_EXEC | PROT_BTI;
> +
> + for (phdr = map->l_phdr; phdr < &map->l_phdr[map->l_phnum]; ++phdr)
> + if (phdr->p_type == PT_LOAD && (phdr->p_flags & PF_X))
> + {
> + ElfW(Addr) start = phdr->p_vaddr + map->l_addr;
> + ElfW(Addr) len = phdr->p_memsz;
> + if (__mprotect ((void *)start, len, prot) < 0)
Space after cast.
> + {
> + if (program)
> + _dl_fatal_printf ("%s: mprotect failed to turn on BTI\n",
> + map->l_name);
> + else
> + _dl_signal_error (EINVAL, map->l_name, "dlopen",
> + N_("mprotect failed to turn on BTI"));
> + }
> + }
> + return 0;
> +}
> +
> +/* Enable BTI for L if required. */
> +
> +void
> +_dl_bti_check (struct link_map *l, const char *program)
> +{
> + if (GLRO(dl_aarch64_cpu_features).bti && l->l_mach.bti_guarded)
> + enable_bti (l, program);
> +}
No implicit checks (since there are defined both as int).
> diff --git a/sysdeps/aarch64/dl-prop.h b/sysdeps/aarch64/dl-prop.h
> new file mode 100644
> index 0000000000..6662e4ab14
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-prop.h
> @@ -0,0 +1,170 @@
> +/* Support for GNU properties. AArch64 version.
> + Copyright (C) 2018-2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#ifndef _DL_PROP_H
> +#define _DL_PROP_H
> +
> +#include <not-cancel.h>
> +
> +extern void _dl_bti_check (struct link_map *, const char *)
> + attribute_hidden;
> +
> +static inline void __attribute__ ((always_inline))
> +_rtld_main_check (struct link_map *m, const char *program)
> +{
> + _dl_bti_check (m, program);
> +}
> +
> +static inline void __attribute__ ((always_inline))
> +_dl_open_check (struct link_map *m)
> +{
> + _dl_bti_check (m, 0);
It should be NULL here instead of 0 (glibc code guideline).
> +}
> +
> +static inline void __attribute__ ((unused))
> +_dl_process_aarch64_property (struct link_map *l,
> + const ElfW(Nhdr) *note,
> + const ElfW(Addr) size,
> + const ElfW(Addr) align)
> +{
> + /* The NT_GNU_PROPERTY_TYPE_0 note must be aliged to 4 bytes in
s/aliged/aligned
> + 32-bit objects and to 8 bytes in 64-bit objects. Skip notes
> + with incorrect alignment. */
> + if (align != (__ELF_NATIVE_CLASS / 8))
> + return;
> +
> + const ElfW(Addr) start = (ElfW(Addr)) note;
> +
> + unsigned int feature_1 = 0;
> + unsigned int last_type = 0;
> +
> + while ((ElfW(Addr)) (note + 1) - start < size)
> + {
> + /* Find the NT_GNU_PROPERTY_TYPE_0 note. */
> + if (note->n_namesz == 4
> + && note->n_type == NT_GNU_PROPERTY_TYPE_0
> + && memcmp (note + 1, "GNU", 4) == 0)
> + {
> + /* Check for invalid property. */
> + if (note->n_descsz < 8
> + || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
> + return;
> +
> + /* Start and end of property array. */
> + unsigned char *ptr = (unsigned char *) (note + 1) + 4;
> + unsigned char *ptr_end = ptr + note->n_descsz;
> +
> + do
> + {
> + unsigned int type = *(unsigned int *) ptr;
> + unsigned int datasz = *(unsigned int *) (ptr + 4);
> +
> + /* Property type must be in ascending order. */
> + if (type < last_type)
> + return;
> +
> + ptr += 8;
> + if ((ptr + datasz) > ptr_end)
> + return;
> +
> + last_type = type;
The logic to parse the PT_GNU_PROPERTY is quite similar to the one
at sysdeps/x86/dl-prop.h to parse PT_NOTE. Would it be possible to
maybe try to consolidate the logic somewhere to avoid this code
duplication?
> +
> + if (type == GNU_PROPERTY_AARCH64_FEATURE_1_AND)
> + {
> + /* The size of GNU_PROPERTY_AARCH64_FEATURE_1_AND is 4
> + bytes. When seeing GNU_PROPERTY_AARCH64_FEATURE_1_AND,
> + we stop the search regardless if its size is correct
> + or not. There is no point to continue if this note
> + is ill-formed. */
> + if (datasz != 4)
> + return;
> +
> + feature_1 = *(unsigned int *) ptr;
> + if ((feature_1 & GNU_PROPERTY_AARCH64_FEATURE_1_BTI))
> + l->l_mach.bti_guarded = true;
> +
> + /* Stop if we found the property note. */
> + return;
> + }
> + else if (type > GNU_PROPERTY_AARCH64_FEATURE_1_AND)
> + {
> + /* Stop since property type is in ascending order. */
> + return;
> + }
> +
> + /* Check the next property item. */
> + ptr += ALIGN_UP (datasz, sizeof (ElfW(Addr)));
> + }
> + while ((ptr_end - ptr) >= 8);
> + }
> +
> + /* NB: Note sections like .note.ABI-tag and .note.gnu.build-id are
> + aligned to 4 bytes in 64-bit ELF objects. */
> + note = ((const void *) note
> + + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
> + align));
> + }
> +}
> +
> +#ifdef FILEBUF_SIZE
> +static inline int __attribute__ ((unused))
> +_dl_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph,
> + int fd, struct filebuf *fbp)
> +{
> + if (ph->p_type != PT_GNU_PROPERTY)
> + return 0;
> +
> + const ElfW(Nhdr) *note;
> + ElfW(Nhdr) *note_malloced = NULL;
> + ElfW(Addr) size = ph->p_filesz;
> +
> + if (ph->p_offset + size <= (size_t) fbp->len)
> + note = (const void *) (fbp->buf + ph->p_offset);
> + else
> + {
> + if (size < __MAX_ALLOCA_CUTOFF)
> + note = alloca (size);
> + else
> + note = note_malloced = malloc (size);
> + if (note == NULL)
> + return -1;
> + if (__pread64_nocancel (fd, (void *) note, size, ph->p_offset) != size)
> + {
> + if (note_malloced)
> + free (note_malloced);
> + return -1;
I wonder if we should use a scratch_buffer here instead.
> + }
> + }
> + _dl_process_aarch64_property (l, note, ph->p_filesz, ph->p_align);
> + if (note_malloced)
> + free (note_malloced);
> + return 0;
> +}
> +#endif
> +
> +static inline int __attribute__ ((unused))
> +_rtld_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph)
> +{
> + if (ph->p_type != PT_GNU_PROPERTY)
> + return 0;
Not sure this is the right design to use the same function to handle
both PT_NOTE and PT_GNU_PROPERTY.
> + const ElfW(Nhdr) *note = (const void *) (ph->p_vaddr + l->l_addr);
> + _dl_process_aarch64_property (l, note, ph->p_memsz, ph->p_align);
> + return 0;
> +}
> +
> +#endif /* _DL_PROP_H */
> diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
> index 943a9ee9e4..cc196512d7 100644
> --- a/sysdeps/aarch64/linkmap.h
> +++ b/sysdeps/aarch64/linkmap.h
> @@ -20,4 +20,5 @@ struct link_map_machine
> {
> ElfW(Addr) plt; /* Address of .plt */
> void *tlsdesc_table; /* Address of TLS descriptor hash table. */
> + int bti_guarded; /* Branch Target Identification mechanism enabled. */
Maybe bool here?
> };
> diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h b/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
> index 4ee14b4208..af90d8a626 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
> @@ -72,3 +72,4 @@
> #define HWCAP2_BF16 (1 << 14)
> #define HWCAP2_DGH (1 << 15)
> #define HWCAP2_RNG (1 << 16)
> +#define HWCAP2_BTI (1 << 17)
This it not yet upstream on Linus tree (6e7f2eacf098), but follows
the arm64/for-next/bti branch (8ef8f360cf30be12).
> diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/mman.h b/sysdeps/unix/sysv/linux/aarch64/bits/mman.h
> new file mode 100644
> index 0000000000..ecae046344
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aarch64/bits/mman.h
> @@ -0,0 +1,31 @@
> +/* Definitions for POSIX memory map interface. Linux/AArch64 version.
> + Copyright (C) 2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +#ifndef _SYS_MMAN_H
> +# error "Never use <bits/mman.h> directly; include <sys/mman.h> instead."
> +#endif
> +
> +/* AArch64 specific definitions, should be in sync with
> + arch/arm64/include/uapi/asm/mman.h. */
> +
> +#define PROT_BTI 0x10
Linux specific flags should be protected by __USE_MISC.
> +
> +#include <bits/mman-map-flags-generic.h>
> +
> +/* Include generic Linux declarations. */
> +#include <bits/mman-linux.h>
> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> index 896c588fee..c2385fb498 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> @@ -83,4 +83,7 @@ init_cpu_features (struct cpu_features *cpu_features)
>
> if ((dczid & DCZID_DZP_MASK) == 0)
> cpu_features->zva_size = 4 << (dczid & DCZID_BS_MASK);
> +
> + /* Check if BTI is enabled. */
> + cpu_features->bti = (GLRO (dl_hwcap2) & HWCAP2_BTI);
> }
> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> index 1389cea1b3..88983eb723 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> @@ -64,6 +64,7 @@ struct cpu_features
> {
> uint64_t midr_el1;
> unsigned zva_size;
> + int bti;
Maybe bool here?
> };
>
> #endif /* _CPU_FEATURES_AARCH64_H */
> --
> 2.17.1
next prev parent reply other threads:[~2020-05-07 21:07 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 17:34 [PATCH 00/12] aarch64: branch protection support Szabolcs Nagy
2020-04-30 17:37 ` [PATCH 01/12] elf.h: Add PT_GNU_PROPERTY Szabolcs Nagy
2020-05-07 14:49 ` Adhemerval Zanella
2020-04-30 17:37 ` [PATCH 02/12] elf.h: add aarch64 property definitions Szabolcs Nagy
2020-05-07 14:50 ` Adhemerval Zanella
2020-04-30 17:39 ` [PATCH 03/12] aarch64: Add BTI landing pads to assembly code Szabolcs Nagy
2020-05-07 16:55 ` Adhemerval Zanella
2020-05-11 11:38 ` Szabolcs Nagy
2020-05-11 19:13 ` Adhemerval Zanella
2020-04-30 17:40 ` [PATCH 04/12] aarch64: Rename place holder .S files to .c Szabolcs Nagy
2020-05-07 18:29 ` Adhemerval Zanella
2020-04-30 17:41 ` [PATCH 05/12] aarch64: fix swapcontext for BTI Szabolcs Nagy
2020-05-07 18:42 ` Adhemerval Zanella
2020-04-30 17:42 ` [PATCH 06/12] aarch64: fix RTLD_START " Szabolcs Nagy
2020-05-07 18:49 ` Adhemerval Zanella
2020-05-07 19:24 ` Szabolcs Nagy
2020-05-07 19:55 ` Adhemerval Zanella
2020-05-07 20:14 ` Szabolcs Nagy
2020-05-07 20:20 ` Adhemerval Zanella
2020-04-30 17:42 ` [PATCH 07/12] aarch64: fix syscalls " Szabolcs Nagy
2020-05-07 19:40 ` Adhemerval Zanella
2020-05-11 11:46 ` Szabolcs Nagy
2020-04-30 17:43 ` [PATCH 08/12] Rewrite abi-note.S in C Szabolcs Nagy
2020-04-30 20:07 ` Zack Weinberg
2020-05-01 9:23 ` Szabolcs Nagy
2020-05-01 14:07 ` Zack Weinberg
2020-04-30 17:44 ` [PATCH 09/12] aarch64: support BTI enabled binaries Szabolcs Nagy
2020-05-07 21:07 ` Adhemerval Zanella [this message]
2020-05-11 11:04 ` Szabolcs Nagy
2020-05-11 18:38 ` Adhemerval Zanella
2020-04-30 17:44 ` [PATCH 10/12] aarch64: Add pac-ret support to asm files Szabolcs Nagy
2020-05-08 16:59 ` Adhemerval Zanella
2020-05-11 8:27 ` Szabolcs Nagy
2020-05-11 18:39 ` Adhemerval Zanella
2020-04-30 17:45 ` [PATCH 11/12] aarch64: redefine RETURN_ADDRESS to strip PAC Szabolcs Nagy
2020-05-08 17:44 ` Adhemerval Zanella
2020-05-11 12:38 ` Szabolcs Nagy
2020-05-11 19:15 ` Adhemerval Zanella
2020-05-11 19:21 ` Florian Weimer
2020-05-11 20:13 ` Adhemerval Zanella
2020-05-11 20:18 ` Florian Weimer
2020-05-11 19:22 ` Florian Weimer
2020-05-11 20:45 ` Adhemerval Zanella
2020-05-12 8:42 ` Szabolcs Nagy
2020-04-30 17:45 ` [PATCH 12/12] aarch64: Configure option to build glibc with branch protection Szabolcs Nagy
2020-04-30 19:02 ` Joseph Myers
2020-05-08 17:53 ` Adhemerval Zanella
2020-05-04 11:27 ` [PATCH 00/12] aarch64: branch protection support Szabolcs Nagy
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=6ceb0ca3-2d71-907c-0a91-0b28d20b94fa@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=Sudi.Das@arm.com \
--cc=libc-alpha@sourceware.org \
--cc=szabolcs.nagy@arm.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).