Re: [PATCH] clk: mvebu: Prevent division by zero in clk_double_div_recalc_rate()

[Alexandra Diupina]

Hello, Andrew!


09/09/24 17:02, Andrew Lunn пишет:
> On Mon, Sep 09, 2024 at 04:38:07PM +0300, Alexandra Diupina wrote:
> > get_div() may return zero, so it is necessary to check
> > before calling DIV_ROUND_UP_ULL().
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 8ca4746a78ab ("clk: mvebu: Add the peripheral clock driver for Armada 3700")
> > Signed-off-by: Alexandra Diupina <adiupina@xxxxxxxxxxxxx>
> > ---
> >   drivers/clk/mvebu/armada-37xx-periph.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/mvebu/armada-37xx-periph.c b/drivers/clk/mvebu/armada-37xx-periph.c
> > index 8701a58a5804..d0e1d591e4f2 100644
> > --- a/drivers/clk/mvebu/armada-37xx-periph.c
> > +++ b/drivers/clk/mvebu/armada-37xx-periph.c
> > @@ -343,7 +343,10 @@ static unsigned long clk_double_div_recalc_rate(struct clk_hw *hw,
> >   	div = get_div(double_div->reg1, double_div->shift1);
> >   	div *= get_div(double_div->reg2, double_div->shift2);
> >   
> > -	return DIV_ROUND_UP_ULL((u64)parent_rate, div);
> > +	if (!div)
> > +		return 0;
> Looking at this code, it seems to me some fundamental assumption has
> gone wrong here, if the dividers are 0. We want to know about this,
> and a kernel taking a / 0 exception would be a good way to indicate
> something is very wrong. Won't returning 0 just hide the problem, not
> make it obvious?
>
> Checking for a /0 on user input makes a lot of sense, but here, i
> think you are just hiding bugs. Please consider this when making
> similar changes in other parts of the kernel. Why has a /0 happened?
>
> Tools like SVACE just point at possible problems. You then need to
> look at them in detail, understand the context, and decide on the
> proper fix, which might actually be, a /0 is good.
>
> 	Andrew

The value of div depends on double_div->reg1, double_div->reg2,
double_div->shift1, double_div->shift2.
The fields reg1, reg2, shift1, shift2 in the clk_double_div structure
are filled using the PERIPH_DOUBLEDIV macro, which is called from the
PERIPH_CLK_FULL_DD and PERIPH_CLK_MUX_DD macros (the last 4 arguments).

It is not clear what values can be contained in the registers at the
addresses DIV_SEL0, DIV_SEL1, DIV_SEL2 (can readl() and some bit
operations give a value > 6 in get_div()), so the final value of div can 
be zero.

I used just return 0, since the recalc_rate field in the clk_ops structure
has a comment "If the driver cannot figure out a rate for this clock,
it must return 0.". I'll fix it to kernel exception, thanks for the tip.