linux-kernel - Re: [PATCH] media: uvcvideo: Fix deadlock during uvc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241107215736.GI31474@pendragon.ideasonboard.com>
Date: Thu, 7 Nov 2024 23:57:36 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	syzbot+9446d5e0d25571e6a212@...kaller.appspotmail.com
Subject: Re: [PATCH] media: uvcvideo: Fix deadlock during uvc_probe

Hi Ricardo,

Thank you for the patch.

On Tue, Oct 22, 2024 at 08:30:30AM +0000, Ricardo Ribalda wrote:
> If uvc_probe() fails, it can end up calling uvc_status_unregister() before
> uvc_status_init() is called.
> 
> Fix this by checking if dev->status is NULL or not in
> uvc_status_unregister()

That will not work in case usb_alloc_urb() fails in uvc_status_init().
In that error path, dev->status is freed but the pointer is not set to
NULL. Setting it to NULL should be enough to fix the problem. I'll do
that and apply this patch to my tree.

> Reported-by: syzbot+9446d5e0d25571e6a212@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/linux-media/20241020160249.GD7770@pendragon.ideasonboard.com/T/#m506744621d72a2ace5dd2ab64055be9898112dbd
> Fixes: c5fe3ed618f9 ("media: uvcvideo: Avoid race condition during unregister")
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
>  drivers/media/usb/uvc/uvc_status.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
> index 06c867510c8f..b3527895c2f6 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -294,6 +294,8 @@ int uvc_status_init(struct uvc_device *dev)
>  
>  void uvc_status_unregister(struct uvc_device *dev)
>  {
> +	if (!dev->status)
> +		return;

I'd add a blank line here.

Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>

>  	uvc_status_suspend(dev);
>  	uvc_input_unregister(dev);
>  }
> 
> ---
> base-commit: 698b6e3163bafd61e1b7d13572e2c42974ac85ec
> change-id: 20241022-race-unreg-85295d5fbeee

-- 
Regards,

Laurent Pinchart