linux-kernel - Re: [PATCH v2] media: uvcvideo: Fix crash during unbind if gpio unit is in use

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] [day] [month] [year] [list]
Message-ID: <20241105120315.GP27775@pendragon.ideasonboard.com>
Date: Tue, 5 Nov 2024 14:03:15 +0200
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
	Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
	linux-kernel@...r.kernel.org, stable@...r.kernel.org,
	Sergey Senozhatsky <senozhatsky@...omium.org>
Subject: Re: [PATCH v2] media: uvcvideo: Fix crash during unbind if gpio unit
 is in use

Hi Ricardo,

Thank you for the patch.

On Tue, Nov 05, 2024 at 10:53:59AM +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.

The commit message should explain why you're switching away from
devm_request_threaded_irq().

> 
> 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@...r.kernel.org
> Fixes: 2886477ff987 ("media: uvcvideo: Implement UVC_EXT_GPIO_UNIT")
> Reviewed-by: Sergey Senozhatsky <senozhatsky@...omium.org>
> Signed-off-by: Ricardo Ribalda <ribalda@...omium.org>
> ---
> 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@chromium.org
> ---
>  drivers/media/usb/uvc/uvc_driver.c | 28 +++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvcvideo.h   |  1 +
>  2 files changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> index a96f6ca0889f..af6aec27083c 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,28 @@ 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);
> +
> +	if (!ret)
> +		dev->gpio_unit->gpio.inited = true;

		unit->gpio.inited = true;

> +
> +	return ret;
> +}
> +
> +static void uvc_gpio_cleanup(struct uvc_device *dev)
> +{
> +	if (!dev->gpio_unit || !dev->gpio_unit->gpio.inited)
> +		return;
> +
> +	free_irq(dev->gpio_unit->gpio.irq, dev);
>  }
>  
>  /* ------------------------------------------------------------------------
> @@ -1880,6 +1893,7 @@ static void uvc_delete(struct kref *kref)
>  	struct uvc_device *dev = container_of(kref, struct uvc_device, ref);
>  	struct list_head *p, *n;
>  
> +	uvc_gpio_cleanup(dev);

This belongs to uvc_unregister_video(), or you'll have a race between
the release of the GPIO happening after .disconnect() returns, and
uvc_gpio_event() calling gpiod_get_value_cansleep().

I understand the desire to get such a fix merged quickly, but taking
time to think about race conditions instead of speeding up to get a
patch out of the door would be better. The alternative where I have to
flag race conditions multiple times during review is slower, and is more
work for everybody.

>  	uvc_status_cleanup(dev);
>  	uvc_ctrl_cleanup_device(dev);
>  
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index 07f9921d83f2..376cd670539b 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 inited;

As Sakari, I also prefer "initialized". Another option to save a few
bytes of memory here is to set irq to -1 when request_threaded_irq()
fails and test that in uvc_gpio_cleanup().

>  		} gpio;
>  	};
>  
> 
> ---
> base-commit: c7ccf3683ac9746b263b0502255f5ce47f64fe0a
> change-id: 20241031-uvc-crashrmmod-666de3fc9141

-- 
Regards,

Laurent Pinchart