V2 [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

Message ID CAMe9rOoXP7fz+XaM5hFnzDa=qSzz=8r3HkoddaTscrh7HUt03g@mail.gmail.com
State New
Headers show
Series
  • V2 [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]
Related show

Commit Message

H.J. Lu Aug. 26, 2018, 2:52 p.m.
On Tue, Aug 14, 2018 at 10:32 AM, H.J. Lu <hongjiu.lu@intel.com> wrote:
> The older linker treats .note.gnu.property section as a generic note

> section and just concatenates all .note.gnu.property sections from the

> inputs to the output.  When the older linker is used to created the

> program on CET-enabled OS, the generated output has .note.gnu.property

> section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> enable bits are set even if the program isn't CET enabled.  Such program

> will crash on CET-enabled machines.  This patch updates the note parser:

>

> 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>

> OK for master?

>

> H.J.

> ---

>         [BZ #23509]

>         * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

>         note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

>         Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

>         Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>         * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

>         lc_unknown.


Here is the updated patch which passed CET smoke test:

https://github.com/hjl-tools/cet-smoke-test

-- 
H.J.

Comments

Florian Weimer Sept. 20, 2018, 8:54 a.m. | #1
* H. J. Lu:

> From e1c6cdeb7530c37ddb6688b17435c1c9a373a09c Mon Sep 17 00:00:00 2001

> From: "H.J. Lu" <hjl.tools@gmail.com>

> Date: Fri, 10 Aug 2018 18:43:22 -0700

> Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

>

> The older linker treats .note.gnu.property section as a generic note

> section and just concatenates all .note.gnu.property sections from the

> inputs to the output.  When the older linker is used to created the

> program on CET-enabled OS, the generated output has .note.gnu.property

> section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> enable bits are set even if the program isn't CET enabled.  Such program

> will crash on CET-enabled machines.  This patch updates the note parser:

>

> 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>

> 	[BZ #23509]

> 	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

> 	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> 	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

> 	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> 	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

> 	lc_unknown.


As far as I can tell, this patch catches only the case where a single
PT_NOTE segment contains multiple notes.  If there are multiple PT_NOTE
segments with notes, and each one contains a single note, the patch will
still enable CET.

Is my summary accurate?  Do you think it is safe to treat PT_NOTE
segments in isolation?

If yes, this should be mentioned in the commit message.

This seems to be an unrelated change?

+	      /* Property type must be in ascending order.  */
+	      if (type < last_type)
+		return;

Thanks,
Florian
H.J. Lu Sept. 20, 2018, 12:36 p.m. | #2
On Thu, Sep 20, 2018 at 1:54 AM, Florian Weimer <fweimer@redhat.com> wrote:
> * H. J. Lu:

>

>> From e1c6cdeb7530c37ddb6688b17435c1c9a373a09c Mon Sep 17 00:00:00 2001

>> From: "H.J. Lu" <hjl.tools@gmail.com>

>> Date: Fri, 10 Aug 2018 18:43:22 -0700

>> Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

>>

>> The older linker treats .note.gnu.property section as a generic note

>> section and just concatenates all .note.gnu.property sections from the

>> inputs to the output.  When the older linker is used to created the

>> program on CET-enabled OS, the generated output has .note.gnu.property

>> section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

>> enable bits are set even if the program isn't CET enabled.  Such program

>> will crash on CET-enabled machines.  This patch updates the note parser:

>>

>> 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

>> 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>>

>>       [BZ #23509]

>>       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

>>       note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

>>       Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

>>       Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>>       * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

>>       lc_unknown.

>

> As far as I can tell, this patch catches only the case where a single

> PT_NOTE segment contains multiple notes.  If there are multiple PT_NOTE

> segments with notes, and each one contains a single note, the patch will

> still enable CET.


I believe this is covered by CET smoke test:

https://github.com/hjl-tools/cet-smoke-test

My patch should work in all cases.  If it misses some cases, please add it
to master branch at

https://github.com/hjl-tools/cet-smoke-test

under the "note" directory.

> Is my summary accurate?  Do you think it is safe to treat PT_NOTE

> segments in isolation?


What do you mean by that?  The only assumption I made is that
property note must follow the spec.  Any violation makes it invalid.

> If yes, this should be mentioned in the commit message.

>

> This seems to be an unrelated change?

>

> +             /* Property type must be in ascending order.  */

> +             if (type < last_type)

> +               return;

>


This is the part of property spec.  If properties aren't properly
sorted, it is invalid.

-- 
H.J.
Florian Weimer Sept. 21, 2018, 2:49 p.m. | #3
* H. J. Lu:

>> As far as I can tell, this patch catches only the case where a single

>> PT_NOTE segment contains multiple notes.  If there are multiple PT_NOTE

>> segments with notes, and each one contains a single note, the patch will

>> still enable CET.

>

> I believe this is covered by CET smoke test:

>

> https://github.com/hjl-tools/cet-smoke-test

>

> My patch should work in all cases.  If it misses some cases, please add it

> to master branch at

>

> https://github.com/hjl-tools/cet-smoke-test

>

> under the "note" directory.

>

>> Is my summary accurate?  Do you think it is safe to treat PT_NOTE

>> segments in isolation?

>

> What do you mean by that?  The only assumption I made is that

> property note must follow the spec.  Any violation makes it invalid.


I'm attaching a patched executable which has two PT_NOTE segments with
separately valid GNU property notes (if I did my patching correctly).

I think your code will still accept it.  It is conceivable that a linker
which is not aware of the required merging for property notes would
create such segments, I think.

>> If yes, this should be mentioned in the commit message.

>>

>> This seems to be an unrelated change?

>>

>> +             /* Property type must be in ascending order.  */

>> +             if (type < last_type)

>> +               return;

>>

>

> This is the part of property spec.  If properties aren't properly

> sorted, it is invalid.


Okay, that part makes sense because it's internal to the notes AFAICS.
And note merging without GNU property note awareness will likely violate
this constraint.  Maybe you could add a comment to this effect?

Thanks,
Florian
H.J. Lu Sept. 21, 2018, 3:31 p.m. | #4
On Fri, Sep 21, 2018 at 7:49 AM, Florian Weimer <fweimer@redhat.com> wrote:
> * H. J. Lu:

>

>>> As far as I can tell, this patch catches only the case where a single

>>> PT_NOTE segment contains multiple notes.  If there are multiple PT_NOTE

>>> segments with notes, and each one contains a single note, the patch will

>>> still enable CET.

>>

>> I believe this is covered by CET smoke test:

>>

>> https://github.com/hjl-tools/cet-smoke-test

>>

>> My patch should work in all cases.  If it misses some cases, please add it

>> to master branch at

>>

>> https://github.com/hjl-tools/cet-smoke-test

>>

>> under the "note" directory.

>>

>>> Is my summary accurate?  Do you think it is safe to treat PT_NOTE

>>> segments in isolation?

>>

>> What do you mean by that?  The only assumption I made is that

>> property note must follow the spec.  Any violation makes it invalid.

>

> I'm attaching a patched executable which has two PT_NOTE segments with

> separately valid GNU property notes (if I did my patching correctly).

>

> I think your code will still accept it.  It is conceivable that a linker

> which is not aware of the required merging for property notes would

> create such segments, I think.


My goal is to allow older linkers which don't understand property notes
to generate working executables on CET enabled OS.  It isn't tended to
handle all hand-crafted binaries.  The worst case is that such binaries
won't run on CET enabled OS.  I don't believe it is an issue.  It may even
be a bonus.

Do you have a testcase which may be generated by an old linker on
CET enabled OS?

>>> If yes, this should be mentioned in the commit message.

>>>

>>> This seems to be an unrelated change?

>>>

>>> +             /* Property type must be in ascending order.  */

>>> +             if (type < last_type)

>>> +               return;

>>>

>>

>> This is the part of property spec.  If properties aren't properly

>> sorted, it is invalid.

>

> Okay, that part makes sense because it's internal to the notes AFAICS.

> And note merging without GNU property note awareness will likely violate

> this constraint.  Maybe you could add a comment to this effect?


I updated comments to

              /* Property types must be sorted in ascending order.
                 Ignore property note with the wrong type order.  */
              if (type < last_type)
                return;

Thanks.

-- 
H.J.
Florian Weimer Sept. 22, 2018, 11:07 a.m. | #5
* H. J. Lu:

> My goal is to allow older linkers which don't understand property notes

> to generate working executables on CET enabled OS.  It isn't tended to

> handle all hand-crafted binaries.  The worst case is that such binaries

> won't run on CET enabled OS.  I don't believe it is an issue.  It may even

> be a bonus.

>

> Do you have a testcase which may be generated by an old linker on

> CET enabled OS?


My position is that ELF is an open format and that we don't really know
what kind of link editors are out there and are used in practice.

Other views are certainly possible.  I mainly wanted to understand the
rationale behind the code.  If you do not expect multiple PT_NOTE
segments with properly aligned property notes, I think that's okay.  We
can adjust the dynamic linker again if such binaries show up in the
future.

>> Okay, that part makes sense because it's internal to the notes AFAICS.

>> And note merging without GNU property note awareness will likely violate

>> this constraint.  Maybe you could add a comment to this effect?

>

> I updated comments to

>

>               /* Property types must be sorted in ascending order.

>                  Ignore property note with the wrong type order.  */

>               if (type < last_type)

>                 return;


Thanks.

Florian
Florian Weimer Nov. 7, 2018, 6:27 p.m. | #6
* Florian Weimer:

>>> If yes, this should be mentioned in the commit message.

>>>

>>> This seems to be an unrelated change?

>>>

>>> +             /* Property type must be in ascending order.  */

>>> +             if (type < last_type)

>>> +               return;

>>>

>>

>> This is the part of property spec.  If properties aren't properly

>> sorted, it is invalid.

>

> Okay, that part makes sense because it's internal to the notes AFAICS.

> And note merging without GNU property note awareness will likely violate

> this constraint.  Maybe you could add a comment to this effect?


Hmm, isn't the point that this is the internal type in the note (not the
tag), and there isn't any *note* merging, only note *section* merging in
an old linker.  The latter leaves the note boundaries intact, so the
internal layout will be unmodified.

Or am I confused about this?

Thanks,
Florian
Florian Weimer Nov. 7, 2018, 6:32 p.m. | #7
* H. J. Lu:

> The older linker treats .note.gnu.property section as a generic note

> section and just concatenates all .note.gnu.property sections from the

> inputs to the output.  When the older linker is used to created the

> program on CET-enabled OS, the generated output has .note.gnu.property

> section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> enable bits are set even if the program isn't CET enabled.  Such program

> will crash on CET-enabled machines.  This patch updates the note parser:

>

> 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>

> 	[BZ #23509]

> 	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

> 	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> 	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

> 	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> 	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

> 	lc_unknown.


I would like to move this forward.

I think the code is okay, as long as we are confident that older linkers
will not produce multiple PT_NOTE segments, each containing one
NT_GNU_PROPERTY_TYPE_0 note.  Maybe this should be mentioned in the
commit message (assuming that you agree that my understanding of the
code is correct).

Thanks,
Florian
H.J. Lu Nov. 7, 2018, 8:24 p.m. | #8
On Wed, Nov 7, 2018 at 10:27 AM Florian Weimer <fweimer@redhat.com> wrote:
>

> * Florian Weimer:

>

> >>> If yes, this should be mentioned in the commit message.

> >>>

> >>> This seems to be an unrelated change?

> >>>

> >>> +             /* Property type must be in ascending order.  */

> >>> +             if (type < last_type)

> >>> +               return;

> >>>

> >>

> >> This is the part of property spec.  If properties aren't properly

> >> sorted, it is invalid.

> >

> > Okay, that part makes sense because it's internal to the notes AFAICS.

> > And note merging without GNU property note awareness will likely violate

> > this constraint.  Maybe you could add a comment to this effect?

>

> Hmm, isn't the point that this is the internal type in the note (not the

> tag), and there isn't any *note* merging, only note *section* merging in

> an old linker.  The latter leaves the note boundaries intact, so the

> internal layout will be unmodified.

>


Older linkers will concatenate all input GNU property note sections
into a single output GNU property note section without merging them.

-- 
H.J.
H.J. Lu Nov. 7, 2018, 8:34 p.m. | #9
On Wed, Nov 7, 2018 at 10:32 AM Florian Weimer <fweimer@redhat.com> wrote:
>

> * H. J. Lu:

>

> > The older linker treats .note.gnu.property section as a generic note

> > section and just concatenates all .note.gnu.property sections from the

> > inputs to the output.  When the older linker is used to created the

> > program on CET-enabled OS, the generated output has .note.gnu.property

> > section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> > enable bits are set even if the program isn't CET enabled.  Such program

> > will crash on CET-enabled machines.  This patch updates the note parser:

> >

> > 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> > 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> >

> >       [BZ #23509]

> >       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

> >       note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> >       Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

> >       Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> >       * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

> >       lc_unknown.

>

> I would like to move this forward.

>

> I think the code is okay, as long as we are confident that older linkers

> will not produce multiple PT_NOTE segments, each containing one

> NT_GNU_PROPERTY_TYPE_0 note.  Maybe this should be mentioned in the

> commit message (assuming that you agree that my understanding of the

> code is correct).

>


Like this?

-- 
H.J.
From b3d82373cb499ceaceafe10b07dd1492c18484c2 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 10 Aug 2018 18:43:22 -0700
Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

The older linker treats .note.gnu.property section as a generic note
section and just concatenates all input .note.gnu.property sections
into a single output .note.gnu.property section without merging them.
When the older linker is used to created the program on CET-enabled OS,
the linker output has a single .note.gnu.property section with multiple
NT_GNU_PROPERTY_TYPE_0 notes, some of which have IBT and SHSTK enable
bits set even if the program isn't CET enabled.  Such programs will
crash on CET-enabled machines.  This patch updates the note parser:

1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

	[BZ #23509]
	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip
	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.
	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.
	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add
	lc_unknown.
---
 sysdeps/x86/dl-prop.h  | 51 +++++++++++++++++++++++++++++++++---------
 sysdeps/x86/link_map.h |  9 ++++----
 2 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 26c3131ac5..9ab890d12b 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -49,6 +49,10 @@ _dl_process_cet_property_note (struct link_map *l,
 			      const ElfW(Addr) align)
 {
 #if CET_ENABLED
+  /* Skip if we have seen a NT_GNU_PROPERTY_TYPE_0 note before.  */
+  if (l->l_cet != lc_unknown)
+    return;
+
   /* The NT_GNU_PROPERTY_TYPE_0 note must be aliged to 4 bytes in
      32-bit objects and to 8 bytes in 64-bit objects.  Skip notes
      with incorrect alignment.  */
@@ -57,6 +61,9 @@ _dl_process_cet_property_note (struct link_map *l,
 
   const ElfW(Addr) start = (ElfW(Addr)) note;
 
+  unsigned int feature_1 = 0;
+  unsigned int last_type = 0;
+
   while ((ElfW(Addr)) (note + 1) - start < size)
     {
       /* Find the NT_GNU_PROPERTY_TYPE_0 note.  */
@@ -64,10 +71,18 @@ _dl_process_cet_property_note (struct link_map *l,
 	  && note->n_type == NT_GNU_PROPERTY_TYPE_0
 	  && memcmp (note + 1, "GNU", 4) == 0)
 	{
+	  /* Stop if we see more than one GNU property note which may
+	     be generated by the older linker.  */
+	  if (l->l_cet != lc_unknown)
+	    return;
+
+	  /* Check CET status now.  */
+	  l->l_cet = lc_none;
+
 	  /* Check for invalid property.  */
 	  if (note->n_descsz < 8
 	      || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
-	    break;
+	    return;
 
 	  /* Start and end of property array.  */
 	  unsigned char *ptr = (unsigned char *) (note + 1) + 4;
@@ -78,9 +93,15 @@ _dl_process_cet_property_note (struct link_map *l,
 	      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)
-		break;
+		return;
+
+	      last_type = type;
 
 	      if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
 		{
@@ -89,14 +110,18 @@ _dl_process_cet_property_note (struct link_map *l,
 		     we stop the search regardless if its size is correct
 		     or not.  There is no point to continue if this note
 		     is ill-formed.  */
-		  if (datasz == 4)
-		    {
-		      unsigned int feature_1 = *(unsigned int *) ptr;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
-			l->l_cet |= lc_ibt;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
-			l->l_cet |= lc_shstk;
-		    }
+		  if (datasz != 4)
+		    return;
+
+		  feature_1 = *(unsigned int *) ptr;
+
+		  /* Keep searching for the next GNU property note
+		     generated by the older linker.  */
+		  break;
+		}
+	      else if (type > GNU_PROPERTY_X86_FEATURE_1_AND)
+		{
+		  /* Stop since property type is in ascending order.  */
 		  return;
 		}
 
@@ -112,6 +137,12 @@ _dl_process_cet_property_note (struct link_map *l,
 	      + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
 				      align));
     }
+
+  /* We get here only if there is one or no GNU property note.  */
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
+    l->l_cet |= lc_ibt;
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
+    l->l_cet |= lc_shstk;
 #endif
 }
 
diff --git a/sysdeps/x86/link_map.h b/sysdeps/x86/link_map.h
index ef1206a9d2..9367ed0889 100644
--- a/sysdeps/x86/link_map.h
+++ b/sysdeps/x86/link_map.h
@@ -19,8 +19,9 @@
 /* If this object is enabled with CET.  */
 enum
   {
-    lc_none = 0,			 /* Not enabled with CET.  */
-    lc_ibt = 1 << 0,			 /* Enabled with IBT.  */
-    lc_shstk = 1 << 1,			 /* Enabled with STSHK.  */
+    lc_unknown = 0,			 /* Unknown CET status.  */
+    lc_none = 1 << 0,			 /* Not enabled with CET.  */
+    lc_ibt = 1 << 1,			 /* Enabled with IBT.  */
+    lc_shstk = 1 << 2,			 /* Enabled with STSHK.  */
     lc_ibt_and_shstk = lc_ibt | lc_shstk /* Enabled with both.  */
-  } l_cet:2;
+  } l_cet:3;
Florian Weimer Nov. 8, 2018, 10:52 a.m. | #10
* H. J. Lu:

> On Wed, Nov 7, 2018 at 10:32 AM Florian Weimer <fweimer@redhat.com> wrote:

>>

>> * H. J. Lu:

>>

>> > The older linker treats .note.gnu.property section as a generic note

>> > section and just concatenates all .note.gnu.property sections from the

>> > inputs to the output.  When the older linker is used to created the

>> > program on CET-enabled OS, the generated output has .note.gnu.property

>> > section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

>> > enable bits are set even if the program isn't CET enabled.  Such program

>> > will crash on CET-enabled machines.  This patch updates the note parser:

>> >

>> > 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

>> > 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>> >

>> >       [BZ #23509]

>> >       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

>> >       note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

>> >       Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

>> >       Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

>> >       * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

>> >       lc_unknown.

>>

>> I would like to move this forward.

>>

>> I think the code is okay, as long as we are confident that older linkers

>> will not produce multiple PT_NOTE segments, each containing one

>> NT_GNU_PROPERTY_TYPE_0 note.  Maybe this should be mentioned in the

>> commit message (assuming that you agree that my understanding of the

>> code is correct).

>>

>

> Like this?


Sorry, let me rephrase:

The commit message talks about sections, not segments.  The code deals
with PT_NOTE segments, obviously.

Old linkers produce multiple PT_NOTE segments for the *same* alignment
if they process multiple note sections with differing alignment in a
certain order.  (It does not happen always.)

Based on reading the code, I think that if the first PT_NOTE segment it
encounters contains one NT_GNU_PROPERTY_TYPE_0 note that enables CET, it
will enable CET even if the loader produced multiple PT_NOTE segments,
and there is another (unmerged) NT_GNU_PROPERTY_TYPE_0 note in a
subsequent PT_NOTE segment.  This means that the current code can still
incorrectly enable CET in cases where an old linker produced a multitude
of PT_NOTE sections.

Based on the patch and the discussion so far it is unclear to me whether
you have analyzed the issue and concluded that split PT_NOTE segments of
the same alignment (as generated by old linkers in some cases) are not
relevant, or if it is an accidental omission in the patch.  The
explaination in the patch itself does not help because of the
section/segment discrepancy I mentioned first.

Thanks,
Florian
H.J. Lu Nov. 8, 2018, 1:15 p.m. | #11
On Thu, Nov 8, 2018 at 2:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>

> * H. J. Lu:

>

> > On Wed, Nov 7, 2018 at 10:32 AM Florian Weimer <fweimer@redhat.com> wrote:

> >>

> >> * H. J. Lu:

> >>

> >> > The older linker treats .note.gnu.property section as a generic note

> >> > section and just concatenates all .note.gnu.property sections from the

> >> > inputs to the output.  When the older linker is used to created the

> >> > program on CET-enabled OS, the generated output has .note.gnu.property

> >> > section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> >> > enable bits are set even if the program isn't CET enabled.  Such program

> >> > will crash on CET-enabled machines.  This patch updates the note parser:

> >> >

> >> > 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> >> > 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> >> >

> >> >       [BZ #23509]

> >> >       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

> >> >       note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> >> >       Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

> >> >       Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> >> >       * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

> >> >       lc_unknown.

> >>

> >> I would like to move this forward.

> >>

> >> I think the code is okay, as long as we are confident that older linkers

> >> will not produce multiple PT_NOTE segments, each containing one

> >> NT_GNU_PROPERTY_TYPE_0 note.  Maybe this should be mentioned in the

> >> commit message (assuming that you agree that my understanding of the

> >> code is correct).

> >>

> >

> > Like this?

>

> Sorry, let me rephrase:

>

> The commit message talks about sections, not segments.  The code deals

> with PT_NOTE segments, obviously.


See my reply before.

> Old linkers produce multiple PT_NOTE segments for the *same* alignment

> if they process multiple note sections with differing alignment in a

> certain order.  (It does not happen always.)


True.

> Based on reading the code, I think that if the first PT_NOTE segment it

> encounters contains one NT_GNU_PROPERTY_TYPE_0 note that enables CET, it

> will enable CET even if the loader produced multiple PT_NOTE segments,

> and there is another (unmerged) NT_GNU_PROPERTY_TYPE_0 note in a

> subsequent PT_NOTE segment.  This means that the current code can still

> incorrectly enable CET in cases where an old linker produced a multitude

> of PT_NOTE sections.


Linkers group input note sections with the same name into a single output note
section with the same name.  One output note section is placed in one PT_NOTE
segment.  There won't be "another (unmerged) NT_GNU_PROPERTY_TYPE_0
note in a subsequent PT_NOTE segment."

> Based on the patch and the discussion so far it is unclear to me whether

> you have analyzed the issue and concluded that split PT_NOTE segments of

> the same alignment (as generated by old linkers in some cases) are not

> relevant, or if it is an accidental omission in the patch.  The

> explaination in the patch itself does not help because of the

> section/segment discrepancy I mentioned first.

>



-- 
H.J.
H.J. Lu Nov. 8, 2018, 1:39 p.m. | #12
On Thu, Nov 8, 2018 at 5:15 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Thu, Nov 8, 2018 at 2:52 AM Florian Weimer <fweimer@redhat.com> wrote:

> >

> > * H. J. Lu:

> >

> > > On Wed, Nov 7, 2018 at 10:32 AM Florian Weimer <fweimer@redhat.com> wrote:

> > >>

> > >> * H. J. Lu:

> > >>

> > >> > The older linker treats .note.gnu.property section as a generic note

> > >> > section and just concatenates all .note.gnu.property sections from the

> > >> > inputs to the output.  When the older linker is used to created the

> > >> > program on CET-enabled OS, the generated output has .note.gnu.property

> > >> > section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK

> > >> > enable bits are set even if the program isn't CET enabled.  Such program

> > >> > will crash on CET-enabled machines.  This patch updates the note parser:

> > >> >

> > >> > 1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> > >> > 2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> > >> >

> > >> >       [BZ #23509]

> > >> >       * sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip

> > >> >       note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.

> > >> >       Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.

> > >> >       Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

> > >> >       * sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add

> > >> >       lc_unknown.

> > >>

> > >> I would like to move this forward.

> > >>

> > >> I think the code is okay, as long as we are confident that older linkers

> > >> will not produce multiple PT_NOTE segments, each containing one

> > >> NT_GNU_PROPERTY_TYPE_0 note.  Maybe this should be mentioned in the

> > >> commit message (assuming that you agree that my understanding of the

> > >> code is correct).

> > >>

> > >

> > > Like this?

> >

> > Sorry, let me rephrase:

> >

> > The commit message talks about sections, not segments.  The code deals

> > with PT_NOTE segments, obviously.

>

> See my reply before.

>

> > Old linkers produce multiple PT_NOTE segments for the *same* alignment

> > if they process multiple note sections with differing alignment in a

> > certain order.  (It does not happen always.)

>

> True.

>

> > Based on reading the code, I think that if the first PT_NOTE segment it

> > encounters contains one NT_GNU_PROPERTY_TYPE_0 note that enables CET, it

> > will enable CET even if the loader produced multiple PT_NOTE segments,

> > and there is another (unmerged) NT_GNU_PROPERTY_TYPE_0 note in a

> > subsequent PT_NOTE segment.  This means that the current code can still

> > incorrectly enable CET in cases where an old linker produced a multitude

> > of PT_NOTE sections.

>

> Linkers group input note sections with the same name into a single output note

> section with the same name.  One output note section is placed in one PT_NOTE

> segment.  There won't be "another (unmerged) NT_GNU_PROPERTY_TYPE_0

> note in a subsequent PT_NOTE segment."

>

> > Based on the patch and the discussion so far it is unclear to me whether

> > you have analyzed the issue and concluded that split PT_NOTE segments of

> > the same alignment (as generated by old linkers in some cases) are not

> > relevant, or if it is an accidental omission in the patch.  The

> > explaination in the patch itself does not help because of the

> > section/segment discrepancy I mentioned first.

> >


Here is the updated patch with

Linkers group input note sections with the same name into one output
note section with the same name.  One output note section is placed in
one PT_NOTE segment.  Since new linkers merge input .note.gnu.property
sections into one output .note.gnu.property section, there is only
one NT_GNU_PROPERTY_TYPE_0 note in one PT_NOTE segment with new linkers.
Since older linkers treat input .note.gnu.property section as a generic
note section and just concatenate all input .note.gnu.property sections
into one output .note.gnu.property section without merging them, we may
see multiple NT_GNU_PROPERTY_TYPE_0 notes in one PT_NOTE segment with
older linkers.


-- 
H.J.
From e26a2994f0e856c9de62978717d8768e53a3e083 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 10 Aug 2018 18:43:22 -0700
Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

Linkers group input note sections with the same name into one output
note section with the same name.  One output note section is placed in
one PT_NOTE segment.  Since new linkers merge input .note.gnu.property
sections into one output .note.gnu.property section, there is only
one NT_GNU_PROPERTY_TYPE_0 note in one PT_NOTE segment with new linkers.
Since older linkers treat input .note.gnu.property section as a generic
note section and just concatenate all input .note.gnu.property sections
into one output .note.gnu.property section without merging them, we may
see multiple NT_GNU_PROPERTY_TYPE_0 notes in one PT_NOTE segment with
older linkers.

When an older linker is used to created the program on CET-enabled OS,
the linker output has a single .note.gnu.property section with multiple
NT_GNU_PROPERTY_TYPE_0 notes, some of which have IBT and SHSTK enable
bits set even if the program isn't CET enabled.  Such programs will
crash on CET-enabled machines.  This patch updates the note parser:

1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

	[BZ #23509]
	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip
	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.
	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.
	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add
	lc_unknown.
---
 sysdeps/x86/dl-prop.h  | 51 +++++++++++++++++++++++++++++++++---------
 sysdeps/x86/link_map.h |  9 ++++----
 2 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 26c3131ac5..9ab890d12b 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -49,6 +49,10 @@ _dl_process_cet_property_note (struct link_map *l,
 			      const ElfW(Addr) align)
 {
 #if CET_ENABLED
+  /* Skip if we have seen a NT_GNU_PROPERTY_TYPE_0 note before.  */
+  if (l->l_cet != lc_unknown)
+    return;
+
   /* The NT_GNU_PROPERTY_TYPE_0 note must be aliged to 4 bytes in
      32-bit objects and to 8 bytes in 64-bit objects.  Skip notes
      with incorrect alignment.  */
@@ -57,6 +61,9 @@ _dl_process_cet_property_note (struct link_map *l,
 
   const ElfW(Addr) start = (ElfW(Addr)) note;
 
+  unsigned int feature_1 = 0;
+  unsigned int last_type = 0;
+
   while ((ElfW(Addr)) (note + 1) - start < size)
     {
       /* Find the NT_GNU_PROPERTY_TYPE_0 note.  */
@@ -64,10 +71,18 @@ _dl_process_cet_property_note (struct link_map *l,
 	  && note->n_type == NT_GNU_PROPERTY_TYPE_0
 	  && memcmp (note + 1, "GNU", 4) == 0)
 	{
+	  /* Stop if we see more than one GNU property note which may
+	     be generated by the older linker.  */
+	  if (l->l_cet != lc_unknown)
+	    return;
+
+	  /* Check CET status now.  */
+	  l->l_cet = lc_none;
+
 	  /* Check for invalid property.  */
 	  if (note->n_descsz < 8
 	      || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
-	    break;
+	    return;
 
 	  /* Start and end of property array.  */
 	  unsigned char *ptr = (unsigned char *) (note + 1) + 4;
@@ -78,9 +93,15 @@ _dl_process_cet_property_note (struct link_map *l,
 	      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)
-		break;
+		return;
+
+	      last_type = type;
 
 	      if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
 		{
@@ -89,14 +110,18 @@ _dl_process_cet_property_note (struct link_map *l,
 		     we stop the search regardless if its size is correct
 		     or not.  There is no point to continue if this note
 		     is ill-formed.  */
-		  if (datasz == 4)
-		    {
-		      unsigned int feature_1 = *(unsigned int *) ptr;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
-			l->l_cet |= lc_ibt;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
-			l->l_cet |= lc_shstk;
-		    }
+		  if (datasz != 4)
+		    return;
+
+		  feature_1 = *(unsigned int *) ptr;
+
+		  /* Keep searching for the next GNU property note
+		     generated by the older linker.  */
+		  break;
+		}
+	      else if (type > GNU_PROPERTY_X86_FEATURE_1_AND)
+		{
+		  /* Stop since property type is in ascending order.  */
 		  return;
 		}
 
@@ -112,6 +137,12 @@ _dl_process_cet_property_note (struct link_map *l,
 	      + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
 				      align));
     }
+
+  /* We get here only if there is one or no GNU property note.  */
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
+    l->l_cet |= lc_ibt;
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
+    l->l_cet |= lc_shstk;
 #endif
 }
 
diff --git a/sysdeps/x86/link_map.h b/sysdeps/x86/link_map.h
index ef1206a9d2..9367ed0889 100644
--- a/sysdeps/x86/link_map.h
+++ b/sysdeps/x86/link_map.h
@@ -19,8 +19,9 @@
 /* If this object is enabled with CET.  */
 enum
   {
-    lc_none = 0,			 /* Not enabled with CET.  */
-    lc_ibt = 1 << 0,			 /* Enabled with IBT.  */
-    lc_shstk = 1 << 1,			 /* Enabled with STSHK.  */
+    lc_unknown = 0,			 /* Unknown CET status.  */
+    lc_none = 1 << 0,			 /* Not enabled with CET.  */
+    lc_ibt = 1 << 1,			 /* Enabled with IBT.  */
+    lc_shstk = 1 << 2,			 /* Enabled with STSHK.  */
     lc_ibt_and_shstk = lc_ibt | lc_shstk /* Enabled with both.  */
-  } l_cet:2;
+  } l_cet:3;
Florian Weimer Nov. 8, 2018, 5:41 p.m. | #13
* H. J. Lu:

> Here is the updated patch with

>

> Linkers group input note sections with the same name into one output

> note section with the same name.  One output note section is placed in

> one PT_NOTE segment.  Since new linkers merge input .note.gnu.property

> sections into one output .note.gnu.property section, there is only

> one NT_GNU_PROPERTY_TYPE_0 note in one PT_NOTE segment with new linkers.

> Since older linkers treat input .note.gnu.property section as a generic

> note section and just concatenate all input .note.gnu.property sections

> into one output .note.gnu.property section without merging them, we may

> see multiple NT_GNU_PROPERTY_TYPE_0 notes in one PT_NOTE segment with

> older linkers.


Thanks.  This hypothesis regarding older linkers seems reasonable, and
the explanation is very clear now.  The code looks okay to me.

Would you please backport this to the 2.28 release branch as well?

Thanks,
Florian
H.J. Lu Nov. 8, 2018, 6:57 p.m. | #14
On Thu, Nov 8, 2018 at 9:41 AM Florian Weimer <fweimer@redhat.com> wrote:
>

> * H. J. Lu:

>

> > Here is the updated patch with

> >

> > Linkers group input note sections with the same name into one output

> > note section with the same name.  One output note section is placed in

> > one PT_NOTE segment.  Since new linkers merge input .note.gnu.property

> > sections into one output .note.gnu.property section, there is only

> > one NT_GNU_PROPERTY_TYPE_0 note in one PT_NOTE segment with new linkers.

> > Since older linkers treat input .note.gnu.property section as a generic

> > note section and just concatenate all input .note.gnu.property sections

> > into one output .note.gnu.property section without merging them, we may

> > see multiple NT_GNU_PROPERTY_TYPE_0 notes in one PT_NOTE segment with

> > older linkers.

>

> Thanks.  This hypothesis regarding older linkers seems reasonable, and

> the explanation is very clear now.  The code looks okay to me.


I checked it into master branch.

> Would you please backport this to the 2.28 release branch as well?

>


I am backporting this patch to 2.28 branch.

-- 
H.J.
From 3e8d8dd5afba18a847ff7a80f473336f777cc329 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Thu, 8 Nov 2018 10:06:58 -0800
Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

Linkers group input note sections with the same name into one output
note section with the same name.  One output note section is placed in
one PT_NOTE segment.  Since new linkers merge input .note.gnu.property
sections into one output .note.gnu.property section, there is only
one NT_GNU_PROPERTY_TYPE_0 note in one PT_NOTE segment with new linkers.
Since older linkers treat input .note.gnu.property section as a generic
note section and just concatenate all input .note.gnu.property sections
into one output .note.gnu.property section without merging them, we may
see multiple NT_GNU_PROPERTY_TYPE_0 notes in one PT_NOTE segment with
older linkers.

When an older linker is used to created the program on CET-enabled OS,
the linker output has a single .note.gnu.property section with multiple
NT_GNU_PROPERTY_TYPE_0 notes, some of which have IBT and SHSTK enable
bits set even if the program isn't CET enabled.  Such programs will
crash on CET-enabled machines.  This patch updates the note parser:

1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

	[BZ #23509]
	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip
	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.
	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.
	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add
	lc_unknown.

(cherry picked from commit d524fa6c35e675eedbd8fe6cdf4db0b49c658026)
---
 ChangeLog              | 10 +++++++++
 NEWS                   |  1 +
 sysdeps/x86/dl-prop.h  | 51 +++++++++++++++++++++++++++++++++---------
 sysdeps/x86/link_map.h |  9 ++++----
 4 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index db4ac3b76a..86bdf17989 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2018-11-08  H.J. Lu  <hongjiu.lu@intel.com>
+
+	[BZ #23509]
+	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip
+	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
+	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.
+	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.
+	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add
+	lc_unknown.
+
 2018-11-05  Andreas Schwab  <schwab@suse.de>
 
 	[BZ #22927]
diff --git a/NEWS b/NEWS
index b85be4a9c1..e5ca5903ec 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,7 @@ The following bugs are resolved with this release:
   [22927] libanl: properly cleanup if first helper thread creation failed
   [23400] stdlib/test-bz22786.c creates temporary files in glibc source tree
   [23497] readdir64@GLIBC_2.1 cannot parse the kernel directory stream
+  [23509] CET enabled glibc is incompatible with the older linker
   [23521] nss_files aliases database file stream leak
   [23538] pthread_cond_broadcast: Fix waiters-after-spinning case
   [23562] signal: Use correct type for si_band in siginfo_t
diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 26c3131ac5..9ab890d12b 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -49,6 +49,10 @@ _dl_process_cet_property_note (struct link_map *l,
 			      const ElfW(Addr) align)
 {
 #if CET_ENABLED
+  /* Skip if we have seen a NT_GNU_PROPERTY_TYPE_0 note before.  */
+  if (l->l_cet != lc_unknown)
+    return;
+
   /* The NT_GNU_PROPERTY_TYPE_0 note must be aliged to 4 bytes in
      32-bit objects and to 8 bytes in 64-bit objects.  Skip notes
      with incorrect alignment.  */
@@ -57,6 +61,9 @@ _dl_process_cet_property_note (struct link_map *l,
 
   const ElfW(Addr) start = (ElfW(Addr)) note;
 
+  unsigned int feature_1 = 0;
+  unsigned int last_type = 0;
+
   while ((ElfW(Addr)) (note + 1) - start < size)
     {
       /* Find the NT_GNU_PROPERTY_TYPE_0 note.  */
@@ -64,10 +71,18 @@ _dl_process_cet_property_note (struct link_map *l,
 	  && note->n_type == NT_GNU_PROPERTY_TYPE_0
 	  && memcmp (note + 1, "GNU", 4) == 0)
 	{
+	  /* Stop if we see more than one GNU property note which may
+	     be generated by the older linker.  */
+	  if (l->l_cet != lc_unknown)
+	    return;
+
+	  /* Check CET status now.  */
+	  l->l_cet = lc_none;
+
 	  /* Check for invalid property.  */
 	  if (note->n_descsz < 8
 	      || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
-	    break;
+	    return;
 
 	  /* Start and end of property array.  */
 	  unsigned char *ptr = (unsigned char *) (note + 1) + 4;
@@ -78,9 +93,15 @@ _dl_process_cet_property_note (struct link_map *l,
 	      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)
-		break;
+		return;
+
+	      last_type = type;
 
 	      if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
 		{
@@ -89,14 +110,18 @@ _dl_process_cet_property_note (struct link_map *l,
 		     we stop the search regardless if its size is correct
 		     or not.  There is no point to continue if this note
 		     is ill-formed.  */
-		  if (datasz == 4)
-		    {
-		      unsigned int feature_1 = *(unsigned int *) ptr;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
-			l->l_cet |= lc_ibt;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
-			l->l_cet |= lc_shstk;
-		    }
+		  if (datasz != 4)
+		    return;
+
+		  feature_1 = *(unsigned int *) ptr;
+
+		  /* Keep searching for the next GNU property note
+		     generated by the older linker.  */
+		  break;
+		}
+	      else if (type > GNU_PROPERTY_X86_FEATURE_1_AND)
+		{
+		  /* Stop since property type is in ascending order.  */
 		  return;
 		}
 
@@ -112,6 +137,12 @@ _dl_process_cet_property_note (struct link_map *l,
 	      + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
 				      align));
     }
+
+  /* We get here only if there is one or no GNU property note.  */
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
+    l->l_cet |= lc_ibt;
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
+    l->l_cet |= lc_shstk;
 #endif
 }
 
diff --git a/sysdeps/x86/link_map.h b/sysdeps/x86/link_map.h
index ef1206a9d2..9367ed0889 100644
--- a/sysdeps/x86/link_map.h
+++ b/sysdeps/x86/link_map.h
@@ -19,8 +19,9 @@
 /* If this object is enabled with CET.  */
 enum
   {
-    lc_none = 0,			 /* Not enabled with CET.  */
-    lc_ibt = 1 << 0,			 /* Enabled with IBT.  */
-    lc_shstk = 1 << 1,			 /* Enabled with STSHK.  */
+    lc_unknown = 0,			 /* Unknown CET status.  */
+    lc_none = 1 << 0,			 /* Not enabled with CET.  */
+    lc_ibt = 1 << 1,			 /* Enabled with IBT.  */
+    lc_shstk = 1 << 2,			 /* Enabled with STSHK.  */
     lc_ibt_and_shstk = lc_ibt | lc_shstk /* Enabled with both.  */
-  } l_cet:2;
+  } l_cet:3;

Patch

From e1c6cdeb7530c37ddb6688b17435c1c9a373a09c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 10 Aug 2018 18:43:22 -0700
Subject: [PATCH] Check multiple NT_GNU_PROPERTY_TYPE_0 notes [BZ #23509]

The older linker treats .note.gnu.property section as a generic note
section and just concatenates all .note.gnu.property sections from the
inputs to the output.  When the older linker is used to created the
program on CET-enabled OS, the generated output has .note.gnu.property
section with multiple NT_GNU_PROPERTY_TYPE_0 notes whose IBT and SHSTK
enable bits are set even if the program isn't CET enabled.  Such program
will crash on CET-enabled machines.  This patch updates the note parser:

1. Skip note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
2. Check multiple NT_GNU_PROPERTY_TYPE_0 notes.

	[BZ #23509]
	* sysdeps/x86/dl-prop.h (_dl_process_cet_property_note): Skip
	note parsing if a NT_GNU_PROPERTY_TYPE_0 note has been processed.
	Update the l_cet field when processing NT_GNU_PROPERTY_TYPE_0 note.
	Check multiple NT_GNU_PROPERTY_TYPE_0 notes.
	* sysdeps/x86/link_map.h (l_cet): Expand to 3 bits,  Add
	lc_unknown.
---
 sysdeps/x86/dl-prop.h  | 51 +++++++++++++++++++++++++++++++++---------
 sysdeps/x86/link_map.h |  9 ++++----
 2 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/sysdeps/x86/dl-prop.h b/sysdeps/x86/dl-prop.h
index 26c3131ac5..9ab890d12b 100644
--- a/sysdeps/x86/dl-prop.h
+++ b/sysdeps/x86/dl-prop.h
@@ -49,6 +49,10 @@  _dl_process_cet_property_note (struct link_map *l,
 			      const ElfW(Addr) align)
 {
 #if CET_ENABLED
+  /* Skip if we have seen a NT_GNU_PROPERTY_TYPE_0 note before.  */
+  if (l->l_cet != lc_unknown)
+    return;
+
   /* The NT_GNU_PROPERTY_TYPE_0 note must be aliged to 4 bytes in
      32-bit objects and to 8 bytes in 64-bit objects.  Skip notes
      with incorrect alignment.  */
@@ -57,6 +61,9 @@  _dl_process_cet_property_note (struct link_map *l,
 
   const ElfW(Addr) start = (ElfW(Addr)) note;
 
+  unsigned int feature_1 = 0;
+  unsigned int last_type = 0;
+
   while ((ElfW(Addr)) (note + 1) - start < size)
     {
       /* Find the NT_GNU_PROPERTY_TYPE_0 note.  */
@@ -64,10 +71,18 @@  _dl_process_cet_property_note (struct link_map *l,
 	  && note->n_type == NT_GNU_PROPERTY_TYPE_0
 	  && memcmp (note + 1, "GNU", 4) == 0)
 	{
+	  /* Stop if we see more than one GNU property note which may
+	     be generated by the older linker.  */
+	  if (l->l_cet != lc_unknown)
+	    return;
+
+	  /* Check CET status now.  */
+	  l->l_cet = lc_none;
+
 	  /* Check for invalid property.  */
 	  if (note->n_descsz < 8
 	      || (note->n_descsz % sizeof (ElfW(Addr))) != 0)
-	    break;
+	    return;
 
 	  /* Start and end of property array.  */
 	  unsigned char *ptr = (unsigned char *) (note + 1) + 4;
@@ -78,9 +93,15 @@  _dl_process_cet_property_note (struct link_map *l,
 	      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)
-		break;
+		return;
+
+	      last_type = type;
 
 	      if (type == GNU_PROPERTY_X86_FEATURE_1_AND)
 		{
@@ -89,14 +110,18 @@  _dl_process_cet_property_note (struct link_map *l,
 		     we stop the search regardless if its size is correct
 		     or not.  There is no point to continue if this note
 		     is ill-formed.  */
-		  if (datasz == 4)
-		    {
-		      unsigned int feature_1 = *(unsigned int *) ptr;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
-			l->l_cet |= lc_ibt;
-		      if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
-			l->l_cet |= lc_shstk;
-		    }
+		  if (datasz != 4)
+		    return;
+
+		  feature_1 = *(unsigned int *) ptr;
+
+		  /* Keep searching for the next GNU property note
+		     generated by the older linker.  */
+		  break;
+		}
+	      else if (type > GNU_PROPERTY_X86_FEATURE_1_AND)
+		{
+		  /* Stop since property type is in ascending order.  */
 		  return;
 		}
 
@@ -112,6 +137,12 @@  _dl_process_cet_property_note (struct link_map *l,
 	      + ELF_NOTE_NEXT_OFFSET (note->n_namesz, note->n_descsz,
 				      align));
     }
+
+  /* We get here only if there is one or no GNU property note.  */
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_IBT))
+    l->l_cet |= lc_ibt;
+  if ((feature_1 & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
+    l->l_cet |= lc_shstk;
 #endif
 }
 
diff --git a/sysdeps/x86/link_map.h b/sysdeps/x86/link_map.h
index ef1206a9d2..9367ed0889 100644
--- a/sysdeps/x86/link_map.h
+++ b/sysdeps/x86/link_map.h
@@ -19,8 +19,9 @@ 
 /* If this object is enabled with CET.  */
 enum
   {
-    lc_none = 0,			 /* Not enabled with CET.  */
-    lc_ibt = 1 << 0,			 /* Enabled with IBT.  */
-    lc_shstk = 1 << 1,			 /* Enabled with STSHK.  */
+    lc_unknown = 0,			 /* Unknown CET status.  */
+    lc_none = 1 << 0,			 /* Not enabled with CET.  */
+    lc_ibt = 1 << 1,			 /* Enabled with IBT.  */
+    lc_shstk = 1 << 2,			 /* Enabled with STSHK.  */
     lc_ibt_and_shstk = lc_ibt | lc_shstk /* Enabled with both.  */
-  } l_cet:2;
+  } l_cet:3;
-- 
2.17.1