Re: [PATCH net] 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:
> 
> 	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.

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