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

[sashiko-bot@kernel.org]

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