Re: [PATCH net-next v10 06/11] net: ethernet: oa_tc6: Support for vendor specific MMS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: oa_tc6: oa_tc6_set_vend1_mms() called after PHY init in
  oa_tc6_init()
- [Medium] net: oa_tc6: NULL deref in oa_tc6_phy_exit() on capability
  read failure
--

commit e3d2308dcadfed4c734bdcf29ba226ef0116d5e7
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Support for vendor specific MMS

This commit adds oa_tc6_set_vend1_mms() to allow MAC-PHY drivers to map
MDIO_MMD_VEND1 Clause 45 accesses to a vendor-specific Memory Map Select
(MMS) value.

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_main.c b/drivers/net/ethernet/oa_tc6/oa_tc6_main.c
> index e793d7fb0635f..5b2454b759740 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6_main.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_main.c
> @@ -266,6 +266,18 @@ static int oa_tc6_update_standard_capability(struct oa_tc6 *tc6)
>  	return 0;
>  }
>  
> +/**
> + * oa_tc6_set_vend1_mms - Add vendor specific MDIO_MMD to OA TC6 MMS
> + * mapper value.
> + * @tc6: oa_tc6 struct.
> + * @mms: vendor defined MMS value for VEND1 mdio device.
> + */
> +void oa_tc6_set_vend1_mms(struct oa_tc6 *tc6, int mms)
> +{
> +	tc6->vend1_mms = mms;
> +}
> +EXPORT_SYMBOL_GPL(oa_tc6_set_vend1_mms);

[Severity: Medium]
Can oa_tc6_set_vend1_mms() be called before the internal PHY is probed
and initialized?

struct oa_tc6 is allocated inside oa_tc6_init(), which initializes
tc6->vend1_mms to -EOPNOTSUPP and then calls oa_tc6_phy_init(tc6) before
returning the tc6 pointer to the caller:

drivers/net/ethernet/oa_tc6/oa_tc6_main.c:oa_tc6_phy_init() {
	...
	ret = oa_tc6_mdiobus_register(tc6);
	if (ret)
		return ret;

	tc6->phydev = phy_find_first(tc6->mdiobus);
	...
	ret = phy_connect_direct(tc6->netdev, tc6->phydev,
				 &oa_tc6_handle_link_change,
				 PHY_INTERFACE_MODE_INTERNAL);
}

Because oa_tc6_mdiobus_register() and phy_connect_direct() synchronously
run the PHY driver's .probe(), .get_features(), .soft_reset(), and
.config_init() callbacks inside oa_tc6_init(), any Clause 45 access to
MDIO_MMD_VEND1 during PHY initialization sees tc6->vend1_mms ==
-EOPNOTSUPP and fails.  Calling oa_tc6_set_vend1_mms() after
oa_tc6_init() returns also modifies tc6->vend1_mms without
synchronization while the MDIO bus is already registered and live.

Passing vend1_mms into oa_tc6_init() (for example via struct
oa_tc6_quirks or an init config struct) would set it before
oa_tc6_phy_init() runs.

[ ... ]

> @@ -1658,6 +1672,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
>  	SET_NETDEV_DEV(netdev, &spi->dev);
>  	mutex_init(&tc6->spi_ctrl_lock);
>  	spin_lock_init(&tc6->tx_skb_lock);
> +	tc6->vend1_mms = -EOPNOTSUPP;
>  	tc6->ttsc_current_id = OA_TC6_TTSCA_REG_ID;
>  	INIT_LIST_HEAD(&tc6->tx_ts_skb_q);

[Severity: Medium]
In oa_tc6_init(), does a failure in oa_tc6_update_standard_capability()
cause a NULL pointer dereference in oa_tc6_phy_exit()?

Commit "net: ethernet: oa_tc6: Support for hardware timestamp" earlier in
this series added oa_tc6_update_standard_capability() before
oa_tc6_phy_init(), jumping to phy_exit on failure:

drivers/net/ethernet/oa_tc6/oa_tc6_main.c:oa_tc6_init() {
	...
	ret = oa_tc6_update_standard_capability(tc6);
	if (ret) {
		dev_err(&tc6->spi->dev, "Failed to read capability\n");
		goto phy_exit;
	}
	...
	ret = oa_tc6_phy_init(tc6);
	...
phy_exit:
	oa_tc6_phy_exit(tc6);
	return ERR_PTR(ret);
}

Because oa_tc6_phy_init() has not run yet at that point, tc6->phydev and
tc6->mdiobus are still NULL, so oa_tc6_phy_exit() passes NULL to
phy_disconnect() and mdiobus_unregister() whenever OA_TC6_BROKEN_PHY is
not set.  Returning ERR_PTR(ret) directly, as the adjacent error paths
before oa_tc6_phy_init() do, avoids calling oa_tc6_phy_exit() before PHY
initialization.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7@onsemi.com?part=6