linux-kernel - Re: [PATCH] pwm: xilinx: Fix overflow issue in 32-bit width PWM mode.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <10a8b1d3-e435-0a6b-9241-2eb660f6c94f@amd.com>
Date:   Thu, 15 Dec 2022 14:58:40 +0100
From:   Michal Simek <michal.simek@....com>
To:     Kenneth Sloat <ksloat@...ignlinxhs.com>,
        "sean.anderson@...o.com" <sean.anderson@...o.com>,
        "michal.simek@...inx.com" <michal.simek@...inx.com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pwm: xilinx: Fix overflow issue in 32-bit width PWM mode.



On 12/15/22 14:43, Kenneth Sloat wrote:
> Hi Michal thanks for your reply.
> 
>> On 12/12/22 14:59, Kenneth Sloat wrote:
>>> This timer HW supports 8, 16 and 32-bit timer widths. This
>>> driver uses a u32 to store the max value of the timer.
>>> Because addition is done to this max value, when operating
>>> in 32-bit mode, this will result in overflow that makes it
>>> impossible to set the timer period and thus the PWM itself.
>>>
>>> To fix this, simply make max a u64. This was tested on a
>>> Zynq UltraScale+.
> 
>> Can you please be more accurate where that overflow is happening.
>> I see that value is set only at probe like
>>
>> priv->max = BIT_ULL(width) - 1;
>>
>>
>> No doubt that there are calculation based on u64 types.
>>
>>
> 
> It actually does not happen in probe but when applying the PWM settings, here:
> 
> 	period_cycles = min_t(u64, period_cycles, priv->max + 2);

ok. It means (u64)priv->max + 2

will solve the problem too.

> 	if (period_cycles < 2)
> 		return -ERANGE;
> 
> If the timer is 32 bit, priv->max + 2 will roll over to 1, and thus will always be rejected as out of range. So, likely at minimum, a cast on priv->max would be needed here first.
> 
> duty_cycles would also have the same issue:
> 	duty_cycles = min_t(u64, duty_cycles, priv->max + 2);

and here as well.

>>>
>>> Signed-off-by: Ken Sloat <ksloat@...ignlinxhs.com>
>>> ---
>>>     include/clocksource/timer-xilinx.h | 2 +-
>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/clocksource/timer-xilinx.h b/include/clocksource/timer-xilinx.h
>>> index c0f56fe6d22a..d116f18de899 100644
>>> --- a/include/clocksource/timer-xilinx.h
>>> +++ b/include/clocksource/timer-xilinx.h
>>> @@ -41,7 +41,7 @@ struct regmap;
>>>     struct xilinx_timer_priv {
>>>            struct regmap *map;
>>>            struct clk *clk;
>>> -       u32 max;
>>> +       u64 max;
>>>     };
>>>
>>>     /**
>>> --
>>> 2.17.1
>>>
>>
>> Thanks,
>> Michal
> 
> Are you are good with the code change as is? If so, what do you propose? Should I amend the commit message with more details about where the overflow is occurring?

I would update commit message with both cases with simply saying that one way is 
to recast priv->max calculation because type is taken from priv->max which is 
u32 and one way to fix it is to recast it or change the type.
And that you are using second approach because it is more cleaner.

Thanks,
Michal