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);
}
