Re: KASAN: use-after-free Read in v4l2_release (3)
From: Andrey Konovalov
Date: Fri Jan 24 2020 - 07:41:19 EST
On Fri, Jan 24, 2020 at 3:29 AM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
>
> On Thu, 23 Jan 2020 14:19:47 +0200 Laurent Pinchart wrote:
> > On Thu, Jan 23, 2020 at 06:27:07PM +0800, Hillf Danton wrote:
> > > Wed, 22 Jan 2020 14:58:08 -0800 (PST)
> > > > syzbot has found a reproducer for the following crash on:
> > > >
> > > > HEAD commit: 4cc301ee usb: gadget: add raw-gadget interface
> > > > git tree: https://github.com/google/kasan.git usb-fuzzer
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=17f5a721e00000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=9ba75825443d54bd
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=75287f75e2fedd69d680
> > > > compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16a0b6f1e00000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1327dd76e00000
> > > >
> > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > Reported-by: syzbot+75287f75e2fedd69d680@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > >
> > > > usbvision_set_audio: can't write iopin register for audio switching
> > > > usbvision_radio_close: Final disconnect
> > > > ==================================================================
> > > > BUG: KASAN: use-after-free in v4l2_release+0x2f1/0x390 drivers/media/v4l2-core/v4l2-dev.c:459
> > > > Read of size 4 at addr ffff8881caba1068 by task v4l_id/1913
>
> <snip>
>
> > > Add the release callback for usbvision video device and use it to release
> > > resources when the last reference to the device goes away.
> >
> > Would you be able to submit this with a commit message and your
> > Signed-off-by line ?
>
> ---8<---
> Subject: [PATCH] media: usbvision: add the release callback for video device
> From: Hillf Danton <hdanton@xxxxxxxx>
>
> To fix the UAF syzbot reported,
>
> BUG: KASAN: use-after-free in v4l2_release+0x2f1/0x390 drivers/media/v4l2-core/v4l2-dev.c:459
>
> a release cb which is a simple wrapper of usbvision_release() is added
> for releasing resources as the last reference to the usbvision video
> device goes away.
>
> Reported-by: syzbot <syzbot+75287f75e2fedd69d680@xxxxxxxxxxxxxxxxxxxxxxxxx>
> Fixes: 2aa689dd8057 ("[media] usbvision: embed video_device")
> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
> ---
>
> --- a/drivers/media/usb/usbvision/usbvision-video.c
> +++ b/drivers/media/usb/usbvision/usbvision-video.c
> @@ -401,7 +401,6 @@ static int usbvision_v4l2_close(struct f
>
> if (r) {
> printk(KERN_INFO "%s: Final disconnect\n", __func__);
> - usbvision_release(usbvision);
> return 0;
> }
>
> @@ -409,6 +408,11 @@ static int usbvision_v4l2_close(struct f
> return v4l2_fh_release(file);
> }
>
> +static void usbvision_video_device_release(struct video_device *vdev)
> +{
> + struct usb_usbvision *usbvision = video_get_drvdata(vdev);
> + usbvision_release(usbvision);
> +}
>
> /*
> * usbvision_ioctl()
> @@ -1181,7 +1185,7 @@ static struct video_device usbvision_vid
> .fops = &usbvision_fops,
> .ioctl_ops = &usbvision_ioctl_ops,
> .name = "usbvision-video",
> - .release = video_device_release_empty,
> + .release = usbvision_video_device_release,
> .tvnorms = USBVISION_NORMS,
> };
>
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -206,7 +206,10 @@ static void v4l2_device_release(struct d
> }
> #endif
>
> - /* Do not call v4l2_device_put if there is no release callback set.
> + /*
> + * Decrease v4l2_device refcount
> + *
> + * Do not call v4l2_device_put if there is no release callback set.
> * Drivers that have no v4l2_device release callback might free the
> * v4l2_dev instance in the video_device release callback below, so we
> * must perform this check here.
> @@ -214,16 +217,12 @@ static void v4l2_device_release(struct d
> * TODO: In the long run all drivers that use v4l2_device should use the
> * v4l2_device release callback. This check will then be unnecessary.
> */
> - if (v4l2_dev->release == NULL)
> - v4l2_dev = NULL;
> + if (v4l2_dev->release)
> + v4l2_device_put(v4l2_dev);
>
> /* Release video_device and perform other
> cleanups as needed. */
> vdev->release(vdev);
> -
> - /* Decrease v4l2_device refcount */
> - if (v4l2_dev)
> - v4l2_device_put(v4l2_dev);
> }
>
> static struct class video_class = {
> --
#syz test: https://github.com/google/kasan.git ae179410
Subject: [PATCH] media: usbvision: add the release callback for video device
From: Hillf Danton <hdanton@sina.com>
To fix the UAF syzbot reported,
BUG: KASAN: use-after-free in v4l2_release+0x2f1/0x390 drivers/media/v4l2-core/v4l2-dev.c:459
a release cb which is a simple wrapper of usbvision_release() is added
for releasing resources as the last reference to the usbvision video
device goes away.
Reported-by: syzbot <syzbot+75287f75e2fedd69d680@syzkaller.appspotmail.com>
Fixes: 2aa689dd8057 ("[media] usbvision: embed video_device")
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Hillf Danton <hdanton@sina.com>
---
--- a/drivers/media/usb/usbvision/usbvision-video.c
+++ b/drivers/media/usb/usbvision/usbvision-video.c
@@ -401,7 +401,6 @@ static int usbvision_v4l2_close(struct f
if (r) {
printk(KERN_INFO "%s: Final disconnect\n", __func__);
- usbvision_release(usbvision);
return 0;
}
@@ -409,6 +408,11 @@ static int usbvision_v4l2_close(struct f
return v4l2_fh_release(file);
}
+static void usbvision_video_device_release(struct video_device *vdev)
+{
+ struct usb_usbvision *usbvision = video_get_drvdata(vdev);
+ usbvision_release(usbvision);
+}
/*
* usbvision_ioctl()
@@ -1181,7 +1185,7 @@ static struct video_device usbvision_vid
.fops = &usbvision_fops,
.ioctl_ops = &usbvision_ioctl_ops,
.name = "usbvision-video",
- .release = video_device_release_empty,
+ .release = usbvision_video_device_release,
.tvnorms = USBVISION_NORMS,
};
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -206,7 +206,10 @@ static void v4l2_device_release(struct d
}
#endif
- /* Do not call v4l2_device_put if there is no release callback set.
+ /*
+ * Decrease v4l2_device refcount
+ *
+ * Do not call v4l2_device_put if there is no release callback set.
* Drivers that have no v4l2_device release callback might free the
* v4l2_dev instance in the video_device release callback below, so we
* must perform this check here.
@@ -214,16 +217,12 @@ static void v4l2_device_release(struct d
* TODO: In the long run all drivers that use v4l2_device should use the
* v4l2_device release callback. This check will then be unnecessary.
*/
- if (v4l2_dev->release == NULL)
- v4l2_dev = NULL;
+ if (v4l2_dev->release)
+ v4l2_device_put(v4l2_dev);
/* Release video_device and perform other
cleanups as needed. */
vdev->release(vdev);
-
- /* Decrease v4l2_device refcount */
- if (v4l2_dev)
- v4l2_device_put(v4l2_dev);
}
static struct class video_class = {