linux-kernel - Re: [PATCH] clk: spacemit: fix error handling in ccu_pll_set_rate/_round

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aH7jM_q3y85o2Daf@ketchup>
Date: Tue, 22 Jul 2025 01:02:43 +0000
From: Haylen Chu <heylenay@....org>
To: Akhilesh Patil <akhilesh@...iitb.ac.in>, mturquette@...libre.com,
	sboyd@...nel.org, dlan@...too.org, elder@...cstar.com,
	inochiama@...look.com, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, heylenay@...look.com, paul.walmsley@...ive.com,
	palmer@...belt.com, aou@...s.berkeley.edu, alex@...ti.fr
Cc: linux-clk@...r.kernel.org, linux-riscv@...ts.infradead.org,
	spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org,
	unicornxdotw@...mail.com, jszhang@...nel.org,
	zhangmeng.kevin@...ux.spacemit.com, akhileshpatilvnit@...il.com,
	skhan@...uxfoundation.org
Subject: Re: [PATCH] clk: spacemit: fix error handling in
 ccu_pll_set_rate/_round_rate

On Tue, Jul 22, 2025 at 12:29:23AM +0530, Akhilesh Patil wrote:
> Initialize best_entry pointer with NULL in ccu_pll_lookup_best_rate()
> to avoid returning garbage value when function fails to assign it
> a valid rate entry.

This doesn't sound very reasonable to me. Looking through
ccu_pll_lookup_best_rate(),

	static const struct ccu_pll_rate_tbl *
	ccu_pll_lookup_best_rate(struct ccu_pll *pll, unsigned long rate)
	{
	        struct ccu_pll_config *config = &pll->config;
	        const struct ccu_pll_rate_tbl *best_entry;
	        unsigned long best_delta = ULONG_MAX;
	        int i;

	        for (i = 0; i < config->tbl_num; i++) {
	                const struct ccu_pll_rate_tbl *entry = &config->rate_tbl[i];
	                unsigned long delta = abs_diff(entry->rate, rate);

	                if (delta < best_delta) {
	                        best_delta = delta;
	                        best_entry = entry;
	                }
	        }

	        return best_entry;
	}

best_entry is assigned as long as there's one entry fits the delta
better. Since best_delta is set to ULONG_MAX, any entry with non-zero
rates fits the required rate "better" at start of the loop. As long as
we have at least one non-zero entry defined for the PLL, best_entry is
always initialized and ccu_pll_lookup_best_rate() cannot return an
invalid pointer. And all existing PLLs do define at least one entry.

> Avoid passing invalid rate entry reference to
> ccu_pll_update_param by adding appropriate error handling in
> ccu_pll_set_rate and ccu_pll_round_rate.
> Address the effects of uninitialized pointer as reported
> by smatch and coverity static code analysis tools.
> 
> Addresses-Coverity-ID: 1649164
> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/r/202505111057.ejK2J56K-lkp@intel.com/

Thus this looks like a false-positive to me.

> Signed-off-by: Akhilesh Patil <akhilesh@...iitb.ac.in>

Regards,
Haylen Chu

> ---
>  drivers/clk/spacemit/ccu_pll.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> index 4427dcfbbb97..3fc6a30f98b7 100644
> --- a/drivers/clk/spacemit/ccu_pll.c
> +++ b/drivers/clk/spacemit/ccu_pll.c
> @@ -21,7 +21,7 @@ static const struct ccu_pll_rate_tbl *ccu_pll_lookup_best_rate(struct ccu_pll *p
>  							       unsigned long rate)
>  {
>  	struct ccu_pll_config *config = &pll->config;
> -	const struct ccu_pll_rate_tbl *best_entry;
> +	const struct ccu_pll_rate_tbl *best_entry = NULL;
>  	unsigned long best_delta = ULONG_MAX;
>  	int i;
>  
> @@ -107,6 +107,10 @@ static int ccu_pll_set_rate(struct clk_hw *hw, unsigned long rate,
>  	const struct ccu_pll_rate_tbl *entry;
>  
>  	entry = ccu_pll_lookup_best_rate(pll, rate);
> +
> +	if (!entry)
> +		return -EINVAL;
> +
>  	ccu_pll_update_param(pll, entry);
>  
>  	return 0;
> @@ -129,8 +133,11 @@ static long ccu_pll_round_rate(struct clk_hw *hw, unsigned long rate,
>  			       unsigned long *prate)
>  {
>  	struct ccu_pll *pll = hw_to_ccu_pll(hw);
> +	const struct ccu_pll_rate_tbl *entry;
> +
> +	entry = ccu_pll_lookup_best_rate(pll, rate);
>  
> -	return ccu_pll_lookup_best_rate(pll, rate)->rate;
> +	return entry ? entry->rate : 0;
>  }
>  
>  static int ccu_pll_init(struct clk_hw *hw)
> -- 
> 2.34.1
>