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

[Dmitry Vyukov]

On Fri, Jun 25, 2021 at 3:08 PM Hillf Danton <hdanton@xxxxxxxx> 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.
>
> Let me explain. The whole point of the added mutex is solely to prevent
> the disconnector from freeing the stk camera while there are still
> openers of the video device, and trylock is used to walk around deadlock
> because multiple openers are allowed. In function it is equivelant to the
> usual method on top of open count and waitqueue, something like
>
>         mutex_lock;
>         stk_cam->open_cnt++;    // mutex_trylock(&stk_cam->free_mutex);
>         mutex_unlock;
>
> at file open time, and
>
>         mutex_lock;
>         stk_cam->open_cnt = 0;
>         wake_up(&stk_cam->waitq); // mutex_unlock(&stk_cam->free_mutex);
>         mutex_unlock;
>
> at file release time, and
>
>         wait_event(stk_cam->waitq,
>                 stk_cam->open_cnt == 0); // mutex_lock(&stk_cam->free_mutex);
>                                         // mutex_unlock(&stk_cam->free_mutex);
>         kfree(stk_cam);
>
> at disconnect time, but has fewer lines of code to type.

But if trylock has failed, then the file release will still unlock the
mutex and unlocking a mutex without a prior lock is not permitted.

Or, I assume disconnect needs to wait for all files to be released.
This won't be the case with a mutex, because when the first file is
released, mutex is unlocked and disconnect can proceed.

But maybe I am still missing something.
Are you sure your use of mutex complies with the rules?
https://elixir.bootlin.com/linux/v5.13-rc7/source/include/linux/mutex.h#L31


> What is more crucial however is why the mechanisms in video core are
> failing to prevent uaf like this one from coming true. Lets wait for
> lights from the video folks.