From: Dmitry Vyukov <dvyukov@google.com>
To: Hillf Danton <hdanton@sina.com>
Cc: syzbot <syzbot+19c5a4b75931e8d63aab@syzkaller.appspotmail.com>,
ezequiel@collabora.com, hverkuil-cisco@xs4all.nl,
lijian@yulong.com, linux-kernel@vger.kernel.org,
linux-media@vger.kernel.org, mchehab@kernel.org,
sakari.ailus@linux.intel.com, syzkaller-bugs@googlegroups.com
Subject: Re: [syzbot] KASAN: use-after-free Read in v4l2_ioctl (2)
Date: Fri, 25 Jun 2021 11:53:59 +0200 [thread overview]
Message-ID: <CACT4Y+amrcRo=1KuKHoN7L6JoCH0Bakt5dveZt7iZDhqpSu4nA@mail.gmail.com> (raw)
In-Reply-To: <20210625094638.1791-1-hdanton@sina.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.
next prev parent reply other threads:[~2021-06-25 9:54 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-25 4:53 [syzbot] KASAN: use-after-free Read in v4l2_ioctl (2) syzbot
[not found] ` <20210625085140.1735-1-hdanton@sina.com>
2021-06-25 9:08 ` Dmitry Vyukov
[not found] ` <20210625094638.1791-1-hdanton@sina.com>
2021-06-25 9:53 ` Dmitry Vyukov [this message]
[not found] ` <20210625130813.84-1-hdanton@sina.com>
2021-06-25 13:51 ` Dmitry Vyukov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CACT4Y+amrcRo=1KuKHoN7L6JoCH0Bakt5dveZt7iZDhqpSu4nA@mail.gmail.com' \
--to=dvyukov@google.com \
--cc=ezequiel@collabora.com \
--cc=hdanton@sina.com \
--cc=hverkuil-cisco@xs4all.nl \
--cc=lijian@yulong.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=syzbot+19c5a4b75931e8d63aab@syzkaller.appspotmail.com \
--cc=syzkaller-bugs@googlegroups.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).