On Tue, Dec 17, 2024 at 01:49:30AM +0900, Jeongjun Park 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.
Hi Jeongjun, thanks for the patch. Did you find a reproducer for that problem or has it just been found by code inspection? As discussed here [1], s >= nr_subbufs should really never happen as we already cap nr_pages. [1] https://lore.kernel.org/all/78e20e98-bdfc-4d7b-a59c-988b81fcc...@redhat.com/, > > Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions") > Signed-off-by: Jeongjun Park <aha310...@gmail.com> > --- > kernel/trace/ring_buffer.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c > index 7e257e855dd1..83da74bf7bd6 100644 > --- a/kernel/trace/ring_buffer.c > +++ b/kernel/trace/ring_buffer.c > @@ -6994,9 +6994,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu > *cpu_buffer, > { > unsigned long nr_subbufs, nr_pages, nr_vma_pages, pgoff = vma->vm_pgoff; > unsigned int subbuf_pages, subbuf_order; > - struct page **pages; > + struct page **pages, *page; > int p = 0, s = 0; > - int err; > + int err, off; > > /* Refuse MP_PRIVATE or writable mappings */ > if (vma->vm_flags & VM_WRITE || vma->vm_flags & VM_EXEC || > @@ -7055,14 +7055,14 @@ 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]); > - int off = 0; > - I believe we can keep the struct page and off declaration within the while loop. > if (WARN_ON_ONCE(s >= nr_subbufs)) { > err = -EINVAL; > goto out; > } > > + page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]); > + off = 0; > + > for (; off < (1 << (subbuf_order)); off++, page++) { > if (p >= nr_pages) > break; > --
