Re: [PATCH net 1/2] net/ipv4: reserve exthdrlen as headroom instead of counting it in skb->len

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

tencentos_secure(TencentOS安全公共邮箱) wrote:
> When sending through an xfrm route (rt->dst.header_len > 0),
> __ip_append_data() counts the bundle header space ("exthdrlen")
> into skb->len of the first fragment skb: the skb setup does
> 
> 	skb_reserve(skb, hh_len);
> 	data = skb_put(skb, fraglen + exthdrlen - pagedlen);
> 	skb_set_network_header(skb, exthdrlen);
> 
> so a fully filled first skb has skb->len == maxfraglen + exthdrlen.
> Since commit 353e5c9abd90 ("ipv4: Fix IPsec slowpath fragmentation
> problem"), the next iteration's fraggap = skb_prev->len - maxfraglen
> therefore inherits the whole bundle header length instead of the
> intended alignment slop.
> 
> fraggap is relayed into the next skb by skb_copy_and_csum_bits(), but
> the MSG_MORE (alloclen = mtu) and linear (alloclen = fraglen) branches
> account for fraggap in the allocation only for as long as datalen stays
> uncapped.  Once datalen is capped at maxfraglen - fragheaderlen, the
> bytes copied into the fresh skb exceed the skb head allocation and run
> past the end of the buffer, corrupting skb_shared_info.  The
> paged-branch accounting of
> commit eca856950f7c ("ipv4: account for fraggap on the paged
> allocation path") relies on datalen accounting for fraggap, which
> stops being true exactly when datalen is capped.
> 
> Fix the root cause by reserving exthdrlen as headroom, like IPv6 has
> done since commit 1f85851e17b6 ("ipv6: fix incorrent ipv6 ipsec
> packet fragment") in 2012:
> 
> 	skb_reserve(skb, hh_len + exthdrlen);
> 	data = skb_put(skb, fraglen - pagedlen);
> 	skb_reset_network_header(skb);
> 
> With this, every fragment skb satisfies skb->len <= maxfraglen, so
> fraggap <= mtu - maxfraglen <= 7 (the alignment slop the relay was
> designed for) and the copy destination is provably in bounds.
> 
> Allocation sizes, tail positions, tailroom and truesize are unchanged
> (alloc_extra already carries exthdrlen), and when dst.header_len == 0
> this is a literal no-op.
> 
> Fixes: 353e5c9abd90 ("ipv4: Fix IPsec slowpath fragmentation problem")
> Reported-by: TencentOS Corvus AI <corvus@tencent.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Anakin Zhang <tencentos_secure@tencent.com>

LGTM on the general approach:

The XFRM inserted headers must only be inserted once on the first
fragment.

The same approach is already taken in IPv6. Though there the variable
is dst_exthdrlen, not to be confused with exthdrlen.

My main concern is that this code is notoriously fragile, with the
same variables used in different calculations / overlapping uses.

Writing past skb head is obviously bad. Ideally we could catch that
with a patch that is trivially reviewable for stable. Not sure that
exists. But I'm a little hesitant to send to stable something hard
to establish the correctness of.

I can definitely stare at it for longer to increase confidence.
Also Cc:ing the IPSec maintainer for an extra set of expert eyes.

> ---
>  net/ipv4/ip_output.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index eaa6fab..b84d56c 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1150,16 +1150,16 @@ alloc_new_skb:
>  			 */
>  			skb->ip_summed = csummode;
>  			skb->csum = 0;
> -			skb_reserve(skb, hh_len);
> +			skb_reserve(skb, hh_len + exthdrlen);
>  
>  			/*
>  			 *	Find where to start putting bytes.
>  			 */
> -			data = skb_put(skb, fraglen + exthdrlen - pagedlen);
> -			skb_set_network_header(skb, exthdrlen);
> +			data = skb_put(skb, fraglen - pagedlen);
> +			skb_reset_network_header(skb);
>  			skb->transport_header = (skb->network_header +
>  						 fragheaderlen);
> -			data += fragheaderlen + exthdrlen;
> +			data += fragheaderlen;
>  
>  			if (fraggap) {
>  				skb->csum = skb_copy_and_csum_bits(
> -- 
> 2.55.0
>