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

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.
--

commit 93cf16801f16f118e3a22ae17f7d80b0cc1d49bc
Author: Christian Marangi [off-list ref]
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.

[ ... ]

[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.

[ ... ]

+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