Re: [PATCH 09/12] aarch64: support BTI enabled binaries

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 05/07/2020 18:07, Adhemerval Zanella wrote:
> 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"?

i was struggling to deal with this, i'm happy
to create a new hook for pt_gnu_property
(that's actually cleaner in aarch64, but x86
will have to continue to look at PT_NOTE for
the same).

it requires more generic changes though and
related code repetitions.

> > +	  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?

yes it's similar but not the same.

x86 tries to deal with multiple property notes
which does not happen on aarch64.

i can try to refactor the code and see if
that works.

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

this logic is copied from x86, on aarch64 local buffer
should work with current linkers (since they will only
add at most one gnu property note to PT_GNU_PROPERTY),
but we don't know what happens in the future so the
malloc fallback is probably required.

i think ideally the segment is mmaped into memory and
we can just use that, but i assumed the logic is there
for a reason on x86.

> > +++ 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?

ok.

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

yes, now renamed to bti-user (because the kernel
code can also use bti protection) it is scheduled
for next linux, this patchset depends on that work,
but they have to be tested together.

> > +/* 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.  

in posix the PROT_ prefix is reserved for sys/mman.h
so there is no namespace issue with this.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html

see also PROT_SAO on powerpc.

> > +++ 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?

ok.