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

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

[Rustam Subkhankulov]

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.

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.

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.

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)
-- 
2.34.1

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

[Hans de Goede]

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)