Vincent Donnefort <vdonnef...@google.com> wrote: > > 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.
[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 <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. 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; > > >> --
