| Message ID | 20241022-race-unreg-v1-1-2212f364d9de@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Fix deadlock during uvc_probe | expand |
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
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
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); }
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(+) --- base-commit: 698b6e3163bafd61e1b7d13572e2c42974ac85ec change-id: 20241022-race-unreg-85295d5fbeee Best regards,