linux-kernel - Re: [PATCH resent] cpufreq: sparc: Fix exception handling in two functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20230403033529.x6n3ihhkypwizq3b@vireshk-i7>
Date:   Mon, 3 Apr 2023 09:05:29 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     Markus Elfring <Markus.Elfring@....de>
Cc:     kernel-janitors@...r.kernel.org, linux-pm@...r.kernel.org,
        "Rafael J. Wysocki" <rafael@...nel.org>, cocci@...ia.fr,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH resent] cpufreq: sparc: Fix exception handling in two
 functions

On 25-03-23, 15:02, Markus Elfring wrote:
> Date: Sat, 18 Mar 2023 11:40:11 +0100
> 
> The label “err_out” was used to jump to another pointer check despite of
> the detail in the implementation of the functions “us2e_freq_init”
> and “us3_freq_init” that it was determined already that the corresponding
> variable contained a null pointer (because of a failed memory allocation).
> 
> 1. Use additional labels.
> 
> 2. Reorder jump targets at the end.
> 
> 3. Delete an extra pointer check which became unnecessary
>    with this refactoring.
> 
> 
> This issue was detected by using the Coccinelle software.
> 
> Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac ("Linux-2.6.12-rc2")
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/cpufreq/sparc-us2e-cpufreq.c | 12 ++++++------
>  drivers/cpufreq/sparc-us3-cpufreq.c  | 12 ++++++------
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/cpufreq/sparc-us2e-cpufreq.c b/drivers/cpufreq/sparc-us2e-cpufreq.c
> index 92acbb25abb3..8534d8b1af56 100644
> --- a/drivers/cpufreq/sparc-us2e-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us2e-cpufreq.c
> @@ -324,12 +324,12 @@ static int __init us2e_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;

I would just return error from here.

> 
>  		us2e_freq_table = kzalloc((NR_CPUS * sizeof(*us2e_freq_table)),
>  			GFP_KERNEL);
>  		if (!us2e_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us2e_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -346,11 +346,11 @@ static int __init us2e_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us2e_driver = NULL;
> -		}
>  		kfree(us2e_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us2e_driver = NULL;
> +reset_freq_table:
>  		us2e_freq_table = NULL;

This wasn't set at this point, no point clearing it here. Also this
clearing of global variables isn't required at all, as after this
point no other function shall get called.

similar comments for the other file.

>  		return ret;
>  	}
> diff --git a/drivers/cpufreq/sparc-us3-cpufreq.c b/drivers/cpufreq/sparc-us3-cpufreq.c
> index e41b35b16afd..325f61bb2e40 100644
> --- a/drivers/cpufreq/sparc-us3-cpufreq.c
> +++ b/drivers/cpufreq/sparc-us3-cpufreq.c
> @@ -172,12 +172,12 @@ static int __init us3_freq_init(void)
>  		ret = -ENOMEM;
>  		driver = kzalloc(sizeof(*driver), GFP_KERNEL);
>  		if (!driver)
> -			goto err_out;
> +			goto reset_freq_table;
> 
>  		us3_freq_table = kzalloc((NR_CPUS * sizeof(*us3_freq_table)),
>  			GFP_KERNEL);
>  		if (!us3_freq_table)
> -			goto err_out;
> +			goto free_driver;
> 
>  		driver->init = us3_freq_cpu_init;
>  		driver->verify = cpufreq_generic_frequency_table_verify;
> @@ -194,11 +194,11 @@ static int __init us3_freq_init(void)
>  		return 0;
> 
>  err_out:
> -		if (driver) {
> -			kfree(driver);
> -			cpufreq_us3_driver = NULL;
> -		}
>  		kfree(us3_freq_table);
> +free_driver:
> +		kfree(driver);
> +		cpufreq_us3_driver = NULL;
> +reset_freq_table:
>  		us3_freq_table = NULL;
>  		return ret;
>  	}
> --
> 2.40.0

-- 
viresh