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

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

Doruk Tan Ozturk wrote:
> packet_parse_headers() resets the MAC header only for a SOCK_RAW frame
> whose socket did not bind a protocol. A protocol-bound SOCK_RAW socket,
> any SOCK_DGRAM frame, and the legacy SOCK_PACKET path therefore leave
> skb->mac_header unset here.
> 
> For frames sent via __dev_queue_xmit() this is harmless: it resets the
> MAC header unconditionally. But the packet-socket PACKET_QDISC_BYPASS
> path uses dev_direct_xmit(), which does not, so the frame reaches
> ndo_start_xmit() with the MAC header unset. A driver that reads
> eth_hdr(skb) on transmit then dereferences skb->head + (u16)~0, an
> out-of-bounds access ~64 KiB past the head -- the same class fixed for
> one consumer in commit f5089008f90c ("macsec: do not read an unset MAC
> header in macsec_encrypt()").
> 
> packet_parse_headers() runs only on the transmit path, where skb->data
> points at the start of the L2 header for every packet-socket type
> regardless of its length: SOCK_RAW and SOCK_PACKET carry a user-supplied
> header and SOCK_DGRAM has one built by dev_hard_header(). Reset the MAC
> header unconditionally, mirroring __dev_queue_xmit(), so the frame is
> anchored on the bypass path too.
> 
> 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")

Link: https://lore.kernel.org/netdev/20260723020337.19040-1-doruk@0sec.ai/

and perhaps also

Link: https://lore.kernel.org/netdev/20260713194010.54642-1-doruk@0sec.ai/

> Cc: stable@vger.kernel.org
> Assisted-by: 0sec:multi-model
> Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>

Reviewed-by: Willem de Bruijn <willemb@google.com>

My slight caveat about variable length protocols remain (though
perhaps not, with the recent removal of ax25). But evidently this did
not matter for __dev_queue_xmit either. Probably because drivers for
such protocols did not incorrectly assume that mac hdr was set, unlike
the three drivers identified as vulnerable. In any case, fine to have
equivalence between dev_direct_xmit and __dev_queue_xmit. Over time,
these changes chip away at the whole point of a "fast path", but
that's a separate discussion.

> ---
> v2: address Willem de Bruijn's review -- clarify that __dev_queue_xmit()
>     already resets so only the dev_direct_xmit()/PACKET_QDISC_BYPASS path
>     is affected; note skb->data is the L2 start for all packet-socket TX
>     types regardless of L2 length; move the explanatory comment out of the
>     code into the commit log (kept one terse line).
>  net/packet/af_packet.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index e75d2932475a..5ae0511e89e3 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1924,11 +1924,12 @@ static void packet_parse_headers(struct sk_buff *skb, struct socket *sock)
>  {
>  	int depth;
>  
> +	/* On TX skb->data is the L2 header; anchor it for all socket types. */
> +	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
>