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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.