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

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 <[email protected]>

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/[email protected]?part=1