Re: [PATCH net-next v9 1/9] net: dsa: add tag driver for LAN9645X

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The tag driver drops the socket buffer (`skb`) without freeing it when encountering errors in both the RX and TX paths, leading to memory leaks.
- [High] Hardware-accelerated VLAN tags are missed during transmission, leading to incorrect VLAN assignment and corruption of the Internal Frame Header (IFH) by the conduit NIC hardware.
--

--- Patch [1]: [PATCH net-next v9 1/9] net: dsa: add tag driver for LAN9645X ---
commit b8cd048a28aade1f235bef2a39bd42989aa9f813
Author: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>

net: dsa: add tag driver for LAN9645X

This commit adds a new DSA tag driver for the LAN9645x switch using a front port
as a CPU port (NPI mode). The driver configures an asymmetrical frame format,
using a long prefix on extraction and no prefix on injection.

> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 0000000000000..c9049e3620a12
> --- /dev/null
> +++ b/net/dsa/tag_lan9645x.c
[ ... ]
> +static void lan9645x_xmit_get_vlan_info(struct sk_buff *skb,
> +					struct net_device *br,
> +					u32 *vlan_tci, u32 *tag_type)
> +{
> +	struct vlan_ethhdr *hdr;
> +	u16 proto, tci;
> +
> +	if (!br || !br_vlan_enabled(br)) {
> +		*vlan_tci = 0;
> +		*tag_type = LAN9645X_IFH_TAG_TYPE_C;
> +		return;
> +	}
> +
> +	hdr = (struct vlan_ethhdr *)skb_mac_header(skb);
> +	br_vlan_get_proto(br, &proto);
> +
> +	if (skb_headlen(skb) >= VLAN_ETH_HLEN &&
> +	    ntohs(hdr->h_vlan_proto) == proto) {
> +		vlan_remove_tag(skb, &tci);
> +		*vlan_tci = tci;

[Severity: High]
Does this miss hardware-accelerated VLAN tags?

If the DSA user device inherits the NETIF_F_HW_VLAN_CTAG_TX feature from the
conduit device, the networking stack retains the VLAN tag in skb->vlan_tci
with skb_vlan_tag_present(skb) == true rather than pushing it into the payload.

Because this code doesn't check for skb_vlan_tag_present() to clear the tag
via __vlan_hwaccel_clear_tag(skb), it falls back to the port's PVID and leaves
skb->vlan_tci intact. 

When the skb is later passed to the conduit device, its NIC hardware could
offload the VLAN tag insertion. Since this driver just prepended a 28-byte IFH,
the NIC hardware might insert the 4-byte VLAN tag at offset 12 (expecting a
standard Ethernet frame), overwriting and corrupting the IFH.

> +	} else {
> +		rcu_read_lock();
> +		br_vlan_get_pvid_rcu(br, &tci);
> +		rcu_read_unlock();
> +		*vlan_tci = tci;
> +	}
[ ... ]
> +static struct sk_buff *lan9645x_xmit(struct sk_buff *skb,
> +				     struct net_device *ndev)
> +{
> +	struct dsa_port *dp = dsa_user_to_port(ndev);
> +	struct dsa_switch *ds = dp->ds;
> +	u32 vlan_tci, tag_type;
> +	u32 qos_class;
> +	void *ifh;
> +
> +	lan9645x_xmit_get_vlan_info(skb, dsa_port_bridge_dev_get(dp), &vlan_tci,
> +				    &tag_type);
> +
> +	/* We need to make sure frame has the proper size after IFH is stripped
> +	 * by hw.
> +	 */
> +	if (__skb_put_padto(skb, ETH_ZLEN, false))
> +		return NULL;

[Severity: High]
Does this leak the skb when __skb_put_padto() fails?

Since the free_on_error argument is false, __skb_put_padto() returns an error
without freeing the buffer. 

By returning NULL here without freeing the skb, the buffer could leak under
memory pressure. DSA core requires tag drivers to explicitly free the skb if
they return NULL to drop the packet.

> +
> +	qos_class = netdev_get_num_tc(ndev) ?
[ ... ]
> +static struct sk_buff *lan9645x_rcv(struct sk_buff *skb,
> +				    struct net_device *ndev)
> +{
> +	u32 src_port, qos_class, vlan_tci, tag_type, popcnt, etype_ofs, cpuq;
> +	struct dsa_port *dp;
> +	u32 ifh_gap_len = 0;
> +	u16 vlan_tpid;
> +	u8 *ifh;
> +
> +	/* DSA master already consumed DMAC,SMAC,ETYPE from long prefix. Go back
> +	 * to beginning of frame.
> +	 */
> +	skb_push(skb, ETH_HLEN);
> +
> +	if (unlikely(!pskb_may_pull(skb, LAN9645X_TOTAL_TAG_LEN)))
> +		return NULL;

[Severity: High]
Can this leak the skb if pskb_may_pull() fails?

If a received packet is smaller than expected, pskb_may_pull() fails and
returns false. 

Returning NULL here without freeing the skb causes a memory leak, which could
be triggered by receiving malformed, undersized packets.

> +
> +	/* IFH starts after our long prefix */
> +	ifh = skb_pull(skb, LAN9645X_LONG_PREFIX_LEN);
[ ... ]
> +	skb_pull(skb, LAN9645X_IFH_LEN);
> +
> +	if (unlikely(!pskb_may_pull(skb, ifh_gap_len + ETH_HLEN)))
> +		return NULL;

[Severity: High]
Would this also leak the skb for the same reason if pskb_may_pull() fails?

> +
> +	skb_pull(skb, ifh_gap_len);
> +	skb_reset_mac_header(skb);
[ ... ]
> +	skb->dev = dsa_conduit_find_user(ndev, 0, src_port);
> +	if (WARN_ON_ONCE(!skb->dev)) {
> +		/* This should never happen since we have disabled reflection
> +		 * back to the CPU.
> +		 */
> +		return NULL;
> +	}

[Severity: High]
Is it possible to leak the skb here if dsa_conduit_find_user() returns NULL?

Returning NULL without explicitly freeing the skb drops the reference to the
buffer without freeing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-dsa_lan9645x_switch_driver_base-v9-0-0d1512a326d7@microchip.com?part=1