| Message ID | tencent_11A0A5D80DEC925EDDA2DDB8990F4FC88D07@qq.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [V2] ring-buffer: fix overflow in __rb_map_vma | expand |
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 > --
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
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)
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(-)