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 :-)
Regards,
Hans
