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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Returning NULL from `ks8995_xmit()` after `vlan_insert_tag()` fails causes a double free in the DSA core.
- [High] Returning NULL from `ks8995_rcv()` after `skb_vlan_untag()` fails causes a double free in the DSA core.
- [Medium] `ks8995_xmit()` fails to clear 802.1ad hardware-accelerated VLAN tags, causing corrupted tag order on the wire.
- [Low] Unvalidated portmask can cause undefined behavior via `ilog2(0)` for untagged `0x8100` management packets.
--

commit 8a732a10d6f1205df5b3687abb422f16fbcd2942
Author: Linus Walleij <linusw@kernel.org>

net: dsa: tag_ks8995: Add the KS8995 tag handling

The KS8995 100Mbit switch can do proper DSA per-port tagging
with the proper set-up. This adds the code to handle ingress
and egress KS8995 tags.

> diff --git a/net/dsa/tag_ks8995.c b/net/dsa/tag_ks8995.c
> new file mode 100644
> --- /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)) {

[Severity: Medium]
Does this code intentionally ignore 802.1ad hardware-accelerated VLAN tags?

If the packet contains a valid 802.1ad tag, it appears the hwaccel tag is not
cleared here. Later, the MAC driver might prepend the uncleared 802.1ad tag
before the STPID tag, which could break the switch's expectation that the
STPID tag is the first tag after the MAC addresses.

> +		tci = skb_vlan_tag_get(skb);
> +		__vlan_hwaccel_clear_tag(skb);
> +		have_hwaccel_tag = true;
> +	}
> +
> +	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: High]
Can returning NULL here lead to a double free in the DSA core?

The vlan_insert_tag() function consumes and frees the skb on allocation
failure before returning NULL. If ks8995_xmit() propagates this NULL back
to the caller:

dsa_user_xmit()
    nskb = p->xmit(skb, dev);
    if (!nskb) {
        kfree_skb(skb);
        return NETDEV_TX_OK;
    }
    
Won't dsa_user_xmit() unconditionally execute kfree_skb() on the original skb
pointer, freeing it a second time?

[ ... ]

> +static struct sk_buff *ks8995_rcv(struct sk_buff *skb, struct net_device *dev)
> +{
> +	int portmask;
> +	u16 etype;
> +
> +	/* We are expecting all received packets to have a mangled VLAN
> +	 * TPID, so drop anything else. Because of the non-standard TPID,
> +	 * don't even bother looking for a tag in the hwaccel area.
> +	 *
> +	 * We have to inspect the ethertype directly because skb->protocol
> +	 * will contain garbage.
> +	 */
> +	etype = ntohs(*(__be16 *)dsa_etype_header_pos_rx(skb));
> +	if ((etype & KS8995M_STPID_STD) != ETH_P_8021Q) {
> +		netdev_dbg(dev, "%s: dropped ethertype 0x%04x\n",
> +			   __func__, etype);
> +		return NULL;
> +	}
> +	netdev_dbg(dev, "%s: received ethertype %04x\n",
> +		   __func__, 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: High]
Could returning NULL after skb_vlan_untag() fails cause a double free?

The skb_vlan_untag() function frees the skb internally if it encounters
an error, such as a failure in pskb_may_pull(). If this function returns
NULL to the DSA core:

dsa_switch_rcv()
    nskb = cpu_dp->rcv(skb, dev);
    if (!nskb) {
        kfree_skb(skb);
        return 0;
    }

Will the caller unconditionally execute kfree_skb() on the original skb
pointer that was already freed by skb_vlan_untag()?

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

[Severity: Low]
Is it possible for portmask to be 0 here?

If an unmodified 802.1Q frame arrives at the management port with standard
TPID 0x8100 and no STPID port bits set, the earlier validation passes, and
the portmask extracted from the lower nibble evaluates to 0.

While the kernel's ilog2 macro safely evaluates ilog2(0) to -1, passing 0
to ilog2() is technically undefined behavior in C. Would it be better to
validate portmask before passing it to ilog2() and dsa_conduit_find_user()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-ks8995-to-ksz8-v4-0-e15149ef21e7@kernel.org?part=3