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 <[email protected]>
> ---
Reviewed-by: Christian Schoenebeck <[email protected]>
> 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))
> );
>
>
>
