Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] Untrusted meta->nr_subbufs from persistent trace buffer memory is used without bounds checking against the allocated mask size, causing a heap out-of-bounds write. - [High] Incorrect offset calculation in rb_meta_subbuf_idx omits the size of the ring_buffer_cpu_meta structure, which can cause the function to return a corrupted subbuffer index (off-by-one). -- commit 0ea297fd27320f1e57a1d58538c8b811080de4c8 Author: Vincent Donnefort <[email protected]> ring-buffer: Prevent truncation of nr_pages / nr_subbufs Although ring_buffer_per_cpu::nr_pages is defined as unsigned long, it is capped to 32-bits in a few places, limiting the operations possible on a very large buffer. Use unsigned long where appropriate and prevent truncation of values using nr_pages (or nr_subbufs). > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 6b914abd80e3c..504a01a1d3807 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c [ ... ] > @@ -1871,7 +1869,7 @@ static bool rb_cpu_meta_valid(struct > ring_buffer_cpu_meta *meta, int cpu, > } > > buffers_start = meta->first_buffer; > - buffers_end = meta->first_buffer + (subbuf_size * meta->nr_subbufs); > + buffers_end = meta->first_buffer + (meta->nr_subbufs * PAGE_SIZE); > > /* Is the head and commit buffers within the range of buffers? */ > if (meta->head_buffer < buffers_start || [Severity: Critical] This is a pre-existing issue, but does rb_cpu_meta_valid() lack bounds checking on the untrusted meta->nr_subbufs before it is used? A bit further down in this function, the code does: bitmap_clear(subbuf_mask, 0, meta->nr_subbufs); and then loops up to meta->nr_subbufs doing: set_bit(meta->buffers[i], subbuf_mask); Since subbuf_mask is allocated in rb_range_meta_init() using nr_pages + 1, and meta->nr_subbufs is read directly from persistent memory, could a larger meta->nr_subbufs value from a previous boot cause a heap out-of-bounds write when clearing and setting bits? [ ... ] > @@ -2345,8 +2344,8 @@ static int rbm_show(struct seq_file *m, void *v) > rb_meta_subbuf_idx(meta, (void *)meta->head_buffer)); > seq_printf(m, "commit_buffer: %d\n", > rb_meta_subbuf_idx(meta, (void > *)meta->commit_buffer)); > - seq_printf(m, "subbuf_size: %d\n", meta->subbuf_size); > - seq_printf(m, "nr_subbufs: %d\n", meta->nr_subbufs); > + seq_printf(m, "subbuf_size: %u\n", meta->subbuf_size); > + seq_printf(m, "nr_subbufs: %u\n", meta->nr_subbufs); > return 0; > } [Severity: High] This is a pre-existing issue, but is the calculation in rb_meta_subbuf_idx() omitting the size of the ring_buffer_cpu_meta structure? Looking at rb_meta_subbuf_idx(): subbuf_array = (void *)meta + sizeof(int) * meta->nr_subbufs; subbuf_array = (void *)ALIGN((unsigned long)subbuf_array, meta->subbuf_size); Does this missing sizeof(struct ring_buffer_cpu_meta) cause the function to return an off-by-one subbuffer index for specific persistent trace buffer sizes? If sizeof(int) * meta->nr_subbufs modulo PAGE_SIZE is between PAGE_SIZE - sizeof(struct ring_buffer_cpu_meta) and PAGE_SIZE - 1, the alignment might mask the offset differently, returning a corrupted index. Could this trigger the WARN_ON in rb_setup_ids_meta_page() and lead to a NULL pointer dereference when mapping the persistent ring buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
