Re: [PATCH] dell-smbios: fix double free in dell_smbios_init() and fixes in dell_smbios_exit()

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 12/16/22 00:17, Rustam Subkhankulov wrote:
> If an error occurs in function build_tokens_sysfs(), then all the memory
> that has been allocated is correctly freed at certain labels at the end
> of this function.
> 
> build_tokens_sysfs() returns a non-zero value on error, function
> free_group() is called, resulting in a double-free. Removing
> free_group() function call will fix this problem.

You say that removing the free_group() function call will fix this problem
and I agree, but the patch does not actually remove the free_group()
call.

> Also, it seems that instead of free_group() call, there should be
> exit_dell_smbios_smm() and exit_dell_smbios_wmi() calls, since there is
> initialization, but there is no release of resources in case of an error.

This is correct too, but again not what the patch does ...

Please submit a new patch which actually replaces the free_group
call in this error-path with calling exit_dell_smbios_wmi() +
exit_dell_smbios_smm()

> Since calling 'exit' functions for 'smm' and 'wmi' is unsafe if
> initialization failed, in dell_smbios_exit() and dell_smbios_init()
> we need to call 'exit' only if initialization before was successful.

This is actually not correct, exit_dell_smbios_wmi() checks
an internal wmi_supported flag and exit_dell_smbios_smm()
checks if init_dell_smbios_smm() has created its platform_device.

There are some error-exit paths in init_dell_smbios_wmi() and
exit_dell_smbios_smm() which do not properly clear wmi_supported
resp. platform_device. Fixing those would be good, but adding new
variables inside dell-smbios-base.c to track this is not necessary.

Note the fix clearing wmi_supported / platform_device should
be done in a separate patch from the one replacing the free_group()
call.

Regards,

Hans





> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rustam Subkhankulov <subkhankulov@ispras.ru>
> Fixes: 25d47027e100 ("platform/x86: dell-smbios: Link all dell-smbios-* modules together")
> ---
>  drivers/platform/x86/dell/dell-smbios-base.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-smbios-base.c b/drivers/platform/x86/dell/dell-smbios-base.c
> index fc086b66f70b..cfef8cdd1215 100644
> --- a/drivers/platform/x86/dell/dell-smbios-base.c
> +++ b/drivers/platform/x86/dell/dell-smbios-base.c
> @@ -29,6 +29,8 @@ static struct device_attribute *token_location_attrs;
>  static struct device_attribute *token_value_attrs;
>  static struct attribute **token_attrs;
>  static DEFINE_MUTEX(smbios_mutex);
> +static bool wmi_initialized;
> +static bool smm_initialized;
>  
>  struct smbios_device {
>  	struct list_head list;
> @@ -607,6 +609,9 @@ static int __init dell_smbios_init(void)
>  			goto fail_sysfs;
>  	}
>  
> +	wmi_initialized = !(wmi);
> +	smm_initialized = !(smm);
> +
>  	return 0;
>  
>  fail_sysfs:
> @@ -628,8 +633,16 @@ static int __init dell_smbios_init(void)
>  
>  static void __exit dell_smbios_exit(void)
>  {
> -	exit_dell_smbios_wmi();
> -	exit_dell_smbios_smm();
> +	if (wmi_initialized) {
> +		exit_dell_smbios_wmi();
> +		wmi_initialized = 0;
> +	}
> +
> +	if (smm_initialized) {
> +		exit_dell_smbios_smm();
> +		smm_initialized = 0;
> +	}
> +
>  	mutex_lock(&smbios_mutex);
>  	if (platform_device) {
>  		if (da_tokens)