On Tue, Dec 17, 2024 at 10:28:49AM +0900, Jeongjun Park wrote:
>
>
> > Vincent Donnefort <vdonnef...@google.com> wrote:
> > 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/,
>
> I didn't find the bug caused by this separately, but I found it while
> analyzing
> the code. However, since it has been confirmed that syzbot
> has a reproducer that generates oob and uaf, this will definitely be
> reproduced.
Could you share that reproducer? Or at least the steps. As this situation should
never happen a, follow-up fix will be necessary.
>
> The reason I suggested this patch is because I think the logic of the code
> is a bit inappropriate. Normally, a range check is performed before using
> a specific variable as an index of an array. Of course, in this loop, the page
> structure pointer that was oob-read will not be copied to the pages array,
> but I don't think it's very appropriate to read the array using a variable
> value that may be out of range as an index before the range check.
> Therefore, I suggest patching it like this.
Of course, no question about that.
>
> >
> >>
> >> 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.
>
> The reason I modified it this way is that, since this loop will always be
> entered if there are no other issues, these variables will be used in
> many situations, so I think it is quite inefficient to continue to declare
> variables
> in a loop where you don't know how many times it will be repeated.
> So, I think that declaring variables in advance and then continuously
> initializing
> their values is advantageous in terms of performance and there are
> no other issues. What do you think?
I'm pretty sure the compiler would do the right thing here and no additional
step would result from declaring both variables inside the loop.
>
> Regards,
>
> Jeongjun Park
>
> >
> >> 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;
> >> --
