Re: [PATCH net] net/packet: reset the MAC header on the packet-socket transmit path

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Willem de Bruijn wrote:
> Doruk Tan Ozturk wrote:
> > packet_parse_headers() resets the MAC header only for a SOCK_RAW frame
> > whose socket did not bind a protocol:
> > 
> > 	if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
> > 	    sock->type == SOCK_RAW) {
> > 		skb_reset_mac_header(skb);
> > 		skb->protocol = dev_parse_header_protocol(skb);
> > 	}
> > 
> > Every other outgoing packet-socket frame therefore reaches
> > ndo_start_xmit() with the MAC header unset: a SOCK_RAW socket bound to a
> 
> The normal __dev_queue_xmit calls skb_reset_mac_header.
> 
> This therefore only applies to the dev_direct_xmit path.
> 
> > specific protocol (for example socket(AF_PACKET, SOCK_RAW,
> > htons(ETH_P_IP))), any SOCK_DGRAM frame (its header is built by
> > dev_hard_header(), which does not set mac_header), and the legacy
> > SOCK_PACKET path. A driver that reads eth_hdr(skb) on transmit then
> > dereferences skb->head + (u16)~0, an out-of-bounds access about 64 KiB
> > past the head.
> 
> This is not currently an expected invariant, but I see from a comment
> below the --- that this approach is proposed instead of fixing up the
> buggy drivers.
> 
> > 
> > This is the same class fixed for one consumer in commit f5089008f90c
> > ("macsec: do not read an unset MAC header in macsec_encrypt()"); other
> > TX .xmit paths that read eth_hdr(skb)->h_dest (several DSA taggers,
> > ibmveth, sja1105, the atlantic PTP path) have the same problem.
> > 
> > packet_parse_headers() runs only on the transmit path
> > (packet_sendmsg_spkt(), tpacket_fill_skb(), packet_snd()), and there
> > skb->data is the start of the L2 header for every packet-socket type.
> 
> There are some subtle points with variable length L2 protocols that
> need to be reviewed.

Come to think of it, if __dev_queue_xmit can unconditionally reset the
mac header, then so can other tx paths.

> > Reset the MAC header unconditionally so it is anchored for all of them,
> > fixing the class at the source rather than hardening each consumer.
> > 
> > The protocol probe is unchanged. A CONFIG_DEBUG_NET build stops warning
> > about an unset mac header in skb_mac_header() on these paths, and the
> > out-of-bounds eth_hdr() read no longer occurs.
> > 
> > Found by 0sec (https://0sec.ai) using automated source analysis;
> > verified against source and matched to the macsec KASAN report in
> > f5089008f90c. Compile-tested.
> > 
> > Fixes: 75c65772c3d1 ("net/packet: Ask driver for protocol if not provided by user")
> > Cc: stable@vger.kernel.org
> > Assisted-by: 0sec:multi-model
> > Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
> > ---
> > This supersedes the per-consumer series "[PATCH net 0/3] net: dont read
> > an unset MAC header on the raw/qdisc-bypass TX path"
> > (https://lore.kernel.org/netdev/20260713194010.54642-1-doruk@0sec.ai/),
> > per Jakubs suggestion to fix the problem at the source rather than
> > hardening each driver.  Vladimir Oltean had reviewed 2/3 of that series;
> > this is a different (source) fix, so I have not carried the tags.
> >  net/packet/af_packet.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index e75d2932475a..adfb9c19a3ca 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -1924,11 +1924,21 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
> >  {
> >  	int depth;
> >  
> > +	/*
> > +	 * packet_parse_headers() runs only on the transmit path
> > +	 * (packet_sendmsg_spkt(), tpacket_fill_skb(), packet_snd()), where
> > +	 * skb->data is the start of the L2 header for every packet-socket
> > +	 * type: SOCK_RAW and SOCK_PACKET carry a user-supplied header and
> > +	 * SOCK_DGRAM has one built by dev_hard_header(). Anchor the MAC
> > +	 * header for all of them so a frame does not reach ndo_start_xmit()
> > +	 * with the MAC header unset, where a driver reading eth_hdr(skb) on
> > +	 * TX would dereference an out-of-bounds offset (skb->head + (u16)~0).
> > +	 */
> 
> This is too verbose. Better captured in the commit message.
> 
> > +	skb_reset_mac_header(skb);
> > +
> >  	if ((!skb->protocol || skb->protocol == htons(ETH_P_ALL)) &&
> > -	    sock->type == SOCK_RAW) {
> > -		skb_reset_mac_header(skb);
> > +	    sock->type == SOCK_RAW)
> >  		skb->protocol = dev_parse_header_protocol(skb);
> > -	}
> >  
> >  	/* Move network header to the right position for VLAN tagged packets */
> >  	if (likely(skb->dev->type == ARPHRD_ETHER) &&
> > -- 
> > 2.43.0
> > 
> 
>