Re: [PATCH v2 1/6] media: video-i2c: avoid accessing released memory area when removing driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux