On Monday, December 4, 2023 9:23:20 PM CET JP Kobryn wrote: > An out of bounds read can occur within the tracepoint 9p_protocol_dump. In > the fast assign, there is a memcpy that uses a constant size of 32 (macro > named P9_PROTO_DUMP_SZ). When the copy is invoked, the source buffer is not > guaranteed match this size. It was found that in some cases the source > buffer size is less than 32, resulting in a read that overruns. > > The size of the source buffer seems to be known at the time of the > tracepoint being invoked. The allocations happen within p9_fcall_init(), > where the capacity field is set to the allocated size of the payload > buffer. This patch tries to fix the overrun by changing the fixed array to > a dynamically sized array and using the minimum of the capacity value or > P9_PROTO_DUMP_SZ as its length. The trace log statement is adjusted to > account for this. Note that the trace log no longer splits the payload on > the first 16 bytes. The full payload is now logged to a single line. > > To repro the orignal problem, operations to a plan 9 managed resource can > be used. The simplest approach might just be mounting a shared filesystem > (between host and guest vm) using the plan 9 protocol while the tracepoint > is enabled. > > mount -t 9p -o trans=virtio <mount_tag> <mount_path> > > The bpftrace program below can be used to show the out of bounds read. > Note that a recent version of bpftrace is needed for the raw tracepoint > support. The script was tested using v0.19.0. > > /* from include/net/9p/9p.h */ > struct p9_fcall { > u32 size; > u8 id; > u16 tag; > size_t offset; > size_t capacity; > struct kmem_cache *cache; > u8 *sdata; > bool zc; > }; > > tracepoint:9p:9p_protocol_dump > { > /* out of bounds read can happen when this tracepoint is enabled */ > } > > rawtracepoint:9p_protocol_dump > { > $pdu = (struct p9_fcall *)arg1; > $dump_sz = (uint64)32; > > if ($dump_sz > $pdu->capacity) { > printf("reading %zu bytes from src buffer of %zu bytes\n", > $dump_sz, $pdu->capacity); > } > } > > Signed-off-by: JP Kobryn <inwardvessel@xxxxxxxxx> > --- Reviewed-by: Christian Schoenebeck <linux_oss@xxxxxxxxxxxxx> > include/trace/events/9p.h | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/include/trace/events/9p.h b/include/trace/events/9p.h > index 4dfa6d7f83ba..cd104a1343e2 100644 > --- a/include/trace/events/9p.h > +++ b/include/trace/events/9p.h > @@ -178,18 +178,21 @@ TRACE_EVENT(9p_protocol_dump, > __field( void *, clnt ) > __field( __u8, type ) > __field( __u16, tag ) > - __array( unsigned char, line, P9_PROTO_DUMP_SZ ) > + __dynamic_array(unsigned char, line, > + min_t(size_t, pdu->capacity, P9_PROTO_DUMP_SZ)) > ), > > TP_fast_assign( > __entry->clnt = clnt; > __entry->type = pdu->id; > __entry->tag = pdu->tag; > - memcpy(__entry->line, pdu->sdata, P9_PROTO_DUMP_SZ); > + memcpy(__get_dynamic_array(line), pdu->sdata, > + __get_dynamic_array_len(line)); > ), > - TP_printk("clnt %lu %s(tag = %d)\n%.3x: %16ph\n%.3x: %16ph\n", > + TP_printk("clnt %lu %s(tag = %d)\n%*ph\n", > (unsigned long)__entry->clnt, show_9p_op(__entry->type), > - __entry->tag, 0, __entry->line, 16, __entry->line + 16) > + __entry->tag, __get_dynamic_array_len(line), > + __get_dynamic_array(line)) > ); > > >