[PATCH] clk: clocking-wizard: Fix overflow in clockout0 divisor calculation

[PATCH] clk: clocking-wizard: Fix overflow in clockout0 divisor calculation

[shaopeijie]

From: Peijie Shao <shaopeijie@gmail.com>

On 32-bit platforms (e.g., Zynq 7000 series), parent_rate * 1000
may overflow, leading to incorrect divisor calculation.

The issue was first found on Zynq7020 with "parent_rate=1GHz"
and "xlnx,nr-outputs=2".

Cast parent_rate to u64 and use the ULL version of DIV_ROUND*
instead.

Signed-off-by: Peijie Shao <shaopeijie@gmail.com>
---
 drivers/clk/xilinx/clk-xlnx-clock-wizard.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
index edf8c0eaca27..5abbc6b83b72 100644
--- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
+++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
@@ -881,10 +881,10 @@ static int clk_wzrd_dynamic_reconfig_f(struct clk_hw *hw, unsigned long rate,
 	struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
 	void __iomem *div_addr = divider->base + divider->offset;
 
-	rate_div = DIV_ROUND_DOWN_ULL(parent_rate * 1000, rate);
+	rate_div = DIV_ROUND_DOWN_ULL((u64)parent_rate * 1000, rate);
 	clockout0_div = rate_div / 1000;
 
-	pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
+	pre = DIV_ROUND_CLOSEST_ULL((u64)parent_rate * 1000, rate);
 	f = (u32)(pre - (clockout0_div * 1000));
 	f = f & WZRD_CLKOUT_FRAC_MASK;
 	f = f << WZRD_CLKOUT_DIVIDE_WIDTH;
-- 
2.43.5

Re: [PATCH] clk: clocking-wizard: Fix overflow in clockout0 divisor calculation

[sashiko-bot]

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

Pre-existing issues:
- [High] The return value of `min_t()` is ignored, failing to cap `value` to the maximum limit.
- [High] Uncasted 32-bit integer overflows in multiple clock calculation functions.
- [High] Lack of rate validation causes divide-by-zero kernel panics.
--

commit 4504b18bf7e5e8a384d6dae3f8540a24668aa024
Author: Peijie Shao <shaopeijie@gmail.com>

clk: clocking-wizard: Fix overflow in clockout0 divisor calculation

This patch fixes a 32-bit overflow issue when calculating the divisor by
casting parent_rate to u64 and using the ULL version of DIV_ROUND macros.

> diff --git a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> index edf8c0eaca27c..5abbc6b83b728 100644
> --- a/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> +++ b/drivers/clk/xilinx/clk-xlnx-clock-wizard.c
> @@ -881,10 +881,10 @@ static int clk_wzrd_dynamic_reconfig_f(struct clk_hw *hw, unsigned long rate,
>  	struct clk_wzrd_divider *divider = to_clk_wzrd_divider(hw);
>  	void __iomem *div_addr = divider->base + divider->offset;
>  
> -	rate_div = DIV_ROUND_DOWN_ULL(parent_rate * 1000, rate);
> +	rate_div = DIV_ROUND_DOWN_ULL((u64)parent_rate * 1000, rate);

[Severity: High]
This isn't a bug introduced by this patch, but if rate is 0,
DIV_ROUND_DOWN_ULL will trigger a divide-by-zero kernel panic. 

In clk_wzrd_determine_rate_f(), the rate request is just an empty stub that
returns 0 without validating req->rate:

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_determine_rate_f() {
    ...
    static int clk_wzrd_determine_rate_f(struct clk_hw *hw,
                                         struct clk_rate_request *req)
    {
        return 0;
    }
    ...
}

Can this allow a rate of 0 to reach clk_wzrd_dynamic_reconfig_f() and panic
the kernel?

A similar lack of rate validation exists in clk_wzrd_determine_rate():

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

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

[Severity: High]
This is a pre-existing issue, but if req->rate is 0, DIV_ROUND_CLOSEST
divides by zero. Additionally, if an out-of-bounds large rate evaluates div
to 0, does the subsequent line trigger another divide-by-zero?

>  	clockout0_div = rate_div / 1000;
>  
> -	pre = DIV_ROUND_CLOSEST((parent_rate * 1000), rate);
> +	pre = DIV_ROUND_CLOSEST_ULL((u64)parent_rate * 1000, rate);
>  	f = (u32)(pre - (clockout0_div * 1000));
>  	f = f & WZRD_CLKOUT_FRAC_MASK;
>  	f = f << WZRD_CLKOUT_DIVIDE_WIDTH;


While reviewing this file, I noticed another pre-existing issue in
clk_wzrd_dynamic_reconfig():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_dynamic_reconfig() {
    ...
    /* Cap the value to max */
    min_t(u32, value, WZRD_DR_MAX_INT_DIV_VALUE);

    /* Set divisor and clear phase offset */
    writel(value, div_addr);
    ...
}

[Severity: High]
This is a pre-existing issue, but the return value of min_t() is silently
discarded here. Does this mean the hardware register receives an uncapped
value that could exceed WZRD_DR_MAX_INT_DIV_VALUE?


Since this patch fixes a 32-bit arithmetic overflow, I noticed similar
overflows in other clock calculation functions. In
clk_wzrd_get_divisors_ver():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_get_divisors_ver() {
    ...
    for (m = mmin; m <= mmax; m++) {
        for (d = dmin; d <= dmax; d++) {
            vco_freq = DIV_ROUND_CLOSEST((parent_rate * m), d);
    ...
}

[Severity: High]
This is a pre-existing issue, but does parent_rate * m suffer from the exact
same 32-bit overflow on 32-bit platforms before it gets divided?

There are also similar uncasted multiplications in
clk_wzrd_recalc_rate_all_ver():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_recalc_rate_all_ver() {
    ...
        parent_rate = DIV_ROUND_CLOSEST((parent_rate * mult), WZRD_FRAC_GRADIENT);
    } else {
        parent_rate = parent_rate * mult;
    }
    ...
}

And in clk_wzrd_determine_rate_all():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_determine_rate_all() {
    ...
    req->rate = mult_frac(req->best_parent_rate, m * 1000 + divider->m_frac,
                          d * (o * 1000 + divider->o_frac));
    ...
}

And in clk_wzrd_ver_determine_rate_all():

drivers/clk/xilinx/clk-xlnx-clock-wizard.c:clk_wzrd_ver_determine_rate_all() {
    ...
    int_freq =  divider_recalc_rate(hw, req->best_parent_rate * m, div,
                                    divider->table,
                                    divider->flags, divider->width);

    if (req->rate > int_freq) {
        f = DIV_ROUND_CLOSEST_ULL(req->rate * WZRD_FRAC_POINTS,
                                  int_freq);
        req->rate = DIV_ROUND_CLOSEST(int_freq * f, WZRD_FRAC_POINTS);
    }
    ...
}

[Severity: High]
These are also pre-existing issues, but should they be cast to u64 as well
to prevent silent wraparounds during clock calculations?

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