[PATCH net v4] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[PATCH net v4] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[bo cao]

The second half of the MTU guard in __ip6_append_data() has been dead
code since the initial git import:

	if (mtu <= fragheaderlen ||
	    ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr))
		goto emsgsize;

(x & ~7) is unsigned and >= 0, and fragheaderlen is at least
sizeof(struct ipv6hdr) = 40 > sizeof(struct frag_hdr) = 8, so the
second condition can never be true.

For mtu - fragheaderlen in [1,7], (x & ~7) == 0, so

	maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen - 8

underflows below fragheaderlen.  Every later bound computed as
maxfraglen - fragheaderlen then wraps to 0xfffffff8, in particular the
datalen clamp inside the fragment loop, which never fires and lets
fraglen = datalen + fragheaderlen escape the MTU constraint.

With MSG_MORE on an egress device without NETIF_F_SG, alloclen = mtu
while skb_put() is called with the unclamped fraglen, so a single
sendmsg() on a raw IPv6 socket with a 1240-byte HOPOPTS cmsg and
IPV6_MTU = fragheaderlen + 1 triggers:

  skbuff: skb_over_panic: len:61280 put:61280 ...
  kernel BUG at net/core/skbuff.c
  skb_put __ip6_append_data ip6_append_data rawv6_sendmsg

The same re-lowering exists with a transport-mode xfrm bundle: ESP/AH
bytes counted in rt->dst.header_len are not part of fragheaderlen
(ESP lacks XFRM_TYPE_NON_FRAGMENT), and ip6_append_data_mtu()
subtracts dst.header_len from mtu for the first fragment, so after
the entry guard passes, the recomputed maxfraglen can be pushed below
fragheaderlen the same way.

Remove the dead expression and, when the datagram actually has to be
fragmented, require room for at least one 8-octet payload block after
the fragment header, matching the minimum ip6_fragment() already
enforces once skb->len > mtu.  The check is placed after the dontfrag
and maxnonfragsize handling so that unfragmented datagrams that fit
within mtu, and the dontfrag PMTU notification path, keep their
current behaviour.

reserved_headerlen accounts for the first fragment's xfrm header
space regardless of whether this append is the first one on the cork:
a subsequent append that pushes an initially unfragmented datagram
past mtu still fragments, and ip6_append_data_mtu() subtracts
dst.header_len from mtu for the first fragment.  The check runs
before any zerocopy uarg is allocated, so the emsgsize path needs no
cleanup.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Suggested-by: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Link: https://lore.kernel.org/netdev/willemdebruijn.kernel.22a3131380a8c@gmail.com/
Signed-off-by: bo cao <treebacker@foxmail.com>
---
Changes in v2: move the check into ip6_append_data_mtu() (review of v1).
Changes in v3: collapse the fix into a single entry check, leaving
ip6_append_data_mtu() unchanged (review of v2).
Changes in v4: gate the check on fragmentation actually being needed
and place it after the dontfrag / maxnonfragsize handling, so valid
unfragmented datagrams and the dontfrag PMTU notification path are not
affected; include dst.header_len in reserved_headerlen regardless of
!skb, so a subsequent corked append that fragments still accounts for
the first fragment's xfrm header space (review of v3).
---
 net/ipv6/ip6_output.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 25fe0ba98b1a..4353b154f216 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1477,7 +1477,7 @@ static int __ip6_append_data(struct sock *sk,
 	bool paged, hold_tskey = false, extra_uref = false;
 	struct ipv6_txoptions *opt = v6_cork->opt;
 	int csummode = CHECKSUM_NONE;
-	unsigned int maxnonfragsize, headersize;
+	unsigned int maxnonfragsize, headersize, reserved_headerlen;
 	unsigned int wmem_alloc_delta = 0;
 
 	skb = skb_peek_tail(queue);
@@ -1499,8 +1499,7 @@ static int __ip6_append_data(struct sock *sk,
 		     (opt ? opt->opt_flen + opt->opt_nflen : 0) +
 		     rt->rt6i_nfheader_len;
 
-	if (mtu <= fragheaderlen ||
-	    ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr))
+	if (mtu <= fragheaderlen)
 		goto emsgsize;
 
 	maxfraglen = ((mtu - fragheaderlen) & ~7) + fragheaderlen -
@@ -1533,6 +1532,29 @@ static int __ip6_append_data(struct sock *sk,
 		return -EMSGSIZE;
 	}
 
+	/*
+	 * If the datagram has to be fragmented, each non-final fragment
+	 * must leave room for at least 8 bytes of payload after the
+	 * fragment header, matching the minimum ip6_fragment() enforces
+	 * once skb->len > mtu.  This rejects the MTUs that make maxfraglen
+	 * underflow below fragheaderlen and defeat the datalen clamp,
+	 * which leads to skb_over_panic() from skb_put().
+	 *
+	 * reserved_headerlen accounts for the first fragment's xfrm header
+	 * space (with a transport-mode xfrm bundle, ESP/AH bytes counted
+	 * in dst.header_len are not part of fragheaderlen), regardless of
+	 * whether this append is the first one on the cork: a subsequent
+	 * append that pushes an initially unfragmented datagram past mtu
+	 * still fragments, and ip6_append_data_mtu() subtracts
+	 * dst.header_len from mtu for the first fragment.
+	 */
+	reserved_headerlen = fragheaderlen;
+	if (!(rt->dst.flags & DST_XFRM_TUNNEL))
+		reserved_headerlen += rt->dst.header_len;
+	if (reserved_headerlen + cork->length + length > mtu &&
+	    mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8)
+		goto emsgsize;
+
 	/* CHECKSUM_PARTIAL only with no extension headers and when
 	 * we are not going to fragment
 	 */
-- 
2.39.5 (Apple Git-154)

Re: [PATCH net v4] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net v4] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[bo cao]

Apologies -- this information was already provided in the v3 thread,
and it was my oversight not to carry it into the v4 commit message as
the bot had asked.  It will be included in the commit message of any
future revision.

As replied on the v3 thread:

The issue was found during an LLM-assisted source audit of the IPv6
fragmentation path (manual code inspection with an LLM's help, not a
production hit or a syzbot report).

It was then verified at runtime with a standalone reproducer: a
single sendmsg() on a raw IPv6 socket with a 1240-byte HOPOPTS cmsg
and IPV6_MTU set to fragheaderlen + 1 triggers the skb_over_panic()
described in the commit message (tested on 6.x, MSG_MORE on a device
without NETIF_F_SG).  The xfrm transport-mode variant from the v2
discussion was identified the same way, by inspection of the
ip6_append_data_mtu() recompute path.

bo cao

Re: [PATCH net v4] ipv6: fix dead half of the MTU guard in __ip6_append_data()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] ipv6: incomplete xfrm mtu check in __ip6_append_data()
  underflows pagedlen
- [Medium] The new fragmentation predicate in __ip6_append_data()
  (net/ipv6/ip6_output.c:1551-1556) counts non-fragmentable XFRM header…
- [Low] The new block comment in __ip6_append_data()
  (net/ipv6/ip6_output.c, lines ~1543-1549), repeated in the commit…

--

ipv6: fix dead half of the MTU guard in __ip6_append_data()

This patch removes the second half of the MTU guard in
__ip6_append_data(), which could never be true. It adds a new check: if
the datagram has to be fragmented and the MTU cannot hold the fragment
header plus one 8-octet payload block, the send fails with -EMSGSIZE.
For non-tunnel xfrm bundles the check also counts rt->dst.header_len.

Link: https://lore.kernel.org/netdev/willemdebruijn.kernel.22a3131380a8c@gmail.com/

> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a6..4353b154f216f 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c

[ ... ]

> @@ -1533,6 +1532,29 @@ static int __ip6_append_data(struct sock *sk,
>  		return -EMSGSIZE;
>  	}
>  
> +	/*
> +	 * If the datagram has to be fragmented, each non-final fragment
> +	 * must leave room for at least 8 bytes of payload after the
> +	 * fragment header, matching the minimum ip6_fragment() enforces
> +	 * once skb->len > mtu.  This rejects the MTUs that make maxfraglen
> +	 * underflow below fragheaderlen and defeat the datalen clamp,
> +	 * which leads to skb_over_panic() from skb_put().
> +	 *
> +	 * reserved_headerlen accounts for the first fragment's xfrm header
> +	 * space (with a transport-mode xfrm bundle, ESP/AH bytes counted
> +	 * in dst.header_len are not part of fragheaderlen), regardless of
> +	 * whether this append is the first one on the cork: a subsequent
> +	 * append that pushes an initially unfragmented datagram past mtu
> +	 * still fragments, and ip6_append_data_mtu() subtracts
> +	 * dst.header_len from mtu for the first fragment.
> +	 */

[Severity: Low]
Is this description accurate for a subsequent append? The commit message
gives the same rationale.

On a subsequent corked append, skb_peek_tail(queue) returns the skb that
is already queued, so skb is non-NULL. When the loop reaches
alloc_new_skb with skb_prev == NULL, it passes that skb to
ip6_append_data_mtu():

			if (!skb || !skb_prev)
				ip6_append_data_mtu(&mtu, &maxfraglen,
						    fragheaderlen, skb, rt,
						    orig_mtu);

ip6_append_data_mtu() then takes the non-first-fragment branch:

net/ipv6/ip6_output.c:ip6_append_data_mtu() {
		if (!skb) {
			/* first fragment, reserve header_len */
			*mtu = orig_mtu - rt->dst.header_len;
		} else {
			...
			*mtu = orig_mtu;
		}
}

So on this path no dst.header_len is reserved for the first fragment. The
first skb is filled and trimmed at the maxfraglen computed from orig_mtu.

Take a non-tunnel xfrm bundle with:

  fragheaderlen + 16 <= mtu < fragheaderlen + dst.header_len + 16

Does the new check return -EMSGSIZE on subsequent appends in that range,
even though maxfraglen would not underflow there?

Could the comment and the commit message be updated to match what the
code does? Alternatively, the check could be adjusted for subsequent
appends.

> +	reserved_headerlen = fragheaderlen;
> +	if (!(rt->dst.flags & DST_XFRM_TUNNEL))
> +		reserved_headerlen += rt->dst.header_len;
> +	if (reserved_headerlen + cork->length + length > mtu &&
> +	    mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8)
> +		goto emsgsize;

[Severity: High]
Does reserved_headerlen cover everything that ip6_append_data_mtu()
takes away from the first fragment?

The new minimum counts only fragheaderlen and rt->dst.header_len. It
does not count transhdrlen (the L4 header plus opt->opt_flen on the
first append) or rt->dst.trailer_len. After ip6_append_data_mtu()
lowers mtu by rt->dst.header_len for the first fragment, the loop
clamps datalen to maxfraglen - fragheaderlen - rt->dst.trailer_len.
That value can still be smaller than transhdrlen.

Here is a concrete case. Take a transport-mode AH bundle with
dst.header_len = 24 and trailer_len = 0, 1200 bytes of HOPOPTS
(fragheaderlen = 1240), an 8-byte fragmentable dstopt (opt_flen = 8,
so transhdrlen = 16 for UDP), mtu 1280 and a 2000-byte send. Both
entry checks pass:

  headersize + transhdrlen = 1264, not > 1280
  mtu < reserved_headerlen + 16 is 1280 < 1280, false

ip6_append_data_mtu() then sets mtu to 1256 and maxfraglen to 1248,
so datalen is clamped to 8, which is below transhdrlen.

On the paged branch (for example MSG_SPLICE_PAGES on a NETIF_F_SG
device), pagedlen = datalen - transhdrlen - fraggap wraps to
0xfffffff8. The wrap cancels out in copy = datalen - transhdrlen -
fraggap - pagedlen, so copy is 0 and the copy < 0 check does not
fire. skb_put(skb, fraglen - pagedlen) then puts 1256 bytes, which
is past maxfraglen.

On the next fragment, fraggap is computed as 1256 - 1248 = 8 before
ip6_append_data_mtu() restores maxfraglen to 1272. Then
skb_copy_and_csum_bits(skb_prev, maxfraglen, ...) reads at offset
1272 from a 1256-byte skb_prev. That hits the DEBUG_NET_WARN_ON_ONCE()
in skb_copy_and_csum_bits() and puts garbage into the second
fragment.

On the non-paged branch the same input reaches copy < 0 and fails
with -EINVAL partway through the loop, instead of taking the emsgsize
path. With ESP and dst.trailer_len > 8, the datalen clamp itself
wraps.

The commit message says this patch also covers the transport-mode
xfrm re-lowering. Should the minimum also include transhdrlen and
rt->dst.trailer_len, or is there a better place to bound this?

Separately, the headersize + transhdrlen > mtu check, the dontfrag
check and the CHECKSUM_PARTIAL and MSG_ZEROCOPY decisions all compare
against the unreduced mtu. A UDPv6 datagram that only needs
fragmenting after ip6_append_data_mtu() subtracts dst.header_len can
still get CHECKSUM_PARTIAL or zerocopy, and it skips the dontfrag
path. Is that intended?

[Severity: Medium]
Does this count rt6i_nfheader_len twice?

fragheaderlen already includes it:

	fragheaderlen = sizeof(struct ipv6hdr) + rt->rt6i_nfheader_len +
			(opt ? opt->opt_nflen : 0);

xfrm_bundle_create() adds XFRM_TYPE_NON_FRAGMENT states to both
dst.header_len and nfheader_len:

net/xfrm/xfrm_policy.c:xfrm_bundle_create() {
	...
		header_len += xfrm[i]->props.header_len;
		if (xfrm[i]->type->flags & XFRM_TYPE_NON_FRAGMENT)
			nfheader_len += xfrm[i]->props.header_len;
	...
}

__ip6_append_data() itself subtracts the overlap elsewhere:

		dst_exthdrlen = rt->dst.header_len - rt->rt6i_nfheader_len;

Here is a concrete case. Take a Mobile IPv6 route-optimization destopt
state. It has header_len = nfheader_len = 24 and is not DST_XFRM_TUNNEL.
Add mtu 1280 and 1184 bytes of HOPOPTS. Then fragheaderlen is 1248 and
reserved_headerlen is 1272.

A corked UDP send of 0 bytes with MSG_MORE passes the new check (1272 + 8
is not > 1280) and queues a 1256-byte skb.

Appending 1 byte then meets both new conditions (1281 > 1280 and
1280 < 1288), so it returns -EMSGSIZE and calls ipv6_local_error().

Before this patch, the existing-skb path appended the byte
(copy = 1280 - 1256 = 24). After __ip6_make_skb() pulls fragheaderlen
and the mip6 type inserts its 24-byte header, the wire size is
40 + 1184 + 24 + 8 + 1 = 1257, which fits within mtu.

Is this a newly rejected datagram that never needed fragmentation? The
patch is tagged for stable.

On a first append, the existing ip6_append_data_mtu() computation counts
nfheader_len twice in the same way, so such a send already fails with
-EINVAL. The new behavior change applies only to subsequent corked
appends.

Would it make sense to add only rt->dst.header_len - rt->rt6i_nfheader_len
here, as dst_exthdrlen does?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/tencent_0A4B784EEB3D358B09B43AA2B2F13AE44505%40qq.com