linux-kernel - Re: [PATCH net-next] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

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]

Date:   Wed, 30 Nov 2022 09:55:50 +0000
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Frank <Frank.Sae@...or-comm.com>
Cc:     Peter Geis <pgwipeout@...il.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>, yinghong.zhang@...or-comm.com,
        fei.zhang@...or-comm.com, hua.sun@...or-comm.com,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: Add driver for Motorcomm yt8531
 gigabit ethernet phy

On Wed, Nov 30, 2022 at 05:49:28PM +0800, Frank wrote:
> +/**
> + * yt8531_set_wol() - turn wake-on-lan on or off
> + * @phydev: a pointer to a &struct phy_device
> + * @wol: a pointer to a &struct ethtool_wolinfo
> + *
> + * NOTE: YTPHY_WOL_CONFIG_REG, YTPHY_WOL_MACADDR2_REG, YTPHY_WOL_MACADDR1_REG
> + * and YTPHY_WOL_MACADDR0_REG are common ext reg.
> + *
> + * returns 0 or negative errno code
> + */
> +static int yt8531_set_wol(struct phy_device *phydev,
> +			  struct ethtool_wolinfo *wol)
> +{

So this is called from the .set_wol method directly, and won't have the
MDIO bus lock taken...

> +	struct net_device *p_attached_dev;
> +	const u16 mac_addr_reg[] = {
> +		YTPHY_WOL_MACADDR2_REG,
> +		YTPHY_WOL_MACADDR1_REG,
> +		YTPHY_WOL_MACADDR0_REG,
> +	};
> +	const u8 *mac_addr;
> +	u16 mask;
> +	u16 val;
> +	int ret;
> +	u8 i;
> +
> +	if (wol->wolopts & WAKE_MAGIC) {
> +		p_attached_dev = phydev->attached_dev;
> +		if (!p_attached_dev)
> +			return -ENODEV;
> +
> +		mac_addr = (const u8 *)p_attached_dev->dev_addr;
> +		if (!is_valid_ether_addr(mac_addr))
> +			return -EINVAL;
> +
> +		/* Store the device address for the magic packet */
> +		for (i = 0; i < 3; i++) {
> +			ret = ytphy_write_ext(phydev, mac_addr_reg[i],
> +					      ((mac_addr[i * 2] << 8)) |
> +						      (mac_addr[i * 2 + 1]));

This accesses the MDIO bus without taking the lock.

> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		/* Enable WOL feature */
> +		mask = YTPHY_WCR_PULSE_WIDTH_MASK | YTPHY_WCR_INTR_SEL;
> +		val = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
> +		val |= YTPHY_WCR_TYPE_PULSE | YTPHY_WCR_PULSE_WIDTH_672MS;
> +		ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, val);

This accesses the MDIO bus without taking the lock.

> +		if (ret < 0)
> +			return ret;
> +
> +		/* Enable WOL interrupt */
> +		ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG, 0,
> +				   YTPHY_IER_WOL);

This accesses the MDIO bus without taking the lock.

> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		/* Disable WOL feature */
> +		mask = YTPHY_WCR_ENABLE | YTPHY_WCR_INTR_SEL;
> +		ret = ytphy_modify_ext(phydev, YTPHY_WOL_CONFIG_REG, mask, 0);

This accesses the MDIO bus without taking the lock.

> +
> +		/* Disable WOL interrupt */
> +		ret = __phy_modify(phydev, YTPHY_INTERRUPT_ENABLE_REG,
> +				   YTPHY_IER_WOL, 0);

This accesses the MDIO bus without taking the lock.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

Which makes this function entirely unsafe as another thread can change
the YTPHY_PAGE_SELECT register between writing that register and
accessing the YTPHY_PAGE_DATA register.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!