[DTrace-devel] [PATCH 5/6] usdt: typed args and arg remapping

[Nick Alcock]

On 22 Oct 2024, Kris Van Hees told this:

> On Fri, Oct 18, 2024 at 08:58:07PM +0100, Nick Alcock wrote:
>> This change propagates the arg type and mapping info we just got into the
>> pid_probespec_t onward into the underlying dt_uprobe_t, shuffles the native
>> types before handing off a 1:1 mapping to the dt_probe_t, and calls
>> dt_cg_tramp_remap_args() to apply the requested remappings.  With this, USDT
>> probes should now have visible types which obey the :-notation translators
>> specified in their .d fils (this is now tested).
>> 
>> The probe info has been moved from the underlying probe to the overlying one
>> as part of this: underlying probes don't have any user-visible args to
>> get info for, and their probe_info functions are never called.
>> 
>> We make several assumptions for simplicity, all noted by TODOs in the
>> source:
>> 
>>  - overlying probes all have the same native types and remappings, so we
>>    don't need to look at more than one underlying probe to figure out what
>>    they are.  Given the structure of DOF it seems to me this must always be
>>    true, and even a malicious attacker could not produce DOF which said
>>    otherwise.
>
> You cannot place more than one USDT at a particular tracepoint (underlying
> probe) so it is guaranteed that all overlying probes can only differ in terms
> of their provider name.

I'm fairly sure I'm wrong above -- this is not validated, so malicious
DOF can perfectly well do that, simply by emitting multiple distinct
probes citing the same (or overlapping) sets of addresses! We could, of
course, add validation...

Just because DTrace is the only legitimate generator of DOF embedded
into programs, it is not true that it is the only *possible* such
program. If DTrace gets popular and attackers realise that a program
running as root is consuming DOF provided by arbitrary programs owned by
anyone, this will surely not long remain the case. Non-structural
properties of DOF not validated by dof_parser.c cannot be relied upon by
code consuming DOF contributed by USDT programs. (So this is a genuine
problem, though it might just be missing validation -- which will be
*fun* to implement in an environment with very limited malloc.)

>                         The USDT design in DTrace encapsulates the USDT probes
> (and providers) as instances of a meta-provider that acts as a template for its
> instances.  So, all instances contain the same probes, together with all their
> data (arg data, tracepoints, etc).  So, this is a guarantee and need not be
> mentioned as an assumption or TODO.

I wish :(

>>  - any given underlying probe locus is either a PID or a USDT probe, never
>>    both, so we can just remap the args at the start of the trampoline and
>>    then never touch them again.  This is obviously wrong, but the both-at
>>    the-same-locus case is not yet implemented, and significant changes need
>>    to be made to create_underlying() to fix this.  Right now we can't easily
>>    even tell if a given locus has both types of probe on it... fixing this
>>    is literally a matter of shuffling the pid probes after the usdt probes
>>    and then doing a single dt_cg_tramp_restore_args before all the pid
>>    probes fire.
>
> I would rewrite this to point out that PID and USDT probes being at the same
> locus is not implemented yet, and that this implementation assumes that either
> all probes at a particular locus are PID or that they are all USDT.

Rephrased a bit.

> I would avoid speculative statements about how this might be fixed in the
> future (however near that may be) because there may still be design decisions
> to be made about that and there is no need to bring it up here.

Oh, I thought we'd already decided that! I'll drop the last couple of
sentences.

>>    PID args are already broken before this commit (something to do with the
>>    wildcard USDT work?) so I cannot test if this commit broke them further,
>>    but it shouldn't have.
>
> This does not belong here but rather in a bug report.

Errr... a bug report about a not-yet-released DTrace with a the USDT
patch series half-applied? I don't think we've ever had bug reports
about non-released half-applied patch series before and I'd have assumed
that a bug report was exactly what you *wouldn't* want about something
in that sort of state of flux.

Obviously I wasn't planning to leave this here in the long term, but at
least this way if you tested the series it is obvious that a few tests
are expected to fail and some things have not yet been verified. I have
no idea where you'd expect me to say that if not in the commit message
for the change in question.

> In addition, I think it would be good to add a few more test cases to cover
> edge cases:
>
>  - A probe that has native arguments, but no translated arguments.
>    E.g.
> 	probe place3(int i, char *j) : ();
>
>  - A probe that has native arguments that do not get used.
>    E.g.
> 	probe place4(int i, char *j) : (char *j);

Good point, implemented! (The first one was fun.)

>> +	int		nargc;		   /* number of native args */
>> +	int		xargc;		   /* number of xlated args */
>> +        int		remapargc;	   /* number of remapped args */
>> +	char		*nargv;		   /* array of native args */
>> +	char		*xargv;		   /* array of xlated args */
>> +	int8_t		*remapargs;	   /* remapped arg indexes */
>
> Since all overlying USDT probes associated with a given underlying probe
> (uprobe) have the sane argument type data (native, translated, and mapping),
> and PID probes do not have arguments by definition, you can certainly store
> the data here.  But why in this raw format rather than a dt_argdesc_t array
> (and an argc member to indicate the number of array elements - xargc).

I started out doing that, but it meant infiltrating knowledge about
dt_argdesc_t into dt_pid_create_usdt_probes_proc(), and it proved
outright confusion-inducing when it turned out that dt_prov_uprobe.c had
to pass down a *different mapping* with reshuffled elements, because of
the need to move native args around in the translator. So the
dt_argdesc_t approach gained us nothing and made things noticeably
harder to understand :(

> Calls to the probe_info() callback will want an array of dt_argdesc_t elements
> to be provided, and nothing else needs the rarw format.  So, you might as well
> store that array here rather than the raw data, and implement the probe_info()
> callback for USDT as a pass-through that simply takes the data from here (from
> the underlying probe), copies it (using strdup on the strings because the
> caller will free them), and assigned it to *argv.

Yes, if it wasn't that the uprobe probe_info needs to reshuffle it, and
the only reason it needs to do that is the nature of the uprobe
trampoline (so knowledge about that shuffling should not be propagated
into dt_pid.c: as you note, in the future it would be nice to not need
the trampoline arg remapper at all, so we don't want to spread things
that imply its existence too far if we can avoid it).

So... maybe later once we're not shuffling native args anywhere, if
we're sure that nobody is likely to want to do that?

>> +test_prov$1:::place2
>> +/stringof(args[0]) != "foo" || args[1] != 255 || args[2] != 255 || stringof(args[3]) != "foo"/
>> +{
>> +	printf("args are %s, %d, %d, %s; should be \"foo\", 255, 255, \"foo\"",
>> +	    stringof(args[0]), args[1], args[2], stringof(args[3]));
>> +	exit(1);
>> +}
>
> This test should also test that argN (N = 0 .. 3) has the correct value as
> well.  Later we may change that which would require the test to be updated,
> but the current immplementation you provide is aimed at ensuring that argN and
> args[N] are identical, so you should test that in this case also (because the
> other test does not involve different types, which could make a difference).

Good point! Hmm we may need to copyin() there too.

(Adjusted all tests in this family.)

-- 
NULL && (void)