Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
From:   "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To:     David Howells <dhowells@redhat.com>
Cc:     dhowells@redhat.com, christophe.jaillet@wanadoo.fr,
        linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] KASAN: null-ptr-deref Read in __free_pages
Date:   Mon, 21 Mar 2022 10:02:57 +0100
Message-ID: <1745593.TLkxdtWsSY@leap>
In-Reply-To: <977073.1647852411@warthog.procyon.org.uk>
References: <1821968.CQOukoFCf9@leap> <000000000000b1807c05daad8f98@google.com> <977073.1647852411@warthog.procyon.org.uk>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

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