Re: [PATCH] media: em28xx: keep device state alive for registered video nodes

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 28/06/2026 02:31, Yousef Alhouseen wrote:
> The V4L2 core takes a video_device reference before invoking the
> driver open callback. That reference does not protect em28xx state
> because all three video_device objects are embedded in em28xx_v4l2 and
> use video_device_release_empty().
> 
> If initialization fails after registering a node, the error path can
> unregister it and drop the last em28xx_v4l2 reference while a concurrent
> open has passed the core registration check. The open callback then
> dereferences the freed video_device in video_drvdata(), as observed by
> KASAN. A disconnect has the same lifetime gap.
> 
> Give each successfully registered video node references to both the
> enclosing V4L2 state and the parent em28xx device. Release those
> references from the video_device release callback, after the core has
> drained pending opens and existing file references.

This patch series should fix this issue properly:

https://patchwork.linuxtv.org/project/linux-media/list/?series=26968

Rejecting this patch, manually manipulating refcounts is not the way to go.

Regards,

	Hans

> 
> Fixes: ef74a0b9ff56 ("[media] em28xx: move video_device structs from struct em28xx to struct v4l2")
> Reported-by: syzbot+39ff299961a7c07f00f0@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=39ff299961a7c07f00f0
> Cc: stable@vger.kernel.org
> Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-video.c | 35 +++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index da0422c65e5f..4274a9bcb432 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -2289,6 +2289,31 @@ static void em28xx_free_v4l2(struct kref *ref)
>  	kfree(v4l2);
>  }
>  
> +static void em28xx_vdev_release(struct video_device *vdev)
> +{
> +	struct em28xx_v4l2 *v4l2;
> +	struct em28xx *dev;
> +
> +	switch (vdev->vfl_type) {
> +	case VFL_TYPE_VIDEO:
> +		v4l2 = container_of(vdev, struct em28xx_v4l2, vdev);
> +		break;
> +	case VFL_TYPE_VBI:
> +		v4l2 = container_of(vdev, struct em28xx_v4l2, vbi_dev);
> +		break;
> +	case VFL_TYPE_RADIO:
> +		v4l2 = container_of(vdev, struct em28xx_v4l2, radio_dev);
> +		break;
> +	default:
> +		WARN_ON_ONCE(1);
> +		return;
> +	}
> +
> +	dev = v4l2->dev;
> +	kref_put(&v4l2->ref, em28xx_free_v4l2);
> +	kref_put(&dev->ref, em28xx_free_device);
> +}
> +
>  /*
>   * em28xx_v4l2_open()
>   * inits the device and starts isoc transfer
> @@ -2554,7 +2579,7 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
>  static const struct video_device em28xx_video_template = {
>  	.fops		= &em28xx_v4l_fops,
>  	.ioctl_ops	= &video_ioctl_ops,
> -	.release	= video_device_release_empty,
> +	.release	= em28xx_vdev_release,
>  	.tvnorms	= V4L2_STD_ALL,
>  };
>  
> @@ -2583,7 +2608,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
>  static struct video_device em28xx_radio_template = {
>  	.fops		= &radio_fops,
>  	.ioctl_ops	= &radio_ioctl_ops,
> -	.release	= video_device_release_empty,
> +	.release	= em28xx_vdev_release,
>  };
>  
>  /* I2C possible address to saa7115, tvp5150, msp3400, tvaudio */
> @@ -2965,6 +2990,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>  			"unable to register video device (error=%i).\n", ret);
>  		goto unregister_dev;
>  	}
> +	kref_get(&v4l2->ref);
> +	kref_get(&dev->ref);
>  
>  	/* Allocate and fill vbi video_device struct */
>  	if (em28xx_vbi_supported(dev) == 1) {
> @@ -2999,6 +3026,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>  				"unable to register vbi device\n");
>  			goto unregister_dev;
>  		}
> +		kref_get(&v4l2->ref);
> +		kref_get(&dev->ref);
>  	}
>  
>  	if (em28xx_boards[dev->model].radio.type == EM28XX_RADIO) {
> @@ -3012,6 +3041,8 @@ static int em28xx_v4l2_init(struct em28xx *dev)
>  				"can't register radio device\n");
>  			goto unregister_dev;
>  		}
> +		kref_get(&v4l2->ref);
> +		kref_get(&dev->ref);
>  		dev_info(&dev->intf->dev,
>  			 "Registered radio device as %s\n",
>  			 video_device_node_name(&v4l2->radio_dev));