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

[asmadeus@codewreck.org]

JP Kobryn wrote on Mon, Dec 04, 2023 at 12:23:20PM -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
> 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@gmail.com>

Thanks, I've updated the patch locally; will push to -next after testing
later tonight and to Linus next week.

-- 
Dominique Martinet | Asmadeus