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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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] The patch adds a failure exit inside the fragmentation loop of
  __ip6_append_data() (net/ipv6/ip6_output.c:1662-1666).
- [Medium] The new entry guard in __ip6_append_data() counts the fragment
  header twice, and it disagrees with the patch's other new check in…
- [Medium] Both new checks reject a datagram whether or not it actually
  needs fragmenting.
- [Low] The commit message has several defects: - The subject line is
  repeated as the first line of the body.

--

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

This patch replaces the always-false second half of the MTU guard in
__ip6_append_data() with a check that requires payload room after the
fragment header. ip6_append_data_mtu() now returns false when the
first-fragment recompute (which subtracts dst.header_len) falls below a
similar bound. When that happens, the fragmentation loop jumps to the
emsgsize label.

Link: https://lore.kernel.org/netdev/179321375161.3134.12690504630171635307@kernel.org/

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

[Severity: Low]
This isn't a bug, but the subject line is repeated as the first line of
the body.

> __ip6_append_data() validates the path MTU against fragheaderlen
> before the fragmentation loop, but the second half of the guard is
> always false: it compares (mtu - fragheaderlen) + fragheaderlen
> against sizeof(struct frag_hdr) (8), while fragheaderlen is at least
> 1280 for any packet carrying a Hop-by-Hop options header of 1240
> bytes.

The old second half is false for every packet once mtu > fragheaderlen.
fragheaderlen is always at least sizeof(struct ipv6hdr) (40), which is
larger than sizeof(struct frag_hdr) (8).

Could the explanation say that directly, instead of citing a 1240 byte
Hop-by-Hop header? The specific example suggests the guard might be live
for other header sizes.

> An IPV6_MTU socket option that makes mtu - fragheaderlen
> underflow below fragheaderlen therefore passes the guard

This phrase reads as garbled. Isn't it maxfraglen that drops below
fragheaderlen or wraps?

Also, the Link: tag sits below the --- separator with the v1 changelog,
so git am would drop it.

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

[ ... ]

> @@ -1357,8 +1357,20 @@ static void ip6_append_data_mtu(unsigned int *mtu,
>  	if (!(rt->dst.flags & DST_XFRM_TUNNEL)) {
>  		if (!skb) {
>  			/* first fragment, reserve header_len */
> -			*mtu = orig_mtu - rt->dst.header_len;
> +			int new_mtu = (int)orig_mtu - (int)rt->dst.header_len;
>
> +			/*

[ ... ]

> +			 */
> +			if (new_mtu < (int)(fragheaderlen +
> +					    sizeof(struct frag_hdr) + 8))
> +				return false;
> +			*mtu = new_mtu;

[ ... ]

> @@ -1499,13 +1513,23 @@ 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 -
>  		     sizeof(struct frag_hdr);
>
> +	/*
> +	 * Each non-final fragment must leave room for at least 8 bytes of
> +	 * payload after the fragment header, matching ip6_fragment()'s
> +	 * mtu < hlen + sizeof(struct frag_hdr) + 8 check.  This rejects
> +	 * MTUs that make maxfraglen underflow below fragheaderlen and
> +	 * defeat the datalen clamp, which leads to skb_over_panic() from
> +	 * skb_put().
> +	 */
> +	if (maxfraglen < fragheaderlen + sizeof(struct frag_hdr) + 8)
> +		goto emsgsize;

[Severity: Medium]
Is the fragment header being counted twice here? maxfraglen already has
sizeof(struct frag_hdr) subtracted:

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

So maxfraglen < fragheaderlen + 8 + 8 is the same as
((mtu - fragheaderlen) & ~7) < 24. That requires 16 bytes of payload per
non-final fragment, not the 8 that the comment describes.

ip6_fragment() only requires mtu - hlen >= 16:

    if (mtu < hlen + sizeof(struct frag_hdr) + 8)
        goto fail_toobig;

The new check in ip6_append_data_mtu() compares mtu rather than
maxfraglen, so it also only requires 16. It accepts
new_mtu == fragheaderlen + 16 and then sets
maxfraglen = fragheaderlen + 8, a value this entry guard rejects.

That doesn't seem to match the commit message's statement that the same
bound applies "both at cork setup and after every recompute in
ip6_append_data_mtu()".

As a result, sends with mtu - fragheaderlen in [16, 23] now fail with
-EMSGSIZE. Before this patch they produced valid fragments with 8 bytes
of payload. One example is a fragheaderlen of 1264 with an MTU of 1280,
which an unprivileged user can reach with a large IPV6_RTHDR SRH.

Could both sites use the same formula? For example, compare mtu against
fragheaderlen + sizeof(struct frag_hdr) + 8, or compare maxfraglen
against fragheaderlen + 8.

[Severity: Medium]
Does this also reject datagrams that don't need fragmenting at all? The
check runs before anything is known about the datagram length, so any
send with mtu - fragheaderlen < 24 gets -EMSGSIZE.

For example, take mtu - fragheaderlen = 20, opt_flen = 0, and a UDP send
with 4 bytes of data (12 bytes including the UDP header). The old code
computed maxfraglen = fragheaderlen + 8. cork->length (12) <= mtu, so
the datalen clamp was skipped. The datagram went out as one unfragmented
packet of fragheaderlen + 12 bytes.

With this patch that send fails with -EMSGSIZE.

The new check in ip6_append_data_mtu() behaves the same way. On a
transport-mode xfrm route it rejects the first skb whenever
orig_mtu - dst.header_len - fragheaderlen < 16, even if the whole
datagram fits.

The skb_over_panic() only happens on the fragmenting path, when this
clamp is applied:

    if (datalen > (cork->length <= mtu ? mtu : maxfraglen) - fragheaderlen)
        datalen = maxfraglen - fragheaderlen - rt->dst.trailer_len;

ip6_fragment() likewise only applies its bound when it actually
fragments.

Should these checks only apply when the datagram needs fragmenting? If
not, could the commit message mention that single-packet sends in this
range are now refused?

> +
>  	/* as per RFC 7112 section 5, the entire IPv6 Header Chain must fit
>  	 * the first fragment
>  	 */
> @@ -1635,10 +1659,11 @@ static int __ip6_append_data(struct sock *sk,
>  			else
>  				fraggap = 0;
>  			/* update mtu and maxfraglen if necessary */
> -			if (!skb || !skb_prev)
> -				ip6_append_data_mtu(&mtu, &maxfraglen,
> -						    fragheaderlen, skb, rt,
> -						    orig_mtu);
> +			if ((!skb || !skb_prev) &&
> +			    !ip6_append_data_mtu(&mtu, &maxfraglen,
> +						 fragheaderlen, skb, rt,
> +						 orig_mtu))
> +				goto emsgsize;

[Severity: High]
Can this leak the zerocopy uarg? ip6_append_data_mtu() only returns
false in the !skb branch. That means this exit is reached through
"if (!skb) goto alloc_new_skb", after __ip6_append_data() has already
done its setup:

    } else if (sock_flag(sk, SOCK_ZEROCOPY)) {
        uarg = msg_zerocopy_realloc(sk, length, skb_zcopy(skb),
                                    false);
        ...
        extra_uref = !skb_zcopy(skb);
        ...
            skb_zcopy_set(skb, uarg, &extra_uref);
    ...
        tskey = atomic_inc_return(&sk->sk_tskey) - 1;
        hold_tskey = true;
    ...
    cork->length += length;
    if (!skb)
        goto alloc_new_skb;

The emsgsize label only reports the error and returns:

    emsgsize:
        pmtu = max_t(int, mtu - headersize + sizeof(struct ipv6hdr), 0);
        ipv6_local_error(sk, EMSGSIZE, fl6, pmtu);
        return -EMSGSIZE;

The cleanup lives under the error label:

    error:
        net_zcopy_put_abort(uarg, extra_uref);
        cork->length -= length;
        ...
        if (hold_tskey)
            atomic_dec(&sk->sk_tskey);

With skb == NULL, skb_zcopy_set() does nothing, so no skb owns the new
uarg from msg_zerocopy_alloc(). Its sock_hold(sk), its optmem charge and
its locked_vm accounting would never be released. sk_zckey and sk_tskey
would be left with gaps, and cork->length would stay inflated.

One way to trigger this is a UDPv6 MSG_ZEROCOPY send on a route covered
by a transport-mode SA. The non-fragmentable headers have to be sized so
that orig_mtu - dst.header_len < fragheaderlen + 16, for example with an
IPV6_RTHDR SRH, which needs no capability. That is the same case this
hunk is meant to handle.

The pmtu reported here also looks off. *mtu is only written after the
new check passes, so the value given to ipv6_local_error() is computed
from orig_mtu and ignores dst.header_len. A sender that follows that
hint would fail again.

Would it work to set err = -EMSGSIZE, call ipv6_local_error() and then
goto error here? Alternatively, the first-fragment header_len check
could run before the uarg and tskey setup, since all of its inputs are
known at that point.

>
>  			skb_prev = skb;
>

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