[DTrace-devel] [PATCH 02/14] Replace not-NULL test with more general runtime test

[Nick Alcock]

On 2 May 2023, Eugene Loh via DTrace-devel verbalised:

> On 5/2/23 11:32, Nick Alcock wrote:
>>> There are times, however, when even a D pointer cannot safely be
>>> dereferenced.  This will especially be true once NULL string pointers
>>> will be supported.
>> Err, why not soup up the not-null check to handle null strings, then?
>> (And what other cases are there?)
>
> At this point in the patch series, it has nothing to do with NULL
> strings.  In fact, this patch also removes the xfail on a test that is
> unrelated to NULL strings.  All the test does is stores a pointer in
> an associative array, reads it back, and tries to dereference it.  At
> this point, what check can we perform to validate the pointer? 

Oh true! I was thinking "this is pointless, we know that dptrs must be
valid because we stored them", but of course once it's in a map we lose
track of where it came from, and so does the verifier. And it can be a
pointer which was completely unvalidated.

> Maybe I'm missing something.  Feel free to make a more detailed suggestion.

No, you're right.

>>> @@ -5899,10 +5898,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>>>   						 dnp->dn_reg);
>>>   			}
>>>   -			if (dnp->dn_child->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
>>> -				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
>>> -			else
>>> -				dt_cg_load_scalar(dnp, op, size, dlp, drp);
>>> +			dt_cg_load_scalar(dnp, op, size, dlp, drp);
>> This does turn a simple load asm instruction into a helper call for
>> things for which a load instruction should always work (if non-null!).
>
> Which things?  Anyhow, for a narrower set of things than we were allowing.  That's why the test was failing.  Just checking for
> ALLOCA/DPTR was insufficient.  Need a more stringent test.

Well, ALLOCA-tainted pointers in particular and I'd guess DPTR too don't
need any validation (except to placate the verifier) because we know
perfectly well that the thing they are pointing to is valid, because we
pointed it there and it's our own arena.

But the cost is probably undetectable and it *is* simpler and reduces
special cases that don't even have a comment to say they're special.

>> Is this always even possible?
>
> What is "this"?

Can you use that helper to read arbitrary kernel memory, or is it
restricted somehow? (But I just looked at the helper implementation and
I don't think it is after all.)

-- 
NULL && (void)