[PATCH] elf: Properly compute offsets of desc and next note

[H.J. Lu]

On Thu, Nov 16, 2017 at 7:22 AM, Nick Clifton <nickc@redhat.com> wrote:
> Hi H.J.
>
>> According to gABI, in a note entry, the note name field, not note name
>> size, is padded for the note descriptor.
>
> Is it possible for there to be notes in already existing binaries where
> the name field was not padded up to the alignment boundary, but instead
> just contains random bytes after the terminating NUL ?  If so, will the
> patched code attempt to interpret these extra bytes and so run into
> problems ?

We check if the size is valid:

     in.descsz = H_GET_32 (abfd, xnp->descsz);
      in.descdata = p + ELF_NOTE_DESC_OFFSET (in.namesz, align);
      in.descpos = offset + (in.descdata - buf);
      if (in.descsz != 0
          && (in.descdata >= buf + size
              || in.descsz > buf - in.descdata + size))
        return FALSE;

and

        inote.descdata = ((char *) external
                            + ELF_NOTE_DESC_OFFSET (inote.namesz,
                                                    section->sh_addralign));
          inote.descpos  = offset + (inote.descdata - (char *) pnotes);
          next = ((char *) external
                  + ELF_NOTE_NEXT_OFFSET (inote.namesz, inote.descsz,
                                          section->sh_addralign));
...

     /* PR 17531: file: 3443835e.  */
      /* PR 17531: file: id:000000,sig:11,src:006986,op:havoc,rep:4.  */
      if ((size_t) (inote.descdata - inote.namedata) < inote.namesz
          || (size_t) (inote.descdata - inote.namedata) > data_remaining
          || (size_t) (next - inote.descdata) < inote.descsz
          || ((size_t) (next - inote.descdata)
              > data_remaining - (size_t) (inote.descdata - inote.namedata)))
        {
          warn (_("note with invalid namesz and/or descsz found at
offset 0x%lx\n"),
                (unsigned long) ((char *) external - (char *) pnotes));
          warn (_(" type: 0x%lx, namesize: 0x%08lx, descsize: 0x%08lx\n"),
                inote.type, inote.namesz, inote.descsz);
          break;
        }

>
> By the way - does the gABI mandate what value(s) should be used for padding ?
>

No.

>> Also notes
>> are aligned to 4 bytes in 32-bit objects and 8 bytes in 64-bit objects.
>
> I don't mind enforcing this requirement in the notes that the binutils
> generates from now on, but I think that we ought to be relaxed about
> accepting 64-bit notes that have already been generated with only 4-byte
> alignment.

Of course.  That is why my patch uses alignment of note section or note segment,
instead of assuming alignment based on ELF file class.

>
>> Since on Linux, .note.ABI-tag and .note.gnu.build-id notes are always
>> aligned to 4 bytes, we need to use alignment of note section or note
>> segment, instead of assuming alignment based on ELF file class.
>
> IE, going against the gABI specification, yes ?

Yes.

> Also - doesn't this imply that the alignment of the note section or note
> segment in 64-bit binaries is incorrect.  IE, since in theory they should
> contain 8-byte aligned notes, their alignment ought to be 8-bytes as well.

True.  .note.ABI-tag and .note.gnu.build-id notes in 64-bit objects don't
follow gABI.

> Wouldn't it be better to update the gABI document to say that the alignment
> of individual notes must be equal to the alignment of their containing
> section or segment, and not dependent upon their ELF class ?

This is implied.  All notes in one PT_NOTE segment must have the same
alignment.  Otherwise, you can't parse notes.

> Alternatively, if we are going to have special cases for certain notes,
> then these ought to be covered in the gABI document, and we need to
> consider if there are other notes, possibly for non-x86 architectures,
> that need to be covered as well.
>

I will send an email to gABI.

> Either way, I do not think that your patch should go in right now.  We
> definitely need some more discussion on this issue.
>

Why?  My patch doesn't break anything.


-- 
H.J.