2018年10月1日(月) 18:40 Hans Verkuil <hverkuil@xxxxxxxxx>: > > 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.
