Message-ID: <CAO9qdTGwQJvVZt2HtnBUdJGLti0ZWuPeqVFginJYcwY2xLnwRg@mail.gmail.com>
Date: Tue, 17 Dec 2024 22:42:53 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Vincent Donnefort <vdonnefort@...gle.com>
Cc: rostedt@...dmis.org, mhiramat@...nel.org, mathieu.desnoyers@...icios.com, 
	david@...hat.com, linux-kernel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH] ring-buffer: fix array bounds checking

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

[1] When tested with a reproducer, pgoff was 8, subbuf_order was 0, and
subbuf_pages was 1. However, nr_subbufs was 3, so oob-read or uaf occurred.

[1] : https://syzkaller.appspot.com/text?tag=ReproC&x=14514730580000

>
> >
> > 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@...il.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.

Okay. In that case, I will just remove the variable declaration related patches
and send you the v2 patch right away.

Regards,

Jeongjun Park

>
> >
> > 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;
> > >> --

Powered by blists - more mailing lists