[PATCH net v2 2/2] net/ipv4: reject appends with negative fragment copy

[PATCH net v2 2/2] net/ipv4: reject appends with negative fragment copy

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

__ip_append_data() caps datalen at maxfraglen - fragheaderlen when more
fragments are needed, and later computes

	copy = datalen - transhdrlen - fraggap - pagedlen;

If transhdrlen + fraggap exceeds datalen, copy goes negative in the
non-paged allocation branches, where pagedlen is zero, invalidating
the offset/length accounting. In the paged allocation branch,
pagedlen = datalen - transhdrlen - fraggap wraps unsigned instead.
The subsequent copy calculation cancels to zero, so checking copy
alone does not catch this invalid fragment geometry.

This is reachable independently of the xfrm fraggap bug fixed by the
preceding patch. icmp_timestamp() passes a 20-byte transport header.
With a small MTU and IPv4 options, the capped datalen can be smaller
than that header even when fraggap is zero. The negative copy then
propagates a negative offset into icmp_glue_bits(), and a timestamp
reply with corrupted timestamp fields can be emitted.

For example, with MTU 68 and 40 bytes of IPv4 options, fragheaderlen
is 60, maxfraglen is 68, datalen is capped at 8, and fraggap is zero,
so the 20-byte transport header cannot fit.

Reject this geometry before allocation with -EINVAL, covering all three
alloclen branches and matching the existing __ip6_append_data() guard.
A negative copy cannot produce a valid append, so this does not reject a
send that could otherwise construct a correct packet. 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Anakin Zhang <tencentos_secure@tencent.com>
---
v2:
- Document the independently reachable ICMP Timestamp corruption path.
- Distinguish negative copy in non-paged allocation branches from
  unsigned pagedlen wrap in the paged allocation branch.
- Update the subject and Fixes tag accordingly.
- Drop the unrelated xfrm Reported-by tag; no code changes.

 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 v2 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().
The MSG_MORE (alloclen = mtu) and linear (alloclen = fraglen) branches
rely on datalen accounting for the moved tail. If datalen is capped
below transhdrlen + fraggap, the copy can extend beyond skb->end and
corrupt skb_shared_info. The paged-branch accounting introduced by
commit eca856950f7c ("ipv4: account for fraggap on the paged
allocation path") also relies on datalen being large enough for the
transport header and moved tail.

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

The corresponding IPv6 variable is dst_exthdrlen; IPv6 exthdrlen
instead holds the IPv6 extension-header length.

With this, exthdrlen no longer contributes to skb->len or to the next
iteration's fraggap. When fraggap is nonzero, it is bounded by
mtu - maxfraglen <= 7, the alignment slop that the relay was designed for.

For the same inputs to the skb setup block, the allocation request and
tail offset are unchanged (alloc_extra already carries exthdrlen).
The network and transport header offsets from skb->head are unchanged
as well. Excluding exthdrlen from skb->len intentionally changes the
subsequent fragmentation calculations. When exthdrlen is zero, these
skb setup changes have no effect.

Fixes: 353e5c9abd90 ("ipv4: Fix IPsec slowpath fragmentation problem")
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Reviewed-by: Willem de Bruijn <willemb@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Anakin Zhang <tencentos_secure@tencent.com>
---
v2:
- Correct the too-strong skb->len/fraggap wording.
- Clarify the IPv6 dst_exthdrlen terminology.
- Describe the skb_shared_info corruption without claiming a write to
  an adjacent slab object; qualify the local layout comparison.
- Add Willem's Reviewed-by; no code changes.

 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 v2 2/2] net/ipv4: reject appends with negative fragment copy

[Willem de Bruijn]

tencentos_secure(TencentOS安全公共邮箱) wrote:
> __ip_append_data() caps datalen at maxfraglen - fragheaderlen when more
> fragments are needed, and later computes
> 
> 	copy = datalen - transhdrlen - fraggap - pagedlen;
> 
> If transhdrlen + fraggap exceeds datalen, copy goes negative in the
> non-paged allocation branches, where pagedlen is zero, invalidating
> the offset/length accounting. In the paged allocation branch,
> pagedlen = datalen - transhdrlen - fraggap wraps unsigned instead.
> The subsequent copy calculation cancels to zero, so checking copy
> alone does not catch this invalid fragment geometry.
> 
> This is reachable independently of the xfrm fraggap bug fixed by the
> preceding patch. icmp_timestamp() passes a 20-byte transport header.
> With a small MTU and IPv4 options, the capped datalen can be smaller
> than that header even when fraggap is zero. The negative copy then
> propagates a negative offset into icmp_glue_bits(), and a timestamp
> reply with corrupted timestamp fields can be emitted.
> 
> For example, with MTU 68 and 40 bytes of IPv4 options, fragheaderlen
> is 60, maxfraglen is 68, datalen is capped at 8, and fraggap is zero,
> so the 20-byte transport header cannot fit.
> 
> Reject this geometry before allocation with -EINVAL, covering all three
> alloclen branches and matching the existing __ip6_append_data() guard.
> A negative copy cannot produce a valid append, so this does not reject a
> send that could otherwise construct a correct packet. 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: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Anakin Zhang <tencentos_secure@tencent.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
> v2:
> - Document the independently reachable ICMP Timestamp corruption path.
> - Distinguish negative copy in non-paged allocation branches from
>   unsigned pagedlen wrap in the paged allocation branch.
> - Update the subject and Fixes tag accordingly.
> - Drop the unrelated xfrm Reported-by tag; no code changes.
> 
>  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;

nit: EMSGSIZE might be better

> +				goto error;
> +			}
> +