Re: [syzbot] KASAN: use-after-free Read in v4l2_ioctl (2)

[Dmitry Vyukov <dvyukov@google.com>]

On Fri, Jun 25, 2021 at 11:46 AM Hillf Danton <hdanton@sina.com> wrote:
>
> On Fri, 25 Jun 2021 11:08:57 +0200 Dmitry Vyukov wrote:
> >On Fri, Jun 25, 2021 at 10:52 AM Hillf Danton wrote:
> >>
> >> Given the uaf in the ioctl path, open count is needed and should be
> >> maintained by stk and is implemented in the diff below with mutex - it
> >> is locked at file open time, released at file release time and aquired
> >> at disconnect time.
> >>
> >> This can be a quick fix to the uaf, though, lights on why the video_get(vdev)
> >> in v4l2_open() fails to prevent stk camera from going home too early are
> >> welcome. Is it the fault on the driver side without an eye on open count?
> >>
> >> +++ x/drivers/media/usb/stkwebcam/stk-webcam.c
> >> @@ -624,8 +624,10 @@ static int v4l_stk_open(struct file *fp)
> >>                 dev->first_init = 0;
> >>
> >>         err = v4l2_fh_open(fp);
> >> -       if (!err)
> >> +       if (!err) {
> >>                 usb_autopm_get_interface(dev->interface);
> >> +               mutex_trylock(&dev->free_mutex);
> >
> >I haven't read all of it, but doing mutex_trylock w/o checking the
> >return value looks very fishy. Can it ever be the right thing to
> >do?... E.g. the next line we unconditionally do mutex_unlock, are we
> >potentially unlocking a non-locked mutex?
>
> I am having difficulty understanding your point until I see next line...

Right, the next line unlocks a different mutex, so ignore the part
about the next line.

But I am still confused about the intention of trylock w/o using the
return value. I fail to imagine any scenarios where it's the right
thing to do.


> we have the same habit in regard to replying mails that deliver fix out
> of our boxes.
>
> What is your local time now? Wakeup without downing a pint of black tea?
> Or still working in the late night?

It's 11:53am, so I am properly caffeinated already :)

> Thanks for taking a look at it.
>
> Hillf
> >
> >
> >> +       }
> >>         mutex_unlock(&dev->lock);
> >>         return err;
> >>  }
> >> @@ -633,6 +635,7 @@ static int v4l_stk_open(struct file *fp)
> >>  static int v4l_stk_release(struct file *fp)
> >>  {
> >>         struct stk_camera *dev = video_drvdata(fp);
> >> +       int rc;
> >>
> >>         mutex_lock(&dev->lock);
> >>         if (dev->owner == fp) {
> >> @@ -645,7 +648,9 @@ static int v4l_stk_release(struct file *
> >>
> >>         usb_autopm_put_interface(dev->interface);
> >>         mutex_unlock(&dev->lock);
> >> -       return v4l2_fh_release(fp);
> >> +       rc = v4l2_fh_release(fp);
> >> +       mutex_unlock(&dev->free_mutex);
> >> +       return rc;
> >>  }
> >>
> >>  static ssize_t stk_read(struct file *fp, char __user *buf,
> >> @@ -1306,6 +1311,7 @@ static int stk_camera_probe(struct usb_i
> >>
> >>         spin_lock_init(&dev->spinlock);
> >>         mutex_init(&dev->lock);
> >> +       mutex_init(&dev->free_mutex);
> >>         init_waitqueue_head(&dev->wait_frame);
> >>         dev->first_init = 1; /* webcam LED management */
> >>
> >> @@ -1385,6 +1391,8 @@ static void stk_camera_disconnect(struct
> >>         video_unregister_device(&dev->vdev);
> >>         v4l2_ctrl_handler_free(&dev->hdl);
> >>         v4l2_device_unregister(&dev->v4l2_dev);
> >> +       mutex_lock(&dev->free_mutex);
> >> +       mutex_unlock(&dev->free_mutex);
> >>         kfree(dev);
> >>  }
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20210625094638.1791-1-hdanton%40sina.com.