Re: [PATCH net-next v19 09/10] net: dsa: mt7530: generalize and move common function to lib module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Negative array indexing (`fields[-1]`) in `mt7530_get_ethtool_stats` causes an out-of-bounds read and kernel panic.
- [High] Bridge port MAC learning logic evaluates to zero, permanently enabling learning.
- [High] FDB write operations store corrupted all-zero MAC addresses due to incorrect right-shifting and swapped bit ranges.
- [High] Semantic mismatch in the port matrix (`pm`) cache breaks port enablement and corrupts port control registers.
- [High] MIB counter `REG_FIELD_ID` definitions specify an invalid MSB of 32, triggering undefined behavior in GENMASK on 32-bit platforms.
- [High] A copy-paste error omits the hardware trap configuration for the `:03` MAC DA, bypassing IEEE 802.1Q filtering rules.
- [High] Replacing atomic register writes with field-specific RMWs preserves dirty hardware register states during VLAN 0 setup.
- [High] Unchecked regmap field reads leak uninitialized kernel stack memory to userspace and break hardware isolation policies.
- [Medium] TX and RX RMON packet size histograms are incorrectly swapped, reporting TX stats as RX and vice versa.
- [Medium] Multicast packet statistics are incorrectly discarded and never reported in `mt7530_read_port_stats64`.
- [Medium] MIB counters are not flushed during initialization because the driver writes `0` instead of `1` to the respective flush bits.
--

--- Patch [9]: [PATCH net-next v19 09/10] net: dsa: mt7530: generalize and move common function to lib module ---
commit 0ff2707d21a36354cbb42ab9fd85231c636ec13a
Author: Christian Marangi <ansuelsmth@gmail.com>

net: dsa: mt7530: generalize and move common function to lib module

In preparation of support of DSA Switch from Airoha, create a mt7530
library module and move there all the common function.

> diff --git a/drivers/net/dsa/mt7530-lib.c b/drivers/net/dsa/mt7530-lib.c
> new file mode 100644
> index 0000000000000..0ad5f11283e79
> --- /dev/null
> +++ b/drivers/net/dsa/mt7530-lib.c
> @@ -0,0 +1,1094 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include <linux/device.h>
> +#include <linux/if_bridge.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <net/dsa.h>
> +
> +#include "mt7530-lib.h"
> +
> +void mt7530_lib_mib_reset(struct mt7530_lib_priv *priv)
> +{
> +	regmap_field_write(priv->fields[MT7530_CCR_MIB_ENABLE], 0);
> +	regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_GOOD], 0);
> +	regmap_field_write(priv->fields[MT7530_CCR_RX_OCT_CNT_BAD], 0);
> +	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_GOOD], 0);
> +	regmap_field_write(priv->fields[MT7530_CCR_TX_OCT_CNT_BAD], 0);

[Severity: Medium]
Does writing 0 here correctly trigger the flush?

The hardware typically requires these specific bits to be asserted (1) to
clear the counters, similar to the original CCR_MIB_FLUSH bitmask.

Writing 0 might leave the flush operation disabled, causing the hardware to
report leftover garbage across reboots.

[ ... ]

> +void mt7530_lib_trap_frames(struct mt7530_lib_priv *priv)
> +{
> +	/* Trap 802.1X PAE frames and BPDUs to the CPU port(s) and egress
> +	 * them with the EG_TAG attribute set to disabled (system default)
> +	 * so that any VLAN tags in the frame are not modified by the
> +	 * switch egress VLAN tag processing. This preserves VLAN tags
> +	 * for reception on VLAN sub-interfaces.
> +	 */
> +	regmap_field_write(priv->fields[MT7530_BPDU_EG_TAG],
> +			   MT7530_VLAN_EG_DISABLED);

[ ... ]

> +	/* Trap frames with :03 and :0E MAC DAs to the CPU port(s) and
> +	 * egress them with EG_TAG disabled.
> +	 */
> +	regmap_field_write(priv->fields[MT7530_R02_BPDU_FR], 1);
> +	regmap_field_write(priv->fields[MT7530_R02_EG_TAG],
> +			   MT7530_VLAN_EG_DISABLED);
> +	regmap_field_write(priv->fields[MT7530_R02_PORT_FW],
> +			   TO_CPU_FW_CPU_ONLY);

[Severity: High]
Is there a copy-paste error here?

The comment mentions trapping :03 and :0E MAC DAs, but the code modifies
MT7530_R02 instead of MT7530_R03.

This leaves the :03 frame trap unconfigured, potentially bypassing IEEE
802.1Q filtering rules.

> +	regmap_field_write(priv->fields[MT7530_R0E_BPDU_FR], 1);
> +	regmap_field_write(priv->fields[MT7530_R0E_EG_TAG],
> +			   MT7530_VLAN_EG_DISABLED);
> +	regmap_field_write(priv->fields[MT7530_R0E_PORT_FW],
> +			   TO_CPU_FW_CPU_ONLY);
> +}
> +EXPORT_SYMBOL_GPL(mt7530_lib_trap_frames);
> +
> +static void mt7530_update_port_member(struct mt7530_lib_priv *priv, int port,
> +				      const struct net_device *bridge_dev,
> +				      bool join) __must_hold(priv->reg_mutex)
> +{
> +	struct dsa_port *dp = dsa_to_port(priv->ds, port), *other_dp;
> +	struct mt7530_port *p = &priv->ports[port], *other_p;
> +	struct dsa_port *cpu_dp = dp->cpu_dp;
> +	u32 port_bitmap = BIT(cpu_dp->index);
> +	int other_port;
> +	bool isolated;
> +
> +	dsa_switch_for_each_user_port(other_dp, priv->ds) {
> +		other_port = other_dp->index;
> +		other_p = &priv->ports[other_port];
> +
> +		if (dp == other_dp)
> +			continue;
> +
> +		/* Add/remove this port to/from the port matrix of the other
> +		 * ports in the same bridge. If the port is disabled, port
> +		 * matrix is kept and not being setup until the port becomes
> +		 * enabled.
> +		 */
> +		if (!dsa_port_offloads_bridge_dev(other_dp, bridge_dev))
> +			continue;
> +
> +		isolated = p->isolated && other_p->isolated;
> +
> +		if (join && !isolated) {
> +			other_p->pm |= BIT(port);
> +			port_bitmap |= BIT(other_port);
> +		} else {
> +			other_p->pm &= ~BIT(port);
> +		}
> +
> +		if (other_p->enable)
> +			regmap_fields_write(priv->fields[MT7530_PCR_MATRIX],
> +					    other_port, other_p->pm);

[Severity: High]
Could this corrupt the port control registers?

The legacy mt7530_port_enable() function (which is still present) stores
pre-shifted values in pm (e.g., PCR_MATRIX(BIT(cpu_dp->index))).

Here, pm is treated as an unshifted bitmask and passed to
regmap_fields_write(), which shifts the entire value left by 16 bits.

This will shift the pre-shifted CPU port bit completely out of bounds
(overflowing to 0) and isolate the port from the CPU.

> +	}
> +
> +	/* Add/remove the all other ports to this port matrix. For !join
> +	 * (leaving the bridge), only the CPU port will remain in the port matrix
> +	 * of this port.
> +	 */
> +	p->pm = port_bitmap;
> +	if (priv->ports[port].enable)
> +		regmap_fields_write(priv->fields[MT7530_PCR_MATRIX],
> +				    port, port_bitmap);
> +}
> +
> +void mt7530_lib_read_port_stats(struct regmap_field *low,
> +				struct regmap_field *high,
> +				int port, uint64_t *data)
> +{
> +	u32 val;
> +
> +	regmap_fields_read(low, port, &val);
> +	*data = val;

[Severity: High]
What happens if the underlying MDIO bus read fails here?

The return value of regmap_fields_read() is not checked, meaning val could
remain uninitialized.

This would leak kernel stack memory to userspace via ethtool. Similarly, in
mt7530_hw_vlan_update() below, it uses uninitialized stack data to build port
masks, randomly altering VLAN memberships.

> +
> +	if (high) {
> +		regmap_fields_read(high, port, &val);
> +		*data |= (u64)val << 32;
> +	}
> +}
> +EXPORT_SYMBOL_GPL(mt7530_lib_read_port_stats);

[ ... ]

> +static void mt7530_lib_fdb_write(struct mt7530_lib_priv *priv, u16 vid,
> +				 u8 port_mask, const u8 *mac,
> +				 u8 aging, u8 type)
> +{
> +	regmap_field_write(priv->fields[MT7530_ATWD_CVID], vid);
> +	regmap_field_write(priv->fields[MT7530_ATWD_IVL], 1);
> +	regmap_field_write(priv->fields[MT7530_ATWD_FID], FID_BRIDGED);
> +	regmap_field_write(priv->fields[MT7530_ATWD_AGE_TIMER], aging);
> +	regmap_field_write(priv->fields[MT7530_ATWD_PORT_MAP], port_mask);
> +	/* STATIC_ENT indicate that entry is static wouldn't
> +	 * be aged out and STATIC_EMP specified as erasing an
> +	 * entry
> +	 */
> +	regmap_field_write(priv->fields[MT7530_ATWD_ENT_STATUS], type);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_5],
> +			   mac[5] >> MAC_BYTE_5);

[Severity: High]
Will this result in all-zero MAC addresses being written to the FDB?

The mac array contains 8-bit u8 values. Right-shifting a u8 by MAC_BYTE_5
(which is 16) or MAC_BYTE_4 (which is 24) will always evaluate to 0.

Since regmap_field_write() automatically handles shifting values into their
proper field positions, it seems we should pass the raw mac[x] bytes without
shifting.

> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_4],
> +			   mac[4] >> MAC_BYTE_4);
> +	regmap_field_write(priv->fields[MT7530_ATWD_MAC_BYTE_3],
> +			   mac[3] >> MAC_BYTE_3);

[ ... ]

> +int mt7530_lib_port_bridge_flags(struct mt7530_lib_priv *priv, int port,
> +				 struct switchdev_brport_flags flags,
> +				 struct netlink_ext_ack *extack)
> +{
> +	struct dsa_switch *ds = priv->ds;
> +
> +	if (flags.mask & BR_LEARNING)
> +		regmap_fields_write(priv->fields[MT7530_PSC_SA_DIR],
> +				    port, flags.val & BR_LEARNING);

[Severity: High]
Does this write permanently enable MAC learning?

BR_LEARNING is BIT(5) (32). The MT7530_PSC_SA_DIR field is a 1-bit field at
bit 4.

When regmap_fields_write() shifts the value by 4 and masks it with 0x10,
(32 << 4) & 0x10 evaluates to 0.

Additionally, the SA_DIR bit disables learning when set to 1, so writing 0
ensures learning can never be disabled.

> +
> +	if (flags.mask & BR_FLOOD)
> +		regmap_field_update_bits(priv->fields[MT7530_UNU_FFP],
> +					 BIT(port),
> +					 flags.val & BR_FLOOD ? BIT(port) : 0);

[ ... ]

> +static void mt7530_hw_vlan_update(struct mt7530_lib_priv *priv, u16 vid,
> +				  struct mt7530_hw_vlan_entry *entry,
> +				  mt7530_vlan_op vlan_op)
> +{
> +	u32 val;
> +
> +	/* Fetch entry */
> +	mt7530_vlan_cmd(priv, MT7530_VTCR_RD_VID, vid);
> +
> +	regmap_field_read(priv->fields[MT7530_VAWD_PORT_MEM], &val);
> +
> +	entry->old_members = val;
> +
> +	/* Manipulate entry */
> +	vlan_op(priv, entry);
> +
> +	/* Flush result to hardware */
> +	mt7530_vlan_cmd(priv, MT7530_VTCR_WR_VID, vid);
> +}

[ ... ]

> +int mt7530_lib_setup_vlan0(struct mt7530_lib_priv *priv)
> +{
> +	regmap_field_write(priv->fields[MT7530_VAWD_IVL_MAC], 1);
> +	regmap_field_write(priv->fields[MT7530_VAWD_EG_CON], 1);

[Severity: High]
Could this leave MT7530_VAWD1 in an inconsistent state?

The original code wrote directly to the MT7530_VAWD1 register, effectively
clearing unspecified bits like VTAG_EN.

Because VAWD1 is a data window that retains the state of the last accessed
VLAN entry, selectively modifying individual fields with read-modify-write
might leave VTAG_EN in an undefined state from a previous operation,
randomly tagging VLAN 0 frames.

> +	regmap_field_write(priv->fields[MT7530_VAWD_PORT_MEM],
> +			   MT7530_ALL_MEMBERS);
> +	regmap_field_write(priv->fields[MT7530_VAWD_FID],
> +			   FID_BRIDGED);

[ ... ]

> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 5f56a423b1475..b2d3c71e24e97 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -24,10 +24,159 @@
>  #include <net/pkt_cls.h>
>  
>  #include "mt7530.h"
> +#include "mt7530-lib.h"
>  
>  #define MT7530_STATS_POLL_INTERVAL	(1 * HZ)
>  #define MT7530_STATS_RATE_LIMIT		(HZ / 10)
>  
> +static const struct mt7530_reg_field mt753x_fields[] = {
> +	{ MT7530_BC_FFP, REG_FIELD(MT753X_MFC, 24, 31) },
> +	{ MT7530_UNM_FFP, REG_FIELD(MT753X_MFC, 16, 23) },

[ ... ]

> +	{ MT7530_ATWD_ENT_STATUS, REG_FIELD(MT7530_ATWD, 2, 3), },
> +	{ MT7530_ATWD_MAC_BYTE_5, REG_FIELD(MT7530_ATA2, 24, 31), },
> +	{ MT7530_ATWD_MAC_BYTE_4, REG_FIELD(MT7530_ATA2, 16, 23), },

[Severity: High]
Are the bit ranges for MAC_BYTE_4 and MAC_BYTE_5 swapped here?

The original definitions and typical byte ordering suggest MAC_BYTE_5 should
be bits 16-23 and MAC_BYTE_4 should be bits 24-31.

> +	{ MT7530_ATWD_MAC_BYTE_3, REG_FIELD(MT7530_ATA1, 0, 7), },
> +	{ MT7530_ATWD_MAC_BYTE_2, REG_FIELD(MT7530_ATA1, 8, 15), },
> +	{ MT7530_ATWD_MAC_BYTE_1, REG_FIELD(MT7530_ATA1, 16, 23), },

[ ... ]

> +	{ MT7530_GMACCR_MAX_RX_JUMBO, REG_FIELD(MT7530_GMACCR, 2, 5) },
> +	{ MT7530_GMACCR_MAX_RX_PKT_LEN, REG_FIELD(MT7530_GMACCR, 0, 1) },
> +
> +	{ MT7530_MIB_TX_DROP, REG_FIELD_ID(MT7530_PORT_MIB_TX_DROP, 0, 32, MT7530_NUM_PORTS, 0x100) },
> +	{ MT7530_MIB_TX_CRC_ERR, REG_FIELD_ID(MT7530_PORT_MIB_TX_CRC_ERR, 0, 32, MT7530_NUM_PORTS, 0x100) },

[Severity: High]
Does specifying 32 as the MSB here cause undefined behavior on 32-bit
platforms?

The REG_FIELD_ID macro takes inclusive bounds, so an MSB of 32 results in
GENMASK(32, 0).

On 32-bit architectures, this evaluates to a negative shift count
(~0UL << -1). Should the bound be 0, 31 instead?

> +	{ MT7530_MIB_TX_UNICAST, REG_FIELD_ID(MT7530_PORT_MIB_TX_UNICAST, 0, 32, MT7530_NUM_PORTS, 0x100) },
> +	{ MT7530_MIB_TX_MULTICAST, REG_FIELD_ID(MT7530_PORT_MIB_TX_MULTICAST, 0, 32, MT7530_NUM_PORTS, 0x100) },
> +	{ MT7530_MIB_TX_BROADCAST, REG_FIELD_ID(MT7530_PORT_MIB_TX_BROADCAST, 0, 32, MT7530_NUM_PORTS, 0x100) },

[ ... ]

>  static struct mt753x_pcs *pcs_to_mt753x_pcs(struct phylink_pcs *pcs)
>  {
>  	return container_of(pcs, struct mt753x_pcs, pcs);
> @@ -35,15 +184,15 @@ static struct mt753x_pcs *pcs_to_mt753x_pcs(struct phylink_pcs *pcs)
>  
>  /* String, offset, and register size in bytes if different from 4 bytes */
>  static const struct mt7530_mib_desc mt7530_mib[] = {
> -	MIB_DESC(1, MT7530_PORT_MIB_TX_DROP, "TxDrop"),
> -	MIB_DESC(1, MT7530_PORT_MIB_TX_CRC_ERR, "TxCrcErr"),
> -	MIB_DESC(1, MT7530_PORT_MIB_TX_COLLISION, "TxCollision"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_DROP, "RxDrop"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_FILTERING, "RxFiltering"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_CRC_ERR, "RxCrcErr"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_CTRL_DROP, "RxCtrlDrop"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_INGRESS_DROP, "RxIngressDrop"),
> -	MIB_DESC(1, MT7530_PORT_MIB_RX_ARL_DROP, "RxArlDrop"),
> +	MIB_DESC(MT7530_MIB_TX_DROP, -1, "TxDrop"),
> +	MIB_DESC(MT7530_MIB_TX_CRC_ERR, -1, "TxCrcErr"),

[ ... ]

> @@ -744,34 +796,22 @@ mt7530_get_strings(struct dsa_switch *ds, int port, u32 stringset,
>  		ethtool_puts(&data, mt7530_mib[i].name);
>  }
>  
> -static void
> -mt7530_read_port_stats(struct mt7530_priv *priv, int port,
> -		       u32 offset, u8 size, uint64_t *data)
> -{
> -	u32 val, reg = MT7530_PORT_MIB_COUNTER(port) + offset;
> -
> -	val = mt7530_read(priv, reg);
> -	*data = val;
> -
> -	if (size == 2) {
> -		val = mt7530_read(priv, reg + 4);
> -		*data |= (u64)val << 32;
> -	}
> -}
> -
>  static void
>  mt7530_get_ethtool_stats(struct dsa_switch *ds, int port,
>  			 uint64_t *data)
>  {
>  	struct mt7530_priv *priv = ds->priv;
>  	const struct mt7530_mib_desc *mib;
> +	struct mt7530_lib_priv *lib_priv;
>  	int i;
>  
> +	lib_priv = &priv->lib_priv;
>  	for (i = 0; i < ARRAY_SIZE(mt7530_mib); i++) {
>  		mib = &mt7530_mib[i];
>  
> -		mt7530_read_port_stats(priv, port, mib->offset, mib->size,
> -				       data + i);
> +		mt7530_lib_read_port_stats(lib_priv->fields[mib->field_low],
> +					   lib_priv->fields[mib->field_high],
> +					   port, data + i);

[Severity: Critical]
Will this cause an out-of-bounds read and kernel panic?

The MIB_DESC macro initializes field_high to -1 for 32-bit single-register
counters.

In mt7530_get_ethtool_stats(), this unconditionally accesses
lib_priv->fields[-1], which evaluates to the non-NULL struct regmap pointer.

Passing this invalid regmap_field pointer to mt7530_lib_read_port_stats()
will crash when regmap_fields_read() attempts to dereference it.

>  	}
>  }
>  

[ ... ]

> +static const struct mt7530_rmon_hist mt7530_rmon_hist_info = {
> +	.ranges_fields = (unsigned int []){
> +		MT7530_MIB_TX_PKT_SZ_64,
> +		MT7530_MIB_TX_PKT_SZ_65_TO_127,
> +		MT7530_MIB_TX_PKT_SZ_128_TO_255,
> +		MT7530_MIB_TX_PKT_SZ_256_TO_511,
> +		MT7530_MIB_TX_PKT_SZ_512_TO_1023,
> +		MT7530_MIB_TX_PKT_SZ_1024_TO_MAX,
> +	},
> +	.ranges_tx_fields = (unsigned int []){
> +		MT7530_MIB_RX_PKT_SZ_64,
> +		MT7530_MIB_RX_PKT_SZ_65_TO_127,
> +		MT7530_MIB_RX_PKT_SZ_128_TO_255,
> +		MT7530_MIB_RX_PKT_SZ_256_TO_511,
> +		MT7530_MIB_RX_PKT_SZ_512_TO_1023,
> +		MT7530_MIB_RX_PKT_SZ_1024_TO_MAX,
> +	},

[Severity: Medium]
Could these arrays be swapped?

The ranges_fields array is populated with TX macros, while ranges_tx_fields
uses RX macros.

When these are mapped later in mt7530_lib_get_rmon_stats(), it appears to
report TX stats as RX and vice versa.

> +	.ranges = mt7530_rmon_ranges,
> +	.num_ranges = ARRAY_SIZE(mt7530_rmon_ranges) - 1,
> +};
> +
>  static void mt7530_get_rmon_stats(struct dsa_switch *ds, int port,
>  				  struct ethtool_rmon_stats *rmon_stats,
>  				  const struct ethtool_rmon_hist_range **ranges)

[ ... ]

>  static void mt7530_read_port_stats64(struct mt7530_priv *priv, int port,
>  				     struct rtnl_link_stats64 *storage)
>  {
> +	struct mt7530_lib_priv *lib_priv = &priv->lib_priv;
>  	uint64_t data;
>  
>  	/* MIB counter doesn't provide a FramesTransmittedOK but instead
> @@ -903,38 +884,40 @@ static void mt7530_read_port_stats64(struct mt7530_priv *priv, int port,
>  	 * To simulate a global frame counter, read Unicast and addition Multicast
>  	 * and Broadcast later
>  	 */
> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_UNICAST, 1,
> -			       &storage->rx_packets);
> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_MULTICAST, 1,
> -			       &storage->multicast);
> +	mt7530_lib_read_port_stats(lib_priv->fields[MT7530_MIB_RX_UNICAST],
> +				   NULL, port, &storage->rx_packets);
> +	mt7530_lib_read_port_stats(lib_priv->fields[MT7530_MIB_RX_MULTICAST],
> +				   NULL, port, &data);
>  	storage->rx_packets += storage->multicast;

[Severity: Medium]
Does this correctly track multicast packets?

The counter is read into the local variable data, but then the code adds
storage->multicast to rx_packets.

Since storage->multicast is never assigned the read value, it seems the
multicast packet count is lost.

> -	mt7530_read_port_stats(priv, port, MT7530_PORT_MIB_RX_BROADCAST, 1,
> -			       &data);
> +	mt7530_lib_read_port_stats(lib_priv->fields[MT7530_MIB_RX_BROADCAST],
> +				   NULL, port, &data);
>  	storage->rx_packets += data;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728154846.1828945-1-ansuelsmth@gmail.com?part=9