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: <657b9999.5d0a0220.ec414.ed08@mx.google.com> Date: Thu, 14 Dec 2023 18:29:28 +0100 From: Christian Marangi <ansuelsmth@...il.com> To: "Russell King (Oracle)" <linux@...linux.org.uk> Cc: 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>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Vladimir Oltean <olteanv@...il.com>, David Epping <david.epping@...singlinkelectronics.com>, Harini Katakam <harini.katakam@....com>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [net-next PATCH v7 2/4] net: phy: extend PHY package API to support multiple global address On Thu, Dec 14, 2023 at 11:54:26PM +0000, Russell King (Oracle) wrote: > On Thu, Dec 14, 2023 at 05:54:51PM +0100, Christian Marangi wrote: > > What I don't like is the wrap check. > > > > But I wonder... Isn't it easier to have > > > > unsigned int addr = shared->base_addr + addr_offset; > > > > and check if >= PHY_MAC_ADDR? > > > > Everything is unsigned (so no negative case) and wrap is not possible as > > nothing is downcasted. > > I'm afraid that I LOL'd at "wrap is not possible" ! Of course it's > possible. Here's an example: > Yes I just think about it and I'm also LOLing at the "not possible"... > shared->base_addr is 20 > addr_offset is ~0 (or -1 casted to an unsigned int) > addr becomes 19 > > How about: > > if (addr_offset >= PHY_MAX_ADDR) > return -EIO; > > addr = shared->base_addr + addr_offset; > if (addr >= PHY_MAX_ADDR) > return -EIO; > > and then we could keep 'addr' as u8. Ok just to make sure static int phy_package_address(struct phy_device *phydev, unsigned int addr_offset) { struct phy_package_shared *shared = phydev->shared; unsigned int addr; if (addr_offset >= PHY_MAX_ADDR) return -EIO; addr = shared->base_addr + addr_offset; if (addr >= PHY_ADDR_MAX) return -EIO; /* we know that addr will be in the range 0..31 and thus the * implicit cast to a signed int is not a problem. */ return addr; } And call u8 addr = phy_package_address(phydev, addr_offset); Maybe one if can be skipped with the following fun thing? static int phy_package_address(struct phy_device *phydev, unsigned int addr_offset) { struct phy_package_shared *shared = phydev->shared; u8 base_addr = shared->base_addr; if (addr_offset >= PHY_MAX_ADDR - base_addr) return -EIO; /* we know that addr will be in the range 0..31 and thus the * implicit cast to a signed int is not a problem. */ return base_addr + addr_offset; } (don't hate me it's late here and my brain is half working ahahha) > > Honestly, I have wondered why the mdio bus address is a signed int, but > never decided to do anything about it. > Maybe because direct usage of mdiobus_ is discouraged and phy_write will use an addr that is already validated. -- Ansuel
Powered by blists - more mailing lists