ring-buffer: Fix a oob in __rb_map_vma

[PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Edward Adam Davis 1 year ago

syzbot report a slab-out-of-bounds in __rb_map_vma. [1]

Overflow occurred when performing the following calculation:
nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff;

Add a check before the calculation to avoid this problem.

[1]
BUG: KASAN: slab-out-of-bounds in __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
Read of size 8 at addr ffff8880767dd2b8 by task syz-executor187/5836

CPU: 0 UID: 0 PID: 5836 Comm: syz-executor187 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0xc3/0x620 mm/kasan/report.c:489
 kasan_report+0xd9/0x110 mm/kasan/report.c:602
 __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
 ring_buffer_map+0x56e/0x9b0 kernel/trace/ring_buffer.c:7138
 tracing_buffers_mmap+0xa6/0x120 kernel/trace/trace.c:8482
 call_mmap include/linux/fs.h:2183 [inline]
 mmap_file mm/internal.h:124 [inline]
 __mmap_new_file_vma mm/vma.c:2291 [inline]
 __mmap_new_vma mm/vma.c:2355 [inline]
 __mmap_region+0x1786/0x2670 mm/vma.c:2456
 mmap_region+0x127/0x320 mm/mmap.c:1348
 do_mmap+0xc00/0xfc0 mm/mmap.c:496
 vm_mmap_pgoff+0x1ba/0x360 mm/util.c:580
 ksys_mmap_pgoff+0x32c/0x5c0 mm/mmap.c:542
 __do_sys_mmap arch/x86/kernel/sys_x86_64.c:89 [inline]
 __se_sys_mmap arch/x86/kernel/sys_x86_64.c:82 [inline]
 __x64_sys_mmap+0x125/0x190 arch/x86/kernel/sys_x86_64.c:82
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
Tested-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 kernel/trace/ring_buffer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7e257e855dd1..15c43d7415d5 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -7019,6 +7019,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 	lockdep_assert_held(&cpu_buffer->mapping_lock);
 
 	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
+	if (((nr_subbufs + 1) << subbuf_order) < pgoff)
+		return -EINVAL;
+
 	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
 
 	nr_vma_pages = vma_pages(vma);
-- 
2.47.0

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Steven Rostedt 1 year ago

On Mon, 16 Dec 2024 22:07:57 +0800
Edward Adam Davis <eadavis@qq.com> wrote:

> syzbot report a slab-out-of-bounds in __rb_map_vma. [1]
> 
> Overflow occurred when performing the following calculation:
> nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff;
> 
> Add a check before the calculation to avoid this problem.
> 
> [1]
> BUG: KASAN: slab-out-of-bounds in __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
> Read of size 8 at addr ffff8880767dd2b8 by task syz-executor187/5836
> 
> CPU: 0 UID: 0 PID: 5836 Comm: syz-executor187 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
>  print_address_description mm/kasan/report.c:378 [inline]
>  print_report+0xc3/0x620 mm/kasan/report.c:489
>  kasan_report+0xd9/0x110 mm/kasan/report.c:602
>  __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
>  ring_buffer_map+0x56e/0x9b0 kernel/trace/ring_buffer.c:7138
>  tracing_buffers_mmap+0xa6/0x120 kernel/trace/trace.c:8482
>  call_mmap include/linux/fs.h:2183 [inline]
>  mmap_file mm/internal.h:124 [inline]
>  __mmap_new_file_vma mm/vma.c:2291 [inline]
>  __mmap_new_vma mm/vma.c:2355 [inline]
>  __mmap_region+0x1786/0x2670 mm/vma.c:2456
>  mmap_region+0x127/0x320 mm/mmap.c:1348
>  do_mmap+0xc00/0xfc0 mm/mmap.c:496
>  vm_mmap_pgoff+0x1ba/0x360 mm/util.c:580
>  ksys_mmap_pgoff+0x32c/0x5c0 mm/mmap.c:542
>  __do_sys_mmap arch/x86/kernel/sys_x86_64.c:89 [inline]
>  __se_sys_mmap arch/x86/kernel/sys_x86_64.c:82 [inline]
>  __x64_sys_mmap+0x125/0x190 arch/x86/kernel/sys_x86_64.c:82
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> 
> Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
> Tested-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  kernel/trace/ring_buffer.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 7e257e855dd1..15c43d7415d5 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7019,6 +7019,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>  	lockdep_assert_held(&cpu_buffer->mapping_lock);
>  
>  	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
> +	if (((nr_subbufs + 1) << subbuf_order) < pgoff)
> +		return -EINVAL;
> +

A proper fix is being discussed here:

https://lore.kernel.org/linux-trace-kernel/20241216164931.57323-1-aha310510@gmail.com/

Thank you,

-- Steve

>  	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
>  
>  	nr_vma_pages = vma_pages(vma);

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Edward Adam Davis 12 months ago

On Tue, 17 Dec 2024 12:46:02 -0500, Steven Rostedt wrote:
> On Mon, 16 Dec 2024 22:07:57 +0800
> Edward Adam Davis <eadavis@qq.com> wrote:
> 
> > syzbot report a slab-out-of-bounds in __rb_map_vma. [1]
> > 
> > Overflow occurred when performing the following calculation:
> > nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff;
> > 
> > Add a check before the calculation to avoid this problem.
> > 
> > [1]
> > BUG: KASAN: slab-out-of-bounds in __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
> > Read of size 8 at addr ffff8880767dd2b8 by task syz-executor187/5836
> > 
> > CPU: 0 UID: 0 PID: 5836 Comm: syz-executor187 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
> > Call Trace:
> >  <TASK>
> >  __dump_stack lib/dump_stack.c:94 [inline]
> >  dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
> >  print_address_description mm/kasan/report.c:378 [inline]
> >  print_report+0xc3/0x620 mm/kasan/report.c:489
> >  kasan_report+0xd9/0x110 mm/kasan/report.c:602
> >  __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
> >  ring_buffer_map+0x56e/0x9b0 kernel/trace/ring_buffer.c:7138
> >  tracing_buffers_mmap+0xa6/0x120 kernel/trace/trace.c:8482
> >  call_mmap include/linux/fs.h:2183 [inline]
> >  mmap_file mm/internal.h:124 [inline]
> >  __mmap_new_file_vma mm/vma.c:2291 [inline]
> >  __mmap_new_vma mm/vma.c:2355 [inline]
> >  __mmap_region+0x1786/0x2670 mm/vma.c:2456
> >  mmap_region+0x127/0x320 mm/mmap.c:1348
> >  do_mmap+0xc00/0xfc0 mm/mmap.c:496
> >  vm_mmap_pgoff+0x1ba/0x360 mm/util.c:580
> >  ksys_mmap_pgoff+0x32c/0x5c0 mm/mmap.c:542
> >  __do_sys_mmap arch/x86/kernel/sys_x86_64.c:89 [inline]
> >  __se_sys_mmap arch/x86/kernel/sys_x86_64.c:82 [inline]
> >  __x64_sys_mmap+0x125/0x190 arch/x86/kernel/sys_x86_64.c:82
> >  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> >  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
> >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > 
> > Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
> > Tested-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  kernel/trace/ring_buffer.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> > index 7e257e855dd1..15c43d7415d5 100644
> > --- a/kernel/trace/ring_buffer.c
> > +++ b/kernel/trace/ring_buffer.c
> > @@ -7019,6 +7019,9 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
> >  	lockdep_assert_held(&cpu_buffer->mapping_lock);
> >  
> >  	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
> > +	if (((nr_subbufs + 1) << subbuf_order) < pgoff)
> > +		return -EINVAL;
> > +
> 
> A proper fix is being discussed here:
First, my fix is the first one.
Second, the root cause of the problem is an overflow when calculating nr_pages.
> 
> https://lore.kernel.org/linux-trace-kernel/20241216164931.57323-1-aha310510@gmail.com/
> 
> Thank you,
> 
> -- Steve
> 
The calculation of nr_pages below overflows because the pgoff value is 8,
the nr_subbufs value is 3, and the subbuf_order value is 0.
> >  	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
> >  
> >  	nr_vma_pages = vma_pages(vma);
BR,
Edward

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Steven Rostedt 12 months ago

On Wed, 18 Dec 2024 07:43:46 +0800
Edward Adam Davis <eadavis@qq.com> wrote:

> > 
> > A proper fix is being discussed here:  
> First, my fix is the first one.

Yes I saw that.

> Second, the root cause of the problem is an overflow when calculating nr_pages.
> > 
> > https://lore.kernel.org/linux-trace-kernel/20241216164931.57323-1-aha310510@gmail.com/
> > 
> > Thank you,
> > 
> > -- Steve
> >   
> The calculation of nr_pages below overflows because the pgoff value is 8,
> the nr_subbufs value is 3, and the subbuf_order value is 0.

So basically you are saying that passing in the the mmap with the pgoff is
what's causing it.

> > >  	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
> > >  
> > >  	nr_vma_pages = vma_pages(vma);  


Thanks, I believe I now have a reproducer. And yes, I'll take your patch.
(If Vincent is OK with it).

Here's the reproducer:

------------------------8<-------------------------
#include <fcntl.h>
#include <stdlib.h>
#include <unistd.h>
#include <asm/types.h>
#include <sys/mman.h>

int main(int argc, char **argv)
{
	int page_size = getpagesize();
	int fd;
	void *meta;

	system("echo 1 > /sys/kernel/tracing/buffer_size_kb");
	fd = open("/sys/kernel/tracing/per_cpu/cpu0/trace_pipe_raw", O_RDONLY);

	meta = mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, page_size * 5);
}
------------------------>8-------------------------

Thanks,


-- Steve

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Vincent Donnefort 12 months ago

On Tue, Dec 17, 2024 at 07:40:15PM -0500, Steven Rostedt wrote:
> On Wed, 18 Dec 2024 07:43:46 +0800
> Edward Adam Davis <eadavis@qq.com> wrote:
> 
> > > 
> > > A proper fix is being discussed here:  
> > First, my fix is the first one.
> 
> Yes I saw that.
> 
> > Second, the root cause of the problem is an overflow when calculating nr_pages.
> > > 
> > > https://lore.kernel.org/linux-trace-kernel/20241216164931.57323-1-aha310510@gmail.com/
> > > 
> > > Thank you,
> > > 
> > > -- Steve
> > >   
> > The calculation of nr_pages below overflows because the pgoff value is 8,
> > the nr_subbufs value is 3, and the subbuf_order value is 0.
> 
> So basically you are saying that passing in the the mmap with the pgoff is
> what's causing it.
> 
> > > >  	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
> > > >  
> > > >  	nr_vma_pages = vma_pages(vma);  
> 
> 
> Thanks, I believe I now have a reproducer. And yes, I'll take your patch.
> (If Vincent is OK with it).

I wanted to look at the reproducer sent by Jeongjung yesterday but got
preempted. My bad.

To avoid repeating the (nr_subbufs + 1) << subbuf_order How about?

-       nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
+       nr_pages = ((nr_subbufs + 1) << subbuf_order); /* + meta-page */
+
+       if (pgoff > nr_pages)
+               return -EINVAL;
+
+       nr_pages -= pgoff;


And probably also 

Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")

> 
> Here's the reproducer:
> 
> ------------------------8<-------------------------
> #include <fcntl.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <asm/types.h>
> #include <sys/mman.h>
> 
> int main(int argc, char **argv)
> {
> 	int page_size = getpagesize();
> 	int fd;
> 	void *meta;
> 
> 	system("echo 1 > /sys/kernel/tracing/buffer_size_kb");
> 	fd = open("/sys/kernel/tracing/per_cpu/cpu0/trace_pipe_raw", O_RDONLY);
> 
> 	meta = mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, page_size * 5);
> }
> ------------------------>8-------------------------
> 
> Thanks,
> 
> 
> -- Steve

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Steven Rostedt 12 months ago

On Wed, 18 Dec 2024 09:13:43 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:

> And probably also 
> 
> Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")

I don't require patch submitters to add Fixes tags. It's more the
responsibility of the maintainer to do that. I still have to validate it as
there's been several times someone adds a Fixes tag which wasn't the right
commit that it fixed.

-- Steve

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Vincent Donnefort 12 months ago

On Wed, Dec 18, 2024 at 08:19:58AM -0500, Steven Rostedt wrote:
> On Wed, 18 Dec 2024 09:13:43 +0000
> Vincent Donnefort <vdonnefort@google.com> wrote:
> 
> > And probably also 
> > 
> > Fixes: 117c39200d9d ("ring-buffer: Introducing ring-buffer mapping functions")
> 
> I don't require patch submitters to add Fixes tags. It's more the
> responsibility of the maintainer to do that. I still have to validate it as
> there's been several times someone adds a Fixes tag which wasn't the right
> commit that it fixed.
> 
> -- Steve

Ack.

Aside, there's a selftest to check for the overflow of subbufs with the
mapping... but of course it didn't test with offset > nr_subbufs.

Do you think it is worth to extend it to cover this case? I'm happy to do a
quick patch.

-- 
Vincent

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Steven Rostedt 12 months ago

On Wed, 18 Dec 2024 14:31:31 +0000
Vincent Donnefort <vdonnefort@google.com> wrote:

> Aside, there's a selftest to check for the overflow of subbufs with the
> mapping... but of course it didn't test with offset > nr_subbufs.
> 
> Do you think it is worth to extend it to cover this case? I'm happy to do a
> quick patch.

Yes, it should check that case.

Thanks,

-- Steve

[PATCH V2] ring-buffer: fix overflow in __rb_map_vma

Posted by Edward Adam Davis 12 months ago

syzbot report a slab-out-of-bounds in __rb_map_vma. [1]

Overflow occurred when performing the following calculation:
nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff;

Add a check before the calculation to avoid this problem.

[1]
BUG: KASAN: slab-out-of-bounds in __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
Read of size 8 at addr ffff8880767dd2b8 by task syz-executor187/5836

CPU: 0 UID: 0 PID: 5836 Comm: syz-executor187 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0xc3/0x620 mm/kasan/report.c:489
 kasan_report+0xd9/0x110 mm/kasan/report.c:602
 __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
 ring_buffer_map+0x56e/0x9b0 kernel/trace/ring_buffer.c:7138
 tracing_buffers_mmap+0xa6/0x120 kernel/trace/trace.c:8482
 call_mmap include/linux/fs.h:2183 [inline]
 mmap_file mm/internal.h:124 [inline]
 __mmap_new_file_vma mm/vma.c:2291 [inline]
 __mmap_new_vma mm/vma.c:2355 [inline]
 __mmap_region+0x1786/0x2670 mm/vma.c:2456
 mmap_region+0x127/0x320 mm/mmap.c:1348
 do_mmap+0xc00/0xfc0 mm/mmap.c:496
 vm_mmap_pgoff+0x1ba/0x360 mm/util.c:580
 ksys_mmap_pgoff+0x32c/0x5c0 mm/mmap.c:542
 __do_sys_mmap arch/x86/kernel/sys_x86_64.c:89 [inline]
 __se_sys_mmap arch/x86/kernel/sys_x86_64.c:82 [inline]
 __x64_sys_mmap+0x125/0x190 arch/x86/kernel/sys_x86_64.c:82
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: updated according to Vincent Donnefort's suggestion, to avoid repeating the (nr_subbufs + 1) << subbuf_order

 kernel/trace/ring_buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7e257e855dd1..20f0e01b7a50 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -7019,7 +7019,11 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 	lockdep_assert_held(&cpu_buffer->mapping_lock);
 
 	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
-	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
+	nr_pages = ((nr_subbufs + 1) << subbuf_order); /* + meta-page */
+	if (nr_pages < pgoff)
+		return -EINVAL;
+
+	nr_pages -= pgoff;
 
 	nr_vma_pages = vma_pages(vma);
 	if (!nr_vma_pages || nr_vma_pages > nr_pages)
-- 
2.47.0

Re: [PATCH V2] ring-buffer: fix overflow in __rb_map_vma

Posted by Steven Rostedt 12 months ago

On Wed, 18 Dec 2024 19:42:22 +0800
Edward Adam Davis <eadavis@qq.com> wrote:

> Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
> V1 -> V2: updated according to Vincent Donnefort's suggestion, to avoid repeating the (nr_subbufs + 1) << subbuf_order
> 
>  kernel/trace/ring_buffer.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Also, when sending a new version of a patch, do not reply to the previous
version as that hides the new patch. It should start a new thread.
Otherwise it screws up tooling and also hides patches. I've missed patches
because they were replied to the previous patch. It makes it much harder on
the maintainer when someone does that.

What I usually do to maintain a history chain is have:

Signed-off-by: ...
---
Changes since v1: https://lore.kernel.org/all/tencent_E036A29600368E4A2075A7774D67023CFD09@qq.com/

- Updated according to Vincent Donnefort's suggestion, to avoid repeating
  the (nr_subbufs + 1) << subbuf_order

-- Steve

Re: [PATCH V2] ring-buffer: fix overflow in __rb_map_vma

Posted by Steven Rostedt 12 months ago

On Wed, 18 Dec 2024 19:42:22 +0800
Edward Adam Davis <eadavis@qq.com> wrote:

> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 7e257e855dd1..20f0e01b7a50 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -7019,7 +7019,11 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
>  	lockdep_assert_held(&cpu_buffer->mapping_lock);
>  
>  	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
> -	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
> +	nr_pages = ((nr_subbufs + 1) << subbuf_order); /* + meta-page */
> +	if (nr_pages < pgoff)

That probably should be <= as if it was equal...

> +		return -EINVAL;
> +
> +	nr_pages -= pgoff;

nr_pages would be zero and

>  
>  	nr_vma_pages = vma_pages(vma);
>  	if (!nr_vma_pages || nr_vma_pages > nr_pages)

this would return true, which the next line is:

		return -EINVAL;

Why not catch it before going through all that?

-- Steve


> --

[PATCH V3] ring-buffer: fix overflow in __rb_map_vma

Posted by Edward Adam Davis 12 months ago

syzbot report a slab-out-of-bounds in __rb_map_vma. [1]

Overflow occurred when performing the following calculation:
nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff;

Add a check before the calculation to avoid this problem.

[1]
BUG: KASAN: slab-out-of-bounds in __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
Read of size 8 at addr ffff8880767dd2b8 by task syz-executor187/5836

CPU: 0 UID: 0 PID: 5836 Comm: syz-executor187 Not tainted 6.13.0-rc2-syzkaller-00159-gf932fb9b4074 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 11/25/2024
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:94 [inline]
 dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
 print_address_description mm/kasan/report.c:378 [inline]
 print_report+0xc3/0x620 mm/kasan/report.c:489
 kasan_report+0xd9/0x110 mm/kasan/report.c:602
 __rb_map_vma+0x9ab/0xae0 kernel/trace/ring_buffer.c:7058
 ring_buffer_map+0x56e/0x9b0 kernel/trace/ring_buffer.c:7138
 tracing_buffers_mmap+0xa6/0x120 kernel/trace/trace.c:8482
 call_mmap include/linux/fs.h:2183 [inline]
 mmap_file mm/internal.h:124 [inline]
 __mmap_new_file_vma mm/vma.c:2291 [inline]
 __mmap_new_vma mm/vma.c:2355 [inline]
 __mmap_region+0x1786/0x2670 mm/vma.c:2456
 mmap_region+0x127/0x320 mm/mmap.c:1348
 do_mmap+0xc00/0xfc0 mm/mmap.c:496
 vm_mmap_pgoff+0x1ba/0x360 mm/util.c:580
 ksys_mmap_pgoff+0x32c/0x5c0 mm/mmap.c:542
 __do_sys_mmap arch/x86/kernel/sys_x86_64.c:89 [inline]
 __se_sys_mmap arch/x86/kernel/sys_x86_64.c:82 [inline]
 __x64_sys_mmap+0x125/0x190 arch/x86/kernel/sys_x86_64.c:82
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f

Reported-by: syzbot+345e4443a21200874b18@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=345e4443a21200874b18
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
V1 -> V2: updated according to Vincent Donnefort's suggestion, to avoid repeating the (nr_subbufs + 1) << subbuf_order
V2 -> V3: Add equal condition judgment

 kernel/trace/ring_buffer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 7e257e855dd1..20f0e01b7a50 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -7019,7 +7019,11 @@ static int __rb_map_vma(struct ring_buffer_per_cpu *cpu_buffer,
 	lockdep_assert_held(&cpu_buffer->mapping_lock);
 
 	nr_subbufs = cpu_buffer->nr_pages + 1; /* + reader-subbuf */
-	nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
+	nr_pages = ((nr_subbufs + 1) << subbuf_order); /* + meta-page */
+	if (nr_pages <= pgoff)
+		return -EINVAL;
+
+	nr_pages -= pgoff;
 
 	nr_vma_pages = vma_pages(vma);
 	if (!nr_vma_pages || nr_vma_pages > nr_pages)
-- 
2.47.0

Re: [PATCH] ring-buffer: Fix a oob in __rb_map_vma

Posted by Jeongjun Park 12 months ago


> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> On Wed, 18 Dec 2024 07:43:46 +0800
> Edward Adam Davis <eadavis@qq.com> wrote:
> 
>>> 
>>> A proper fix is being discussed here:  
>> First, my fix is the first one.
> 
> Yes I saw that.
> 
>> Second, the root cause of the problem is an overflow when calculating nr_pages.
>>> 
>>> https://lore.kernel.org/linux-trace-kernel/20241216164931.57323-1-aha310510@gmail.com/
>>> 
>>> Thank you,
>>> 
>>> -- Steve
>>> 

Oh, I did not propose a patch that fixes the root cause of this oob. My patch 
is a separate patch that fixes the wrong order, such as reading the array 
before checking the variable s with the WARN function in the loop. So I think 
both of these patches should be applied.

Regards,

Jeongjun Park

>> The calculation of nr_pages below overflows because the pgoff value is 8,
>> the nr_subbufs value is 3, and the subbuf_order value is 0.
> 
> So basically you are saying that passing in the the mmap with the pgoff is
> what's causing it.
> 
>>>>    nr_pages = ((nr_subbufs + 1) << subbuf_order) - pgoff; /* + meta-page */
>>>> 
>>>>    nr_vma_pages = vma_pages(vma);  
> 
> 
> Thanks, I believe I now have a reproducer. And yes, I'll take your patch.
> (If Vincent is OK with it).
> 
> Here's the reproducer:
> 
> ------------------------8<-------------------------
> #include <fcntl.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <asm/types.h>
> #include <sys/mman.h>
> 
> int main(int argc, char **argv)
> {
>    int page_size = getpagesize();
>    int fd;
>    void *meta;
> 
>    system("echo 1 > /sys/kernel/tracing/buffer_size_kb");
>    fd = open("/sys/kernel/tracing/per_cpu/cpu0/trace_pipe_raw", O_RDONLY);
> 
>    meta = mmap(NULL, page_size, PROT_READ, MAP_SHARED, fd, page_size * 5);
> }
> ------------------------>8-------------------------
> 
> Thanks,
> 
> 
> -- Steve