Re: [PATCH v3 09/13] aarch64: enable BTI at runtime

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 15/05/2020 11:40, Szabolcs Nagy wrote:
> From: Sudakshina Das <sudi.das@arm.com>
> 
> Binaries can opt-in to using BTI via an ELF object file 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 (old linkers just append the notes of input objects
> together and add them to the output without checking them for
> consistency which means multiple incompatible GNU property notes
> can be present in PT_NOTE). A new _dl_process_pt_gnu_property
> hook is introduced in dl-prop.h and to keep it maintainable the
> rtld and dlopen code paths use the same function (if the main
> map needs special treatment, that should be inferred by the hook
> from the link map). Unlike the _dt_process_pt_note hook this one
> is called after segments are mapped to avoid unbounded allocation
> and additional read syscall. Otherwise the AArch64 logic follows
> the x86 logic for handling GNU properties (but the code is not
> shared because x86 needs to manage internal CET state and look
> out for multiple property notes).
> 
> BTI property is handled in the loader even if glibc is not built
> with BTI support, so in theory user code can be BTI protected
> independently of glibc. In practice though user binaries are not
> marked with the BTI property if glibc has no support because the
> static linked libc objects (crt files, libc_nonshared.a) are
> unmarked.
> 
> This patch relies on Linux userspace API that is scheduled to be
> merged in Linux 5.8 and now it is in the for-next/bti-user branch
> of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git.
> 
> Co-authored-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

LGTM with a just a nit below.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>

> ---
>  elf/dl-load.c                                 |  13 ++
>  elf/rtld.c                                    |   6 +
>  sysdeps/aarch64/Makefile                      |   4 +
>  sysdeps/aarch64/dl-bti.c                      |  54 +++++++
>  sysdeps/aarch64/dl-prop.h                     | 145 ++++++++++++++++++
>  sysdeps/aarch64/linkmap.h                     |   3 +
>  sysdeps/generic/dl-prop.h                     |  16 +-
>  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    |   2 +
>  sysdeps/x86/dl-prop.h                         |   6 +
>  12 files changed, 279 insertions(+), 5 deletions(-)
>  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 06f2ba7264..9c37ec1098 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -1188,6 +1188,19 @@ _dl_map_object_from_fd (const char *name, const char *origname, int fd,
>  				  maplength, has_holes, loader);
>      if (__glibc_unlikely (errstring != NULL))
>        goto call_lose;
> +
> +    /* Process program headers again after load segments are mapped.  */

Maybe add a brief explanation of why it is done after load segment mapping?

> +    for (ph = phdr; ph < &phdr[l->l_phnum]; ++ph)
> +      switch (ph->p_type)
> +	{
> +	case PT_GNU_PROPERTY:
> +	  if (_dl_process_pt_gnu_property (l, ph))
> +	    {
> +	      errstring = N_("cannot process GNU property segment");
> +	      goto call_lose;
> +	    }
> +	  break;
> +	}
>    }
>  
>    if (l->l_ld == 0)

Ok.

> diff --git a/elf/rtld.c b/elf/rtld.c
> index 5ccc3c2dbb..97a0bbf4dc 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -1506,6 +1506,12 @@ 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:
> +	if (_dl_process_pt_gnu_property (main_map, ph))
> +	  _dl_error_printf (
> +"ERROR: '%s': cannot process GNU property segment.\n", _dl_argv[0]);
> +	break;
> +
>        case PT_NOTE:
>  	if (_rtld_process_pt_note (main_map, ph))
>  	  _dl_error_printf ("\

Ok.

> 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..6003686601
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-bti.c
> @@ -0,0 +1,54 @@
> +/* AArch64 BTI functions.
> +   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)
> +	  {
> +	    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"));

Is EINVAL the only possible error here (EACCES or ENOMEM might be still
possible)?

> +	  }
> +      }
> +  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)
> +    enable_bti (l, program);
> +}

Ok.

> diff --git a/sysdeps/aarch64/dl-prop.h b/sysdeps/aarch64/dl-prop.h
> new file mode 100644
> index 0000000000..b6f8a88667
> --- /dev/null
> +++ b/sysdeps/aarch64/dl-prop.h
> @@ -0,0 +1,145 @@
> +/* 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, NULL);
> +}
> +
> +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 aligned to 4 bytes in
> +     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;
> +
> +	      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 = 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);
> +	}
> +
> +      note = ((const void *) note
> +	      + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
> +				      align));
> +    }
> +}
> +
> +#ifdef FILEBUF_SIZE
> +static inline int __attribute__ ((always_inline))
> +_dl_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph,
> +		     int fd, struct filebuf *fbp)
> +{
> +  return 0;
> +}
> +#endif
> +
> +static inline int __attribute__ ((always_inline))
> +_rtld_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph)
> +{
> +  return 0;
> +}
> +
> +static inline int
> +_dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
> +{
> +  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 */

Ok.

> diff --git a/sysdeps/aarch64/linkmap.h b/sysdeps/aarch64/linkmap.h
> index 943a9ee9e4..847a03ace2 100644
> --- a/sysdeps/aarch64/linkmap.h
> +++ b/sysdeps/aarch64/linkmap.h
> @@ -16,8 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <https://www.gnu.org/licenses/>.  */
>  
> +#include <stdbool.h>
> +
>  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.  */
>  };

Ok.

> diff --git a/sysdeps/generic/dl-prop.h b/sysdeps/generic/dl-prop.h
> index 6b0f2aa95a..4192049739 100644
> --- a/sysdeps/generic/dl-prop.h
> +++ b/sysdeps/generic/dl-prop.h
> @@ -20,11 +20,11 @@
>  #define _DL_PROP_H
>  
>  /* The following functions are used by the dynamic loader and the
> -   dlopen machinery to process PT_NOTE entries in the binary or
> -   shared object.  The notes can be used to change the behaviour of
> -   the loader, and as such offer a flexible mechanism for hooking in
> -   various checks related to ABI tags or implementing "flag day" ABI
> -   transitions.  */
> +   dlopen machinery to process PT_NOTE and PT_GNU_PROPERTY entries in
> +   the binary or shared object.  The notes can be used to change the
> +   behaviour of the loader, and as such offer a flexible mechanism
> +   for hooking in various checks related to ABI tags or implementing
> +   "flag day" ABI transitions.  */
>  
>  static inline void __attribute__ ((always_inline))
>  _rtld_main_check (struct link_map *m, const char *program)
> @@ -51,4 +51,10 @@ _rtld_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph)
>    return 0;
>  }
>  
> +static inline int __attribute__ ((always_inline))
> +_dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
> +{
> +  return 0;
> +}
> +
>  #endif /* _DL_PROP_H */

Ok.

> 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)

Ok.

> 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
> +
> +#include <bits/mman-map-flags-generic.h>
> +
> +/* Include generic Linux declarations.  */
> +#include <bits/mman-linux.h>

Ok (although I am still not sure if this should be inside a __USE_MISC).

> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.c b/sysdeps/unix/sysv/linux/aarch64/cpu-features.c
> index 896c588fee..b9ab827aca 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 supported.  */
> +  cpu_features->bti = GLRO (dl_hwcap2) & HWCAP2_BTI;
>  }

Ok.

> diff --git a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> index 1389cea1b3..a81f186ec2 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/cpu-features.h
> @@ -20,6 +20,7 @@
>  #define _CPU_FEATURES_AARCH64_H
>  
>  #include <stdint.h>
> +#include <stdbool.h>
>  
>  #define MIDR_PARTNUM_SHIFT	4
>  #define MIDR_PARTNUM_MASK	(0xfff << MIDR_PARTNUM_SHIFT)
> @@ -64,6 +65,7 @@ struct cpu_features
>  {
>    uint64_t midr_el1;
>    unsigned zva_size;
> +  bool bti;
>  };
>  
>  #endif /* _CPU_FEATURES_AARCH64_H  */

Ok.

> diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
> index 516f88ea80..8649314f9d 100644
> --- a/sysdeps/x86/dl-prop.h
> +++ b/sysdeps/x86/dl-prop.h
> @@ -191,4 +191,10 @@ _rtld_process_pt_note (struct link_map *l, const ElfW(Phdr) *ph)
>    return 0;
>  }
>  
> +static inline int __attribute__ ((always_inline))
> +_dl_process_pt_gnu_property (struct link_map *l, const ElfW(Phdr) *ph)
> +{
> +  return 0;
> +}
> +
>  #endif /* _DL_PROP_H */
> 

Ok.