Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <83323dac-3bb7-91a5-5e44-b78141fe63c5@redhat.com>
Date:   Thu, 12 Jan 2023 17:40:57 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH] dell-smbios: fix double free in dell_smbios_init() and
 fixes in dell_smbios_exit()
Content-Language: en-US, nl
To:     Rustam Subkhankulov <subkhankulov@ispras.ru>
Cc:     Mark Gross <markgross@kernel.org>,
        ye xingchen <ye.xingchen@zte.com.cn>,
        "Darren Hart (VMware)" <dvhart@infradead.org>,
        Mario Limonciello <mario.limonciello@dell.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        Alexey Khoroshilov <khoroshilov@ispras.ru>
References: <20221215231741.154355-1-subkhankulov@ispras.ru>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20221215231741.154355-1-subkhankulov@ispras.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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)