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

[Andrey Konovalov <andreyknvl@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4824 bytes --]

On Fri, Jan 24, 2020 at 3:29 AM Hillf Danton <hdanton@sina.com> 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@syzkaller.appspotmail.com
> > > >
> > > > 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@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 = {
> --

#syz test: https://github.com/google/kasan.git ae179410

[-- Attachment #2: usbvision.patch --]
[-- Type: application/x-patch, Size: 2585 bytes --]