[PATCH] tracing: Fix use-after-free in trace_pipe read on sub-buffer order change

[PATCH] tracing: Fix use-after-free in trace_pipe read on sub-buffer order change

[Deepanshu Kartikey]

Writing to buffer_subbuf_size_kb calls ring_buffer_subbuf_order_set(),
which frees every sub-buffer of the ring buffer, including the reader
page, and replaces them with newly allocated ones.

Readers of trace_pipe hold pointers into those pages. ring_buffer_peek()
looks up an event under cpu_buffer->reader_lock but returns the event
pointer after dropping the lock, and peek_next_entry() then calls
ring_buffer_event_length() and ring_buffer_event_data() on it. If the
sub-buffer order is changed in that window, the reader dereferences
freed memory:

  BUG: KASAN: use-after-free in ring_buffer_peek+0x3e0/0x430
  Read of size 1 at addr ffff88802a4cf010 by task syz-executor989/6002

  Freed by:
   free_buffer_page kernel/trace/ring_buffer.c:398 [inline]
   ring_buffer_subbuf_order_set+0x1325/0x18e0 kernel/trace/ring_buffer.c:7444
   buffer_subbuf_size_write+0x182/0x280 kernel/trace/trace.c:8221

Take trace_access_lock(RING_BUFFER_ALL_CPUS) around the order change.
This is the lock trace_pipe readers already hold across their entire
peek-and-print loop, so the swap can no longer race with a reader that
is dereferencing a peeked event.

Fixes: f9b94daa542a ("ring-buffer: Set new size of the ring buffer sub page")
Reported-by: syzbot+685955db58555575fdd2@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=685955db58555575fdd2
Tested-by: syzbot+685955db58555575fdd2@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 kernel/trace/trace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 395238b2b715..c38a5de602e0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -8214,6 +8214,8 @@ buffer_subbuf_size_write(struct file *filp, const char __user *ubuf,
 	/* Do not allow tracing while changing the order of the ring buffer */
 	tracing_stop_tr(tr);
 
+	trace_access_lock(RING_BUFFER_ALL_CPUS);
+
 	old_order = ring_buffer_subbuf_order_get(tr->array_buffer.buffer);
 	if (old_order == order)
 		goto out;
@@ -8253,6 +8255,7 @@ buffer_subbuf_size_write(struct file *filp, const char __user *ubuf,
 #endif
 	(*ppos)++;
  out:
+	trace_access_unlock(RING_BUFFER_ALL_CPUS);
 	if (ret)
 		cnt = ret;
 	tracing_start_tr(tr);
-- 
2.43.0

Re: [PATCH] tracing: Fix use-after-free in trace_pipe read on sub-buffer order change

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch prevents a use-after-free for `trace_pipe` readers but leaves standard `trace` (seq_file) readers and `tracing_splice_read_pipe()` vulnerable to the exact same use-after-free.
- [High] Snapshot buffer allocation and freeing call `ring_buffer_subbuf_order_set()` without holding `trace_access_lock`, leaving snapshot readers vulnerable to the exact same Use-After-Free race.
- [High] Ring buffer resizing (`buffer_size_kb`) frees active pages without taking `trace_access_lock`, leading to a use-after-free for `seq_file` iterators.
--

--- Patch [1]: [PATCH] tracing: Fix use-after-free in trace_pipe read on sub-buffer order change ---
commit 36ad4aeed5301b8016b09a675a4d6aac8686489f
Author: Deepanshu Kartikey <kartikey406@gmail.com>

tracing: Fix use-after-free in trace_pipe read on sub-buffer order change

Writing to buffer_subbuf_size_kb calls ring_buffer_subbuf_order_set(),
which frees every sub-buffer of the ring buffer, including the reader
page, and replaces them with newly allocated ones. This patch adds
trace_access_lock protection to prevent a use-after-free when readers
hold pointers into those pages.

Closes: https://syzkaller.appspot.com/bug?extid=685955db58555575fdd2

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 19b0d7a53c589..34328916245df 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
[ ... ]
> @@ -8216,6 +8216,8 @@ buffer_subbuf_size_write(struct file *filp, const char __user *ubuf,
>  	/* Do not allow tracing while changing the order of the ring buffer */
>  	tracing_stop_tr(tr);
>  
> +	trace_access_lock(RING_BUFFER_ALL_CPUS);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does the same use-after-free
still exist for seq_file readers and tracing_splice_read_pipe()?

While trace_pipe readers are now protected here, s_start() and
tracing_splice_read_pipe() appear to peek the first event before taking
the access lock:

kernel/trace/trace.c:tracing_splice_read_pipe() {
    if (!iter->ent && !trace_find_next_entry_inc(iter)) {
        ret = -EFAULT;
        goto out_err;
    }

    trace_event_read_lock();
    trace_access_lock(iter->cpu_file);
    ...
}

If a concurrent write to buffer_subbuf_size_kb runs during this unprotected
window, could iter->ent be left pointing to freed memory?

[Severity: High]
This is a pre-existing issue, but do snapshot buffers need similar
protection?

It looks like free_snapshot() and tracing_alloc_snapshot_instance() call
ring_buffer_subbuf_order_set() without holding trace_access_lock:

kernel/trace/trace_snapshot.c:free_snapshot() {
    ...
    ring_buffer_subbuf_order_set(tr->snapshot_buffer.buffer, 0);
    ring_buffer_resize(tr->snapshot_buffer.buffer, 1, RING_BUFFER_ALL_CPUS);
    ...
}

Since the snapshot seq_file reader drops trace_types_lock before iterating,
could a concurrent read of the snapshot file and a write to the snapshot
file trigger the same use-after-free by freeing the pages while readers
are active?

[Severity: High]
This is also a pre-existing issue, but does ring buffer resizing
leave seq_file iterators vulnerable to a similar race?

When writing to buffer_size_kb, __tracing_resize_ring_buffer() is called
without taking trace_access_lock:

kernel/trace/trace.c:__tracing_resize_ring_buffer() {
    tracing_stop_tr(tr);
    ret = ring_buffer_resize(tr->array_buffer.buffer, size, cpu);
    ...
}

Since ring_buffer_resize() frees active pages directly, could this cause a
use-after-free if a seq_file reader is concurrently holding an event pointer
that points into one of the freed pages?

>  	old_order = ring_buffer_subbuf_order_get(tr->array_buffer.buffer);
>  	if (old_order == order)
>  		goto out;
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817140655.5694-1-kartikey406@gmail.com?part=1