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: <657b921d.5d0a0220.7815b.87dd@mx.google.com>
Date: Thu, 14 Dec 2023 17:54:51 +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 05:05:39PM +0000, Russell King (Oracle) wrote:
> On Thu, Dec 14, 2023 at 01:10:24PM +0100, Christian Marangi wrote:
> > @@ -1998,46 +1999,54 @@ int __phy_hwtstamp_set(struct phy_device *phydev,
> >  		       struct kernel_hwtstamp_config *config,
> >  		       struct netlink_ext_ack *extack);
> >  
> > -static inline int phy_package_read(struct phy_device *phydev, u32 regnum)
> > +static inline int phy_package_read(struct phy_device *phydev,
> > +				   unsigned int addr_offset, u32 regnum)
> >  {
> >  	struct phy_package_shared *shared = phydev->shared;
> > +	u8 addr = shared->base_addr + addr_offset;
> >  
> > -	if (!shared)
> > +	if (addr >= PHY_MAX_ADDR)
> >  		return -EIO;
> 
> I did notice that you're using u8 in patch 1 as well - and while it's
> fine in patch 1 (because we validate the range of the value we will
> assign to that variable) that is not the case here.
> 
> Yes, shared->base_addr is a u8, but addr_offset is an unsigned int,
> and this is implicitly cast-down to a u8 in the calculation of addr,
> chopping off the bits above bit 7.
> 
> How about this approach:
> 
> static int phy_package_address(struct phy_device *phydev,
> 			       unsigned int addr_offset)
> {
> 	struct phy_package_shared *shared = phydev->shared;
> 	unsigned int addr = shared->addr + addr_offset;
> 
> 	/* detect wrap */
> 	if (addr < addr_offset)
> 		return -EIO;
> 
> 	/* detect invalid address */
> 	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 then these functions all become:
> 
> 	int addr = phy_package_address(phydev, addr_offset);
> 
> 	if (addr < 0)
> 		return addr;
> 
> I'll give you that this is belt and braces, but it avoids problems
> should a negative errno value be passed in as addr_offset (which will
> be cast to a very large positive integer.)

I also feel an helper is needed (since as you pointed out in the mmd
function we would have duplicated logic)

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.

After the check value is O.K. and can be trated as an int in
mdiobus_read (as we are sure it's in the limits and positive)

If this is correct, and the thing is a simple condition I think the
helper is not needed (or should we use it anyway for consistency in each
function?)

> 
> Andrew, any opinions on how far this should be taken?
> 

-- 
	Ansuel

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ