Re: KASAN: use-after-free Read in v4l2_release (3)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 = {

[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux