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

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

平之 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.  Commit 5e34af4142ff ("net: ipv6:
> fix skb_over_panic in __ip6_append_data") tightened the first half
> (mtu == fragheaderlen) but left the dead second half in place.
> 
> 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 ...
> [   37.607856] kernel BUG at net/core/skbuff.c:214!
> [   37.607865] Internal error: Oops - BUG: 00000000f2000800 [#1]  SMP
> ...
> [   37.616606] Call trace:
> [   37.616606]  skb_panic+0x5c/0x60 (P)
> [   37.616778]  skb_put+0x9c/0xa8
> [   37.616952]  __ip6_append_data+0xf80/0x1948
> [   37.617172]  ip6_append_data+0x94/0x2a8
> [   37.617363]  rawv6_sendmsg+0x588/0x988
> [   37.617716]  inet_sendmsg+0x4c/0xa8
> [   37.617891]  ____sys_sendmsg+0x268/0x308
> [   37.618105]  ___sys_sendmsg+0xb4/0x140
> [   37.618299]  __sys_sendmsg+0xa8/0x140
> 
> Replace the dead expression with a direct check of the payload space
> available in a fragment, which rejects mtu - fragheaderlen in [1,8].
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: bo cao <treebacker@foxmail.com>

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

> ---
>  net/ipv6/ip6_output.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
> index 25fe0ba98b1a..7d4d4d45e207 100644
> --- a/net/ipv6/ip6_output.c
> +++ b/net/ipv6/ip6_output.c
> @@ -1500,7 +1500,7 @@ static int __ip6_append_data(struct sock *sk,
>  		     rt->rt6i_nfheader_len;
>  
>  	if (mtu <= fragheaderlen ||
> -	    ((mtu - fragheaderlen) & ~7) + fragheaderlen <= sizeof(struct frag_hdr))
> +	    mtu - fragheaderlen <= sizeof(struct frag_hdr))
>  		goto emsgsize;

Thanks for the patch. Not critical, but could even consider a single check:

    if (mtu <= fragheaderlen + sizeof(struct frag_hdr))

Checkers may complain that the right hand side could overflow, but in
practice fragheaderlen is bounded.

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