[PATCH net 2/2] net/ipv4: fail append when the moved fraggap exceeds the fragment data

[PATCH net 2/2] net/ipv4: fail append when the moved fraggap exceeds the fragment data

[=?gb18030?B?dGVuY2VudG9zX3NlY3VyZShUZW5jZW50T1Owssiruau5stPKz+Qp?=]

When the first fragment of a cork is already at maxfraglen, the next
__ip_append_data() iteration relays the previous fragment's tail,
fraggap = skb_prev->len - maxfraglen, into the new skb. The moved
fraggap is charged against datalen by computing

	copy = datalen - transhdrlen - fraggap - pagedlen;

but once datalen is capped at maxfraglen - fragheaderlen it can become
smaller than transhdrlen + fraggap. Then copy goes negative: no payload
is copied and the iteration makes no progress. Worse, in the paged
branch (MSG_SPLICE_PAGES / MSG_ZEROCOPY / GSO) pagedlen = datalen -
transhdrlen - fraggap additionally wraps unsigned, breaking the
alloclen + pagedlen == fraglen invariant that commit eca856950f7c
("ipv4: account for fraggap on the paged allocation path") relies
on; the loop then either queues skbs with garbage tail accounting
(silent packet corruption) or, when maxfraglen == fragheaderlen,
never makes progress and grinds on until -ENOBUFS.

Reject such an append before any allocation: if the moved tail
(transhdrlen + fraggap) cannot fit in the fragment's datalen, the
iteration cannot make progress, so fail with -EINVAL. This covers all
three alloclen branches, including the paged one. With the preceding
fix fraggap is bounded by the alignment slop, but the condition stays
reachable with extreme geometries (e.g. an effective mtu too small for
fragheaderlen + transhdrlen), so pin the geometry explicitly, as
__ip6_append_data() has done since commit 232cd35d0804 ("ipv6: fix
out of bound writes in __ip6_append_data()").

This cannot reject a send that would otherwise succeed: the guard only
fires when transhdrlen + fraggap > datalen, which is exactly when copy
would come out negative, or, on the paged branch, when pagedlen would
wrap.  No send can get past an iteration with a negative copy -- no
payload is copied, so the loop makes no progress.  All callers flush
the cork on append error, and returning -EINVAL in mid-cork state
already happens in this function (MSG_ZEROCOPY ubuf mismatch).

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>
---
 net/ipv4/ip_output.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index b84d56c..5db8b06 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1105,6 +1105,11 @@ alloc_new_skb:
 			fraglen = datalen + fragheaderlen;
 			pagedlen = 0;
 
+			if (unlikely(transhdrlen + fraggap > datalen)) {
+				err = -EINVAL;
+				goto error;
+			}
+
 			alloc_extra = hh_len + 15;
 			alloc_extra += exthdrlen;
 
-- 
2.55.0

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

[=?gb18030?B?dGVuY2VudG9zX3NlY3VyZShUZW5jZW50T1Owssiruau5stPKz+Qp?=]

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

Re: [PATCH net 2/2] net/ipv4: fail append when the moved fraggap exceeds the fragment data

[Eric Dumazet]

Le lun. 26 oct. 2026 à 12:23, tencentos_secure(TencentOS安全公共邮箱)
<tencentos_secure@tencent.com> a écrit :
>
> When the first fragment of a cork is already at maxfraglen, the next
> __ip_append_data() iteration relays the previous fragment's tail,
> fraggap = skb_prev->len - maxfraglen, into the new skb. The moved
> fraggap is charged against datalen by computing
>
>         copy = datalen - transhdrlen - fraggap - pagedlen;
>
> but once datalen is capped at maxfraglen - fragheaderlen it can become
> smaller than transhdrlen + fraggap. Then copy goes negative: no payload
> is copied and the iteration makes no progress. Worse, in the paged
> branch (MSG_SPLICE_PAGES / MSG_ZEROCOPY / GSO) pagedlen = datalen -
> transhdrlen - fraggap additionally wraps unsigned, breaking the
> alloclen + pagedlen == fraglen invariant that commit eca856950f7c
> ("ipv4: account for fraggap on the paged allocation path") relies
> on; the loop then either queues skbs with garbage tail accounting
> (silent packet corruption) or, when maxfraglen == fragheaderlen,
> never makes progress and grinds on until -ENOBUFS.
>
> Reject such an append before any allocation: if the moved tail
> (transhdrlen + fraggap) cannot fit in the fragment's datalen, the
> iteration cannot make progress, so fail with -EINVAL. This covers all
> three alloclen branches, including the paged one. With the preceding
> fix fraggap is bounded by the alignment slop, but the condition stays
> reachable with extreme geometries (e.g. an effective mtu too small for
> fragheaderlen + transhdrlen), so pin the geometry explicitly, as
> __ip6_append_data() has done since commit 232cd35d0804 ("ipv6: fix
> out of bound writes in __ip6_append_data()").
>

I do not think this is reachable once patch 1 is applied.

ip_setup_cork() rejects cork->fragsize < 68 (inetdev_valid_mtu()), and
fragheaderlen is at most 60, so a capped datalen is at least 8, and
maxfraglen == fragheaderlen cannot happen.

transhdrlen is non zero only on the first iteration with an empty
queue, where fraggap is 0, and current callers pass at most 8 bytes
(UDP, ICMP, ping). On later iterations transhdrlen is 0 and, with
patch 1, fraggap <= mtu - maxfraglen <= 7.

So transhdrlen + fraggap > datalen can not be true.

Please give a concrete setup that reaches this check after patch 1,
or present the check as defense in depth matching
__ip6_append_data() and drop the Fixes: and Cc: stable tags.

> This cannot reject a send that would otherwise succeed: the guard only
> fires when transhdrlen + fraggap > datalen, which is exactly when copy
> would come out negative, or, on the paged branch, when pagedlen would
> wrap.  No send can get past an iteration with a negative copy -- no
> payload is copied, so the loop makes no progress.  All callers flush
> the cork on append error, and returning -EINVAL in mid-cork state
> already happens in this function (MSG_ZEROCOPY ubuf mismatch).
>
> 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>
> ---
>  net/ipv4/ip_output.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
> index b84d56c..5db8b06 100644
> --- a/net/ipv4/ip_output.c
> +++ b/net/ipv4/ip_output.c
> @@ -1105,6 +1105,11 @@ alloc_new_skb:
>                         fraglen = datalen + fragheaderlen;
>                         pagedlen = 0;
>
> +                       if (unlikely(transhdrlen + fraggap > datalen)) {
> +                               err = -EINVAL;
> +                               goto error;
> +                       }
> +
>                         alloc_extra = hh_len + 15;
>                         alloc_extra += exthdrlen;
>
> --
> 2.55.0
>

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

[Willem de Bruijn]

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
>