Re: [PATCH] ring-buffer: fix array bounds checking

[Vincent Donnefort <vdonnefort@google.com>]

On Tue, Dec 17, 2024 at 10:28:49AM +0900, Jeongjun Park wrote:
> 
> 
> > Vincent Donnefort <vdonnefort@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-988b81fcc58b@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 <aha310510@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;
> >> --