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

[Christian Schoenebeck <linux_oss@crudebyte.com>]

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@gmail.com>
> ---

Reviewed-by: Christian Schoenebeck <linux_oss@crudebyte.com>

>  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))
>   );
>  
>  
>