On 10/02/18 18:17, Akinobu Mita wrote: > 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. > Looks good. Regards, Hans
