Re: [PATCH net-next v5 3/5] net: dsa: tag_ks8995: Add the KS8995 tag handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Double free of skb in ks8995_xmit() and ks8995_rcv() on tag allocation failures.
- [High] ks8995_xmit() fails to clear hardware-accelerated VLAN tags for non-802.1Q protocols (like 802.1ad QinQ), breaking DSA port isolation.
--

commit 69e87755967f9829df353d79046ed354251e2a4a
Author: Linus Walleij <linusw@kernel.org>

net: dsa: tag_ks8995: Add the KS8995 tag handling

This commit adds ingress and egress tagging support for the KS8995 100Mbit
switch in the DSA subsystem. The tag is implemented as a modified 0x8100
ethertype tag.

> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> index 0000000000000..5bdb182715827
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c

[ ... ]

> +static struct sk_buff *ks8995_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct vlan_ethhdr *hdr = vlan_eth_hdr(skb);
> +	bool have_hwaccel_tag = false;
> +	u16 tci = 0, portmask;
> +
> +	/* Prepare the special KS8995 tags */
> +	portmask = dsa_xmit_port_mask(skb, dev);
> +
> +	if (skb_vlan_tag_present(skb) && skb->vlan_proto == htons(ETH_P_8021Q)) {
> +		tci = skb_vlan_tag_get(skb);
> +		__vlan_hwaccel_clear_tag(skb);
> +		have_hwaccel_tag = true;
> +	}

[Severity: High]
Does this strict check for ETH_P_8021Q fail to clear hardware-accelerated
VLAN tags for other protocols, such as 802.1ad?

If a non-802.1Q hwaccel tag is left intact, the conduit driver might offload
it as the outermost tag on the wire. Since the KS8995 switch expects its
STPID tag to immediately follow the MAC addresses, could seeing the uncleared
tag instead cause it to misinterpret the DSA routing tag and bypass port
isolation?

> +
> +	if (have_hwaccel_tag || hdr->h_vlan_proto != htons(ETH_P_8021Q)) {
> +		skb = vlan_insert_tag(skb, KS8995M_STPID(portmask), tci);
> +		if (!skb)
> +			return NULL;

[Severity: Critical]
Can this cause a double-free of the skb on allocation failure?

Looking at vlan_insert_tag(), it unconditionally consumes and frees the skb if
it fails to allocate memory. However, returning NULL from a DSA tagger's xmit
routine instructs the caller, dsa_user_xmit(), to explicitly free the original
skb again.

> +		hdr = vlan_eth_hdr(skb);
> +		netdev_dbg(dev, "%s: inserted VLAN TAG %04x TCI %04x\n",
> +			   __func__, hdr->h_vlan_proto, hdr->h_vlan_TCI);
> +	} else {

[ ... ]

> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	int portmask;
> +	u16 etype;

[ ... ]

> +	/* Move the custom DSA+VLAN tag into the hwaccel area and strip
> +	 * it from the skb head
> +	 */
> +	skb = skb_vlan_untag(skb);
> +	if (!skb) {
> +		netdev_err(dev, "%s: unable to untag skb\n", __func__);
> +		return NULL;
> +	}

[Severity: Critical]
Will this result in a double-free of the skb?

Similar to the xmit path, skb_vlan_untag() frees the skb when it encounters an
error. Returning NULL from ks8995_rcv() instructs dsa_switch_rcv() to free the
skb a second time.

> +
> +	portmask = FIELD_GET(KS8995M_STPID_PORTMASK, etype);
> +	netdev_dbg(dev, "%s: etype %04x portmask %04x (%d)\n",
> +		   __func__, etype, portmask, ilog2(portmask));

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604-ks8995-to-ksz8-v5-0-98baeb45e665@kernel.org?part=3