Hi Mita-san,
On Fri, Oct 05, 2018 at 11:59:20PM +0900, 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);
> }
So the remove function would only contain video_unregister_device()? That's
the way it should be, as far as I can see.
--
Regards,
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx
