Michael S. Tsirkin wrote: > As reported first by Paulos Yibelo <[email protected]> (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 <[email protected]> > Reported-by: Chris Arges <[email protected]> > Tested-by: Chris Arges <[email protected]> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > Suggested-by: Willem de Bruijn <[email protected]> > Signed-off-by: Michael S. Tsirkin <[email protected]> > --- > 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);
