2022-08-29 13:37:57

by Soumya Negi

[permalink] [raw]
Subject: [RFT PATCH] media: em28xx: Enable v4l2 file ops at the end of em28xx_v4l2_init()

Fix syzbot bug KASAN: use-after-free Read in v4l2_fh_open
Syzbot link:
https://syzkaller.appspot.com/bug?id=0e3c97f1c4112e102c9988afd5eff079350eab7f
Repro: https://syzkaller.appspot.com/text?tag=ReproC&x=12663ebdd00000

The bug arises because the em28xx driver registers a v4l2 video
device(struct video_device) with the V4L2 interface in
em28xx_v4l2_init() but the v4l2 extension initialization eventually
fails and the video device is unregistered. v4l2_open() in the V4L2
interface views the device as registered and allows calls to
em28xx_v4l2_open(). Due to race between video_unregister_device() and
v4l2_open(), em28xx_v4l2_open() is accessing video device after it has
been freed(by the release callback) and is passing it on to
v4l2_fh_open().

In em28xx_v4l2_init(), temporarily disable v4l2 file operations on
struct video_device devices(video, vbi, radio) before registering them
and enable the file ops at the end when v4l2 extension initialization
succeeds.

Reported-by: [email protected]
Signed-off-by: Soumya Negi <[email protected]>
---
drivers/media/usb/em28xx/em28xx-video.c | 45 +++++++++++++++++++++----
1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 8181c0e6a25b..900a1eacb1c7 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2320,6 +2320,19 @@ static int em28xx_v4l2_close(struct file *filp)
return 0;
}

+/* Used to temporarily disable file operations on video_device until successful
+ * initialization in em28xx_v4l2_init().
+ */
+static const struct v4l2_file_operations em28xx_v4l_fops_empty = {
+ .owner = THIS_MODULE,
+ .open = NULL,
+ .release = NULL,
+ .read = NULL,
+ .poll = NULL,
+ .mmap = NULL,
+ .unlocked_ioctl = NULL,
+};
+
static const struct v4l2_file_operations em28xx_v4l_fops = {
.owner = THIS_MODULE,
.open = em28xx_v4l2_open,
@@ -2375,12 +2388,22 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
};

static const struct video_device em28xx_video_template = {
- .fops = &em28xx_v4l_fops,
+ .fops = &em28xx_v4l_fops_empty,
.ioctl_ops = &video_ioctl_ops,
.release = video_device_release_empty,
.tvnorms = V4L2_STD_ALL,
};

+/* Used to temporarily disable file operations on video_device until successful
+ * initialization in em28xx_v4l2_init().
+ */
+static const struct v4l2_file_operations radio_fops_empty = {
+ .owner = THIS_MODULE,
+ .open = NULL,
+ .release = NULL,
+ .unlocked_ioctl = NULL,
+};
+
static const struct v4l2_file_operations radio_fops = {
.owner = THIS_MODULE,
.open = em28xx_v4l2_open,
@@ -2404,7 +2427,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
};

static struct video_device em28xx_radio_template = {
- .fops = &radio_fops,
+ .fops = &radio_fops_empty,
.ioctl_ops = &radio_ioctl_ops,
.release = video_device_release_empty,
};
@@ -2833,9 +2856,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
"can't register radio device\n");
goto unregister_dev;
}
- dev_info(&dev->intf->dev,
- "Registered radio device as %s\n",
- video_device_node_name(&v4l2->radio_dev));
}

/* Init entities at the Media Controller */
@@ -2851,14 +2871,27 @@ static int em28xx_v4l2_init(struct em28xx *dev)
}
#endif

+ /* Enable v4l2 file operations for v4l2 video video_device */
+ v4l2->vdev.fops = &em28xx_v4l_fops;
dev_info(&dev->intf->dev,
"V4L2 video device registered as %s\n",
video_device_node_name(&v4l2->vdev));

- if (video_is_registered(&v4l2->vbi_dev))
+ if (video_is_registered(&v4l2->vbi_dev)) {
+ /* Enable v4l2 file operations for v4l2 vbi video_device */
+ v4l2->vbi_dev.fops = &em28xx_v4l_fops;
dev_info(&dev->intf->dev,
"V4L2 VBI device registered as %s\n",
video_device_node_name(&v4l2->vbi_dev));
+ }
+
+ if (video_is_registered(&v4l2->radio_dev)) {
+ /* Enable v4l2 file operations for v4l2 radio video_device */
+ v4l2->radio_dev.fops = &radio_fops;
+ dev_info(&dev->intf->dev,
+ "Registered radio device as %s\n",
+ video_device_node_name(&v4l2->radio_dev));
+ }

/* Save some power by putting tuner to sleep */
v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
--
2.17.1


2022-09-06 15:33:50

by Hans Verkuil

[permalink] [raw]
Subject: Re: [RFT PATCH] media: em28xx: Enable v4l2 file ops at the end of em28xx_v4l2_init()

Hi Soumya,

On 29/08/2022 15:25, Soumya Negi wrote:
> Fix syzbot bug KASAN: use-after-free Read in v4l2_fh_open
> Syzbot link:
> https://syzkaller.appspot.com/bug?id=0e3c97f1c4112e102c9988afd5eff079350eab7f
> Repro: https://syzkaller.appspot.com/text?tag=ReproC&x=12663ebdd00000
>
> The bug arises because the em28xx driver registers a v4l2 video
> device(struct video_device) with the V4L2 interface in
> em28xx_v4l2_init() but the v4l2 extension initialization eventually
> fails and the video device is unregistered. v4l2_open() in the V4L2
> interface views the device as registered and allows calls to
> em28xx_v4l2_open(). Due to race between video_unregister_device() and
> v4l2_open(), em28xx_v4l2_open() is accessing video device after it has
> been freed(by the release callback) and is passing it on to
> v4l2_fh_open().
>
> In em28xx_v4l2_init(), temporarily disable v4l2 file operations on
> struct video_device devices(video, vbi, radio) before registering them
> and enable the file ops at the end when v4l2 extension initialization
> succeeds.

That's not the right approach. The problem is the roll-your-own v4l2->ref
refcount. Instead this should use the refcount that is built into struct
v4l2_device.

Specifically v4l2_dev->release should be set to the release function, and
v4l2_device_put(&v4l2->v4l2_dev); should be called in remove() or in the
error path of em28xx_v4l2_init().

Using this the release callback of v4l2_device will be called when the last
open file handle is closed, i.e. it keeps track of the open()s.

The roll-your-own approach in the em28xx driver (written before this v4l2_device
refcounting existed), is not taking that into account, so that causes these
problems.

What is also bad is that em28xx_vb2_setup() is called after the video devices
are registered. That should happen before.

Regards,

Hans

>
> Reported-by: [email protected]
> Signed-off-by: Soumya Negi <[email protected]>
> ---
> drivers/media/usb/em28xx/em28xx-video.c | 45 +++++++++++++++++++++----
> 1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 8181c0e6a25b..900a1eacb1c7 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -2320,6 +2320,19 @@ static int em28xx_v4l2_close(struct file *filp)
> return 0;
> }
>
> +/* Used to temporarily disable file operations on video_device until successful
> + * initialization in em28xx_v4l2_init().
> + */
> +static const struct v4l2_file_operations em28xx_v4l_fops_empty = {
> + .owner = THIS_MODULE,
> + .open = NULL,
> + .release = NULL,
> + .read = NULL,
> + .poll = NULL,
> + .mmap = NULL,
> + .unlocked_ioctl = NULL,
> +};
> +
> static const struct v4l2_file_operations em28xx_v4l_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2375,12 +2388,22 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> };
>
> static const struct video_device em28xx_video_template = {
> - .fops = &em28xx_v4l_fops,
> + .fops = &em28xx_v4l_fops_empty,
> .ioctl_ops = &video_ioctl_ops,
> .release = video_device_release_empty,
> .tvnorms = V4L2_STD_ALL,
> };
>
> +/* Used to temporarily disable file operations on video_device until successful
> + * initialization in em28xx_v4l2_init().
> + */
> +static const struct v4l2_file_operations radio_fops_empty = {
> + .owner = THIS_MODULE,
> + .open = NULL,
> + .release = NULL,
> + .unlocked_ioctl = NULL,
> +};
> +
> static const struct v4l2_file_operations radio_fops = {
> .owner = THIS_MODULE,
> .open = em28xx_v4l2_open,
> @@ -2404,7 +2427,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
> };
>
> static struct video_device em28xx_radio_template = {
> - .fops = &radio_fops,
> + .fops = &radio_fops_empty,
> .ioctl_ops = &radio_ioctl_ops,
> .release = video_device_release_empty,
> };
> @@ -2833,9 +2856,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> "can't register radio device\n");
> goto unregister_dev;
> }
> - dev_info(&dev->intf->dev,
> - "Registered radio device as %s\n",
> - video_device_node_name(&v4l2->radio_dev));
> }
>
> /* Init entities at the Media Controller */
> @@ -2851,14 +2871,27 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> }
> #endif
>
> + /* Enable v4l2 file operations for v4l2 video video_device */
> + v4l2->vdev.fops = &em28xx_v4l_fops;
> dev_info(&dev->intf->dev,
> "V4L2 video device registered as %s\n",
> video_device_node_name(&v4l2->vdev));
>
> - if (video_is_registered(&v4l2->vbi_dev))
> + if (video_is_registered(&v4l2->vbi_dev)) {
> + /* Enable v4l2 file operations for v4l2 vbi video_device */
> + v4l2->vbi_dev.fops = &em28xx_v4l_fops;
> dev_info(&dev->intf->dev,
> "V4L2 VBI device registered as %s\n",
> video_device_node_name(&v4l2->vbi_dev));
> + }
> +
> + if (video_is_registered(&v4l2->radio_dev)) {
> + /* Enable v4l2 file operations for v4l2 radio video_device */
> + v4l2->radio_dev.fops = &radio_fops;
> + dev_info(&dev->intf->dev,
> + "Registered radio device as %s\n",
> + video_device_node_name(&v4l2->radio_dev));
> + }
>
> /* Save some power by putting tuner to sleep */
> v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);

2022-09-08 23:58:48

by Soumya Negi

[permalink] [raw]
Subject: Re: [RFT PATCH] media: em28xx: Enable v4l2 file ops at the end of em28xx_v4l2_init()

On Tue, Sep 06, 2022 at 03:25:58PM +0200, Hans Verkuil wrote:
> Hi Soumya,
>
> On 29/08/2022 15:25, Soumya Negi wrote:
> > Fix syzbot bug KASAN: use-after-free Read in v4l2_fh_open
> > Syzbot link:
> > https://syzkaller.appspot.com/bug?id=0e3c97f1c4112e102c9988afd5eff079350eab7f
> > Repro: https://syzkaller.appspot.com/text?tag=ReproC&x=12663ebdd00000
> >
> > The bug arises because the em28xx driver registers a v4l2 video
> > device(struct video_device) with the V4L2 interface in
> > em28xx_v4l2_init() but the v4l2 extension initialization eventually
> > fails and the video device is unregistered. v4l2_open() in the V4L2
> > interface views the device as registered and allows calls to
> > em28xx_v4l2_open(). Due to race between video_unregister_device() and
> > v4l2_open(), em28xx_v4l2_open() is accessing video device after it has
> > been freed(by the release callback) and is passing it on to
> > v4l2_fh_open().
> >
> > In em28xx_v4l2_init(), temporarily disable v4l2 file operations on
> > struct video_device devices(video, vbi, radio) before registering them
> > and enable the file ops at the end when v4l2 extension initialization
> > succeeds.
>
> That's not the right approach. The problem is the roll-your-own v4l2->ref
> refcount. Instead this should use the refcount that is built into struct
> v4l2_device.
>
> Specifically v4l2_dev->release should be set to the release function, and
> v4l2_device_put(&v4l2->v4l2_dev); should be called in remove() or in the
> error path of em28xx_v4l2_init().
>
> Using this the release callback of v4l2_device will be called when the last
> open file handle is closed, i.e. it keeps track of the open()s.
>
> The roll-your-own approach in the em28xx driver (written before this v4l2_device
> refcounting existed), is not taking that into account, so that causes these
> problems.
>
> What is also bad is that em28xx_vb2_setup() is called after the video devices
> are registered. That should happen before.
>
> Regards,
>
> Hans

Hi Hans,
Thank you for explaining both the issues in detail. I will get working
on both and get back to you.

Regards,
Soumya

> >
> > Reported-by: [email protected]
> > Signed-off-by: Soumya Negi <[email protected]>
> > ---
> > drivers/media/usb/em28xx/em28xx-video.c | 45 +++++++++++++++++++++----
> > 1 file changed, 39 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> > index 8181c0e6a25b..900a1eacb1c7 100644
> > --- a/drivers/media/usb/em28xx/em28xx-video.c
> > +++ b/drivers/media/usb/em28xx/em28xx-video.c
> > @@ -2320,6 +2320,19 @@ static int em28xx_v4l2_close(struct file *filp)
> > return 0;
> > }
> >
> > +/* Used to temporarily disable file operations on video_device until successful
> > + * initialization in em28xx_v4l2_init().
> > + */
> > +static const struct v4l2_file_operations em28xx_v4l_fops_empty = {
> > + .owner = THIS_MODULE,
> > + .open = NULL,
> > + .release = NULL,
> > + .read = NULL,
> > + .poll = NULL,
> > + .mmap = NULL,
> > + .unlocked_ioctl = NULL,
> > +};
> > +
> > static const struct v4l2_file_operations em28xx_v4l_fops = {
> > .owner = THIS_MODULE,
> > .open = em28xx_v4l2_open,
> > @@ -2375,12 +2388,22 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
> > };
> >
> > static const struct video_device em28xx_video_template = {
> > - .fops = &em28xx_v4l_fops,
> > + .fops = &em28xx_v4l_fops_empty,
> > .ioctl_ops = &video_ioctl_ops,
> > .release = video_device_release_empty,
> > .tvnorms = V4L2_STD_ALL,
> > };
> >
> > +/* Used to temporarily disable file operations on video_device until successful
> > + * initialization in em28xx_v4l2_init().
> > + */
> > +static const struct v4l2_file_operations radio_fops_empty = {
> > + .owner = THIS_MODULE,
> > + .open = NULL,
> > + .release = NULL,
> > + .unlocked_ioctl = NULL,
> > +};
> > +
> > static const struct v4l2_file_operations radio_fops = {
> > .owner = THIS_MODULE,
> > .open = em28xx_v4l2_open,
> > @@ -2404,7 +2427,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
> > };
> >
> > static struct video_device em28xx_radio_template = {
> > - .fops = &radio_fops,
> > + .fops = &radio_fops_empty,
> > .ioctl_ops = &radio_ioctl_ops,
> > .release = video_device_release_empty,
> > };
> > @@ -2833,9 +2856,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > "can't register radio device\n");
> > goto unregister_dev;
> > }
> > - dev_info(&dev->intf->dev,
> > - "Registered radio device as %s\n",
> > - video_device_node_name(&v4l2->radio_dev));
> > }
> >
> > /* Init entities at the Media Controller */
> > @@ -2851,14 +2871,27 @@ static int em28xx_v4l2_init(struct em28xx *dev)
> > }
> > #endif
> >
> > + /* Enable v4l2 file operations for v4l2 video video_device */
> > + v4l2->vdev.fops = &em28xx_v4l_fops;
> > dev_info(&dev->intf->dev,
> > "V4L2 video device registered as %s\n",
> > video_device_node_name(&v4l2->vdev));
> >
> > - if (video_is_registered(&v4l2->vbi_dev))
> > + if (video_is_registered(&v4l2->vbi_dev)) {
> > + /* Enable v4l2 file operations for v4l2 vbi video_device */
> > + v4l2->vbi_dev.fops = &em28xx_v4l_fops;
> > dev_info(&dev->intf->dev,
> > "V4L2 VBI device registered as %s\n",
> > video_device_node_name(&v4l2->vbi_dev));
> > + }
> > +
> > + if (video_is_registered(&v4l2->radio_dev)) {
> > + /* Enable v4l2 file operations for v4l2 radio video_device */
> > + v4l2->radio_dev.fops = &radio_fops;
> > + dev_info(&dev->intf->dev,
> > + "Registered radio device as %s\n",
> > + video_device_node_name(&v4l2->radio_dev));
> > + }
> >
> > /* Save some power by putting tuner to sleep */
> > v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
>