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

[Joshi, Sreedevi]

> -----Original Message-----
> From: sreedevi.joshi <joshisre@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, February 6, 2025 1:06 PM
> To: edumazet@xxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; horms@xxxxxxxxxx; ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx
> Cc: Karlsson, Magnus <magnus.karlsson@xxxxxxxxx>; Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>; hawk@xxxxxxxxxx;
> john.fastabend@xxxxxxxxx; almasrymina@xxxxxxxxxx; asml.silence@xxxxxxxxx; lorenzo@xxxxxxxxxx; Lobakin, Aleksander
> <aleksander.lobakin@xxxxxxxxx>; chopps@xxxxxxxx; bigeasy@xxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> bpf@xxxxxxxxxxxxxxx; Joshi, Sreedevi <sreedevi.joshi@xxxxxxxxx>
> Subject: [RFC PATCH net 1/1] net: check transport_header before adding offset
> 
> From: Sreedevi Joshi <sreedevi.joshi@xxxxxxxxx>
> 
> 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 <sreedevi.joshi@xxxxxxxxx>
> ---
>  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.