Re: [PATCH] media: fix media_device_unregister() to destroy media device device resource

[Shuah Khan]

On 03/18/2016 03:52 AM, Mauro Carvalho Chehab wrote:
> Em Thu, 17 Mar 2016 16:46:36 -0600
> Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> 
>> When all drivers except usb-core driver is unbound, destroy the media device
>> resource. Other wise, media device resource will persist in a defunct state.
>> This leads to use-after-free and bad access errors during a subsequent bind.
>> Fix it to destroy the media device resource when last reference is released
>> in media_device_unregister().
>>
>> Signed-off-by: Shuah Khan <shuahkh@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/media/media-device.c | 28 ++++++++++++++++++++++------
>>  1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 070421e..7312612 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -822,22 +822,38 @@ printk("%s: mdev=%p\n", __func__, mdev);
>>  	dev_dbg(mdev->dev, "Media device unregistered\n");
>>  }
>>  
>> +static void media_device_release_devres(struct device *dev, void *res)
>> +{
>> +}
>> +
>> +static void media_device_destroy_devres(struct device *dev)
>> +{
>> +	int ret;
>> +
>> +	ret = devres_destroy(dev, media_device_release_devres, NULL, NULL);
>> +	pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);
>> +}
>> +
>>  void media_device_unregister(struct media_device *mdev)
>>  {
>> +	int ret;
>> +	struct device *dev;
>>  printk("%s: mdev=%p\n", __func__, mdev);
>>  	if (mdev == NULL)
>>  		return;
>>  
>> -	mutex_lock(&mdev->graph_mutex);
>> -	kref_put(&mdev->kref, do_media_device_unregister);
>> -	mutex_unlock(&mdev->graph_mutex);
>> +	ret = kref_put_mutex(&mdev->kref, do_media_device_unregister,
>> +			     &mdev->graph_mutex);
>> +	if (ret) {
>> +		/* do_media_device_unregister() has run */
>> +		dev = mdev->dev;
>> +		mutex_unlock(&mdev->graph_mutex);
> 
> 
>> +		media_device_destroy_devres(dev);
> 
> This doesn't seem right: what happens on drivers that don't use
> devres to allocate struct media_device?
> 

That is okay. devres_destroy() won't find the resource. The way it works
is it will try to find the resource with the match routine and data and
that step will fail it will return -ENOENT. At that point nothing more
is done.

ret = devres_destroy(dev, media_device_release_devres, NULL, NULL);
pr_debug("%s: devres_destroy() returned %d\n", __func__, ret);

devres_destroy() combines the devres_find() and remove. So we are good
here.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978