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

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

[Doruk Tan Ozturk]

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

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

[Willem de Bruijn]

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
> 

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

[Willem de Bruijn]

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