[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 at astralinux.ru>
>> ---
>>   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.