[PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

On Wed, Nov 15, 2017 at 3:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Sun, Nov 12, 2017 at 8:03 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>> On Nov 12 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>
>>> Data after Elf64_Nhdr is the "name" field which is a 4-byte string, "GNU".
>>> There is no misalignment.
>>
>> That's not what your patch does.  You are aligning both the name length
>> and the data length to a 8 byte boundary, making the note size
>> unaligned.
>>
>
> You are right.  Both glibc and binutils get this wrong.  I opened a
> binutils bug:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=22444
>
> I will fix it first and take care of glibc next.
>

Here is the glibc path.

Any comments?

-- 
H.J.

[-- Attachment #2: 0001-Properly-compute-offsets-of-note-descriptor-and-next.patch --]
[-- Type: text/x-patch, Size: 6893 bytes --]

From 0dfdda09f7b0faef5ff166f3e4e099b20571b76c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 31 Oct 2017 04:51:41 -0700
Subject: [PATCH] Properly compute offsets of note descriptor and next note [BZ
 #22370]

A note header has 3 4-bytes fields, followed by note name and note
descriptor.  According to gABI, in a note entry, the note name field,
not note name size, is padded for the note descriptor.  And the note
descriptor field, not note descriptor size, is padded for the next
note entry.  Notes are aligned to 4 bytes in 32-bit objects and 8 bytes
in 64-bit objects.

For all GNU notes, the name is "GNU" which is 4 bytes.  They have the
same format in the first 16 bytes in both 32-bit and 64-bit objects.
They differ by note descriptor size and note type.  So far, .note.ABI-tag
and .note.gnu.build-id notes are always aligned to 4 bytes.  The exsting
codes compute the note size by aligning the note name size and note
descriptor size to 4 bytes.  It happens to produce the same value as
the actual note size by luck since the name size is 4 and offset of the
note descriptor is 16.  But it will produce the wrong size when note
alignment is 8 bytes in 64-bit objects.

This patch defines ELF_NOTE_DESC_OFFSET and ELF_NOTE_NEXT_OFFSET to
properly compute offsets of note descriptor and next note.  It uses
alignment of PT_NOTE segment to support both 4-byte and 8-byte note
alignments in 64-bit objects.

	[BZ #22370]
	* dl-hwcaps.c (ROUND): Removed.
	(_dl_important_hwcaps): Replace ROUND with ELF_NOTE_DESC_OFFSET
	and ELF_NOTE_NEXT_OFFSET.
	* elf/dl-load.c (ROUND): Removed.
	(open_verify): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* elf/readelflib.c (ROUND): Removed.
	(process_elf_file): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* include/elf.h [!_ISOMAC]: Include <libc-pointer-arith.h>.
	[!_ISOMAC] (ELF_NOTE_DESC_OFFSET): New.
	[!_ISOMAC] (ELF_NOTE_NEXT_OFFSET): Likewise.
---
 elf/dl-hwcaps.c  | 13 +++++++------
 elf/dl-load.c    |  8 ++++----
 elf/readelflib.c |  8 ++++----
 include/elf.h    | 16 ++++++++++++++--
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
index 92f2eb45ce..16da4191f0 100644
--- a/elf/dl-hwcaps.c
+++ b/elf/dl-hwcaps.c
@@ -67,6 +67,7 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	  {
 	    const ElfW(Addr) start = (phdr[i].p_vaddr
 				      + GLRO(dl_sysinfo_map)->l_addr);
+	    const ElfW(Addr) align = phdr[i].p_align;
 	    /* The standard ELF note layout is exactly as the anonymous struct.
 	       The next element is a variable length vendor name of length
 	       VENDORLEN (with a real length rounded to ElfW(Word)), followed
@@ -80,7 +81,6 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	    } *note = (const void *) start;
 	    while ((ElfW(Addr)) (note + 1) - start < phdr[i].p_memsz)
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
 		/* The layout of the type 2, vendor "GNU" note is as follows:
 		   .long <Number of capabilities enabled by this note>
 		   .long <Capabilities mask> (as mask >> _DL_FIRST_EXTRA).
@@ -91,17 +91,18 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 		    && !memcmp ((note + 1), "GNU", sizeof "GNU")
 		    && note->datalen > 2 * sizeof (ElfW(Word)) + 2)
 		  {
-		    const ElfW(Word) *p = ((const void *) (note + 1)
-					   + ROUND (sizeof "GNU"));
+		    const ElfW(Word) *p
+		      = ((const void *) note
+			 + ELF_NOTE_DESC_OFFSET (sizeof "GNU", align));
 		    cnt += *p++;
 		    ++p;	/* Skip mask word.  */
 		    dsocaps = (const char *) p; /* Pseudo-string "<b>name"  */
 		    dsocapslen = note->datalen - sizeof *p * 2;
 		    break;
 		  }
-		note = ((const void *) (note + 1)
-			+ ROUND (note->vendorlen) + ROUND (note->datalen));
-#undef ROUND
+		note = ((const void *) note
+			+ ELF_NOTE_NEXT_OFFSET (note->vendorlen,
+						note->datalen, align));
 	      }
 	    if (dsocaps != NULL)
 	      break;
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 1220183ce2..388135f4ae 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1683,6 +1683,7 @@ open_verify (const char *name, int fd,
 	if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
 	  {
 	    ElfW(Addr) size = ph->p_filesz;
+	    ElfW(Addr) align = ph->p_align;
 
 	    if (ph->p_offset + size <= (size_t) fbp->len)
 	      abi_note = (void *) (fbp->buf + ph->p_offset);
@@ -1696,10 +1697,9 @@ open_verify (const char *name, int fd,
 
 	    while (memcmp (abi_note, &expected_note, sizeof (expected_note)))
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-				       + ROUND (abi_note[0])
-				       + ROUND (abi_note[1]);
+		ElfW(Addr) note_size
+		  = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					  align);
 
 		if (size - 32 < note_size)
 		  {
diff --git a/elf/readelflib.c b/elf/readelflib.c
index 9ad56dcc34..b168cb8804 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -131,15 +131,15 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 	      ElfW(Word) *abi_note = (ElfW(Word) *) (file_contents
 						     + segment->p_offset);
 	      ElfW(Addr) size = segment->p_filesz;
+	      ElfW(Addr) align = segment->p_align;
 
 	      while (abi_note [0] != 4 || abi_note [1] != 16
 		     || abi_note [2] != 1
 		     || memcmp (abi_note + 3, "GNU", 4) != 0)
 		{
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		  ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-					 + ROUND (abi_note[0])
-					 + ROUND (abi_note[1]);
+		  ElfW(Addr) note_size
+		    = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					    align);
 
 		  if (size - 32 < note_size || note_size == 0)
 		    {
diff --git a/include/elf.h b/include/elf.h
index f06a33f256..ab76aafb1e 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1,7 +1,19 @@
 #ifndef _ELF_H
 #include <elf/elf.h>
 
-# ifndef _ISOMAC
+#ifndef _ISOMAC
+
+# include <libc-pointer-arith.h>
+
+/* Compute the offset of the note descriptor from size of note entry's
+   owner string and note alignment.  */
+# define ELF_NOTE_DESC_OFFSET(namesz, align) \
+  ALIGN_UP (sizeof (ElfW(Nhdr)) + (namesz), (align))
+
+/* Compute the offset of the next note entry from size of note entry's
+   owner string, size of the note descriptor and note alignment.  */
+# define ELF_NOTE_NEXT_OFFSET(namesz, descsz, align) \
+  ALIGN_UP (ELF_NOTE_DESC_OFFSET ((namesz), (align)) + (descsz), (align))
 
 /* Some information which is not meant for the public and therefore not
    in <elf.h>.  */
@@ -13,5 +25,5 @@
    (DF_1_NOW | DF_1_NODELETE | DF_1_INITFIRST | DF_1_NOOPEN \
     | DF_1_ORIGIN | DF_1_NODEFLIB)
 
-# endif /* !_ISOMAC */
+#endif /* !_ISOMAC */
 #endif /* elf.h */
-- 
2.13.6

Re: [PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[H.J. Lu]

On Thu, Nov 16, 2017 at 5:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Nov 15, 2017 at 3:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Sun, Nov 12, 2017 at 8:03 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>> On Nov 12 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>
>>>> Data after Elf64_Nhdr is the "name" field which is a 4-byte string, "GNU".
>>>> There is no misalignment.
>>>
>>> That's not what your patch does.  You are aligning both the name length
>>> and the data length to a 8 byte boundary, making the note size
>>> unaligned.
>>>
>>
>> You are right.  Both glibc and binutils get this wrong.  I opened a
>> binutils bug:
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=22444
>>
>> I will fix it first and take care of glibc next.
>>
>
> Here is the glibc path.
>
> Any comments?
>

Any comments or objections?


-- 
H.J.

Re: [PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[H.J. Lu]

On Mon, Nov 20, 2017 at 5:04 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Nov 16, 2017 at 5:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Nov 15, 2017 at 3:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Sun, Nov 12, 2017 at 8:03 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>> On Nov 12 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>>
>>>>> Data after Elf64_Nhdr is the "name" field which is a 4-byte string, "GNU".
>>>>> There is no misalignment.
>>>>
>>>> That's not what your patch does.  You are aligning both the name length
>>>> and the data length to a 8 byte boundary, making the note size
>>>> unaligned.
>>>>
>>>
>>> You are right.  Both glibc and binutils get this wrong.  I opened a
>>> binutils bug:
>>>
>>> https://sourceware.org/bugzilla/show_bug.cgi?id=22444
>>>
>>> I will fix it first and take care of glibc next.
>>>
>>
>> Here is the glibc path.
>>
>> Any comments?
>>
>
> Any comments or objections?
>

I updated Linux Extensions to gABI to clarify alignments of NOTE section
and segments:

https://sourceware.org/ml/gnu-gabi/2017-q4/msg00003.html

I will check it in next week.


-- 
H.J.

Re: [PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Fri, Nov 24, 2017 at 6:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Mon, Nov 20, 2017 at 5:04 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Nov 16, 2017 at 5:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Wed, Nov 15, 2017 at 3:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Sun, Nov 12, 2017 at 8:03 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>> On Nov 12 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>>>
>>>>>> Data after Elf64_Nhdr is the "name" field which is a 4-byte string, "GNU".
>>>>>> There is no misalignment.
>>>>>
>>>>> That's not what your patch does.  You are aligning both the name length
>>>>> and the data length to a 8 byte boundary, making the note size
>>>>> unaligned.
>>>>>
>>>>
>>>> You are right.  Both glibc and binutils get this wrong.  I opened a
>>>> binutils bug:
>>>>
>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=22444
>>>>
>>>> I will fix it first and take care of glibc next.
>>>>
>>>
>>> Here is the glibc path.
>>>
>>> Any comments?
>>>
>>
>> Any comments or objections?
>>
>
> I updated Linux Extensions to gABI to clarify alignments of NOTE section
> and segments:
>
> https://sourceware.org/ml/gnu-gabi/2017-q4/msg00003.html
>
> I will check it in next week.
>

Here is the updated patch I will check in next week.  It added:

+     /* NB: Some PT_NOTE segment may have alignment value of 0
+        or 1.  gABI specifies that PT_NOTE segments should be
+        aligned to 4 bytes in 32-bit objects and to 8 bytes in
+        64-bit objects.  As a Linux extension, we also support
+        4 byte alignment in 64-bit objects.  If p_align is less
+        than 4, we treate alignment as 4 bytes.   */
+     ElfW(Addr) align = phdr[i].p_align;
+     if (align < 4)
+       align = 4;

to prevent infinite loop with incorrect segment alignment.


-- 
H.J.

[-- Attachment #2: 0001-Properly-compute-offsets-of-note-descriptor-and-next.patch --]
[-- Type: text/x-patch, Size: 8292 bytes --]

From b7cfacca88269901546529445f0bb757107984b4 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 31 Oct 2017 04:51:41 -0700
Subject: [PATCH] Properly compute offsets of note descriptor and next note [BZ
 #22370]

A note header has 3 4-bytes fields, followed by note name and note
descriptor.  According to gABI, in a note entry, the note name field,
not note name size, is padded for the note descriptor.  And the note
descriptor field, not note descriptor size, is padded for the next
note entry.  Notes are aligned to 4 bytes in 32-bit objects and 8 bytes
in 64-bit objects.

For all GNU notes, the name is "GNU" which is 4 bytes.  They have the
same format in the first 16 bytes in both 32-bit and 64-bit objects.
They differ by note descriptor size and note type.  So far, .note.ABI-tag
and .note.gnu.build-id notes are always aligned to 4 bytes.  The exsting
codes compute the note size by aligning the note name size and note
descriptor size to 4 bytes.  It happens to produce the same value as
the actual note size by luck since the name size is 4 and offset of the
note descriptor is 16.  But it will produce the wrong size when note
alignment is 8 bytes in 64-bit objects.

This patch defines ELF_NOTE_DESC_OFFSET and ELF_NOTE_NEXT_OFFSET to
properly compute offsets of note descriptor and next note.  It uses
alignment of PT_NOTE segment to support both 4-byte and 8-byte note
alignments in 64-bit objects.  To handle PT_NOTE segments with
incorrect alignment, which may lead to an infinite loop, if segment
alignment is less than 4, we treate alignment as 4 bytes.

	[BZ #22370]
	* elf/dl-hwcaps.c (ROUND): Removed.
	(_dl_important_hwcaps): Replace ROUND with ELF_NOTE_DESC_OFFSET
	and ELF_NOTE_NEXT_OFFSET.
	* elf/dl-load.c (ROUND): Removed.
	(open_verify): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* elf/readelflib.c (ROUND): Removed.
	(process_elf_file): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* include/elf.h [!_ISOMAC]: Include <libc-pointer-arith.h>.
	[!_ISOMAC] (ELF_NOTE_DESC_OFFSET): New.
	[!_ISOMAC] (ELF_NOTE_NEXT_OFFSET): Likewise.
---
 elf/dl-hwcaps.c  | 21 +++++++++++++++------
 elf/dl-load.c    | 16 ++++++++++++----
 elf/readelflib.c | 16 ++++++++++++----
 include/elf.h    | 16 ++++++++++++++--
 4 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
index 92f2eb45ce..b5f126b060 100644
--- a/elf/dl-hwcaps.c
+++ b/elf/dl-hwcaps.c
@@ -67,6 +67,15 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	  {
 	    const ElfW(Addr) start = (phdr[i].p_vaddr
 				      + GLRO(dl_sysinfo_map)->l_addr);
+	    /* NB: Some PT_NOTE segment may have alignment value of 0
+	       or 1.  gABI specifies that PT_NOTE segments should be
+	       aligned to 4 bytes in 32-bit objects and to 8 bytes in
+	       64-bit objects.  As a Linux extension, we also support
+	       4 byte alignment in 64-bit objects.  If p_align is less
+	       than 4, we treate alignment as 4 bytes.   */
+	    ElfW(Addr) align = phdr[i].p_align;
+	    if (align < 4)
+	      align = 4;
 	    /* The standard ELF note layout is exactly as the anonymous struct.
 	       The next element is a variable length vendor name of length
 	       VENDORLEN (with a real length rounded to ElfW(Word)), followed
@@ -80,7 +89,6 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	    } *note = (const void *) start;
 	    while ((ElfW(Addr)) (note + 1) - start < phdr[i].p_memsz)
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
 		/* The layout of the type 2, vendor "GNU" note is as follows:
 		   .long <Number of capabilities enabled by this note>
 		   .long <Capabilities mask> (as mask >> _DL_FIRST_EXTRA).
@@ -91,17 +99,18 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 		    && !memcmp ((note + 1), "GNU", sizeof "GNU")
 		    && note->datalen > 2 * sizeof (ElfW(Word)) + 2)
 		  {
-		    const ElfW(Word) *p = ((const void *) (note + 1)
-					   + ROUND (sizeof "GNU"));
+		    const ElfW(Word) *p
+		      = ((const void *) note
+			 + ELF_NOTE_DESC_OFFSET (sizeof "GNU", align));
 		    cnt += *p++;
 		    ++p;	/* Skip mask word.  */
 		    dsocaps = (const char *) p; /* Pseudo-string "<b>name"  */
 		    dsocapslen = note->datalen - sizeof *p * 2;
 		    break;
 		  }
-		note = ((const void *) (note + 1)
-			+ ROUND (note->vendorlen) + ROUND (note->datalen));
-#undef ROUND
+		note = ((const void *) note
+			+ ELF_NOTE_NEXT_OFFSET (note->vendorlen,
+						note->datalen, align));
 	      }
 	    if (dsocaps != NULL)
 	      break;
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 1220183ce2..51a590a03a 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1683,6 +1683,15 @@ open_verify (const char *name, int fd,
 	if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
 	  {
 	    ElfW(Addr) size = ph->p_filesz;
+	    /* NB: Some PT_NOTE segment may have alignment value of 0
+	       or 1.  gABI specifies that PT_NOTE segments should be
+	       aligned to 4 bytes in 32-bit objects and to 8 bytes in
+	       64-bit objects.  As a Linux extension, we also support
+	       4 byte alignment in 64-bit objects.  If p_align is less
+	       than 4, we treate alignment as 4 bytes.   */
+	    ElfW(Addr) align = ph->p_align;
+	    if (align < 4)
+	      align = 4;
 
 	    if (ph->p_offset + size <= (size_t) fbp->len)
 	      abi_note = (void *) (fbp->buf + ph->p_offset);
@@ -1696,10 +1705,9 @@ open_verify (const char *name, int fd,
 
 	    while (memcmp (abi_note, &expected_note, sizeof (expected_note)))
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-				       + ROUND (abi_note[0])
-				       + ROUND (abi_note[1]);
+		ElfW(Addr) note_size
+		  = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					  align);
 
 		if (size - 32 < note_size)
 		  {
diff --git a/elf/readelflib.c b/elf/readelflib.c
index 9ad56dcc34..4872f809e3 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -131,15 +131,23 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 	      ElfW(Word) *abi_note = (ElfW(Word) *) (file_contents
 						     + segment->p_offset);
 	      ElfW(Addr) size = segment->p_filesz;
+	      /* NB: Some PT_NOTE segment may have alignment value of 0
+		 or 1.  gABI specifies that PT_NOTE segments should be
+		 aligned to 4 bytes in 32-bit objects and to 8 bytes in
+		 64-bit objects.  As a Linux extension, we also support
+		 4 byte alignment in 64-bit objects.  If p_align is less
+		 than 4, we treate alignment as 4 bytes.   */
+	      ElfW(Addr) align = segment->p_align;
+	      if (align < 4)
+		align = 4;
 
 	      while (abi_note [0] != 4 || abi_note [1] != 16
 		     || abi_note [2] != 1
 		     || memcmp (abi_note + 3, "GNU", 4) != 0)
 		{
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		  ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-					 + ROUND (abi_note[0])
-					 + ROUND (abi_note[1]);
+		  ElfW(Addr) note_size
+		    = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					    align);
 
 		  if (size - 32 < note_size || note_size == 0)
 		    {
diff --git a/include/elf.h b/include/elf.h
index f06a33f256..ab76aafb1e 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1,7 +1,19 @@
 #ifndef _ELF_H
 #include <elf/elf.h>
 
-# ifndef _ISOMAC
+#ifndef _ISOMAC
+
+# include <libc-pointer-arith.h>
+
+/* Compute the offset of the note descriptor from size of note entry's
+   owner string and note alignment.  */
+# define ELF_NOTE_DESC_OFFSET(namesz, align) \
+  ALIGN_UP (sizeof (ElfW(Nhdr)) + (namesz), (align))
+
+/* Compute the offset of the next note entry from size of note entry's
+   owner string, size of the note descriptor and note alignment.  */
+# define ELF_NOTE_NEXT_OFFSET(namesz, descsz, align) \
+  ALIGN_UP (ELF_NOTE_DESC_OFFSET ((namesz), (align)) + (descsz), (align))
 
 /* Some information which is not meant for the public and therefore not
    in <elf.h>.  */
@@ -13,5 +25,5 @@
    (DF_1_NOW | DF_1_NODELETE | DF_1_INITFIRST | DF_1_NOOPEN \
     | DF_1_ORIGIN | DF_1_NODEFLIB)
 
-# endif /* !_ISOMAC */
+#endif /* !_ISOMAC */
 #endif /* elf.h */
-- 
2.14.3

Re: [PATCH] Properly compute offsets of note descriptor and next note [BZ #22370]

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 2202 bytes --]

On Sat, Nov 25, 2017 at 8:39 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Nov 24, 2017 at 6:45 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Mon, Nov 20, 2017 at 5:04 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Nov 16, 2017 at 5:24 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Wed, Nov 15, 2017 at 3:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Sun, Nov 12, 2017 at 8:03 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
>>>>>> On Nov 12 2017, "H.J. Lu" <hjl.tools@gmail.com> wrote:
>>>>>>
>>>>>>> Data after Elf64_Nhdr is the "name" field which is a 4-byte string, "GNU".
>>>>>>> There is no misalignment.
>>>>>>
>>>>>> That's not what your patch does.  You are aligning both the name length
>>>>>> and the data length to a 8 byte boundary, making the note size
>>>>>> unaligned.
>>>>>>
>>>>>
>>>>> You are right.  Both glibc and binutils get this wrong.  I opened a
>>>>> binutils bug:
>>>>>
>>>>> https://sourceware.org/bugzilla/show_bug.cgi?id=22444
>>>>>
>>>>> I will fix it first and take care of glibc next.
>>>>>
>>>>
>>>> Here is the glibc path.
>>>>
>>>> Any comments?
>>>>
>>>
>>> Any comments or objections?
>>>
>>
>> I updated Linux Extensions to gABI to clarify alignments of NOTE section
>> and segments:
>>
>> https://sourceware.org/ml/gnu-gabi/2017-q4/msg00003.html
>>
>> I will check it in next week.
>>
>
> Here is the updated patch I will check in next week.  It added:
>
> +     /* NB: Some PT_NOTE segment may have alignment value of 0
> +        or 1.  gABI specifies that PT_NOTE segments should be
> +        aligned to 4 bytes in 32-bit objects and to 8 bytes in
> +        64-bit objects.  As a Linux extension, we also support
> +        4 byte alignment in 64-bit objects.  If p_align is less
> +        than 4, we treate alignment as 4 bytes.   */
> +     ElfW(Addr) align = phdr[i].p_align;
> +     if (align < 4)
> +       align = 4;
>
> to prevent infinite loop with incorrect segment alignment.
>

This is the final patch I am checking in now.  I added:

+     if (align < 4)
+       align = 4;
+     else if (align != 4 && align != 8)
+       continue;

to skip note segments which aren't aligned to 8 bytes and 4 bytes or
less.

-- 
H.J.

[-- Attachment #2: 0001-Properly-compute-offsets-of-note-descriptor-and-next.patch --]
[-- Type: application/octet-stream, Size: 8917 bytes --]

From 76dac5320d956354b664adeaced2f322722bd347 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 31 Oct 2017 04:51:41 -0700
Subject: [PATCH] Properly compute offsets of note descriptor and next note [BZ
 #22370]

A note header has 3 4-bytes fields, followed by note name and note
descriptor.  According to gABI, in a note entry, the note name field,
not note name size, is padded for the note descriptor.  And the note
descriptor field, not note descriptor size, is padded for the next
note entry.  Notes are aligned to 4 bytes in 32-bit objects and 8 bytes
in 64-bit objects.

For all GNU notes, the name is "GNU" which is 4 bytes.  They have the
same format in the first 16 bytes in both 32-bit and 64-bit objects.
They differ by note descriptor size and note type.  So far, .note.ABI-tag
and .note.gnu.build-id notes are always aligned to 4 bytes.  The exsting
codes compute the note size by aligning the note name size and note
descriptor size to 4 bytes.  It happens to produce the same value as
the actual note size by luck since the name size is 4 and offset of the
note descriptor is 16.  But it will produce the wrong size when note
alignment is 8 bytes in 64-bit objects.

This patch defines ELF_NOTE_DESC_OFFSET and ELF_NOTE_NEXT_OFFSET to
properly compute offsets of note descriptor and next note.  It uses
alignment of PT_NOTE segment to support both 4-byte and 8-byte note
alignments in 64-bit objects.  To handle PT_NOTE segments with
incorrect alignment, which may lead to an infinite loop, if segment
alignment is less than 4, we treate alignment as 4 bytes since some
note segments have 0 or 1 byte alignment.

	[BZ #22370]
	* elf/dl-hwcaps.c (ROUND): Removed.
	(_dl_important_hwcaps): Replace ROUND with ELF_NOTE_DESC_OFFSET
	and ELF_NOTE_NEXT_OFFSET.
	* elf/dl-load.c (ROUND): Removed.
	(open_verify): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* elf/readelflib.c (ROUND): Removed.
	(process_elf_file): Replace ROUND with ELF_NOTE_NEXT_OFFSET.
	* include/elf.h [!_ISOMAC]: Include <libc-pointer-arith.h>.
	[!_ISOMAC] (ELF_NOTE_DESC_OFFSET): New.
	[!_ISOMAC] (ELF_NOTE_NEXT_OFFSET): Likewise.
---
 elf/dl-hwcaps.c  | 24 ++++++++++++++++++------
 elf/dl-load.c    | 19 +++++++++++++++----
 elf/readelflib.c | 19 +++++++++++++++----
 include/elf.h    | 16 ++++++++++++++--
 4 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/elf/dl-hwcaps.c b/elf/dl-hwcaps.c
index 92f2eb45ce..27ba6122ff 100644
--- a/elf/dl-hwcaps.c
+++ b/elf/dl-hwcaps.c
@@ -67,6 +67,18 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	  {
 	    const ElfW(Addr) start = (phdr[i].p_vaddr
 				      + GLRO(dl_sysinfo_map)->l_addr);
+	    /* NB: Some PT_NOTE segment may have alignment value of 0
+	       or 1.  gABI specifies that PT_NOTE segments should be
+	       aligned to 4 bytes in 32-bit objects and to 8 bytes in
+	       64-bit objects.  As a Linux extension, we also support
+	       4 byte alignment in 64-bit objects.  If p_align is less
+	       than 4, we treate alignment as 4 bytes since some note
+	       segments have 0 or 1 byte alignment.   */
+	    ElfW(Addr) align = phdr[i].p_align;
+	    if (align < 4)
+	      align = 4;
+	    else if (align != 4 && align != 8)
+	      continue;
 	    /* The standard ELF note layout is exactly as the anonymous struct.
 	       The next element is a variable length vendor name of length
 	       VENDORLEN (with a real length rounded to ElfW(Word)), followed
@@ -80,7 +92,6 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 	    } *note = (const void *) start;
 	    while ((ElfW(Addr)) (note + 1) - start < phdr[i].p_memsz)
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
 		/* The layout of the type 2, vendor "GNU" note is as follows:
 		   .long <Number of capabilities enabled by this note>
 		   .long <Capabilities mask> (as mask >> _DL_FIRST_EXTRA).
@@ -91,17 +102,18 @@ _dl_important_hwcaps (const char *platform, size_t platform_len, size_t *sz,
 		    && !memcmp ((note + 1), "GNU", sizeof "GNU")
 		    && note->datalen > 2 * sizeof (ElfW(Word)) + 2)
 		  {
-		    const ElfW(Word) *p = ((const void *) (note + 1)
-					   + ROUND (sizeof "GNU"));
+		    const ElfW(Word) *p
+		      = ((const void *) note
+			 + ELF_NOTE_DESC_OFFSET (sizeof "GNU", align));
 		    cnt += *p++;
 		    ++p;	/* Skip mask word.  */
 		    dsocaps = (const char *) p; /* Pseudo-string "<b>name"  */
 		    dsocapslen = note->datalen - sizeof *p * 2;
 		    break;
 		  }
-		note = ((const void *) (note + 1)
-			+ ROUND (note->vendorlen) + ROUND (note->datalen));
-#undef ROUND
+		note = ((const void *) note
+			+ ELF_NOTE_NEXT_OFFSET (note->vendorlen,
+						note->datalen, align));
 	      }
 	    if (dsocaps != NULL)
 	      break;
diff --git a/elf/dl-load.c b/elf/dl-load.c
index 1220183ce2..10d859bd35 100644
--- a/elf/dl-load.c
+++ b/elf/dl-load.c
@@ -1683,6 +1683,18 @@ open_verify (const char *name, int fd,
 	if (ph->p_type == PT_NOTE && ph->p_filesz >= 32 && ph->p_align >= 4)
 	  {
 	    ElfW(Addr) size = ph->p_filesz;
+	    /* NB: Some PT_NOTE segment may have alignment value of 0
+	       or 1.  gABI specifies that PT_NOTE segments should be
+	       aligned to 4 bytes in 32-bit objects and to 8 bytes in
+	       64-bit objects.  As a Linux extension, we also support
+	       4 byte alignment in 64-bit objects.  If p_align is less
+	       than 4, we treate alignment as 4 bytes since some note
+	       segments have 0 or 1 byte alignment.   */
+	    ElfW(Addr) align = ph->p_align;
+	    if (align < 4)
+	      align = 4;
+	    else if (align != 4 && align != 8)
+	      continue;
 
 	    if (ph->p_offset + size <= (size_t) fbp->len)
 	      abi_note = (void *) (fbp->buf + ph->p_offset);
@@ -1696,10 +1708,9 @@ open_verify (const char *name, int fd,
 
 	    while (memcmp (abi_note, &expected_note, sizeof (expected_note)))
 	      {
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-				       + ROUND (abi_note[0])
-				       + ROUND (abi_note[1]);
+		ElfW(Addr) note_size
+		  = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					  align);
 
 		if (size - 32 < note_size)
 		  {
diff --git a/elf/readelflib.c b/elf/readelflib.c
index 9ad56dcc34..3a303fff5f 100644
--- a/elf/readelflib.c
+++ b/elf/readelflib.c
@@ -131,15 +131,26 @@ process_elf_file (const char *file_name, const char *lib, int *flag,
 	      ElfW(Word) *abi_note = (ElfW(Word) *) (file_contents
 						     + segment->p_offset);
 	      ElfW(Addr) size = segment->p_filesz;
+	      /* NB: Some PT_NOTE segment may have alignment value of 0
+		 or 1.  gABI specifies that PT_NOTE segments should be
+		 aligned to 4 bytes in 32-bit objects and to 8 bytes in
+		 64-bit objects.  As a Linux extension, we also support
+		 4 byte alignment in 64-bit objects.  If p_align is less
+		 than 4, we treate alignment as 4 bytes since some note
+		 segments have 0 or 1 byte alignment.   */
+	      ElfW(Addr) align = segment->p_align;
+	      if (align < 4)
+		align = 4;
+	      else if (align != 4 && align != 8)
+		continue;
 
 	      while (abi_note [0] != 4 || abi_note [1] != 16
 		     || abi_note [2] != 1
 		     || memcmp (abi_note + 3, "GNU", 4) != 0)
 		{
-#define ROUND(len) (((len) + sizeof (ElfW(Word)) - 1) & -sizeof (ElfW(Word)))
-		  ElfW(Addr) note_size = 3 * sizeof (ElfW(Word))
-					 + ROUND (abi_note[0])
-					 + ROUND (abi_note[1]);
+		  ElfW(Addr) note_size
+		    = ELF_NOTE_NEXT_OFFSET (abi_note[0], abi_note[1],
+					    align);
 
 		  if (size - 32 < note_size || note_size == 0)
 		    {
diff --git a/include/elf.h b/include/elf.h
index f06a33f256..ab76aafb1e 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -1,7 +1,19 @@
 #ifndef _ELF_H
 #include <elf/elf.h>
 
-# ifndef _ISOMAC
+#ifndef _ISOMAC
+
+# include <libc-pointer-arith.h>
+
+/* Compute the offset of the note descriptor from size of note entry's
+   owner string and note alignment.  */
+# define ELF_NOTE_DESC_OFFSET(namesz, align) \
+  ALIGN_UP (sizeof (ElfW(Nhdr)) + (namesz), (align))
+
+/* Compute the offset of the next note entry from size of note entry's
+   owner string, size of the note descriptor and note alignment.  */
+# define ELF_NOTE_NEXT_OFFSET(namesz, descsz, align) \
+  ALIGN_UP (ELF_NOTE_DESC_OFFSET ((namesz), (align)) + (descsz), (align))
 
 /* Some information which is not meant for the public and therefore not
    in <elf.h>.  */
@@ -13,5 +25,5 @@
    (DF_1_NOW | DF_1_NODELETE | DF_1_INITFIRST | DF_1_NOOPEN \
     | DF_1_ORIGIN | DF_1_NODEFLIB)
 
-# endif /* !_ISOMAC */
+#endif /* !_ISOMAC */
 #endif /* elf.h */
-- 
2.14.3