Re: [PATCH] 9p: prevent read overrun in protocol dump tracepoint

[Dominique Martinet <asmadeus@codewreck.org>]

Steven Rostedt wrote on Sat, Dec 02, 2023 at 08:14:09PM -0500:
> > AFAICS __entry is a local variable on stack, and array __entry->line not
> > intialized with zeros, i.e. the dump would contain trash at the end. Maybe
> > prepending memset() before memcpy()?

Well spotted!
Now I'm thinking about it we weren't initializing the source buffer
either back when we had (>32) msize allocations, so these already had
been printing garbage, but might as well get this sorted out while we're
here.

> __entry is a macro that points into the ring buffer that gets allocated
> before this is called. TRACE_EVENT() has a __dynamic_array() field that
> can handle variable length arrays. What you can do is turn this into
> something like:
> 
> TRACE_EVENT(9p_protocol_dump,
>             TP_PROTO(struct p9_client *clnt, struct p9_fcall *pdu),
> 
>             TP_ARGS(clnt, pdu),
> 
>             TP_STRUCT__entry(
>                     __field(    void *,         clnt                            )
>                     __field(    __u8,           type                            )
>                     __field(    __u16,          tag                             )
>                     __dynamic_array(unsigned char,  line, min(pdu->capacity, P9_PROTO_DUMP_SZ) )
>                     ),
> 
>             TP_fast_assign(
>                     __entry->clnt   =  clnt;
>                     __entry->type   =  pdu->id;
>                     __entry->tag    =  pdu->tag;
>                     memcpy(__get_dynamic_array(line), pdu->sdata,
> 			   min(pdu->capacity, P9_PROTO_DUMP_SZ));
>                     ),
>             TP_printk("clnt %lu %s(tag = %d)\n%.3x: %16ph\n%.3x: %16ph\n",
>                       (unsigned long)__entry->clnt, show_9p_op(__entry->type),
>                       __entry->tag, 0, __get_dynamic_array(line), 16,
> 		      __get_dynamic_array(line) + 16)

This was just printing garbage in the previous version but %16ph with a
dynamic alloc would be out of range (even the start of the next buffer,
_get_dynamic_array(line) + 16, can be out of range)

Also, for custom tracepoints e.g. bpftrace the program needs to know how
many bytes can be read safely even if it's just for dumping -- unless
dynamic_array is a "fat pointer" that conveys its own size?
(Sorry didn't take the time to check)

So I see two ways forward:
 - We can give up on the 16 bytes split here, add the size in one of the
fields, and print with %*ph using that size.
 - Or just give up and zero the tail; I'm surprised there's no "memcpy
up to x bytes and zero up to y bytes if required" helper but Christian's
suggestion of always doing memset first is probably not that bad
performance-wise if someone's dumping these out already.

I don't have a hard preference here, what do you think?
-- 
Dominique Martinet | Asmadeus