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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: dsa: add tag driver for LAN9645X

Add a tag driver for LAN9645x switches using a front port as the CPU port
(NPI port). The driver configures a long prefix for extraction and no
prefix for injection to handle the internal frame header (IFH).

> diff --git a/net/dsa/tag_lan9645x.c b/net/dsa/tag_lan9645x.c
> new file mode 100644
> index 000000000000..9130fb0d61be
> --- /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 (ntohs(hdr->h_vlan_proto) == proto) {
> +		vlan_remove_tag(skb, &tci);

Is it possible for a maliciously crafted packet (e.g., via AF_PACKET) to have a
length of 14 bytes with h_vlan_proto set to ETH_P_8021Q?

If so, vlan_remove_tag() would read vhdr->h_vlan_TCI at offset 14, causing an
out-of-bounds read.

Furthermore, if the packet contains fragments and the VLAN header is not in the
linear data area, vlan_remove_tag() could corrupt the SKB when shifting the MAC
header.

Would it be safer to use skb_vlan_untag() instead?

> +		*vlan_tci = tci;
> +	} else {
> +		rcu_read_lock();
> +		br_vlan_get_pvid_rcu(br, &tci);
> +		rcu_read_unlock();
> +		*vlan_tci = tci;
> +	}
> +
> +	*tag_type = (proto != ETH_P_8021Q) ? LAN9645X_IFH_TAG_TYPE_S :
> +					     LAN9645X_IFH_TAG_TYPE_C;
> +}

[ ... ]

> +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;
> +	u8 *orig_skb_data = skb->data;
> +	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);
> +	/* IFH starts after our long prefix */
> +	ifh = skb_pull(skb, LAN9645X_LONG_PREFIX_LEN);

Could skb_pull() return NULL here if skb->len is less than 16 bytes?

If it returns NULL, the blind dereference of ifh in the subsequent
lan9645x_ifh_get() calls would cause a NULL pointer dereference.

Additionally, is the 44-byte tag (16-byte prefix + 28-byte IFH) guaranteed to
be in the linear data buffer (skb->data)?

Without calling pskb_may_pull(skb, LAN9645X_TOTAL_TAG_LEN) first, accessing it
via the ifh pointer might result in out-of-bounds reads if the packet payload
is in paged fragments.

> +
> +	popcnt = lan9645x_ifh_get(ifh, IFH_POP_CNT, IFH_POP_CNT_SZ);
> +	etype_ofs = lan9645x_ifh_get(ifh, IFH_ETYPE_OFS, IFH_ETYPE_OFS_SZ);
> +	src_port = lan9645x_ifh_get(ifh, IFH_SRCPORT, IFH_SRCPORT_SZ);
-- 
pw-bot: cr