netdev - Re: [PATCH net] ip_gre: test csum_start instead of transport header

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5c3f80585b414afb4b6bd49f46bcec77c1ba5fc8.camel@gmail.com>
Date:   Wed, 08 Jun 2022 07:54:48 -0700
From:   Alexander H Duyck <alexander.duyck@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, kuba@...nel.org, edumazet@...gle.com,
        pabeni@...hat.com, Willem de Bruijn <willemb@...gle.com>,
        syzbot <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] ip_gre: test csum_start instead of transport header

On Mon, 2022-06-06 at 09:21 -0400, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
> 
> GRE with TUNNEL_CSUM will apply local checksum offload on
> CHECKSUM_PARTIAL packets.
> 
> ipgre_xmit must validate csum_start after an optional skb_pull,
> else lco_csum may trigger an overflow. The original check was
> 
> 	if (csum && skb_checksum_start(skb) < skb->data)
> 		return -EINVAL;
> 
> This had false positives when skb_checksum_start is undefined:
> when ip_summed is not CHECKSUM_PARTIAL. A discussed refinement
> was straightforward
> 
> 	if (csum && skb->ip_summed == CHECKSUM_PARTIAL &&
> 	    skb_checksum_start(skb) < skb->data)
> 		return -EINVAL;
> 
> But was eventually revised more thoroughly:
> - restrict the check to the only branch where needed, in an
>   uncommon GRE path that uses header_ops and calls skb_pull.
> - test skb_transport_header, which is set along with csum_start
>   in skb_partial_csum_set in the normal header_ops datapath.
> 
> Turns out skbs can arrive in this branch without the transport
> header set, e.g., through BPF redirection.
> 
> Revise the check back to check csum_start directly, and only if
> CHECKSUM_PARTIAL. Do leave the check in the updated location.
> Check field regardless of whether TUNNEL_CSUM is configured.
> 
> Link: https://lore.kernel.org/netdev/YS+h%2FtqCJJiQei+W@shredder/
> Link: https://lore.kernel.org/all/20210902193447.94039-2-willemdebruijn.kernel@gmail.com/T/#u
> Fixes: 8a0ed250f911 ("ip_gre: validate csum_start only on pull")
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
>  net/ipv4/ip_gre.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/net/ipv4/ip_gre.c b/net/ipv4/ip_gre.c
> index 7e474a85deaf..3b9cd487075a 100644
> --- a/net/ipv4/ip_gre.c
> +++ b/net/ipv4/ip_gre.c
> @@ -629,21 +629,20 @@ static netdev_tx_t ipgre_xmit(struct sk_buff *skb,
>  	}
>  
>  	if (dev->header_ops) {
> -		const int pull_len = tunnel->hlen + sizeof(struct iphdr);
> -
>  		if (skb_cow_head(skb, 0))
>  			goto free_skb;
>  
>  		tnl_params = (const struct iphdr *)skb->data;
>  
> -		if (pull_len > skb_transport_offset(skb))
> -			goto free_skb;
> -
>  		/* Pull skb since ip_tunnel_xmit() needs skb->data pointing
>  		 * to gre header.
>  		 */
> -		skb_pull(skb, pull_len);
> +		skb_pull(skb, tunnel->hlen + sizeof(struct iphdr));
>  		skb_reset_mac_header(skb);
> +
> +		if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +		    skb_checksum_start(skb) < skb->data)
> +			goto free_skb;
>  	} else {
>  		if (skb_cow_head(skb, dev->needed_headroom))
>  			goto free_skb;

The one thing I might change would be the ordering of your two tests at
the end. It is more likely that skb_checksum_start will be less than
skb->data in most cases as skb->csum_start is initialized to 0 at
allocation. So cases where it is greater than skb->data should be rare
whereas the CHECKSUM_PARTIAL check will come up as true probably more
often then not as it isn't uncommon to see checksum or TSO offloaded
frames.

Anyway functionality-wise this looks good to me and what I suggested is
more of an optimization anyway.

Reviewed-by: Alexander Duyck <alexanderduyck@...com>