| Message ID | 20241216164931.57323-1-aha310510@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | ring-buffer: fix array bounds checking | expand |
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/, > > 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. > 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; > --
> 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. 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. > >> >> 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? 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; >> --
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; > >> --
Vincent Donnefort <vdonnefort@google.com> wrote: > > 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. [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@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; > > >> --
On Tue, 17 Dec 2024 22:42:53 +0900 Jeongjun Park <aha310510@gmail.com> wrote: > > 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 This was fixed by Edwards patch. > Okay. In that case, I will just remove the variable declaration related patches > and send you the v2 patch right away. > I'm not sure this is needed nor is it a bug. while (p < nr_pages) { struct page *page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]); int off = 0; if (WARN_ON_ONCE(s >= nr_subbufs)) { err = -EINVAL; goto out; } The WARN_ON_ONCE() suggests that this should never happen. And I believe it shouldn't. I'm fine if you want to make the change to: while (p < nr_pages) { struct page *page; int off = 0; if (WARN_ON_ONCE(s >= nr_subbufs)) { err = -EINVAL; goto out; } page = virt_to_page((void *)cpu_buffer->subbuf_ids[s]); But it's not a bug fix. It's simply a cleanup that can wait till the next merge window. -- Steve
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; - 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;
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. 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(-) --