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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] NULL pointer dereference in ks8995_rcv() error handling.
- [High] Double-free of SKB when tag insertion or untagging fails.
- [High] Overwriting existing payload VLAN TCI with zeros for inline tagged packets.
- [High] Applying ilog2() to a portmask of 0 causes undefined behavior.
- [High] Modifying a potentially shared or cloned SKB without unsharing in ks8995_xmit().
- [High] Modifying the VLAN header without ensuring header linearization.
--

commit 541608f5cb95c612cb3392581b43467321c30a37
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
> index 0000000000000..6ae6789751d9e
> --- /dev/null
> +++ b/net/dsa/tag_ks8995.c
> @@ -0,0 +1,134 @@
[ ... ]
> +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;
> +	}
> +
> +	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;

If vlan_insert_tag() fails, it consumes the original SKB and returns NULL.
If we return NULL here, will the DSA core in dsa_user_xmit() attempt to
call kfree_skb() on the original SKB pointer, leading to a double-free?

> +		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 {
> +		/* VLAN tag already exists in skb head, modify it in place */
> +		hdr = vlan_eth_hdr(skb);
> +		hdr->h_vlan_proto = KS8995M_STPID(portmask);
> +		hdr->h_vlan_TCI = htons(tci);

Since tci is initialized to 0 and not updated with the existing packet's TCI
when there is no hwaccel tag, does this blindly overwrite the packet's valid
VLAN ID and priority (PCP) with 0?

Also, the code directly modifies the VLAN header here without checking
skb_shared() or skb_cloned(), or calling skb_cow_head(). Can this corrupt
data for other users of the same SKB, such as packet sniffers?

Furthermore, is it safe to modify the VLAN header here without verifying that
the header is in the linear part of the SKB? If the VLAN tag resides in paged
fragments, will writing to these fields corrupt memory out-of-bounds of the
linear buffer? Should there be a call to pskb_may_pull() or
skb_ensure_writable()?

> +		netdev_dbg(dev, "%s: modified VLAN TAG %04x TCI %04x\n",
> +			   __func__, hdr->h_vlan_proto, hdr->h_vlan_TCI);
> +	}
> +
> +	return skb;
> +}
> +
> +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(*(u16 *)dsa_etype_header_pos_rx(skb));
> +	if ((etype & KS8995M_STPID_STD) != ETH_P_8021Q) {
> +		netdev_info(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 protocol %04x vlan protocol  %04x\n",
> +			   __func__, ntohs(skb->protocol), ntohs(skb->vlan_proto));

If skb_vlan_untag() fails, it internally frees the SKB and returns NULL, which
is assigned to skb. Does dereferencing skb->protocol and skb->vlan_proto
in the netdev_err() call here cause a NULL pointer dereference?

Additionally, if we return NULL here after skb_vlan_untag() has already
freed the SKB, will the DSA core in dsa_switch_rcv() attempt to free it
again, leading to a double-free?

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

The ethertype validation allows a standard 802.1Q tag (0x8100) to pass, since
0x8100 & 0xFFF0 == 0x8100. If this happens, portmask will be 0.
Will unconditionally calling ilog2(portmask) with a value of 0 cause
undefined behavior?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-ks8995-to-ksz8-v1-0-70d0ef4aa5f4@kernel.org?part=3