Re: [PATCH] media: em28xx: fix v4l2 use-after-free after failed init

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: em28xx: missing mutex_unlock() in em28xx_v4l2_fini() when v4l2 is NULL
--

commit 5132f3a1057e95055bbcd81d79b2a705ab167335
Author: Drif Abdelmalek Mohamed Said <drifabdelmalekmohamedsaid@gmail.com>

media: em28xx: fix v4l2 use-after-free after failed init

This commit fixes a use-after-free when em28xx_v4l2_init() fails after
v4l2_device_register() by clearing dev->v4l2 under dev->lock on the error
path and reading dev->v4l2 under dev->lock in em28xx_v4l2_fini().

> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 84424284d4869..572e6835b851f 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -2414,7 +2414,7 @@ static int em28xx_v4l2_open(struct file *filp)
>   */
>  static int em28xx_v4l2_fini(struct em28xx *dev)
>  {
> -	struct em28xx_v4l2 *v4l2 = dev->v4l2;
> +	struct em28xx_v4l2 *v4l2;
>  
>  	if (dev->is_audio_only) {
>  		/* Shouldn't initialize IR for this interface */
> @@ -2426,13 +2426,15 @@ static int em28xx_v4l2_fini(struct em28xx *dev)
>  		return 0;
>  	}
>  
> -	if (!v4l2)
> -		return 0;
> -
>  	dev_info(&dev->intf->dev, "Closing video extension\n");
>  
>  	mutex_lock(&dev->lock);
>  
> +	/* matches em28xx_v4l2_init error paths */
> +	v4l2 = dev->v4l2;
> +	if (!v4l2)
> +		return 0;

[Severity: High]
Does this early return in em28xx_v4l2_fini() leak dev->lock when dev->v4l2
is NULL?

When em28xx_v4l2_init() fails (including the unregister_dev error path where
this patch sets dev->v4l2 = NULL), dev->v4l2 is NULL when em28xx_v4l2_fini()
runs later during USB disconnect or module unload. Returning 0 here without
calling mutex_unlock(&dev->lock) leaves dev->lock held.

During USB disconnect in drivers/media/usb/em28xx/em28xx-cards.c,
em28xx_usb_disconnect() calls em28xx_release_resources(dev) immediately
after em28xx_close_extension(dev):

em28xx_usb_disconnect()
  em28xx_close_extension(dev)
    ops->fini(dev) -> em28xx_v4l2_fini(dev)
      mutex_lock(&dev->lock);
      if (!v4l2)
          return 0;
  em28xx_release_resources(dev)
    mutex_lock(&dev->lock);

Wouldn't this immediately self-deadlock on dev->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031204723.1415-1-drifabdelmalekmohamedsaid@gmail.com?part=1