Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 30 Nov 2022 09:55:50 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Frank <Frank.Sae@motor-comm.com>
Cc:     Peter Geis <pgwipeout@gmail.com>, Andrew Lunn <andrew@lunn.ch>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, yinghong.zhang@motor-comm.com,
        fei.zhang@motor-comm.com, hua.sun@motor-comm.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next] net: phy: Add driver for Motorcomm yt8531
 gigabit ethernet phy
Message-ID: <Y4copjAzKpGSeunB@shell.armlinux.org.uk>
References: <20221130094928.14557-1-Frank.Sae@motor-comm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221130094928.14557-1-Frank.Sae@motor-comm.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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!