netdev - Re: [PATCH net-next] net: phy: mdio-bcm-unimac: Cast denominator to unsigned long to avoid overflow

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: <b1acf9d0-872c-487a-9938-6d667959d0d3@gmail.com>
Date: Tue, 12 Mar 2024 12:23:20 -0700
From: Doug Berger <opendmb@...il.com>
To: Florian Fainelli <f.fainelli@...il.com>,
 "Russell King (Oracle)" <linux@...linux.org.uk>,
 Daniil Dulov <d.dulov@...ddin.ru>
Cc: Jakub Kicinski <kuba@...nel.org>,
 Florian Fainelli <florian.fainelli@...adcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Andrew Lunn <andrew@...n.ch>,
 Heiner Kallweit <hkallweit1@...il.com>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
Subject: Re: [PATCH net-next] net: phy: mdio-bcm-unimac: Cast denominator to
 unsigned long to avoid overflow

On 3/12/2024 10:23 AM, Florian Fainelli wrote:
> On 3/12/24 10:18, Russell King (Oracle) wrote:
>> On Tue, Mar 12, 2024 at 07:53:58PM +0300, Daniil Dulov wrote:
>>> The expression priv->clk_freq * 2 can lead to overflow that will cause
>>> a division by zero. So, let's cast it to unsigned long to avoid it.
>>
>> How does casting this help? "unsigned long" can still be 32-bit.
>> Maybe unimac_mdio_probe() should be validating the value it read from
>> DT won't overflow? I suspect that a value of 2.1GHz is way too large
>> for this property in any case.
>>
>> https://en.wikipedia.org/wiki/Management_Data_Input/Output#Electrical_specification
>>
>> (note, this driver is clause-22 only.)
>>
> 
> Had commented on the previous version (not sure why this was not 
> prefixed with v2) that the maximum clock frequency for this clock is 
> 250MHz, the driver could check that to prevent for an overflow, most 
> certainly.

Could also use:
-	div = (rate / (2 * priv->clk_freq)) - 1;
+	div = ((rate / priv->clk_freq) >> 1) - 1;
which is mathematically equivalent without the risk of overflow.

-Doug