2018年10月8日(月) 20:21 Hans Verkuil <hverkuil@xxxxxxxxx>:
>
> On 10/05/2018 04:59 PM, Akinobu Mita wrote:
> > 2018年10月5日(金) 18:36 Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>:
> >>
> >> Hi Hans,
> >>
> >> On Mon, Oct 01, 2018 at 11:40:00AM +0200, Hans Verkuil wrote:
> >>> On 09/23/2018 06:34 PM, Akinobu Mita wrote:
> >>>> The video_i2c_data is allocated by kzalloc and released by the video
> >>>> device's release callback. The release callback is called when
> >>>> video_unregister_device() is called, but it will still be accessed after
> >>>> calling video_unregister_device().
> >>>>
> >>>> Fix the use after free by allocating video_i2c_data by devm_kzalloc() with
> >>>> i2c_client->dev so that it will automatically be released when the i2c
> >>>> driver is removed.
> >>>
> >>> Hmm. The patch is right, but the explanation isn't. The core problem is
> >>> that vdev.release was set to video_i2c_release, but that should only be
> >>> used if struct video_device was kzalloc'ed. But in this case it is embedded
> >>> in a larger struct, and then vdev.release should always be set to
> >>> video_device_release_empty.
> >>
> >> When the driver is unbound, what's acquired using the devm_() family of
> >> functions is released. At the same time, the user still holds a file
> >> handle, and can issue IOCTLs --- but the device's data structures no longer
> >> exist.
> >>
> >> That's not ok, and also the reason why we have the release callback.
> >>
> >> While there are issues elsewhere, this bit of the V4L2 / MC frameworks is
> >> fine.
> >>
> >> Or am I missing something?
> >
> > How about moving the lines causing use-after-free to release callback
> > like below?
> >
> > static void video_i2c_release(struct video_device *vdev)
> > {
> > struct video_i2c_data *data = video_get_drvdata(vdev);
> >
> > v4l2_device_unregister(&data->v4l2_dev);
> > mutex_destroy(&data->lock);
> > mutex_destroy(&data->queue_lock);
> > kfree(data);
> > }
> >
>
> You can test this with v4l2-ctl:
>
> v4l2-ctl --sleep 10
>
> This sleeps 10s, then calls QUERYCAP and closes the file handle.
>
> In another shell you can unbind the driver while v4l2-ctl is sleeping.
>
> Hopefully this works without crashing anything :-)
I tried that and the command finished without crash.
$ v4l2-ctl --sleep 10 -d /dev/video2
Test VIDIOC_QUERYCAP:
VIDIOC_QUERYCAP returned -1 (No such device)
VIDIOC_QUERYCAP: No such device
This -ENODEV should be ok as V4L2_FL_REGISTERED flag has already been
cleared by video_unregister_device().
