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

[Kris Van Hees]

On Mon, Oct 28, 2024 at 12:52:01PM +0000, Nick Alcock wrote:
> 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 validation need not be at the level of dtprobed.  DTrace itself can very
easily determine when a USDT probe pops up at a location (identified by the
underlying probe) that already has a USDT probe.

Malicious DOF encoding data that results in more than one USDT probe being at
a given location, possibly with different argument data, will result in
undefined (yet still safe) behaviour.  But that goes for most other things
that anyone could fake - DTrace does guard against such bad data posing a
security risk though.  It is not necessary for dtprobed to do all validation.

> >                         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 :(

Well, it is a guarantee for what DOF gets generated by DTrace.  Sure, malicious
DOF will result in things going wrong in terms of functionality (but not safety
thankfully), but that is a given, right?  For the purpose of this patch, you
can assume valid data and I think that the commit message can stick to that.

We can come up with a variety of issues that can occur when malicious (or
buggy) DOF gets processed, but I don't think it is useful to provide that
level of detail or analysis in a commit message.  Especially because the
way it is phrased as assumptions it gives the impression that we dont know.
And we do.

My main objection here is really that it appears that your patch is making
assumptions for simplicity, and that is not the case - you are implementing
the correct behaviour based on knowing how the DOF is structured and generated.

If you want to mention something here for peace of mind, perhaps just say that
dtprobed does not validate that e.g. no two USDT probes list the same
tracepoint.  But even that I think is unnecessary - after all, it wasn't at all
mentioned when dtprobed was first implemented, so why would it now suddenly be
an issue?

> >>  - 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.

Point is that you are mentioning that there is a patch before yours that you
say breaks PID args.  Since your series is based on the current development
branch, that means that said patch has already been reviewed and merged.  So,
it makes totla sense to open a bug (github issue) for it so that it can be
looked into.  Mentioning the breakage in a commit message is not going to get
the attention it needs to track down the issue and fix it.  Also, the way it
is mentioned here does not really provide detail on *what* appears broken, i.e.
which test fails, what the results are vs expected, etc...  All stuff you would
add to an issue you file.

I am also not sure what you mean with "wasn't planning to leave this here in
the long term"... as this patch is posted for rwview, I would expect that if
reviewed as OK, I can merge it as-is and so this portion of the commnit msg
would certainly be there for the long term.

> > 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 :(

How would this leak dt_argdesc_t into dt_pid_create_usdt_probes_proc()?  That
function can still handle the raw format.  My point is that the dt_uprobe_t
you store the data in (associated with the underlying probe) can already store
it in dt_argdesc_t format rather than storing the raw data.

It does not pose an issue with the mapping of arguments in the trampoline
because wherever you switch things from being a shuffling to simply being a
i -> i mapping, that can be done as easily based on an dt_argdesc_t array as
it can be done with the raw data.

> > 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?

I really do not see the problem.  It is the same data, just in a different
format.

With the underlying probe keeping the arg data in a dt_argdesc_t array, the
probe_info() calls would still need to provide a copy of the array, with strdup
for the strings, and thus you can easily store i -> i mapping there also and
thus the dt_argdesc_t data on the uderlying probe still retains the correct
napping data that the trampoline will need.

And if/when we adjust things to get away from this silly argN == args[N] thing,
that altering of the mapping index in the dt_argdesc_t elements can simply
be dropped and all will be well (aside from also needing to update the
trampoline code to stop its reshuffling).

> >> +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)