Re: [RFC PATCH v2 2/2] ELF: Add ELF program property parsing support

[Dave Martin]

On Fri, Aug 30, 2019 at 06:03:27PM +0100, Yu-cheng Yu wrote:
> On Fri, 2019-08-30 at 09:34 +0100, Dave Martin wrote:
> > On Fri, Aug 30, 2019 at 06:37:45AM +0100, Kees Cook wrote:
> > > On Fri, Aug 23, 2019 at 06:23:40PM +0100, Dave Martin wrote:
> > > > ELF program properties will needed for detecting whether to enable
> > > > optional architecture or ABI features for a new ELF process.
> > > > 
> > > > For now, there are no generic properties that we care about, so do
> > > > nothing unless CONFIG_ARCH_USE_GNU_PROPERTY=y.
> > > > 
> > > > Otherwise, the presence of properties using the PT_PROGRAM_PROPERTY
> > > > phdrs entry (if any), and notify each property to the arch code.
> > > > 
> > > > For now, the added code is not used.
> > > > 
> > > > Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> > > 
> > > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > 
> > Thanks for the review.
> > 
> > Do you have any thoughts on Yu-Cheng Yu's comments?  It would be nice to
> > early-terminate the scan if we can, but my feeling so far was that the
> > scan is cheap, the number of properties is unlikely to be more than a
> > smallish integer, and the code separation benefits of just calling the
> > arch code for every property probably likely outweigh the costs of
> > having to iterate over every property.  We could always optimise it
> > later if necessary.
> > 
> > I need to double-check that there's no way we can get stuck in an
> > infinite loop with the current code, though I've not seen it in my
> > testing.  I should throw some malformed notes at it though.
> 
> Here is my arch_parse_elf_property() and objdump of the property.
> The parser works fine.

[...]

> int arch_parse_elf_property(u32 type, const void *data, size_t datasz,
>           
>                    bool compat, struct arch_elf_state *state)
> {
>         if (type
> != GNU_PROPERTY_X86_FEATURE_1_AND)
>                 return -ENOENT;

For error returns, I was following this convention:

	EIO: invalid ELF file

	ENOEXEC: valid ELF file, but we can't (or won't) support it

	0: OK, or don't care

This function gets called for every property, including properties that
the arch code may not be interested in, so for properties you don't care
about here you should return 0.

> 
>         if (datasz < sizeof(unsigned int))
>                 return -ENOEXEC;

Should this be != ?

According to the draft x86-64 psABI spec [1],
X86_PROPERTY_FEATURE_1_AND (and all properties based on
GNU_PROPERTY_X86_UINT32_AND_LO) has data consisting of a single 4-byte
unsigned integer.

>         state->gnu_property = *(unsigned int *)data;
>         return 0;
> }
>
> Contents of section .note.gnu.property:
>  400338 04000000 30000000 05000000 474e5500  ....0.......GNU.
>  400348 020000c0 04000000 03000000 00000000  ................
>  400358 000001c0 04000000 00000000 00000000  ................
>  400368 010001c0 04000000 01000000 00000000  ................

Because you return ENOENT except for GNU_PROPERTY_X86_FEATURE_1_AND,
wouldn't we fail on the second and third properties here?
(GNU_PROPERTY_X86_ISA_1_USED, GNU_PROPERTY_X86_FEATURE_2_USED if I
decoded them correctly...)

If not, maybe my code is failing to iterate over all the properties for
some reason.

Cheers
---Dave

[1] https://github.com/hjl-tools/x86-psABI/wiki/X86-psABI
https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf