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

[Christian Schoenebeck <linux_oss@crudebyte.com>]

On Saturday, December 2, 2023 5:35:18 AM CET asmadeus@codewreck.org wrote:
> 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")

Indeed, didn't have this one on screen! Thanks!

> 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.

Yeah, in p9_msg_buf_size() [net/9p/protocol.c] the smallest allocation size
for message types known to be small (at compile-time) is hard coded to 4k.

However for all variable-size message types the size is calculated at runtime
exactly as needed for that particular message being sent. So these 9p message
types can trigger this case (<32). They are currently never rounded up.

[...]
> > 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),

AFAICS __entry is a local variable on stack, and array __entry->line not
intialized with zeros, i.e. the dump would contain trash at the end. Maybe
prepending memset() before memcpy()?

/Christian