On Saturday, December 2, 2023 5:35:18 AM CET asmadeus@xxxxxxxxxxxxx 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