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@xxxxxxxxx>
> 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)
