Re: [PATCH net-next v3 13/13] net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Lockless iteration over network device MAC address lists causes use-after-free.
- [High] Deadlock due to using a plain `spin_lock` for a lock shared with softirq context.
- [Medium] Secondary unicast addresses are discarded by the hardware when the interface is in all-multicast mode.
- [Medium] A transient SPI read failure overwrites the driver's statistics history with zeros.
- [Medium] Clearing MAC filter slots fails to deactivate them, programming a filter for `00:00:00:00:00:00`.
- [Medium] Standard software statistics like `tx_dropped` are dropped and not reported to userspace.
--

commit 0c3599b0c7883e06b169d454b3d9ab26602d78f2
Author: Ciprian Regus <ciprian.regus@analog.com>
Subject: net: ethernet: adi: Add a driver for the ADIN1140 MACPHY

Add a driver for ADIN1140, a 10BASE-T1S MAC-PHY that connects over SPI. It
implements the Open Alliance TC6 protocol for control and frame transfers. The
driver provides a custom mii_bus implementation to work around hardware issues
with C22 access.

> diff --git a/drivers/net/ethernet/adi/adin1140.c b/drivers/net/ethernet/adi/adin1140.c
> new file mode 100644
> index 0000000000000..358e9a11c9936
> --- /dev/null
> +++ b/drivers/net/ethernet/adi/adin1140.c
[ ... ]
> +static int adin1140_mac_filter_set(struct adin1140_priv *priv,
> +				   const u8 *addr, const u8 *mask,
> +				   u8 slot)
> +{
[ ... ]
> +	reg_address = ADIN1140_MAC_ADDR_FILT_UPR_REG + 2 * slot;
> +
> +	ret = oa_tc6_write_register_mms(priv->tc6, OA_TC6_VEND_MMS1,
> +					reg_address,
> +					get_unaligned_be16(&addr[0]) |
> +					ADIN1140_MAC_ADDR_FILT_APPLY2PORT1 |
> +					ADIN1140_MAC_ADDR_FILT_TO_HOST);

[Severity: Medium]
Will this unintentionally activate the filter slot when trying to clear it?

When adin1140_mac_filter_clear() calls this function with an all-zero MAC
address, it unconditionally bitwise-ORs the ADIN1140_MAC_ADDR_FILT_APPLY2PORT1
and ADIN1140_MAC_ADDR_FILT_TO_HOST bits into the register write. This fails
to disable the filter, and instead actively enables the slot to match frames
with the destination MAC address 00:00:00:00:00:00.

Could an attacker on the local network bypass the hardware filter by
sending frames to this address, forcing the MAC to forward them over the
SPI bus? A proper clear might need to write 0 to the register to completely
disable the slot.

[ ... ]
> +static void adin1140_rx_mode_work(struct work_struct *work)
> +{
[ ... ]
> +	mac_addrs = netdev_uc_count(priv->netdev) +
> +		    netdev_mc_count(priv->netdev);
> +
> +	if (priv->netdev->flags & IFF_PROMISC) {
> +		promisc = true;
> +		all_multi = false;
> +	} else if (priv->netdev->flags & IFF_ALLMULTI) {
> +		promisc = false;
> +		all_multi = true;

[Severity: Medium]
Does this logic prevent unicast filters from being programmed when
IFF_ALLMULTI is active?

If IFF_ALLMULTI is set, the logic enters this else-if block but skips the
subsequent block that programs secondary unicast MAC addresses into the
hardware filters. Because promiscuous mode is disabled in this state, will
the hardware filter drop any frames destined for secondary unicast addresses
(such as those used by macvlan interfaces on top of this device)?

> +	} else if (mac_addrs <= ADIN1140_MAC_FILT_AVAIL) {
> +		promisc = false;
> +		all_multi = false;
> +
> +		slot = ADIN1140_MAC_FILT_UC_SLOT + 1;
> +		memset(mask, 0xFF, ETH_ALEN);
> +
> +		netdev_for_each_uc_addr(ha, priv->netdev) {
> +			ret = adin1140_mac_filter_set(priv, ha->addr, mask,
> +						      slot);

[Severity: High]
Does iterating over the network device MAC address lists here without holding
the netif_addr_lock_bh() spinlock cause a use-after-free?

This function executes in a workqueue (process context). Because it does not
acquire the lock, concurrent modifications to these lists from the networking
stack (e.g., via dev_uc_add() or dev_uc_del()) can alter the list structure
or free list nodes during traversal, resulting in use-after-free crashes or
infinite loops.

Since the loop performs sleepable SPI writes, should the driver copy the
addresses into a local array while holding the lock, and then perform the
SPI writes outside the lock?

> +			if (ret)
> +				return;
> +
> +			slot++;
> +		}
> +
> +		netdev_for_each_mc_addr(ha, priv->netdev) {
> +			ret = adin1140_mac_filter_set(priv, ha->addr, mask,
> +						      slot);

[Severity: High]
Can the same use-after-free happen here with the multicast list?

Like the unicast list, traversing the multicast list locklessly in an
asynchronous workqueue leaves the traversal vulnerable to concurrent list
modifications.

[ ... ]
> +static void adin1140_stats_work(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	u64 stat_buff[ADIN1140_STATS_CNT] = {};
> +	struct adin1140_priv *priv;
> +	u32 reg_val;
> +	int ret;
> +	u32 i;
> +
> +	priv = container_of(dwork, struct adin1140_priv, stats_work);
> +
> +	for (i = 0; i < ARRAY_SIZE(adin1140_stat_regs); i++) {
> +		ret = oa_tc6_read_register_mms(priv->tc6, OA_TC6_VEND_MMS1,
> +					       adin1140_stat_regs[i],
> +					       &reg_val);
> +		if (ret)
> +			break;
> +
> +		stat_buff[i] = reg_val;
> +	}
> +
> +	scoped_guard(spinlock, &priv->stat_lock)
> +		memcpy(&priv->stats, stat_buff, sizeof(priv->stats));

[Severity: Medium]
Will a transient SPI read failure overwrite the driver's statistics history
with zeros?

The stat_buff array is initialized to zero. If oa_tc6_read_register_mms()
fails due to a transient SPI error, the loop breaks early, leaving the
remaining elements in stat_buff as zero. The function then unconditionally
copies this partially zeroed buffer over priv->stats. This wipes out all
previously accumulated values for the unread statistics.

Should the driver skip the stats update entirely if an error occurs, so
statistics don't incorrectly report as zero to userspace until the next
successful poll?

[Severity: High]
Does acquiring this plain spinlock in process context create a deadlock risk
with softirq contexts?

The adin1140_stats_work() function runs in process context, while
adin1140_ndo_get_stats64() can be called from softirq context (e.g., during
packet transmission or by qdiscs) via dev_get_stats().

If adin1140_stats_work() acquires the lock in process context and a softirq
fires on the same CPU attempting to read statistics, the softirq will wait
on the same lock, deadlocking the CPU. Does any process context code
acquiring stat_lock need to disable softirqs (e.g., using spinlock_bh)?

[ ... ]
> +static void adin1140_ndo_get_stats64(struct net_device *dev,
> +				     struct rtnl_link_stats64 *storage)
> +{
> +	struct adin1140_priv *priv = netdev_priv(dev);
> +
> +	storage->rx_packets = priv->netdev->stats.rx_packets;
> +	storage->tx_packets = priv->netdev->stats.tx_packets;
> +
> +	storage->rx_bytes = priv->netdev->stats.rx_bytes;
> +	storage->tx_bytes = priv->netdev->stats.tx_bytes;

[Severity: Medium]
Are standard software statistics like tx_dropped discarded here and not
reported to userspace?

The driver manually copies a few fields from priv->netdev->stats to the
storage parameter, but fails to include others like tx_dropped. The oa_tc6
framework specifically increments tc6->netdev->stats.tx_dropped on
transmission errors.

Should this use netdev_stats_to_stats64(storage, &dev->stats) to copy all
base software statistics before adding the driver-specific hardware counters?

> +
> +	scoped_guard(spinlock, &priv->stat_lock)
> +		__adin1140_ndo_get_stats64(priv, storage);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-adin1140-driver-v3-0-5debdb3173c4@analog.com?part=13