[DTrace-devel] [PATCH v3 13/19] alloca: support null pointers

[Kris Van Hees]

On Tue, Mar 29, 2022 at 02:51:38PM +0100, Nick Alcock wrote:
> On 29 Mar 2022, Nick Alcock via DTrace-devel stated:
> 
> > In conclusion: our error checking of copies lies somewhere between
> > nonexistent and worse than that (in that errors lead to outright wrong
> > results).
> 
> Honestly, when I wrote the alloca code I assumed this was *intentional*:
> that we were trying to do all range checks ourselves and this is why we
> were doing explicit notnull checks, etc: so we could raise appropriate
> faults from the C codegen layer, rather than trying to do it in the
> piles of asm which constitute things like the string handling.
> 
> But... if it isn't, I'm honestly not sure what the point of the notnull
> checking even is. I guess it lets us explicitly satisfy language rules
> that various inputs must not be NULL, while letting the BPF helpers do
> checks that are harder like "not NULL but also not valid". Not that I
> know why that distinction matters, given that both cases should yield
> the same result, a BADADDR fault. (Not that they are right now, but
> that's presumably a plain old bug which I should try to fix.)

No, the reason why have notnull checking is because the verifier requires us
to perform not-NULL checks to move pointers of type PTR_TO_MAP_VALUE_OR_NULL to
type PTR_TO_MAP_VALUE.  That is needed in order to actually do something with
those pointers.  And that requires a strict compare with 0.

We most definitely do not need explicit NULL or near-NULL checking for other
things, especially when we already have address validation code in place.
So, a pointer address outside the scratchmem area is an invalid pointer.  We
do not care whether it is past the end of scfatchmem or whether it is near
NULL or whatever.  So you can let casts or whatever else crazy that may
yield an invalid address continue to exist, and trust that your bounds checking
to ensure that an alloca pointer lies within the scratchmem area is sufficient
for validating pointers.

A BADADDR is a BADADDR, regardless of its value.