From a931c6816078af3e306e0f444f492396ce40de31 Mon Sep 17 00:00:00 2001 From: JP Kobryn Date: Mon, 4 Dec 2023 12:23:20 -0800 Subject: 9p: prevent read overrun in protocol dump tracepoint 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 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 Message-ID: <20231204202321.22730-1-inwardvessel@gmail.com> Fixes: 60ece0833b6c ("net/9p: allocate appropriate reduced message buffers") Cc: stable@vger.kernel.org Reviewed-by: Christian Schoenebeck Signed-off-by: Dominique Martinet --- 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)) ); -- cgit v1.2.3 From ff49bf1867578f23a5ffdd38f927f6e1e16796c4 Mon Sep 17 00:00:00 2001 From: Fedor Pchelkin Date: Wed, 6 Dec 2023 23:09:13 +0300 Subject: net: 9p: avoid freeing uninit memory in p9pdu_vreadf If some of p9pdu_readf() calls inside case 'T' in p9pdu_vreadf() fails, the error path is not handled properly. *wnames or members of *wnames array may be left uninitialized and invalidly freed. Initialize *wnames to NULL in beginning of case 'T'. Initialize the first *wnames array element to NULL and nullify the failing *wnames element so that the error path freeing loop stops on the first NULL element and doesn't proceed further. Found by Linux Verification Center (linuxtesting.org). Fixes: ace51c4dd2f9 ("9p: add new protocol support code") Signed-off-by: Fedor Pchelkin Message-ID: <20231206200913.16135-1-pchelkin@ispras.ru> Cc: stable@vger.kernel.org Reviewed-by: Simon Horman Reviewed-by: Christian Schoenebeck Signed-off-by: Dominique Martinet --- net/9p/protocol.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/net/9p/protocol.c b/net/9p/protocol.c index 4e3a2a1ffcb3..0e6603b1ec90 100644 --- a/net/9p/protocol.c +++ b/net/9p/protocol.c @@ -394,6 +394,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, uint16_t *nwname = va_arg(ap, uint16_t *); char ***wnames = va_arg(ap, char ***); + *wnames = NULL; + errcode = p9pdu_readf(pdu, proto_version, "w", nwname); if (!errcode) { @@ -403,6 +405,8 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, GFP_NOFS); if (!*wnames) errcode = -ENOMEM; + else + (*wnames)[0] = NULL; } if (!errcode) { @@ -414,8 +418,10 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, proto_version, "s", &(*wnames)[i]); - if (errcode) + if (errcode) { + (*wnames)[i] = NULL; break; + } } } @@ -423,11 +429,14 @@ p9pdu_vreadf(struct p9_fcall *pdu, int proto_version, const char *fmt, if (*wnames) { int i; - for (i = 0; i < *nwname; i++) + for (i = 0; i < *nwname; i++) { + if (!(*wnames)[i]) + break; kfree((*wnames)[i]); + } + kfree(*wnames); + *wnames = NULL; } - kfree(*wnames); - *wnames = NULL; } } break; -- cgit v1.2.3