[PATCH] media: uvcvideo: Fix deadlock during uvc_probe

[PATCH] media: uvcvideo: Fix deadlock during uvc_probe

[Ricardo Ribalda]

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()

Reported-by: syzbot+9446d5e0d25571e6a212@syzkaller.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@chromium.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;
 	uvc_status_suspend(dev);
 	uvc_input_unregister(dev);
 }

---
base-commit: 698b6e3163bafd61e1b7d13572e2c42974ac85ec
change-id: 20241022-race-unreg-85295d5fbeee

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

Re: [PATCH] media: uvcvideo: Fix deadlock during uvc_probe

[Laurent Pinchart]

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@syzkaller.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@chromium.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@ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Fix deadlock during uvc_probe

[Laurent Pinchart]

On Thu, Nov 07, 2024 at 11:57:36PM +0200, Laurent Pinchart wrote:
> 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.

Not the exact same problem actually, as the issue reported by the bot is
due to the dev->status_lock mutex being uninitialized, and it will get
initialized as soon as uvc_status_init() is called, even if it fails.

There is however another issue, if dev->status is not set to NULL in the
error path, there will be a double-free in uvc_status_cleanup(). I'll
send a patch to fix that, and then apply this one on top.

> > Reported-by: syzbot+9446d5e0d25571e6a212@syzkaller.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@chromium.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@ideasonboard.com>
> 
> >  	uvc_status_suspend(dev);
> >  	uvc_input_unregister(dev);
> >  }
> > 
> > ---
> > base-commit: 698b6e3163bafd61e1b7d13572e2c42974ac85ec
> > change-id: 20241022-race-unreg-85295d5fbeee

-- 
Regards,

Laurent Pinchart