Re: [PATCH net-next v21 4/9] mfd: an8855: Add support for Airoha AN8855 Switch

[Lee Jones <lee@kernel.org>]

On Sat, 12 Sep 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] Integer truncation of u32 address difference to u8 allows bypassing bounds check, leading to out-of-bounds heap accesses.
> - [High] Unprotected concurrent access to `priv->phy_bus` causes unbalanced mutex unlocks, and Devres LIFO cleanup creates a Use-After-Free window.
> - [Low] Selecting MDIO_REGMAP without inheriting its dependency on PHYLIB causes Kconfig unmet direct dependencies warning.
> - [Low] Incorrect usage of FIELD_GET for PHY page comparison against a prepared constant.

Please review, explain or fix the Sashiko review(s).

If this is a set, this request includes the other patches too.

> --
> 
> commit 93cf16801f16f118e3a22ae17f7d80b0cc1d49bc
> Author: Christian Marangi <ansuelsmth@gmail.com>
> Subject: mfd: an8855: Add support for Airoha AN8855 Switch
> 
> This commit introduces core support for the Airoha AN8855 Switch.
> It provides a DSA switch and an NVMEM provider, and utilizes the
> mdio-regmap driver to handle the PHYs.
> 
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index e4fd4572472f9..614b39a538fc8 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> [ ... ]
> > @@ -54,6 +54,19 @@ config MFD_ALTERA_SYSMGR
> >  	  using regmap_mmio accesses for ARM32 parts and SMC calls to
> >  	  EL3 for ARM64 parts.
> >  
> > +config MFD_AIROHA_AN8855
> > +	tristate "Airoha AN8855 Switch Core"
> > +	select MFD_CORE
> > +	select REGMAP
> > +	select MDIO_REGMAP
> > +	depends on NETDEVICES && OF
> 
> [Severity: Low]
> Does this configuration correctly inherit dependencies? Selecting MDIO_REGMAP
> without also inheriting its dependency on PHYLIB can trigger a Kconfig warning
> for unmet direct dependencies, which might lead to build failures if the
> selected infrastructure isn't actually available.
> 
> > +	help
> > +	  Support for the Airoha AN8855 Switch Core. This is an SoC
> > +	  that provides various peripherals, to count, i2c, an Ethrnet
> > +	  Switch, a CPU timer, GPIO, eFUSE.
> > +
> > +	  Currently it provides a DSA switch and a NVMEM provider.
> > +
> >  config MFD_ACT8945A
> >  	tristate "Active-semi ACT8945A"
> >  	select MFD_CORE
> > diff --git a/drivers/mfd/airoha-an8855.c b/drivers/mfd/airoha-an8855.c
> > new file mode 100644
> > index 0000000000000..01f33293a9018
> > --- /dev/null
> > +++ b/drivers/mfd/airoha-an8855.c
> [ ... ]
> > +static void an8855_regmap_lock(struct an8855_core_priv *priv)
> > +{
> > +	if (priv->phy_bus)
> > +		mutex_lock(&priv->phy_bus->mdio_lock);
> > +	mutex_lock_nested(&priv->bus->mdio_lock, MDIO_MUTEX_NESTED);
> > +}
> > +
> > +static void an8855_regmap_unlock(struct an8855_core_priv *priv)
> > +{
> > +	mutex_unlock(&priv->bus->mdio_lock);
> > +	if (priv->phy_bus)
> > +		mutex_unlock(&priv->phy_bus->mdio_lock);
> > +}
> 
> [Severity: High]
> Could these unlocked reads of priv->phy_bus cause unbalanced locking or a
> use-after-free?
> 
> Since the regmap is created earlier in probe (via devm_regmap_init), it might
> be accessible concurrently before priv->phy_bus is initialized. If a
> concurrent access evaluates if (priv->phy_bus) as false here, it will skip
> locking. If the probe thread then assigns priv->phy_bus before
> an8855_regmap_unlock() executes, the unlock path could evaluate true and
> release a mutex it never acquired.
> 
> Additionally, if probe fails later at devm_mfd_add_devices(), devres unwinds
> in LIFO order. This would unregister and free phy_bus, but the regmap remains
> accessible until its own cleanup. During this window, an8855_regmap_lock()
> could attempt to lock a freed priv->phy_bus->mdio_lock.
> 
> [ ... ]
> > +static int an8855_regmap_phy_reset_page(struct an8855_core_priv *priv,
> > +					int phy) __must_hold(&priv->bus->mdio_lock)
> > +{
> > +	/* Check PHY page only for addr shared with switch */
> > +	if (phy != priv->switch_addr)
> > +		return 0;
> > +
> > +	/* Don't restore page if it's not set to Switch page */
> > +	if (priv->current_page != FIELD_GET(AN8855_PHY_PAGE,
> > +					    AN8855_PHY_PAGE_EXTENDED_4))
> > +		return 0;
> 
> [Severity: Low]
> Is this the intended usage of FIELD_GET()? The macro extracts an unshifted
> value, but it is being compared against AN8855_PHY_PAGE_EXTENDED_4, which is
> already shifted via FIELD_PREP_CONST. While this works out safely today
> because the mask GENMASK(2, 0) has a shift of zero, a future change to the
> mask could silently break this comparison logic.
> 
> [ ... ]
> > +static int an8855_phy_register(struct device *dev, struct an8855_core_priv *priv,
> > +			       struct device_node *phy_np)
> > +{
> > +	struct mdio_regmap_config mrc = { };
> > +	struct an8855_phy_priv *phy_priv;
> > +	struct regmap *regmap;
> > +	struct mii_bus *bus;
> > +	u8 phy_offset;
> > +	u32 addr;
> > +	int ret;
> > +
> > +	ret = of_property_read_u32(phy_np, "reg", &addr);
> > +	if (ret)
> > +		return ret;
> > +
> > +	phy_offset = addr - priv->switch_addr;
> > +	if (phy_offset >= AN8855_MAX_PHY_PORT)
> > +		return -EINVAL;
> 
> [Severity: High]
> Could this lead to an out-of-bounds heap access?
> 
> The variable phy_offset is declared as a u8, but it is calculated from a u32
> addr which comes from the device tree. If the device tree provides an
> arbitrarily large reg value (such as priv->switch_addr + 256), the math
> modulo 256 wraps the u8 value to 0.
> 
> This would bypass the bounds check against AN8855_MAX_PHY_PORT. The
> unchecked addr would then be passed into fwnode_mdiobus_register_phy(),
> eventually reaching mdiobus_register_device() which attempts to access
> mdiodev->bus->mdio_map[mdiodev->addr] using the unbounded value.
> 
> [ ... ]
> > +	bus = devm_mdio_regmap_register(dev, &mrc);
> > +	if (IS_ERR(bus))
> > +		return PTR_ERR(bus);
> > +
> > +	if (addr == priv->switch_addr)
> > +		priv->phy_bus = bus;
> 
> [Severity: High]
> This is where priv->phy_bus is initialized. Because the switch regmap is
> already fully registered and accessible prior to this, any concurrent access
> can race with this assignment, triggering the unbalanced lock or
> use-after-free issues mentioned earlier.
> 
> > +
> > +	return fwnode_mdiobus_register_phy(bus, of_fwnode_handle(phy_np), addr);
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260911225414.225980-1-ansuelsmth@gmail.com?part=4

-- 
Lee Jones