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

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Sep 04, 2019 at 05:50:06PM +0100, Kees Cook wrote:
> On Fri, Aug 30, 2019 at 09:34:18AM +0100, Dave Martin wrote:
> > 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 feel like there are already a lot of ways to burn CPU time with
> mangled ELF files, so this potential inefficiently doesn't seem bad to
> me. If we want to add limits here, perhaps cap the property scan depth
> (right now, IIRC, the count is u32, which is big...)

I was thinking more of valid ELF files where the number of properties is
large.

I feel that the GNU properties system will have become unfit for purpose
if the number of defined properties gets large enough for this to be an
issue though.

I'll keep this code as-is for now.

> > 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.
> 
> I think the cursor only performs forward movement, yes? I didn't see a
> loop, but maybe there's something with the program headers that I
> missed.

That's the principle: always step forward, always by a non-zero amount.
So forward progress should be guaranteed...

> > Do you have any objection to merging patch 1 with this one?  For
> > upstreaming purposes, it seems overkill for that to be a separate patch.
> 
> I don't _object_ to it, but I did like having it separate for review.

I'm equally happy to leave them separate; I just wasn't sure how much
they made sense as separate patches.  I'll have a think when I respin.

> > Do you have any opinion on the WARN_ON()s?  They should be un-hittable,
> > so they're documenting assumptions rather than protecting against
> > anything real.  Maybe I should replace them with comments.
> 
> I think they're fine as self-documentation. My rule of thumb has been:
> 
> - don't use BUG*() unless you can defend it to Linus who wants 0 BUG()s.
> - don't use WARN*() if userspace can reach it (and if you're not sure,
>   use the WARN*ONCE() version)
> - use pr_*_ratelimited() if unprivileged userspace can reach it.

OK, I'll probably keep them for now, then.

This isn't a super-hot path, and with multiple kernel_read() calls in
the mix already it's hard to imagine the WARN_ON() calls being a
significant part of the overall cost.

Cheers
---Dave