Re: [PATCH] drm/vkms: Fix use after free and double free on init error

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 12/02/2025 à 09:53, Thomas Zimmermann a écrit :
> 
> 
> Am 12.02.25 um 09:49 schrieb José Expósito:
>> If the driver initialization fails, the vkms_exit() function might
>> access an uninitialized or freed default_config pointer and it might
>> double free it.
>>
>> Fix both possible errors by initializing default_config only when the
>> driver initialization succeeded.
>>
>> Reported-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> Link: https://lore.kernel.org/all/Z5uDHcCmAwiTsGte@louis-chauvet-laptop/
>> Fixes: 2df7af93fdad ("drm/vkms: Add vkms_config type")
>> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> 
> Reviewed-by: Thomas Zimmermann <tzimmremann@suse.de>

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

lore.kernel.org is broken currently, to avoid doing mistakes, I will 
wait for it to be working again so I can apply your patch using dim+b4.

(I removed danvet.vetter@ffwl.ch from CC, the mail server rejected the mail)

Thanks!
Louis Chauvet

> Thanks for posting this patch separately.
> 
> Best regards
> Thomas
> 
>> ---
>>    drivers/gpu/drm/vkms/vkms_drv.c | 15 +++++++++------
>>    1 file changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
>> index 7c142bfc3bd9..b6de91134a22 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.c
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
>> @@ -235,17 +235,19 @@ static int __init vkms_init(void)
>>    	if (!config)
>>    		return -ENOMEM;
>>    
>> -	default_config = config;
>> -
>>    	config->cursor = enable_cursor;
>>    	config->writeback = enable_writeback;
>>    	config->overlay = enable_overlay;
>>    
>>    	ret = vkms_create(config);
>> -	if (ret)
>> +	if (ret) {
>>    		kfree(config);
>> +		return ret;
>> +	}
>>    
>> -	return ret;
>> +	default_config = config;
>> +
>> +	return 0;
>>    }
>>    
>>    static void vkms_destroy(struct vkms_config *config)
>> @@ -269,9 +271,10 @@ static void vkms_destroy(struct vkms_config *config)
>>    
>>    static void __exit vkms_exit(void)
>>    {
>> -	if (default_config->dev)
>> -		vkms_destroy(default_config);
>> +	if (!default_config)
>> +		return;
>>    
>> +	vkms_destroy(default_config);
>>    	kfree(default_config);
>>    }
>>    
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com