On Fri, Jun 25, 2021 at 11:46 AM Hillf Danton <hdanton@xxxxxxxx> 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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20210625094638.1791-1-hdanton%40sina.com.
