Re: [PATCH 1/1] clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: nuvoton: rate mismatch and missing bounds check in determine_rate
- [High] clk: nuvoton: divide by zero due to integer overflow in rate multiplication
--

commit 434b92fcdb326fe48e43db4fafe0804d04aee387
Author: Joey Lu <a0987203069@gmail.com>

    clk: nuvoton: ma35d1-divider: use direct formula instead of divider table

This patch replaces the table-based clock divider with a direct formula in the
Nuvoton MA35D1 ADC clock divider driver. It drops the mask bit mechanism and
computes the divisor explicitly in the recalc, determine, and set rate
callbacks.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c
> index e992e7c303419..03fcf87caae18 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1-divider.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c
[ ... ]
> @@ -29,44 +27,60 @@ static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_h
[ ... ]
>  static int ma35d1_clkdiv_determine_rate(struct clk_hw *hw,
>  					struct clk_rate_request *req)
>  {
>  	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> +	unsigned int val;
> +
> +	if (!req->rate)
> +		return -EINVAL;
>  
> -	return divider_determine_rate(hw, req, dclk->table, dclk->width,
> -				      CLK_DIVIDER_ROUND_CLOSEST);
> +	val = DIV_ROUND_UP(req->best_parent_rate, 2 * req->rate);

[Severity: High]
Does this code divide by zero due to integer overflow in rate multiplication?

If a caller or untrusted user (via debugfs) requests a rate such that
2 * req->rate overflows a 32-bit or 64-bit unsigned integer (for example,
rate = 1UL << (BITS_PER_LONG - 1)), the denominator 2 * req->rate wraps to 0.

This would cause DIV_ROUND_UP(req->best_parent_rate, 0) to evaluate to a
division by zero panic.

[Severity: High]
Can this create a rate mismatch between ma35d1_clkdiv_determine_rate() and
ma35d1_clkdiv_set_rate()?

This calculates the divider using a strict ceiling instead of closest match,
and also fails to check rate boundaries against req->min_rate and
req->max_rate.

> +	if (val == 0)
> +		val = 1;
> +	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
> +		val = clk_div_mask(dclk->width) + 1;
> +
> +	req->rate = DIV_ROUND_CLOSEST_ULL((u64)req->best_parent_rate, 2 * val);

It then promises a closest-rounded integer rate to the clock framework here.
When ma35d1_clkdiv_set_rate() is called later, the recomputation will yield a
different divisor.

> +
> +	return 0;
>  }
>  
>  static int ma35d1_clkdiv_set_rate(struct clk_hw *hw, unsigned long rate, unsigned long parent_rate)
>  {
> -	int value;
> -	unsigned long flags = 0;
> -	u32 data;
>  	struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw);
> +	unsigned long flags;
> +	unsigned int val;
> +	u32 data;
> +
> +	if (!rate)
> +		return -EINVAL;
>  
> -	value = divider_get_val(rate, parent_rate, dclk->table,
> -				dclk->width, CLK_DIVIDER_ROUND_CLOSEST);
> +	val = DIV_ROUND_UP(parent_rate, 2 * rate);

[Severity: High]
Could this identical integer overflow cause division by zero here as well?

Also, because the rate was rounded in ma35d1_clkdiv_determine_rate(),
recalculating the divider using DIV_ROUND_UP(parent_rate, 2 * rate) can yield
a completely different divisor.

For example, with parent_rate=1000 and an original val=6,
ma35d1_clkdiv_determine_rate() promises rate=83.

Then ma35d1_clkdiv_set_rate() calculates DIV_ROUND_UP(1000, 166) = 7, applying
a divider of 7 instead of 6 (which results in an actual rate of 71 instead of
83).

> +	if (val == 0)
> +		val = 1;
> +	if (val > (unsigned int)clk_div_mask(dclk->width) + 1)
> +		val = clk_div_mask(dclk->width) + 1;
>  
>  	spin_lock_irqsave(dclk->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929072106.175202-1-a0987203069@gmail.com?part=1