Re: [PATCH net-next 2/4] net: dsa: tag_hms: Add HMS tag protocol

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The `buf` pointer in `hms_rcv_meta_cmd()` becomes a dangling pointer if `skb_linearize()` reallocates the SKB.
- [High] Incorrect length calculation in `hms_rcv_meta_cmd()` leads to an integer underflow when invoking the switch driver's meta command handler.
- [High] Returning `NULL` after `__vlan_hwaccel_push_inside()` fails in `hms_imprecise_xmit()` causes a double free in the caller.
- [Medium] The Kconfig symbol NET_DSA_TAG_HMS selects a non-existent symbol NET_DSA_TAG_8021Q.
--

commit 5802d819bc05556c0d9f828794cc67145ff4496a
Author: Minghuan Lian <minghuan.lian@nxp.com>
    
net: dsa: tag_hms: Add HMS tag protocol
    
This commit adds a DSA tagger for NXP Heterogeneous Multi-SoC (HMS) switches.
It supports both tag_8021q based transmit and receive handling for regular
data traffic, and an HMS meta frame format for link-local and control traffic.

> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -211,4 +211,13 @@ config NET_DSA_TAG_YT921X
>  	  Say Y or M if you want to enable support for tagging frames for
>  	  Motorcomm YT921x switches.
>  
> +config NET_DSA_TAG_HMS
> +	tristate "Tag driver for NXP Heterogeneous Multi-SoC switches"
> +	select NET_DSA_TAG_8021Q

Does the NET_DSA_TAG_8021Q config symbol exist? Looking through the tree, it
appears tag_8021q.c is compiled unconditionally as part of dsa_core-y rather
than having its own Kconfig symbol. Selecting a non-existent symbol can lead
to unmet dependency warnings or silent build failures.

> +	help
> +	  Say Y or M if you want to enable support for tagging frames for
> +	  NXP Heterogeneous Multi-SoC switch family. This driver uses a

[ ... ]

> diff --git a/net/dsa/tag_hms.c b/net/dsa/tag_hms.c
> new file mode 100644
> --- /dev/null
> +++ b/net/dsa/tag_hms.c

[ ... ]

> +static struct sk_buff *hms_imprecise_xmit(struct sk_buff *skb,
> +					  struct net_device *netdev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(netdev);
> +	unsigned int bridge_num = dsa_port_bridge_num_get(dp);
> +	struct net_device *br = dsa_port_bridge_dev_get(dp);
> +	u16 tx_vid;

[ ... ]

> +	if (unlikely(skb_vlan_tag_present(skb))) {
> +		skb = __vlan_hwaccel_push_inside(skb);
> +		if (!skb) {
> +			WARN_ONCE(1, "Failed to push VLAN tag to payload!\n");
> +			return NULL;

Could returning NULL here lead to a double free? 

If __vlan_hwaccel_push_inside() fails, it consumes and frees the original skb.
However, returning NULL back up to the DSA core (dsa_user_xmit) usually
signals that the tagger did not consume the skb, which leads the DSA core to
call kfree_skb() on the original pointer again.

> +		}
> +	}
> +
> +	return dsa_8021q_xmit(skb, netdev, hms_xmit_tpid(dp), tx_vid);
> +}

[ ... ]

> +static struct sk_buff *hms_rcv_meta_cmd(struct sk_buff *skb, u16 rx_header)
> +{
> +	u8 *buf = dsa_etype_header_pos_rx(skb) + HMS_HEADER_LEN;
> +	int switch_id = HMS_RX_HEADER_SWITCH_ID(rx_header);
> +	int source_port = HMS_RX_HEADER_PORT_ID(rx_header);
> +	struct hms_tagger_data *tagger_data;
> +	struct net_device *master = skb->dev;
> +	struct dsa_port *cpu_dp;
> +	struct dsa_switch *ds;
> +
> +	cpu_dp = master->dsa_ptr;
> +	ds = dsa_switch_find(cpu_dp->dst->index, switch_id);
> +	if (!ds) {
> +		net_err_ratelimited("%s: cannot find switch id %d\n",
> +				    master->name, switch_id);
> +		return NULL;
> +	}
> +
> +	tagger_data = ds->tagger_data;
> +	if (!tagger_data->meta_cmd_handler)
> +		return NULL;
> +
> +	if (skb_is_nonlinear(skb))
> +		if (skb_linearize(skb))
> +			return NULL;

If skb_linearize() reallocates the data buffer to make it contiguous, does the
buf pointer become dangling? It was initialized before the linearization and
might now point to freed memory.

> +
> +	tagger_data->meta_cmd_handler(ds, source_port, buf,
> +				skb->len - HMS_HEADER_LEN - 2 * ETH_ALEN);

Is this length calculation correct for the receive path? 

Since the MAC header has usually already been pulled from the skb by the time
we reach the tagger, skb->len should simply be the payload size. Subtracting
an additional 2 * ETH_ALEN might cause an integer underflow if the metadata
payload is small, resulting in a huge size_t value being passed to the driver
callback.

> +
> +	/* Discard the meta frame */
> +	return NULL;
> +}

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