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

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

bo cao wrote:
> 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.
> 
> Replace the dead expression with a single check that requires room
> for at least one 8-octet payload block after the fragment header,
> matching the minimum ip6_fragment() already enforces:
> 
> 	reserved_headerlen = fragheaderlen;
> 	if (!skb && !(rt->dst.flags & DST_XFRM_TUNNEL))
> 		reserved_headerlen += rt->dst.header_len;
> 
> 	if (mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8)
> 		goto emsgsize;
> 
> For the transport-mode xfrm case, pre-reserving dst.header_len in the
> entry check makes the later ip6_append_data_mtu() recompute safe: the
> subtracted mtu still satisfies the same minimum, so the recomputed
> maxfraglen cannot drop below fragheaderlen either.  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
> Signed-off-by: bo cao <treebacker@foxmail.com>
> ---
> Changes in v3: collapse the fix into the entry check of
> __ip6_append_data() and leave ip6_append_data_mtu() unchanged, as
> suggested in review of v2; this also removes the emsgsize cleanup
> hazard for the zerocopy uarg and the separate maxfraglen check.
> ---
>  net/ipv6/ip6_output.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a..936dad85f1c9 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,11 @@ 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))
> +	reserved_headerlen = fragheaderlen;
> +	if (!skb && !(rt->dst.flags & DST_XFRM_TUNNEL))
> +		reserved_headerlen += rt->dst.header_len;

    ip6_fragment() only enforces `mtu >= hlen + sizeof(struct frag_hdr) + 8`
    once `skb->len > mtu`. Checking it unconditionally here rejects valid
    unfragmented datagrams that fit within `mtu` and never carry a fragment
    header (even with IPV6_DONTFRAG set)

    Placing this before the `v6_cork->dontfrag` check below also skips
    ipv6_local_rxpmtu() when `dontfrag` is set and the payload exceeds `mtu`
    while `mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8`.

    If this guard is moved below the `dontfrag` / `maxnonfragsize` checks and
    gated on fragmentation actually being needed (`reserved_headerlen +
    cork->length + length > mtu`), `reserved_headerlen` should also include
    `rt->dst.header_len` regardless of `!skb`, so a subsequent corked append
    (`skb != NULL`) that pushes an initially unfragmented datagram past `mtu`
    still accounts for the first fragment's xfrm header space.

> +
> +	if (mtu < reserved_headerlen + sizeof(struct frag_hdr) + 8)
>  		goto emsgsize;