Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()

From: Shuah Khan <skhan@linuxfoundation.org>
To: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Valentina Manea <valentina.manea.m@gmail.com>,
	Shuah Khan <shuah@kernel.org>
Cc: Hillf Danton <hdanton@sina.com>,
	linux-usb@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
	Shuah Khan <skhan@linuxfoundation.org>
Subject: Re: [PATCH] usb: usbip: fix error handling of kthread_get_run()
Date: Fri, 5 Feb 2021 09:27:56 -0700	[thread overview]
Message-ID: <6b2b9f7c-a412-0f52-3373-bc58d1e95ad9@linuxfoundation.org> (raw)
In-Reply-To: <20210205135707.4574-1-penguin-kernel@I-love.SAKURA.ne.jp>

On 2/5/21 6:57 AM, Tetsuo Handa wrote:
> syzbot is reporting a crash at vhci_shutdown_connection() [1]. It turned
> out that it was not a NULL pointer dereference but an ERR_PTR(-EINTR)
> pointer dereference, for kthread_create() became killable due to
> commit 786235eeba0e1e85 ("kthread: make kthread_create() killable").
> 
> When SIGKILLed while attach_store() is calling kthread_get_run(),
> ERR_PTR(-EINTR) is stored into vdev->ud.tcp_{rx,tx}, and then
> kthread_stop_put() is called on vdev->ud.tcp_{rx,tx} from
> vhci_shutdown_connection() because vdev->ud.tcp_{rx,tx} != NULL.
> 
> Prior to commit 9720b4bc76a83807 ("staging/usbip: convert to kthread"),
> "current" pointer is assigned to vdev->ud.tcp_{rx,tx} by usbip_thread()
> kernel thread, and hence vdev->ud.tcp_{rx,tx} != NULL meant a valid task
> pointer. Therefore, we should make kthread_get_run() return NULL when
> kthread_create() failed.
> 
> [1] https://syzkaller.appspot.com/bug?id=c21c07f3d51769405e8efc027bdb927515dcc7d6
> 
> Reported-by: syzbot <syzbot+a93fba6d384346a761e3@syzkaller.appspotmail.com>
> Reported-by: syzbot <syzbot+bf1a360e305ee719e364@syzkaller.appspotmail.com>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Fixes: 9720b4bc76a83807 ("staging/usbip: convert to kthread")
> Cc: Arnd Bergmann <arnd@arndb.de>
> ---
>   drivers/usb/usbip/usbip_common.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/usbip/usbip_common.h b/drivers/usb/usbip/usbip_common.h
> index 8be857a4fa13..a7c68097aa1d 100644
> --- a/drivers/usb/usbip/usbip_common.h
> +++ b/drivers/usb/usbip/usbip_common.h
> @@ -286,6 +286,8 @@ struct usbip_device {
>   	if (!IS_ERR(__k)) {						   \
>   		get_task_struct(__k);					   \
>   		wake_up_process(__k);					   \
> +	} else {							   \
> +		__k = NULL;						   \
>   	}								   \
>   	__k;								   \
>   })
> 

Good find. For this fix to be complete, you will have to add checks
for kthread_get_run() NULL return in attach_store() and
usbip_sockfd_store() routines in stub_dev.c and vudc_sysfs.c

thanks,
-- Shuah