[DTrace-devel] [PATCH v4 4/5] usdt: typed args and arg mapping

[Nick Alcock]

On 2 Nov 2024, Kris Van Hees said:

> On Fri, Nov 01, 2024 at 03:57:11PM +0000, Nick Alcock wrote:
>> We make one assumption for simplicity, noted by a TODO in the source.
>> We assume that overlying probes all have the same native types and mappings,
>> so we don't need to look at more than one underlying probe to figure out what
>> they are.  DTrace always generates such a thing, but DOF specifying multiple
>> overlapping probes seems perfectly possible for other programs to generate.
>> This should probably be validated in dof_parser.c in future.
>
> Two comments:
> 1) DOF is DTrace.  If other things generate DOF then they better make sure they
>    generate DOF that is compatible with DTrace or else it may not work.  It is
>    not an assumption that multiple USDT probes cannot share an underlying
>    probe.  That is an actual fact due to the implementation of USDT probes.

I was more noting that this is nowhere validated -- and if we don't
validate it, we cannot control what other programs might choose to
provide. You can say "it may not work" but in practice other generators
are not going to be psychic, and if it works in one test they'll
probably say "that's OK" and release it, and then there are binaries in
the wild with embedded DOF that do whatever wrong thing we're not
thinking of yet :(

More generally, we *really* can't control what *hostile attackers*
provide, and we can assume they'll wriggle straight into any ambiguities
we leave and try to compromise our nice running-as-root DTrace with
their unprivileged binary with maliciously twisted DOF in it.

There is no evil bit we can require them to turn on :)

>    In other words, there is no assumption here, nor a real TODO.  The USDT
>    provider implementation does not support more than 1 USDT probe per
>    underlying probe, so you can depend on the arg data stored with the
>    underlying probe to be truly representative for the overlying USDT probe.

... again, this is assuming that our DTrace is the only possible
generator of DOF. We're not cryptographically signing it so this is
simply not true: it's the only generator *now*. (And even that isn't
true: there are other DTraces around that also generate DOF, and
cross-compilation is a thing so we might be hit with any of their DOFs
too... and God only knows what they look like after all these years of
divergence.)

(The unlikeliness of this scenario is why I haven't fixed this yet, but
left it as a TODO. But see below!)

> 2) Why not validate this in dt_prov_uprobe.c, i.e. when creating the underlying
>    probe?  If it was already created for a USDT probe, then we have an issue
>    because two USDT probes on the same underlying probe is not supported.

Hmmm... in this specific case that might be OK, simply because it's
probably hard to get hostile state past the reduction to dof_parsed_t,
and if that reduction fails we just get a jailed parser child crash. We
can probably draw a distinction between format invalidities (which are
the sort of thing attackers like to play with, and should be detected by
dof_parser.c) and things that are *semantically* unsupported but
format-valid (like overlapping probes: not sure we have much else yet).

I was worrying that we'd have to check arg types and mappings, etc, but
we can make this trivial with a flag on the underlying probe that is
flipped on when a USDT probe is created: if that flag's on and we try
to make a new USDT probe on it, boom!)

Done, I hope, assuming that idea works.

>> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
>> index a8c1ab2761f0..fd7ad1643946 100644
>> --- a/libdtrace/dt_prov_uprobe.c
>> +++ b/libdtrace/dt_prov_uprobe.c
>> @@ -57,7 +57,11 @@ typedef struct dt_uprobe {
>>  	uint64_t	off;
>>  	int		flags;
>>  	tp_probe_t	*tp;
>> -	dt_list_t	probes;		/* pid/USDT probes triggered by it */
>> +	int		argc;		   /* number of args */
>> +	dt_argdesc_t	*args;		   /* args array (points in */
>> +	char		*nargvbuf;	   /* array of native args */
>> +	char		*xargvbuf;	   /* array of xlated args */
>
> Not really arrays, though.  Just a binary blod of concatenated 0-terminated
> strings.

Yeah, bad name, it's a strtab really.  Adjusted.

>          Since their content should have been validated by now, why not just
> store them as a single concatenated blob, e.g. argdata rather than as two
> separate pointers?  You never end up using them anyway once the args data is
> populated based on the strings.  So there is no need to keep it separate.

Mostly because that wasn't clear when I was writing it. Adjusting. It's
basically costless and maybe even slightly clearer. I was worried about
overruns but that is of course impossible.

This code is buggy anyway if xptr isn't set, but that just highlights
that the non-xptr code path can never happen any more: dropped.

>> +/*
>> + * Populate args for an underlying probe.  This really relates to the overlying
>> + * probe (all underlying probes associated with a given underlying probe must
>
> The first 'underlying' here should be 'overlying'.

Oh yes.

>> + * have the same args), but the overlying probe doesn't exist at the point this
>> + * is populated, so we must put it in the underlying probe instead and pull it
>> + * out when the overlying probe info is called for.
>
> You need to be more nuanced though because what you say applies to USDT probes
> only.  Yet, an underlying probe may have an overlying pid probes associated
> with it as well.  What you say does not apply to pid probes.

Agreed! I wasn't thinking about them :( hence the bug noted above.

>> + *
>> + * Move it into dt_argdesc_t's for use later on. The char *'s in that
>> + * structure are pointers into the nargvbuf and xargvbuf arrays, which
>> + * are straight copies of the nargc/xargc in the pid_probespec_t.
>
> As mentioned above, why not combine the two into a single blob?

Done!

>>   * The trampoline will first populate a dt_dctx_t struct.  It will then emulate
>>   * the firing of all dependent pid* probes and their clauses.
>
> pid* and USDT probes and their clauses.

Ack.

>> + *
>> + * uprobe trampolines will also arrange for argument shuffling if the argmap
>> + * array calls for it.
>
> That becomes a problem when a uprobe serves both a USDT probes and a pid
> probe.  We shouldn't be mapping any arguments for pid probes.  No need to have
> this comment here.  If it needs a xomment, put it below where the mapping is
> actually done.

This comment is simply wrong now -- it does a lot more than just emulate
firing, it copies in things for is-enabled probes, shuffles args etc.

Far from removing... augmented.

>>   */
>>  static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>  {
>> @@ -864,7 +960,7 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>  	}
>>  
>>  	/*
>> -	 * USDT
>> +	 * USDT.
>
> Why?

Because 'pid probes' above has a trailing .

A foolish consistency :)

>> @@ -873,13 +969,19 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
>>  
>>  	dt_cg_tramp_copy_args_from_regs(pcb, 0);
>>  
>> +	/*
>> +	 * Apply arg mappings, if needed.
>> +	 */
>> +	if (upp->args != NULL)
>> +		dt_cg_tramp_map_args(pcb, upp->args, upp->argc);
>> +
>
> I would add a dt_cg_tramp_save_args() here, followed by dt_cg_tramp_map_args(),
> because the mapping should act on the saved copy of arguments.  But if we need
> to do anything else with arguments, we want to be able to restore the original
> arguments.  That is a function of the trampoline, not of the mapping function.

... ok? I still think this is wildly less clear and adds unnecessary
coupling between dt_cg_tramp_map_args() and its caller (since
dt_cg_tramp_map_args() now silently misbehaves if you don't save first!
or more likely causes a verifier failure), but since the plan is to get
rid of it eventually, I don't care enough to argue.

> Incidentally, you could consider adding a flag on the dt_uprobe_t to indicate
> whether argument mapping is needed.  It is more likely that we have argument
> type data without mapping than with mapping, so being able to avoid making a
> copy of the args (and going through a loop that won't generate any code) is
> worth it.  Adding PP_HAS_ARGMAP comes to mind?

I took that out because you said it was unnecessary! Adding back. (I too
thought it was unnecessary because no code was generated, but of course
saving the args generates a bunch of code...)

(To me, this is extra evidence that the right place to do all this is in
dt_cg_tramp_map_args(), since it *already verifies* exactly that
remappings are not identity mappings and this would just need to move up
a bit, but as noted I don't care enough to argue. The optimally
efficient approach would be to save only those args that are in the
from- or to-remappings somewhere, but that's probably ridiculous
over-optimization. It only comes down to one or two lines in the end.)

>>  	/*
>>  	 * Retrieve the PID of the process that caused the probe to fire.
>>  	 */
>>  	emit(dlp,  BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
>>  	emit(dlp,  BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
>>  
>> -	/*
>> +        /*
>
> Eeks?  Indentation issues again?

Ugh! Why didn't git log --check tell me about that? sigh.

-- 
NULL && (void)