On Tue, Nov 05, 2024 at 02:21:38PM +0000, Ricardo Ribalda wrote:
> We used the wrong device for the device managed functions. We used the
> usb device, when we should be using the interface device.
>
> If we unbind the driver from the usb interface, the cleanup functions
> are never called. In our case, the IRQ is never disabled.
>
> If an IRQ is triggered, it will try to access memory sections that are
> already free, causing an OOPS.
>
> We cannot use the function devm_request_threaded_irq here. The devm_*
> clean functions are called after the main structure is released by
> uvc_delete.
That may or may not be true, depending on whether or not userspace holds
on to resources.
> Luckily this bug has small impact, as it is only affected by devices
> with gpio units and the user has to unbind the device, a disconnect will
> not trigger this error.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> Reviewed-by: Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> Changes in v5:
> - Revert non refcount, that belongs to a different set
> - Move cleanup to a different function
> - Link to v4: https://lore.kernel.org/r/20241105-uvc-crashrmmod-v4-0-410e548f097a@xxxxxxxxxxxx
>
> Changes in v4: Thanks Laurent.
> - Remove refcounted cleaup to support devres.
> - Link to v3: https://lore.kernel.org/r/20241105-uvc-crashrmmod-v3-1-c0959c8906d3@xxxxxxxxxxxx
>
> Changes in v3: Thanks Sakari.
> - Rename variable to initialized.
> - Other CodeStyle.
> - Link to v2: https://lore.kernel.org/r/20241105-uvc-crashrmmod-v2-1-547ce6a6962e@xxxxxxxxxxxx
>
> Changes in v2: Thanks to Laurent.
> - The main structure is not allocated with devres so there is a small
> period of time where we can get an irq with the structure free. Do not
> use devres for the IRQ.
> - Link to v1: https://lore.kernel.org/r/20241031-uvc-crashrmmod-v1-1-059fe593b1e6@xxxxxxxxxxxx
> ---
> drivers/media/usb/uvc/uvc_driver.c | 27 ++++++++++++++++++++-------
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index a96f6ca0889f..aa937f07b6b5 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -1295,14 +1295,14 @@ static int uvc_gpio_parse(struct uvc_device *dev)
> struct gpio_desc *gpio_privacy;
> int irq;
>
> - gpio_privacy = devm_gpiod_get_optional(&dev->udev->dev, "privacy",
> + gpio_privacy = devm_gpiod_get_optional(&dev->intf->dev, "privacy",
> GPIOD_IN);
> if (IS_ERR_OR_NULL(gpio_privacy))
> return PTR_ERR_OR_ZERO(gpio_privacy);
>
> irq = gpiod_to_irq(gpio_privacy);
> if (irq < 0)
> - return dev_err_probe(&dev->udev->dev, irq,
> + return dev_err_probe(&dev->intf->dev, irq,
> "No IRQ for privacy GPIO\n");
>
> unit = uvc_alloc_new_entity(dev, UVC_EXT_GPIO_UNIT,
> @@ -1329,15 +1329,27 @@ static int uvc_gpio_parse(struct uvc_device *dev)
> static int uvc_gpio_init_irq(struct uvc_device *dev)
> {
> struct uvc_entity *unit = dev->gpio_unit;
> + int ret;
>
> if (!unit || unit->gpio.irq < 0)
> return 0;
>
> - return devm_request_threaded_irq(&dev->udev->dev, unit->gpio.irq, NULL,
> - uvc_gpio_irq,
> - IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> - IRQF_TRIGGER_RISING,
> - "uvc_privacy_gpio", dev);
> + ret = request_threaded_irq(unit->gpio.irq, NULL, uvc_gpio_irq,
> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING |
> + IRQF_TRIGGER_RISING,
> + "uvc_privacy_gpio", dev);
> +
> + unit->gpio.initialized = !ret;
> +
> + return ret;
> +}
> +
> +static void uvc_gpio_cleanup(struct uvc_device *dev)
> +{
> + if (!dev->gpio_unit || !dev->gpio_unit->gpio.initialized)
> + return;
> +
> + free_irq(dev->gpio_unit->gpio.irq, dev);
> }
>
> /* ------------------------------------------------------------------------
> @@ -1982,6 +1994,7 @@ static void uvc_unregister_video(struct uvc_device *dev)
> if (media_devnode_is_registered(dev->mdev.devnode))
> media_device_unregister(&dev->mdev);
> #endif
> + uvc_gpio_cleanup(dev);
Have you checked in details that doing this at the *end* of
uvc_unregister_video(), after lots of resources get unregistered, will
not cause any issue if the IRQ occurs somewhere in the middle of this
function ? It would seem much safer to free the IRQ at the *beginning*
of the function, the same way that drivers should generally stop
hardware activity first, and only then release resources that may be
needed when handling hardware activity.
> }
>
> int uvc_register_video_device(struct uvc_device *dev,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 07f9921d83f2..965a789ed03e 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -234,6 +234,7 @@ struct uvc_entity {
> u8 *bmControls;
> struct gpio_desc *gpio_privacy;
> int irq;
> + bool initialized;
> } gpio;
> };
>
>
> ---
> base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
> change-id: 20241031-uvc-crashrmmod-666de3fc9141
--
Regards,
Laurent Pinchart
