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

[Hans Verkuil]

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: syzbot+b2391895514ed9ef4a8e@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Soumya Negi <soumya.negi97@xxxxxxxxx>
> ---
>  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);