Re: [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress

[Willem de Bruijn]

Michael S. Tsirkin wrote:
> As reported first by Paulos Yibelo <habte.yibelo@xxxxxxxxx> (after which
> others, credited below, found more ways the bug can affect users) a
> virtio net header can place CHECKSUM_PARTIAL at any in-packet offset.
> If that state is retained, later checksum completion can modify the
> packet after protocol processing has made decisions from it.
> 
> Let's fix that: let's validate csum_start and csum_offset and only keep
> CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> headers.  Complete other non-GSO checksums immediately, before any
> protocol processing runs.
> 
> We can't easily "complete the checksum" for GSO we don't recognize - GSO
> assumes knowledge of the protocol. We could segment ignoring the
> checksum then do whatever csum_start and csum_offset say, but that
> seems like an overkill for something that shouldn't happen.
> 
> There's some risk here:
> - non-gso packets which can't be parsed get checksum calculated in
>   software, adding performance overhead
> - gso packets which can't be parsed get dropped
> 
> To mitigate the risks, the following patch will add a sysctl knob to
> bypass the resolution. This is reasonable if the setup is such that
> there is no protocol processing at all.
> 
> UDP tunnel headers get the same handling on the inner header.
> 
> Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> 
> Reported-by: Paulos Yibelo <habte.yibelo@xxxxxxxxx>
> Reported-by: Chris Arges <carges@xxxxxxxxxxxxxx>
> Tested-by: Chris Arges <carges@xxxxxxxxxxxxxx>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Suggested-by: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> ---
>  drivers/net/tap.c          |   8 +-
>  drivers/net/tun.c          |   8 +-
>  drivers/net/virtio_net.c   |   3 +-
>  include/linux/virtio_net.h | 276 ++++++++++++++++++++++++++++++++++++-
>  4 files changed, 281 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/tap.c b/drivers/net/tap.c
> index d7520622e809..f2a9bb443980 100644
> --- a/drivers/net/tap.c
> +++ b/drivers/net/tap.c
> @@ -653,9 +653,9 @@ static ssize_t tap_get_user(struct tap_queue *q, void *msg_control,
>  			rcu_read_unlock();
>  			drop_reason = SKB_DROP_REASON_DEV_HDR;
>  			goto err_kfree;
> -		}
> -		if (err > 0)
> +		} else if (err > 0) {
>  			atomic64_inc(&tap->rx_csum_sw);
> +		}
>  	}
>  
>  	skb_probe_transport_header(skb);
> @@ -1089,9 +1089,9 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
>  		if (err < 0) {
>  			rcu_read_unlock();
>  			goto err_kfree;
> -		}
> -		if (err > 0)
> +		} else if (err > 0) {
>  			atomic64_inc(&tap->rx_csum_sw);
> +		}

All these belong in patch 1 that introduces these branches

>  	}
>  
>  	skb_probe_transport_header(skb);