Re: [PATCH v3 7/8] clk: clocking-wizard: Fix division by zero and unbounded register write

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

Pre-existing issues:
- [High] Divide-by-zero vulnerabilities remain in multiple callback functions for rate == 0.
- [High] Computed divisor in clk_wzrd_determine_rate() is silently truncated, leading to incorrect clock rates.
- [High] Missing upper bounds check in clk_wzrd_ver_dynamic_reconfig() causes register corruption.
- [High] Missing lower bounds check allows 0 to be written to hardware divisor registers.
- [High] Unchecked large rates cause hardware to be programmed with 0, leading to divide-by-zero during recalc.
- [High] High atomic latency and preemption stall during dynamic reconfiguration.
--

--- Patch [7]: [PATCH v3 7/8] clk: clocking-wizard: Fix division by zero and unbounded register write ---
commit 5e3507219b53560de47af4a3c5f59ba7a3f31a01
Author: Shubhrajyoti Datta <shubhrajyoti.datta@amd.com>
Subject: clk: clocking-wizard: Fix division by zero and unbounded register write

This commit addresses a division by zero in clk_wzrd_determine_rate and
adds checks for zero rates in divisor calculations. It also fixes an
unbounded register write in clk_wzrd_dynamic_reconfig by capturing the
result of the min_t calculation.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 4ddcb41353061..b16656a11f4b5 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -296,7 +296,7 @@ static int clk_wzrd_dynamic_reconfig(struct clk_hw *hw, unsigned long rate,
>  	value = DIV_ROUND_CLOSEST(parent_rate, rate);

[Severity: High]
This is a pre-existing issue, but if a consumer requests a clock rate of
0, won't DIV_ROUND_CLOSEST() evaluate a division by zero here?

The patch successfully identifies this issue in clk_wzrd_get_divisors(),
but it appears other callbacks like clk_wzrd_dynamic_reconfig() and
clk_wzrd_ver_dynamic_reconfig() still pass rate directly as a divisor.

[Severity: High]
This is also a pre-existing issue, but does this need a lower bounds check?

If a consumer requests a rate significantly larger than the parent rate,
value evaluates to 0. It bypasses the upper bound and writes 0 directly to
the clock wizard hardware divisor registers.

[Severity: High]
This isn't a bug introduced by this patch, but while you added a cap for
value here, clk_wzrd_ver_dynamic_reconfig() seems to still lack an upper
bounds check.

A small rate there yields a large value, causing the regh variable to
exceed its 8-bit mask and overflow when shifted by WZRD_CLKFBOUT_H_SHIFT,
corrupting adjacent hardware register values.

[Severity: High]
This is a pre-existing issue, but could a similar problem cause a panic
in the fractional clock callbacks?

clk_wzrd_determine_rate_f() is an empty stub that fails to bound req->rate.
As a result, clk_wzrd_dynamic_reconfig_f() can divide by rate, evaluate
to 0, and program a 0 divisor. Later, clk_wzrd_recalc_ratef() reads this 0
from the register and calls mult_frac() with 0 as the divisor, which would
crash the system.

>  
>  	/* Cap the value to max */
> -	min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);
> +	value = min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);

[ ... ]

> @@ -334,6 +334,8 @@ static int clk_wzrd_determine_rate(struct clk_hw *hw,
>  	 * achievable
>  	 */
>  	div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);

[Severity: High]
This is a pre-existing issue, but does this still evaluate a division by
zero if req->rate is 0?

[Severity: High]
This is a pre-existing issue, but won't div be silently truncated here?

Since div is declared as a u8 at the top of clk_wzrd_determine_rate(), if
the requested rate yields a divisor of 256 or larger, it truncates. For
example, 256 truncates to 0, and the new if (!div) logic below will
incorrectly force it to 1, returning a massive clock rate.

Should div be declared as a u32 and correctly capped at the hardware limit
instead?

> +	if (!div)
> +		div = 1;
>  
>  	req->rate = req->best_parent_rate / div;

[ ... ]

> @@ -348,6 +350,9 @@ static int clk_wzrd_get_divisors_ver(struct clk_hw *hw, unsigned long rate,
>  	u32 m, d, o;
>  	u32 mmin, mmax, dmin, dmax, omin, omax;
>  
> +	if (!rate)
> +		return -EINVAL;
> +

[Severity: High]
This is a pre-existing issue, but does this function cause severe
scheduling latency?

When called from clk_wzrd_dynamic_all_ver() or clk_wzrd_dynamic_all(), an
IRQ-disabled spinlock is held across the nested loops in this function
which run for over 50,000 to 100,000 iterations. Performing this massive
search space with 64-bit integer divisions inside a spin_lock_irqsave()
critical section could stall the CPU for milliseconds.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731072049.957624-1-shubhrajyoti.datta@amd.com?part=7