[RFC,net,1/1] net: check transport_header before adding offset

Commit Message

sreedevi.joshi Feb. 6, 2025, 6:05 p.m. UTC

From: Sreedevi Joshi <[e-mail]>

skb_headers_offset_update() adds offset to the transport_header
of skb without checking if it was set. When the transport header
is not set, it's value is 65535. Adding offset to this causes it to
roll over and makes the transport_header value to be less than
network_header.
When a tc ingress hook is attached and it invokes bpf_skb_change_tail()
(to strip off extra bytes at the end of packet or to attach some
extra bytes), the logic in __bpf_skb_change_tail() that calculates
the min_len fails due to the transport_header being incorrectly set.

This issue was discovered when testing with veth interface with both xdp and
tc ingress hooks are attached. veth_convert_skb_to_xdp_buff() calls
skb_pp_cow_data() and it results in this function being called. Since
transport_header is incremented without checking, it results in the condition
where transport_header < network_header. __netif_receive_skb_core() when it
receives this skb, skips reset of the transport header as it is already set.

This is specific to XDP path. When there is no XDP hook, the logic takes a
different route (__netif_rx()) and the reset of the transport header happens in
__netif_receive_skb_core() before it reaches tc ingress hook.

Fixes: f5b1729443fd ("net: Add skb_headers_offset_update helper function.")
Signed-off-by: Sreedevi Joshi <[e-mail]>
---
 net/core/skbuff.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Joshi, Sreedevi Feb. 7, 2025, 2:30 p.m. UTC | #1

> -----Original Message-----
> From: sreedevi.joshi <[e-mail]>
> Sent: Thursday, February 6, 2025 1:06 PM
> To: [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Cc: Karlsson, Magnus <[e-mail]>; Fijalkowski, Maciej <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]; Lobakin, Aleksander
> <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Joshi, Sreedevi <[e-mail]>
> Subject: [RFC PATCH net 1/1] net: check transport_header before adding offset
> 
> From: Sreedevi Joshi <[e-mail]>
> 
> skb_headers_offset_update() adds offset to the transport_header
> of skb without checking if it was set. When the transport header
> is not set, it's value is 65535. Adding offset to this causes it to
> roll over and makes the transport_header value to be less than
> network_header.
> When a tc ingress hook is attached and it invokes bpf_skb_change_tail()
> (to strip off extra bytes at the end of packet or to attach some
> extra bytes), the logic in __bpf_skb_change_tail() that calculates
> the min_len fails due to the transport_header being incorrectly set.
> 
> This issue was discovered when testing with veth interface with both xdp and
> tc ingress hooks are attached. veth_convert_skb_to_xdp_buff() calls
> skb_pp_cow_data() and it results in this function being called. Since
> transport_header is incremented without checking, it results in the condition
> where transport_header < network_header. __netif_receive_skb_core() when it
> receives this skb, skips reset of the transport header as it is already set.
> 
> This is specific to XDP path. When there is no XDP hook, the logic takes a
> different route (__netif_rx()) and the reset of the transport header happens in
> __netif_receive_skb_core() before it reaches tc ingress hook.
> 
> Fixes: f5b1729443fd ("net: Add skb_headers_offset_update helper function.")
> Signed-off-by: Sreedevi Joshi <[e-mail]>
> ---
>  net/core/skbuff.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index a441613a1e6c..79b10abd95f1 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2098,7 +2098,8 @@ void skb_headers_offset_update(struct sk_buff *skb, int off)
>  	if (skb->ip_summed == CHECKSUM_PARTIAL)
>  		skb->csum_start += off;
>  	/* {transport,network,mac}_header and tail are relative to skb->head */
> -	skb->transport_header += off;
> +	if (skb_transport_header_was_set(skb))
> +		skb->transport_header += off;
>  	skb->network_header   += off;
>  	if (skb_mac_header_was_set(skb))
>  		skb->mac_header += off;
> --
> 2.25.1

[] resending due to mail server issues.

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index a441613a1e6c..79b10abd95f1 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -2098,7 +2098,8 @@  void skb_headers_offset_update(struct sk_buff *skb, int off)
 	if (skb->ip_summed == CHECKSUM_PARTIAL)
 		skb->csum_start += off;
 	/* {transport,network,mac}_header and tail are relative to skb->head */
-	skb->transport_header += off;
+	if (skb_transport_header_was_set(skb))
+		skb->transport_header += off;
 	skb->network_header   += off;
 	if (skb_mac_header_was_set(skb))
 		skb->mac_header += off;