Messages in this thread | | | From | "Fabio M. De Francesco" <> | Subject | Re: [syzbot] KASAN: null-ptr-deref Read in __free_pages | Date | Mon, 21 Mar 2022 10:02:57 +0100 |
| |
On luned? 21 marzo 2022 09:46:51 CET David Howells wrote: > Hi Fabio, > > How about using my patch instead? I wonder if __free_pages() should check for > a NULL pages pointer... > > David > --- > watch_queue: Fix NULL dereference in error cleanup > > In watch_queue_set_size(), the error cleanup code doesn't take account of > the fact that __free_page() can't handle a NULL pointer when trying to free > up buffer pages that did get allocated. > > Fix this by only calling __free_page() on non-NULL pointers extracted from > the list (the list was allocated by kcalloc() so was pre-cleared). > > Without the fix, this can lead to something like the following: > > BUG: KASAN: null-ptr-deref in __free_pages+0x1f/0x1b0 mm/page_alloc.c:5473 > Read of size 4 at addr 0000000000000034 by task syz-executor168/3599 > ... > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > __kasan_report mm/kasan/report.c:446 [inline] > kasan_report.cold+0x66/0xdf mm/kasan/report.c:459 > check_region_inline mm/kasan/generic.c:183 [inline] > kasan_check_range+0x13d/0x180 mm/kasan/generic.c:189 > instrument_atomic_read include/linux/instrumented.h:71 [inline] > atomic_read include/linux/atomic/atomic-instrumented.h:27 [inline] > page_ref_count include/linux/page_ref.h:67 [inline] > put_page_testzero include/linux/mm.h:717 [inline] > __free_pages+0x1f/0x1b0 mm/page_alloc.c:5473 > watch_queue_set_size+0x499/0x630 kernel/watch_queue.c:275 > pipe_ioctl+0xac/0x2b0 fs/pipe.c:632 > vfs_ioctl fs/ioctl.c:51 [inline] > __do_sys_ioctl fs/ioctl.c:874 [inline] > __se_sys_ioctl fs/ioctl.c:860 [inline] > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:860 > do_syscall_x64 arch/x86/entry/common.c:50 [inline] > do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > Fixes: c73be61cede5 ("pipe: Add general notification queue support") > Reported-and-tested-by: syzbot+d55757faa9b80590767b@syzkaller.appspotmail.com > Signed-off-by: David Howells <dhowells@redhat.com> > --- > kernel/watch_queue.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/kernel/watch_queue.c b/kernel/watch_queue.c > index 00703444a219..09f35bab496e 100644 > --- a/kernel/watch_queue.c > +++ b/kernel/watch_queue.c > @@ -272,7 +272,8 @@ long watch_queue_set_size(struct pipe_inode_info *pipe, unsigned int nr_notes) > > error_p: > for (i = 0; i < nr_pages; i++) > - __free_page(pages[i]); > + if (pages[i]) > + __free_page(pages[i]); > kfree(pages); > error: > (void) account_pipe_buffers(pipe->user, nr_pages, pipe->nr_accounted); > > I cannot have any objection, it's up to you to decide :)
I just noticed that you now have two tests in one loop: the first is for the index less than an unnecessary high value: the second is for pages[i] being NULL.
However, I have no other objections. Please use your patch instead.
Thanks,
Fabio M. De Francesco
| |