On 10/02/18 18:17, Akinobu Mita wrote:
> 2018年10月1日(月) 18:40 Hans Verkuil <hverk...@xs4all.nl>:
>>
>> 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.
>>
>> That was the real reason for the invalid access.
>
> How about the commit log below?
>
> struct video_device for video-i2c is embedded in a struct video_i2c_data,
> and then vdev.release should always be set to video_device_release_empty.
>
> However, the vdev.release is currently set to video_i2c_release which
> frees the whole struct video_i2c_data. In video_i2c_remove(), some fields
> (v4l2_dev, lock, and queue_lock) in struct video_i2c_data are still
> accessed after video_unregister_device().
>
> This fixes the use after free by setting the vdev.release to
> video_device_release_empty. Also, convert to use devm_kzalloc() for
> allocating a struct video_i2c_data in order to simplify the code.
>
Looks good.
Regards,
Hans
