Re: [PATCH v2 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 5 potential issue(s) to consider:

New issues:
- [High] Integer truncation and logic error causing silent massive overclocking.

Pre-existing issues:
- [High] Unhandled division by zero in rate calculation and configuration callbacks.
- [High] Out-of-bounds register write leading to bitfield corruption.
- [High] Missing lock leading to data races during dynamic hardware reconfiguration.
- [High] Integer overflow in fractional divider calculation on 32-bit systems.
--

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

clk: clocking-wizard: Fix division by zero and unbounded register write

This commit attempts to fix division by zero and unbounded register writes by
clamping the divisor to a minimum of 1 and ensuring min_t results are
correctly assigned. It also adds early returns when the rate is 0 in the
divisor calculation functions.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index 381e396aef0e7..289c32ab5cd80 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 driver requests a clock rate
of 0, does the DIV_ROUND_CLOSEST() macro cause a division by zero panic here?

Since this occurs while holding the divider->lock, would a panic also leak
the global hardware lock?

>  
>  	/* 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);
>  
>  	/* Set divisor and clear phase offset */
>  	writel(value, div_addr);

[ ... ]

> @@ -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 if req->rate is 0, wouldn't the
DIV_ROUND_CLOSEST() macro itself trigger a division by zero panic before it
even reaches the newly added zero check below?

> +	if (!div)
> +		div = 1;

[Severity: High]
Because the div variable in clk_wzrd_determine_rate() is declared as a u8:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_determine_rate() {
    u8 div;
    ...
    div = DIV_ROUND_CLOSEST(req->best_parent_rate, req->rate);
}

Does assigning a valid large divisor (like 256) result in a modulo-256
truncation to 0?

If a consumer requests a valid low frequency that requires a multiple of 256,
this truncation could silently force the divisor to 0. Then, this new logic
would force the divisor to 1, potentially bypassing the hardware limit and
causing unexpected clock frequencies.

>  
>  	req->rate = req->best_parent_rate / div;
>  

[ ... ]

Additionally, there are a few other pre-existing issues in this file worth
investigating:

[Severity: High]
This is a pre-existing issue, but does the Versal variant lack the same bounds
checking that was just fixed above?

Looking at clk_wzrd_ver_dynamic_reconfig():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_ver_dynamic_reconfig() {
    ...
    value = DIV_ROUND_CLOSEST(parent_rate, rate);
    regh = (value / 4);
    ...
    regval = regh | regh << WZRD_CLKFBOUT_H_SHIFT;
    writel(regval, div_addr + 4);
    ...
}

If the rate is low and demands a large divisor, an unbounded value propagates
to regh. Because it is OR-ed without a bitmask, could this overwrite adjacent
configuration bits in the register?

Also, like clk_wzrd_dynamic_reconfig(), does this DIV_ROUND_CLOSEST() call
panic if the rate is 0?


[Severity: High]
This is a pre-existing issue, but is the fractional dynamic reconfiguration
path missing spinlock protection?

While other paths synchronize access to shared wizard initialization
registers, clk_wzrd_dynamic_reconfig_f() seems to omit the lock entirely:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig_f() {
    int err;
    u32 value, pre;
    ...
    writel(WZRD_DR_BEGIN_DYNA_RECONF_5_2,
           divider->base + WZRD_DR_INIT_REG_OFFSET);
    ...
}

Could concurrent rate changes on multiple fractional outputs corrupt the
sequence of shared register writes?


[Severity: High]
This is a pre-existing issue, but does the fractional divider calculation
overflow on 32-bit systems?

Looking at clk_wzrd_dynamic_reconfig_f():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig_f() {
    ...
    rate_div = DIV_ROUND_DOWN_ULL(parent_rate * 1000, rate);
    clockout0_div = rate_div / 1000;
    pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
    ...
}

On 32-bit architectures, parent_rate * 1000 is computed as a 32-bit integer.
If parent_rate exceeds 4.29 MHz, wouldn't this overflow before the implicit
cast to u64 by DIV_ROUND_DOWN_ULL?

Furthermore, do DIV_ROUND_DOWN_ULL() and DIV_ROUND_CLOSEST() trigger a
division by zero if the rate is 0?


[Severity: High]
This is a pre-existing issue, but do the recalculation paths also suffer from
division by zero if the hardware is uninitialized?

Looking at clk_wzrd_recalc_rate_all():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_recalc_rate_all() {
    ...
    return DIV_ROUND_CLOSEST_ULL(parent_rate * mul, d * (o * 1000 + f));
}

And clk_wzrd_recalc_ratef():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_recalc_ratef() {
    ...
    return mult_frac(parent_rate, 1000, (div * 1000) + frac);
}

If the hardware register values (d, o, f, div, frac) are 0 at boot-time,
would dividing by these unchecked values cause a division by zero panic?

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