[PATCH v2] 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
---
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))
);
--
2.43.0
Re: [PATCH] 9p: prevent read overrun in protocol dump tracepoint
On Sun, Dec 03, 2023 at 02:32:15PM +0900, Dominique Martinet wrote: > Steven Rostedt wrote on Sat, Dec 02, 2023 at 11:15:24PM -0500: > > > Also, for custom tracepoints e.g. bpftrace the program needs to know how > > > many bytes can be read safely even if it's just for dumping -- unless > > > dynamic_array is a "fat pointer" that conveys its own size? > > > (Sorry didn't take the time to check) > > > > Yes, there's also a __get_dynamic_array_len(line) that will return the > > allocated length of the line. Is that what you need? > > Yes, thanks! So the lower two bytes of the field are its position in > the entry and the higher two bytes its size; ok. > It doesn't look like bpftrace has any helper for it but that can > probably be sorted out if someone wants to dump data there. > > > Let's update the event to use a dynamic array and have printk fomrat to > use %*ph with that length. > > JP Kobryn, does that sound good to you? I'm not sure what you were > trying to do in the first place. > Do you want to send a v2 or shall I? Sounds good. I'll send out a v2. Thanks Steve for recommending the dynamic array macros. JP > > -- > Dominique Martinet | Asmadeus
[PATCH] 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 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.
A repro can be performed by different ways. The simplest 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
---
include/trace/events/9p.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
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),
--
2.43.0
Re: [PATCH v2] rethook: Use __rcu pointer for rethook::handler
else
> call_rcu(>rcu, free_rethook_node_rcu);
> @@ -153,9 +160,7 @@ NOKPROBE_SYMBOL(rethook_recycle);
> */
> struct rethook_node *rethook_try_get(struct rethook *rh)
> {
> - rethook_handler_t handler = READ_ONCE(rh->handler);
> -
> - lockdep_assert_preemption_disabled();
> + rethook_handler_t handler = rethook_get_handler(rh);
>
> /* Check whether @rh is going to be freed. */
> if (unlikely(!handler))
> @@ -300,7 +305,7 @@ unsigned long rethook_trampoline_handler(struct pt_regs
> *regs,
> rhn = container_of(first, struct rethook_node, llist);
> if (WARN_ON_ONCE(rhn->frame != frame))
> break;
> - handler = READ_ONCE(rhn->rethook->handler);
> + handler = rethook_get_handler(rhn->rethook);
> if (handler)
> handler(rhn, rhn->rethook->data,
> correct_ret_addr, regs);
>
I applied and tested this patch locally on an x86_64 machine and can
confirm there are no RCU-related sparse warnings. Also, kprobe tests
within the ftrace selftest suite succeed just the same as before
applying the patch.
Tested-by: JP Kobryn
