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

[Nick Alcock]

On 28 Mar 2022, Kris Van Hees verbalised:
> We already have validation of memory access for other cases because those
> require the use of bpf_probe_read() which employs the address validation that
> the kernel provides already.  No need to do it ourselves.  We can just ensure
> that we report BADADDR for failed bpf_probe_read( calls.

So... I just checked. Nearly all our bpf_probe_read calls are in the
string stuff, which is a) in raw asm (and thus hard enough to maintain
as it is, and IIRC a nightmare to write) and b) usually does not verify
any such thing and just assumes bpf_probe_read will never fail or
returns wrong results without faulting if it does.  As far as I can
tell, we never raise any faults in bpf/*.S at all, and the code in dt_cg
that calls it never faults in response to erroneous return values either
(unsurprisingly: mostly it can't distinguish fault returns from
non-faulting valid error-return-value returns: the dt_cg calling code
would be the wrong place to raise these faults).

basename.S: returns <= 1 (including negative) return '.'
dirname.S: ditto

index.S: has error-checking of a sort, but all errors yield -1, so no
         BADADDR is ever raised even if you pass in complete garbage and
         bpf_probe_read fails
rindex.S: ditto
strchr.S: ditto
substr.S: errors silently return the empty string (!)
strcmp.S: almost no error-checking.  Most reads from strings are
          assumed never to fail: in the one case there is error-checking
          we just return 0 on error, because a null pointer is obviously
          equal to everything
strjoin.S: errors yield a silent return with no error

lltostr.S: no error-checking.

strlen.c: no error-checking, but at least it's returning -1 on error; of
          course, that's not a BADADDR fault
strchr.S: ditto.

strtok.S: delimiter table copy not error-checked.  Initial temp copy of
          string not error-checked.  Everything else is error-checked,
          but by then it is of course too late.  On error we just return
          0 anyway.  No fault raised.

get_bvar is fine (though this only uses it in the implementation of
DIF_VAR_PPID, which had better not fail :) )

In conclusion: our error checking of copies lies somewhere between
nonexistent and worse than that (in that errors lead to outright wrong
results).

In conclusion: we should *definitely* not remove the extra checks yet.
Given that our rate of spontanoeusly writing accurate error checks in
response to bpf_probe_read* calls appears to be near-zero, I suspect we
should never remove them at all and should probably think about adding
more. At least dt_cg_check_notnull is a single centralized place to add
the damn things.

Alternatively, I could add error checking to all of this and remove the
extra near-nullptr checking when I do that (since we *do* need error
checking there, no matter what). That should be a separate commit
though. How we ensure that future new code properly checks these things
for errors, when we never managed it before, I have no idea.