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

[Jens Emil Schulz Ostergaard]

On Sun, 2026-03-29 at 12:56 -0700, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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?
> 

I think skb_vlan_untag is for the rx path, and will not make sense here. But I
can add a check like

	if (skb_headlen(skb) >= VLAN_ETH_HLEN &&
	    ntohs(hdr->h_vlan_proto) == proto) {
		vlan_remove_tag(skb, &tci);
		*vlan_tci = tci;
	} else {

so we know the linear data area has the bytes.

> > +             *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.
> 


I can add pskb_may_pull calls to ensure skb->data is availble.

> > +
> > +     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