Re: [PATCH v3 2/2] aarch64: Use mmap to add PROT_BTI instead of mprotect [BZ #26831]

[Adhemerval Zanella]

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@xxxxxxxxxx>

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