Re: [DTrace-devel] [PATCH v4 1/5] usdt: get arg types and xlations into DTrace from the DOF

[Nick Alcock <nick.alcock@oracle.com>]

On 2 Nov 2024, Kris Van Hees uttered the following:

> On Fri, Nov 01, 2024 at 07:46:04PM -0400, Kris Van Hees via DTrace-devel wrote:
>> Two comments below (one tiny thing, and one explanatory comment that I *think*
>> I have a better suggestion for, but you may want to rephrase it).
>
> Actually, another issue (see below).
>
>> On Fri, Nov 01, 2024 at 03:57:08PM +0000, Nick Alcock via DTrace-devel wrote:
>> > +static size_t
>> > +strings_len(const char *strtab, size_t count)
>> > +{
>> > +	size_t len = 0;
>> > +
>> > +	for (; count > 0; count--) {
>> > +		size_t this_len = strlen(strtab) + 1;
>> > +
>> > +		len += this_len;
>> > +		strtab += this_len;
>> > +	}
>> > +	return len;
>> > +}
>
> What if the strtab blob does not contain enough strings?  There is no check
> here to ensure that you do not start reading past the end of data?

Then we run off the end! We rely on validation already having been done
by validate_provider() (which checks all this stuff), and if that is
buggy we crash and dtprobed restarts us a few times, gives up, logs a
message and rejects the probes. That's the worst that can happen.

We run in a seccomped jail specifically so we don't need to worry about
things like this :)

>> > +	if (dhpb->dthpb_nargc > 0) {
>> > +		size_t	nargs_size;
>> > +
>> > +		nargs_size = strings_len(dhpb->dthpb_ntypes, dhpb->dthpb_nargc);
>
> No validation that there are nargc type strings.
>
>> > +			xargs_size = strings_len(dhpb->dthpb_xtypes,
>> > +						 dhpb->dthpb_xargc);
>
> No validation that there are nargc type strings.

See above.

>> > +			msg_size = offsetof(dof_parsed_t, xargs.args) +
>> > +				   xargs_size;
>> > +
>> > +			msg = malloc(msg_size);
>> > +			if (!msg)
>> > +				goto oom;
>> > +
>> > +			memset(msg, 0, msg_size);
>> > +
>> > +			msg->size = msg_size;
>> > +			msg->type = DIT_ARGS_XLAT;
>> > +			memcpy(msg->xargs.args, dhpb->dthpb_xtypes, xargs_size);
>> > +			dof_parser_write_one(out, msg, msg_size);
>> > +
>> > +			free(msg);
>> > +
>> > +			/* Then the remapping table. */
>> 
>> remapping -> mapping

... I swear I fixed that. Fixed again.

>> > +			msg->size = msg_size;
>> > +			msg->type = DIT_ARGS_MAP;
>> > +			memcpy(msg->argmap.argmap, dhpb->dthpb_args, map_size);
>
> Is there any validation anywhere that there are map_size bytes to read from
> dhpb->dthpb_args?  Is there any validation anywhere here (or in a later patch)
> that the entries are valid (between 0 and nargc)?

Yep! See above: validate_provider checks all of this (you probably
missed it because it's done under another name: see the end of
emit_provider for where we shuffle things into their new homes.)