Re: [PATCH net-next 3/4] net: dsa: hms: Add NXP i.MX RT1180 NETC switch driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [High] Dereferencing `cpu_dp->user` on a CPU port causes a guaranteed NULL pointer dereference crash during driver probe.
- [High] SPI transfer buffers are allocated on the stack, which violates DMA mapping rules and risks memory corruption.
- [High] Uninitialized kernel stack memory is leaked to the switch firmware over the SPI bus.
- [High] Missing array bounds check for port index derived from Device Tree allows out-of-bounds writes.
- [High] Hardware duplex configuration is never updated from the phylink negotiated state.
- [High] TPID VLAN filtering configuration is updated in software but never applied to the hardware.
- [Medium] Deleted PVIDs are immediately re-applied to the hardware.
- [Medium] FDB dump prematurely aborts, hiding the last entry in the MAC table.
- [Medium] FDB entries are reported to the bridge with inverted static/dynamic status.
- [Medium] A pacing delay intended for firmware command ratelimiting is located outside the mutex, failing to serialize concurrency.
- [Medium] SPI protocol structures use native host byte order instead of a fixed endianness.
- [Medium] The error return value of `hms_dsa_init()` is ignored, leading to an incomplete switch configuration.
- [Low] Kconfig selects a non-existent symbol NET_DSA_TAG_8021Q.
- [Low] Left-shifting into the sign bit triggers Undefined Behavior.
--

commit bc52bd2232e9fe56601b95379d2d4e43ce454f07
Author: Minghuan Lian <minghuan.lian@nxp.com>
Subject: net: dsa: hms: Add NXP i.MX RT1180 NETC switch driver

This commit adds a DSA driver for the NXP i.MX RT1180 NETC Ethernet switch.
It registers the switch with DSA, configures ports, and provides basic
operations like phylink, MTU, and tag_8021q setup. The driver communicates
with the switch firmware using the HMS command interface over SPI.

> diff --git a/drivers/net/dsa/hms/Kconfig b/drivers/net/dsa/hms/Kconfig
> new file mode 100644
> index 0000000000000..4f209b4c64131
> --- /dev/null
> +++ b/drivers/net/dsa/hms/Kconfig
> @@ -0,0 +1,14 @@
[ ... ]
> +config NET_DSA_HMS_SWITCH
[ ... ]
> +	select NET_DSA_TAG_HMS
> +	select NET_DSA_TAG_8021Q

Does NET_DSA_TAG_8021Q exist in the kernel tree? The tag_8021q functionality
seems to be built automatically into dsa_core, so selecting a non-existent
symbol could cause Kconfig unmet dependency warnings.

> diff --git a/drivers/net/dsa/hms/hms_config.c b/drivers/net/dsa/hms/hms_config.c
> new file mode 100644
> index 0000000000000..daddb4db3d294
> --- /dev/null
> +++ b/drivers/net/dsa/hms/hms_config.c
> @@ -0,0 +1,253 @@
[ ... ]
> +int hms_port_mtu_set(struct hms_private *priv, int port, int mtu)
> +{
> +	struct hms_cmd_port_mtu cmd;
> +
> +	cmd.port = port;
> +	cmd.mtu = mtu;
> +
> +	return hms_xfer_set_cmd(priv, HMS_CMD_PORT_MTU_SET,
> +				&cmd, sizeof(cmd));
> +}

Does this code leak uninitialized kernel stack memory to the firmware over SPI?
The cmd structure is allocated on the stack, and some fields are set, but
padding and reserved arrays are not initialized before passing the entire
structure to hms_xfer_set_cmd.

> diff --git a/drivers/net/dsa/hms/hms_config.h b/drivers/net/dsa/hms/hms_config.h
> new file mode 100644
> index 0000000000000..a54adefe794d6
> --- /dev/null
> +++ b/drivers/net/dsa/hms/hms_config.h
> @@ -0,0 +1,259 @@
[ ... ]
> +struct hms_cmd_port_mtu {
> +	u8 port;
> +	u8 reserved;
> +	u16 mtu;
> +};

Should these hardware command structures use fixed endianness types like
__be16 or __le16 instead of native u16? Since these structures are sent
directly over SPI, a big-endian host might swap the bytes and send a
corrupted configuration to the firmware.

> diff --git a/drivers/net/dsa/hms/hms_main.c b/drivers/net/dsa/hms/hms_main.c
> new file mode 100644
> index 0000000000000..b7f7c3a7d3667
> --- /dev/null
> +++ b/drivers/net/dsa/hms/hms_main.c
> @@ -0,0 +1,934 @@
[ ... ]
> +static int hms_fdb_add(struct dsa_switch *ds, int port,
> +		       const unsigned char *addr, u16 vid,
> +		       struct dsa_db db)
> +{
[ ... ]
> +	/* Allow enough time between consecutive calls for adding FDB entry */
> +	usleep_range(HMS_SPI_MSG_RESPONSE_TIME,
> +		     HMS_SPI_MSG_RESPONSE_TIME * 10);
> +
> +	mutex_lock(&priv->fdb_lock);
> +	rc = hms_fdb_entry_add(priv, addr, vid, port);
> +	mutex_unlock(&priv->fdb_lock);
> +
> +	return rc;
> +}

Will this delay effectively pace the firmware commands? Since usleep_range
is called before acquiring priv->fdb_lock, multiple threads could sleep
concurrently and then acquire the lock in rapid succession, bypassing the
intended rate limit.

> +static int hms_fdb_dump(struct dsa_switch *ds, int port,
> +			dsa_fdb_dump_cb_t *cb, void *data)
> +{
[ ... ]
> +	while (1) {
[ ... ]
> +		rc = hms_fdb_entry_get(priv, &fdb, entry_id, &next_id);
[ ... ]
> +		if (next_id == 0) /* This entry is empty */
> +			return 0;
> +
[ ... ]
> +		if (fdb.port_map & BIT(port)) {
[ ... ]
> +			rc = cb(fdb.mac_addr, fdb.vid, fdb.dynamic, data);

Could this return statement prematurely abort the dump, skipping the last valid
entry? If the firmware populates fdb with the last valid MAC table entry and
sets next_id == 0 to indicate there are no more entries, exiting here would
prevent the callback from running for that final entry.

Also, is the static/dynamic status inverted here? The third argument to the
callback is bool is_static, but the code passes fdb.dynamic instead of
!fdb.dynamic.

> +static int hms_parse_ports_node(struct hms_private *priv,
> +				struct device_node *ports_node)
> +{
[ ... ]
> +	for_each_available_child_of_node(ports_node, child) {
[ ... ]
> +		u32 index;
[ ... ]
> +		/* Get switch port number from DT */
> +		if (of_property_read_u32(child, "reg", &index) < 0) {
[ ... ]
> +		priv->phy_mode[index] = phy_mode;
> +	}

Does this code write out of bounds if the device tree provides an invalid port
number? The index is read from the device tree and used to index into
priv->phy_mode and priv->fixed_link without verifying that it is less than
HMS_MAX_NUM_PORTS.

> +static void hms_mac_link_up(struct phylink_config *config,
[ ... ]
> +			    int speed, int duplex,
[ ... ]
> +	mac->speed = speed;
> +	mac->egress = true;
> +
> +	hms_port_phylink_mode_set(priv, mac);

Are we missing an update to the MAC duplex state? The phylink callback provides
the negotiated duplex value, but it is not assigned to mac->duplex before
hms_port_phylink_mode_set is called, which might configure the hardware
with a stale or uninitialized setting.

> +int hms_vlan_filtering(struct dsa_switch *ds, int port, bool enabled,
> +		       struct netlink_ext_ack *extack)
> +{
[ ... ]
> +	if (enabled) {
> +		/* Enable VLAN filtering. */
> +		config->tpid  = ETH_P_8021Q;
> +		config->tpid2 = ETH_P_8021AD;
> +	} else {
> +		/* Disable VLAN filtering. */
> +		config->tpid  = HMS_META_ETYPE;
> +		config->tpid2 = HMS_META_ETYPE;
> +	}
> +
> +	for (port = 0; port < ds->num_ports; port++) {
[ ... ]
> +		rc = hms_commit_pvid(ds, port);

Is this TPID configuration ever sent to the switch hardware? The struct fields
are updated, but it doesn't look like any SPI command is dispatched to apply
the new TPID values to the hardware registers, meaning the VLAN filtering
toggle might not actually take effect.

> +static int hms_bridge_vlan_del(struct dsa_switch *ds, int port,
> +			       const struct switchdev_obj_port_vlan *vlan)
> +{
[ ... ]
> +	rc = hms_vlan_entry_del(priv, vlan->vid, port);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * In case the pvid was deleted, make sure that untagged packets will
> +	 * be dropped.
> +	 */
> +	return hms_commit_pvid(ds, port);
> +}

Does this unintentionally re-apply a deleted PVID to the hardware? When a VLAN
is deleted, priv->bridge_pvid[port] is not cleared if it matched vlan->vid.
When hms_commit_pvid is subsequently called, it reads the still-cached PVID
and commands the hardware to use it again.

> +static int hms_dsa_init(struct hms_private *priv)
> +{
[ ... ]
> +	if (!cpu_dp) {
[ ... ]
> +	if (!is_zero_ether_addr(cpu_dp->mac))
> +		mac = cpu_dp->mac;
> +	else
> +		mac = cpu_dp->user->dev_addr;

Can this cause a null pointer dereference? For a CPU port, dsa_port->user is
null, since the master interface is tracked via cpu_dp->conduit instead.
Dereferencing cpu_dp->user->dev_addr during probe would trigger a panic.

> +static int hms_setup(struct dsa_switch *ds)
> +{
[ ... ]
> +	hms_mac_init(priv);
> +	hms_dsa_init(priv);

Is it safe to ignore the return value of hms_dsa_init? If the initialization
of the CPU port fails and returns an error, the driver proceeds to register the
switch anyway, which could result in a broken data path.

> diff --git a/drivers/net/dsa/hms/hms_spi.c b/drivers/net/dsa/hms/hms_spi.c
> new file mode 100644
> index 0000000000000..22e4393554738
> --- /dev/null
> +++ b/drivers/net/dsa/hms/hms_spi.c
> @@ -0,0 +1,116 @@
[ ... ]
> +int hms_xfer_cmd(struct hms_private *priv,
> +		 enum hms_spi_rw_mode rw, enum hms_cmd cmd,
> +		 void *param, size_t param_len,
> +		 void *resp, size_t resp_len,
> +		 struct ptp_system_timestamp *ptp_sts)
> +{
> +	struct hms_cmd_hdr hdr = {0};
[ ... ]
> +	hdr.cmd = (rw << HMS_CMD_DIR_SHIFT) |
> +		  ((resp_len / HMS_SPI_MSG_WORD_BYTES) << HMS_CMD_LEN_SHIFT) |
> +		  cmd;

Could this trigger undefined behavior? The rw variable is an enum (signed
integer), and HMS_CMD_DIR_SHIFT is 31. If rw is SPI_WRITE (1), shifting
it into the sign bit overflows the 32-bit signed integer. Casting rw to u32
would avoid this.

> +	memset(&hdr_xfer, 0, sizeof(hdr_xfer));
> +	hdr_xfer.tx_buf = &hdr;
> +	hdr_xfer.len = HMS_SPI_MSG_HEADER_SIZE;
[ ... ]
> +	if (rw == SPI_READ)
> +		resp_xfer.rx_buf = resp;
> +	else
> +		resp_xfer.tx_buf = resp;
> +	resp_xfer.len = resp_len;
[ ... ]
> +	rc = spi_sync_transfer(spi, &resp_xfer, 1);

Are we passing stack memory to the SPI controller for DMA? Both hdr and
several resp buffers provided by callers are allocated on the local stack.
Using stack memory for DMA is generally unsafe in the kernel, especially with
CONFIG_VMAP_STACK, as it might not be physically contiguous or cache-aligned,
leading to memory corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260509090632.2959553-1-minghuan.lian@nxp.com?part=3