Re: [syzbot] KASAN: null-ptr-deref Read in __free_pages

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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