Re: [Qemu-devel] [PATCH v6 5/6] linux-user: Parse NT_GNU_PROPERTY_TYPE_0 notes

[Dave Martin]

On Wed, Jun 05, 2019 at 09:57:05PM +0100, Richard Henderson wrote:
> For aarch64, this includes the GNU_PROPERTY_AARCH64_FEATURE_1_BTI bit,
> which indicates that the image should be mapped with guarded pages.

Heads-up: for arm64 I plan to move to making PT_GNU_PROPERTY
authoritiative for ELF on linux: if this is present, we use it to find
the note directly and ignore any other notes; if there is no
PT_GNU_PROPERTY entry then we assume there is no NT_GNU_PROPERTY_TYPE_0
note.

This is not quite decided yet, but to avoid fragmentation I'd prefer
qemu and Linux apply the same policy -- I'll keep you in the loop about
the decision.

I think you can reasonable upstream this patch in qemu and then
subsequently make it stricter without an ABI break.  Upstream GNU ld
generates this entry today.

> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/elfload.c | 83 +++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 75 insertions(+), 8 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index a57b7049dd..1a12c60a33 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -2253,7 +2253,7 @@ static void load_elf_image(const char *image_name, int 
> image_fd,
>      struct elfhdr *ehdr = (struct elfhdr *)bprm_buf;
>      struct elf_phdr *phdr;
>      abi_ulong load_addr, load_bias, loaddr, hiaddr, error;
> -    int i, retval;
> +    int i, retval, prot_exec = PROT_EXEC;
>      const char *errmsg;
>  
>      /* First of all, some simple consistency checks */
> @@ -2288,17 +2288,78 @@ static void load_elf_image(const char *image_name, 
> int image_fd,
>      loaddr = -1, hiaddr = 0;
>      info->alignment = 0;
>      for (i = 0; i < ehdr->e_phnum; ++i) {
> -        if (phdr[i].p_type == PT_LOAD) {
> -            abi_ulong a = phdr[i].p_vaddr - phdr[i].p_offset;
> +        struct elf_phdr *eppnt = phdr + i;
> +
> +        if (eppnt->p_type == PT_LOAD) {
> +            abi_ulong a = eppnt->p_vaddr - eppnt->p_offset;
>              if (a < loaddr) {
>                  loaddr = a;
>              }
> -            a = phdr[i].p_vaddr + phdr[i].p_memsz;
> +            a = eppnt->p_vaddr + eppnt->p_memsz;
>              if (a > hiaddr) {
>                  hiaddr = a;
>              }
>              ++info->nsegs;
> -            info->alignment |= phdr[i].p_align;
> +            info->alignment |= eppnt->p_align;
> +        } else if (eppnt->p_type == PT_NOTE) {
> +#ifdef TARGET_AARCH64
> +            /*
> +             * Process NT_GNU_PROPERTY_TYPE_0.
> +             *
> +             * TODO: The only item that is AArch64 specific is the
> +             * GNU_PROPERTY_AARCH64_FEATURE_1_AND processing at the end.
> +             * If we were to ever process GNU_PROPERTY_X86_*, all of the
> +             * code through checking the gnu0 magic number is sharable.
> +             * But for now, since this *is* only used by AArch64, don't
> +             * process the note elsewhere.
> +             */
> +            const uint32_t gnu0_magic = const_le32('G' | 'N' << 8 | 'U' << 
> 16);
> +            uint32_t note[7];
> +
> +            /*
> +             * The note contents are 7 words, but depending on LP64 vs ILP32
> +             * there may be an 8th padding word at the end.  Check for and
> +             * read the minimum size.  Further checks below will validate
> +             * that the sizes of everything involved are as we expect.
> +             */
> +            if (eppnt->p_filesz < sizeof(note)) {
> +                continue;
> +            }
> +            if (eppnt->p_offset + eppnt->p_filesz <= BPRM_BUF_SIZE) {
> +                memcpy(note, bprm_buf + eppnt->p_offset, sizeof(note));
> +            } else {
> +                retval = pread(image_fd, note, sizeof(note), 
> eppnt->p_offset);
> +                if (retval != sizeof(note)) {
> +                    goto exit_perror;
> +                }
> +            }

Can we police that the segment alignment matches the ELF class (i.e., 8
for 64-bit, 4 for 32-bit)?

hjl specifies this, but it's controversial and sometimes missed --
there's a bug right now in GNU ld where --force-bti generates a note
with alignment 1.

Due to the high chance of screwing this up, it'd be good to police it
wherever appropriate.

> +#ifdef BSWAP_NEEDED
> +            for (i = 0; i < ARRAY_SIZE(note); ++i) {
> +                bswap32s(note + i);
> +            }
> +#endif
> +            /*
> +             * Check that this is a NT_GNU_PROPERTY_TYPE_0 note.
> +             * Again, descsz includes padding.  Full size validation
> +             * awaits checking the final payload.
> +             */
> +            if (note[0] != 4 ||                       /* namesz */
> +                note[1] < 12 ||                       /* descsz */
> +                note[2] != NT_GNU_PROPERTY_TYPE_0 ||  /* type */
> +                note[3] != gnu0_magic) {              /* name */
> +                continue;
> +            }
> +            /*
> +             * Check for the BTI feature.  If present, this indicates
> +             * that all the executable pages of the binary should be
> +             * mapped with PROT_BTI, so that branch targets are enforced.
> +             */
> +            if (note[4] == GNU_PROPERTY_AARCH64_FEATURE_1_AND &&
> +                note[5] == 4 &&
> +                (note[6] & GNU_PROPERTY_AARCH64_FEATURE_1_BTI)) {
> +                prot_exec |= TARGET_PROT_BTI;
> +            }
> +#endif /* TARGET_AARCH64 */
>          }
>      }
>  
> @@ -2358,9 +2419,15 @@ static void load_elf_image(const char *image_name, int 
> image_fd,
>              abi_ulong vaddr, vaddr_po, vaddr_ps, vaddr_ef, vaddr_em, 
> vaddr_len;
>              int elf_prot = 0;
>  
> -            if (eppnt->p_flags & PF_R) elf_prot =  PROT_READ;
> -            if (eppnt->p_flags & PF_W) elf_prot |= PROT_WRITE;
> -            if (eppnt->p_flags & PF_X) elf_prot |= PROT_EXEC;
> +            if (eppnt->p_flags & PF_R) {
> +                elf_prot |= PROT_READ;
> +            }
> +            if (eppnt->p_flags & PF_W) {
> +                elf_prot |= PROT_WRITE;
> +            }
> +            if (eppnt->p_flags & PF_X) {
> +                elf_prot |= prot_exec;
> +            }

Ack.

There are interesting subtleties with segment alignments here: the prot
flags on each segment "bleed" to an adjacent boundary.  I'm not sure
that behaviour is well specified, but we at least get away with it due
to conventions regarding the ordering of segments.

I still have to think about whether this works right for PROT_BTI, which
is a prohibition rather than a permission, suggesting that the bleed
rules might need to be inverted for it.

My current policy is that because and ELF is all-BTI if any page is BTI,
we probably get away with it, without having to do anything special.

[...]

Cheers
---Dave