linux-kernel - RE: [PATCH v2] i2c: altera: Fix potential integer 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] [day] [month] [year] [list]

Date:   Thu, 13 Feb 2020 09:58:15 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Wolfram Sang' <wsa@...-dreams.de>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>
CC:     Thor Thayer <thor.thayer@...ux.intel.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2] i2c: altera: Fix potential integer overflow

From: Wolfram Sang
> Sent: 13 February 2020 09:10
> 
> On Tue, Feb 11, 2020 at 08:47:04AM -0600, Gustavo A. R. Silva wrote:
> > Factor out 100 from the equation and do 32-bit arithmetic (3 * clk_mhz / 10)
> > instead of 64-bit.
> >
> > Notice that clk_mhz is MHz, so the multiplication will never wrap 32 bits
> > and there is no need for div_u64().
> 
> Was there ever? With
> 
> 	u32 clk_mhz = clk_get_rate(idev->i2c_clk) / 1000000;
> 
> a later multiplication with 300 should not wrap u32?
> 
> >  	/* SDA Hold Time, 300ns */
> > -	writel(div_u64(300 * clk_mhz, 1000), idev->base + ALTR_I2C_SDA_HOLD);
> > +	writel(3 * clk_mhz / 10, idev->base + ALTR_I2C_SDA_HOLD);
> 
> The change itself is OK, yet I wonder about the comment above:
> 
> 'clk_mhz * 0.3' will not give a constant 300ns, or?

Depends on the definition of the register.
A count of zero may mean one clock period.
So maybe it could have (3 * clk - 1)/10 instead of (3 * clk + 9)/10.
OTOH nothing probably requires that much hold time.

If that is the 'standard' Altera Avalon slave I2C 'megafunction' I2C
master then it is probably so slow to use it can't matter.
Most of those blocks are crap, they aren't even small.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)