JP Kobryn wrote on Fri, Dec 01, 2023 at 07:04:10PM -0800:
> 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 definition as 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 using the minimum of that
> field (size of source buffer) and the size of destination buffer when
> performing the copy.
Good catch; this is a regression due to a semi-recent optimization in
commit 60ece0833b6c ("net/9p: allocate appropriate reduced message
buffers")
For some reason I thought we rounded up small messages alloc to 4k but
I've just confirmed we don't, so these overruns are quite frequent.
I'll add the fixes tag and cc to stable if there's no other comment.
Thanks!
>
> A repro can be performed by different ways. The simplest 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>
> ---
> include/trace/events/9p.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/trace/events/9p.h b/include/trace/events/9p.h
> index 4dfa6d7f83ba..8690a7086252 100644
> --- a/include/trace/events/9p.h
> +++ b/include/trace/events/9p.h
> @@ -185,7 +185,8 @@ TRACE_EVENT(9p_protocol_dump,
> __entry->clnt = clnt;
> __entry->type = pdu->id;
> __entry->tag = pdu->tag;
> - memcpy(__entry->line, pdu->sdata, P9_PROTO_DUMP_SZ);
> + memcpy(__entry->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),
--
Dominique Martinet | Asmadeus
