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

[Szabolcs Nagy]

The 05/25/2020 16:53, Adhemerval Zanella wrote:
> 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>

thanks for the review.

> > @@ -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;
> > +	}

btw i think the _dl_process_pt_note callback should
be done here too and x86 fixed up accordingly so it
does not need unbounded allocation + pread_nocancel
to process the notes.

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

no, i think passing errno makes more sense here.
i'll fix it.