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
| ||
|
Message-ID: <0851bc91-6a7c-4333-ad8a-3a18083411e3@lunn.ch> Date: Tue, 30 May 2023 22:04:52 +0200 From: Andrew Lunn <andrew@...n.ch> To: "Russell King (Oracle)" <linux@...linux.org.uk> Cc: Jakub Kicinski <kuba@...nel.org>, Dan Carpenter <dan.carpenter@...aro.org>, Oleksij Rempel <linux@...pel-privat.de>, 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, kernel-janitors@...r.kernel.org Subject: Re: [PATCH net] net: phy: fix a signedness bug in genphy_loopback() > > This is what I meant FWIW: > > > > diff --git a/include/linux/phy.h b/include/linux/phy.h > > index 7addde5d14c0..829bd57b8794 100644 > > --- a/include/linux/phy.h > > +++ b/include/linux/phy.h > > @@ -1206,10 +1206,13 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum) > > #define phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \ > > timeout_us, sleep_before_read) \ > > ({ \ > > - int __ret = read_poll_timeout(phy_read, val, val < 0 || (cond), \ > > + int __ret, __val; \ > > + \ > > + __ret = read_poll_timeout(phy_read, __val, __val < 0 || (cond), \ > > sleep_us, timeout_us, sleep_before_read, phydev, regnum); \ > > - if (val < 0) \ > > - __ret = val; \ > > + val = __val; This results in the sign being discarded if val is unsigned. Yes, the test is remove, which i assume will stop Smatch complaining, but it is still broken. > > + if (__val < 0) \ > > + __ret = __val; \ > > if (__ret) \ > > phydev_err(phydev, "%s failed: %d\n", __func__, __ret); \ > > __ret; \ > > I tried enabling -Wtype-limits but it's _very_ noisy :( This is a no go until GENMASK gets fixed :-( However, if that is fixed, we might be able to turn it on. But it will then trigger with this fix. So i still think a BUILD_BUG_ON is a better fix. Help developers get the code correct, rather than work around them getting it wrong. I also wonder if this needs to go down a level. Dan, how often do you see similar problems with the lower level read_poll_timeout() and friends? Andrew
Powered by blists - more mailing lists