[DTrace-devel] [PATCH] Fix dt_tp_event_info() not to overrun buffer

[Kris Van Hees]

On Tue, Nov 28, 2023 at 11:48:07PM -0500, eugene.loh at oracle.com wrote:
> 
> In dt_tp_event_info(), we read a format file for information about a
> raw tracepoint.  In the first pass, we get the event ID and sizing
> information for argument information.  In the second pass, we read
> actual argument information.  Unfortunately, the way we manipulate
> the buffer, we can overrun the buffer.
> 
> Consider this example from OL8, UEK7:
>     # dtrace -lvn sdt:rpcrdma::xprtrdma_inline_thresh
> This reads information from
>     /sys/kernel/debug/tracing/events/rpcrdma/xprtrdma_inline_thresh/format
> including a line like
>     field:unsigned char srcaddr[sizeof(struct sockaddr_in6)];
>     offset:24; size:28; signed:0;
> (Here, we wrap the line around, since we are dealing with long line
> lengths.)
> 
> Note that "buf" points to the beginning of the line.  Then:
>     p = strstr(buf, "size:");
> makes "p" point to "size:".  Then we have
>     sscanf(buf, " field:%[^;]", p);
> which takes the text between "field:" and ";" and writes it to p.
> Well, this is exactly the problem.  That text can be long and overwrite
> the rest of the buffer.  We end up with
>     field:unsigned char srcaddr[sizeof(struct sockaddr_in6)];
>     offset:24; unsigned char srcaddr[sizeof(struct sockaddr_in6)]
> which is quite a bit longer than the original line.

See, this is the problem when using C-like languages for too long.  I must have
been thinking about another C-like language I have done a lot of work in where
sscanf() does not copy data but rather places the address of the start of the
match in the string argument (along with the match size - it uses strings that
are represented as (address, length) data pairs).

But that is obviously not valid in C.  My bad - good catch!

> Fix this.  Specifically, once
>     p = strstr(buf, "size:");
> has been used, recompute p to point earlier in the buffer.  E.g.
>     p = strstr(buf, "field:") + strlen("field:");
> will work in place, but the even simpler solution
>     p = buf;
> also seems to work and is akin to how we handle "__data_loc".

This commit message is way too verbose for the problem you are addressing
though.  It suffices to say that the code is not resetting p to the beginning
of the buffer, and then the patch itself speaks for itself.

> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>

... with the commit message changed as stated above.

> ---
>  libdtrace/dt_provider_tp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libdtrace/dt_provider_tp.c b/libdtrace/dt_provider_tp.c
> index f3e03323..3ded9370 100644
> --- a/libdtrace/dt_provider_tp.c
> +++ b/libdtrace/dt_provider_tp.c
> @@ -196,6 +196,7 @@ dt_tp_event_info(dtrace_hdl_t *dtp, FILE *f, int skip, tp_probe_t *tpp,
>  		if (p != NULL)
>  			size = strtol(p + 5, NULL, 10);
>  
> +		p = buf;
>  		if (sscanf(buf, " field:%[^;]", p) <= 0)
>  			continue;
>  		sscanf(p, "__data_loc %[^;]", p);
> -- 
> 2.18.4
> 
>