em28xx_v4l2_init() publishes the new object in dev->v4l2 before the video device is registered. If a later initialization step fails -past v4l2_device_register-, the error path unregisters the device nodes and drops the initial v4l2_device reference, but previously left dev->v4l2 pointing at the freed object. syzbot reported: BUG: KASAN: slab-use-after-free in v4l2_device_disconnect+0xa4/0xc0 drivers/media/v4l2-core/v4l2-device.c:78 em28xx_v4l2_fini() is called later from USB disconnect. It reads dev->v4l2 before taking dev->lock, and on the failed-init path it owns no reference. The last close of the video node can therefore drop the final v4l2_device reference and free the object between that read and its use in v4l2_device_disconnect(), causing a use-after-free. Fixing this by clearing dev->v4l2 under dev->lock in the init error path before dropping the initial reference, and read it under dev->lock in fini. This makes fini see NULL after a failed init, while the successful path continues to hold the initial reference until fini is done. Fixes: 288254383674 ("media: em28xx: use v4l2_device release callback") Reported-by: syzbot+1e71765881a6fc43185f@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=1e71765881a6fc43185f Cc: stable@vger.kernel.org Signed-off-by: Drif Abdelmalek Mohamed Said --- drivers/media/usb/em28xx/em28xx-video.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c index 84424284d486..572e6835b851 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; + v4l2_device_disconnect(&v4l2->v4l2_dev); em28xx_uninit_usb_xfer(dev, EM28XX_ANALOG_MODE); @@ -3129,6 +3131,9 @@ static int em28xx_v4l2_init(struct em28xx *dev) vb2_video_unregister_device(&v4l2->vdev); } + /* matches check in em28xx_v4l2_fini (under the same lock) */ + dev->v4l2 = NULL; + mutex_unlock(&dev->lock); /* -- 2.43.0