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

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

[Yousef Alhouseen]

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.

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));
-- 
2.54.0

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

[Hans Verkuil]

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));

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

[Yousef Alhouseen]

Understood. I had not found that series. Please drop my patch; I will
defer to the broader lifetime fix in series 26968.

Thanks,
Yousef

On Mon, 29 Jun 2026 09:45:34 +0200, Hans Verkuil
<hverkuil+cisco@kernel.org> wrote:
> 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));