Skip to contentSkip to conversation

media: em28xx: clear dev->v4l2 when v4l2 init fails

PATCH Superseded Diego Fernando Mancera Gomez 1 participant 3 messages Nov 13

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 media: em28xx: clear dev->v4l2 when v4l2 init fails

In em28xx_v4l2_init(), if an error occurs after video_register_device() succeeds, the unregister_dev error path unregisters the video devices and drops the initial v4l2_dev reference, but leaves dev->v4l2 set.

An em28xx_v4l2_open() call already in progress (such as from v4l_id) that blocked on dev->lock while em28xx_v4l2_init() held it will then find dev->v4l2 still non-NULL and succeed on an unregistered node instead of returning -ENODEV. When the device is later disconnected, em28xx_v4l2_fini() also sees dev->v4l2 is still set and calls v4l2_device_put() a second time, dropping the refcount to zero and freeing v4l2 while v4l_id still holds it, causing a slab-use-after-free in v4l2_open().

Fix this by clearing dev->v4l2 under dev->lock in the unregister_dev error path of em28xx_v4l2_init().

Fixes 288254383674 ("media: em28xx: use v4l2_device release callback")
Reported-by [email protected]
Assisted-by Gemini

---
Note: compile-tested drivers/media/usb/em28xx/em28xx_video.o with allmodconfig (no warnings); not runtime-tested locally against the syzbot reproducer.

 drivers/media/usb/em28xx/em28xx-video.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/media/usb/em28xx/em28xx-video.c +6 -0
index 84424284d486..f78325a41124 100644
@@ -3129,6 +3129,12 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		vb2_video_unregister_device(&v4l2->vdev);
 	}
 
+	/*
+	 * Clear dev->v4l2 to prevent an in-progress em28xx_v4l2_open() from
+	 * succeeding on an unregistered node (returning -ENODEV instead) and
+	 * to prevent em28xx_v4l2_fini() from tearing down the instance again.
+	 */
+	dev->v4l2 = NULL;
 	mutex_unlock(&dev->lock);
 
 	/*
Signature · 1 line
2.56.1
#1 lore raw

PATCH v2 1/1 media: em28xx: clear dev->v4l2 when v4l2 init fails

In em28xx_v4l2_init(), if an error occurs after video_register_device() succeeds, the unregister_dev error path unregisters the video devices and drops the initial v4l2_dev reference, but leaves dev->v4l2 set.

An em28xx_v4l2_open() call already in progress (such as from v4l_id) that blocked on dev->lock while em28xx_v4l2_init() held it will then find dev->v4l2 still non-NULL and succeed on an unregistered node instead of returning -ENODEV. When the device is later disconnected, em28xx_v4l2_fini() also sees dev->v4l2 is still set and calls v4l2_device_put() a second time, dropping the refcount to zero and freeing v4l2 while v4l_id still holds it, causing a slab-use-after-free in v4l2_open().

Fix this by clearing dev->v4l2 under dev->lock in the unregister_dev error path of em28xx_v4l2_init().

Fixes 288254383674 ("media: em28xx: use v4l2_device release callback")
Reported-by [email protected]

---
Changes in v2:
- Drop the non-standard 'Assisted-by: Gemini' trailer that failed checkpatch.

Note: compile-tested drivers/media/usb/em28xx/em28xx_video.o with allmodconfig (no warnings); not runtime-tested locally against the syzbot reproducer.

 drivers/media/usb/em28xx/em28xx-video.c | 6 ++++++
 1 file changed, 6 insertions(+)
(quoted diff) +0 -0

b/drivers/media/usb/em28xx/em28xx-video.c
index 84424284d486..f78325a41124 100644

drivers/media/usb/em28xx/em28xx-video.c +6 -0
@@ -3129,6 +3129,12 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		vb2_video_unregister_device(&v4l2->vdev);
 	}
 
+	/*
+	 * Clear dev->v4l2 to prevent an in-progress em28xx_v4l2_open() from
+	 * succeeding on an unregistered node (returning -ENODEV instead) and
+	 * to prevent em28xx_v4l2_fini() from tearing down the instance again.
+	 */
+	dev->v4l2 = NULL;
 	mutex_unlock(&dev->lock);
 
 	/*
Signature · 1 line
2.56.1
#2 lore raw

PATCH v3 1/1 media: em28xx: clear dev->v4l2 when v4l2 init fails

In em28xx_v4l2_init(), if an error occurs after video_register_device() succeeds, the unregister_dev error path unregisters the video devices and drops the initial v4l2_dev reference, but leaves dev->v4l2 set.

An em28xx_v4l2_open() call already in progress (such as from v4l_id) that blocked on dev->lock while em28xx_v4l2_init() held it will then find dev->v4l2 still non-NULL and succeed on an unregistered node instead of returning -ENODEV. When the device is later disconnected, em28xx_v4l2_fini() also sees dev->v4l2 is still set and calls v4l2_device_put() a second time, dropping the refcount to zero and freeing v4l2 while v4l_id still holds it, causing a slab-use-after-free in v4l2_open().

Fix this by clearing dev->v4l2 under dev->lock in the unregister_dev error path of em28xx_v4l2_init().

Fixes 288254383674 ("media: em28xx: use v4l2_device release callback")
Reported-by [email protected]

---
Changes in v3:
- Resend: v2 reached lore with the "diff --git" header line split in two, so the Media CI robot could not apply it. No code changes.

Changes in v2:
- Drop the non-standard 'Assisted-by: Gemini' trailer that failed checkpatch.

Note: compile-tested drivers/media/usb/em28xx/em28xx-video.o with allmodconfig (no warnings); not runtime-tested locally against the syzbot reproducer.

 drivers/media/usb/em28xx/em28xx-video.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/media/usb/em28xx/em28xx-video.c +6 -0
index 84424284d486..f78325a41124 100644
@@ -3129,6 +3129,12 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 		vb2_video_unregister_device(&v4l2->vdev);
 	}
 
+	/*
+	 * Clear dev->v4l2 to prevent an in-progress em28xx_v4l2_open() from
+	 * succeeding on an unregistered node (returning -ENODEV instead) and
+	 * to prevent em28xx_v4l2_fini() from tearing down the instance again.
+	 */
+	dev->v4l2 = NULL;
 	mutex_unlock(&dev->lock);
 
 	/*
Signature · 1 line
2.56.1