Re: [PATCH] ring-buffer: fix incorrect boundary check order
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2025-01-10 17:25:32
Also in:
lkml
On Sat, 11 Jan 2025 01:26:12 +0900 Jeongjun Park [off-list ref] wrote:
If there is a case where the variable s is greater than or equal to nr_subbufs before entering the loop, oob read or use-after-free will occur. This problem occurs because the variable s is used as an index to dereference the struct page before the variable value range check. This logic prevents the wrong address value from being copied to the pages array through the subsequent range check, but oob read still occurs, so the code needs to be modified.
This is *not* a fix! As this should never happen. The logic before it prevents it from happening. The s is calculated from the same logic that calculates nr_pages. If s were to be greater than nr_subbufs it is a bug!
quoted hunk ↗ jump to hunk
Signed-off-by: Jeongjun Park <redacted> --- kernel/trace/ring_buffer.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c index 60210fb5b211..6804ab126802 100644 --- a/kernel/trace/ring_buffer.c +++ b/kernel/trace/ring_buffer.c@@ -7059,7 +7059,7 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer, } while (p < nr_pages) { - struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]); + struct page *page; int off = 0; if (WARN_ON_ONCE(s >= nr_subbufs)) {
Notice that there's a WARN_ON_ONCE() here. That means if this is true, the code is buggy. User space should never be able to trigger this either. That said, I'm not against the patch and will add it for the next merge window. But I will modify the subject and change log to state that this should never happen and if it does the code is buggy and needs to be fixed. That this is just a clean up in case the code is buggy we can trigger the WARN_ON() and not worry about an out of bounds hit. Something like: ring-buffer: Make reading page consistent with the code logic In the loop of __rb_map_vma(), the 's' variable is calculated from the same logic that nr_pages is and they both come from nr_subbufs. But the relationship is not obvious and there's a WARN_ON_ONCE() around the 's' variable to make sure it never becomes equal to nr_subbufs within the loop. If that happens, then the code is buggy and needs to be fixed. The 'page' variable is calculated from cpu_buffer->subbuf_ids[s] which is an array of 'nr_subbufs' entries. If the code becomes buggy and 's' becomes equal to or greater than 'nr_subbufs' then this will be an out of bounds hit before the WARN_ON() is triggered and the code exiting safely. Make the 'page' initialization consistent with the code logic and assign it after the out of bounds check. -- Steve
quoted hunk ↗ jump to hunk
@@ -7067,6 +7067,8 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer, goto out; } + page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]); + for (; off < (1 << (subbuf_order)); off++, page++) { if (p >= nr_pages) break; --