Re: [PATCH net] net: extend virtio_net_hdr csum_start checks to VLAN, IPv6 and IP options

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Oct 07, 2026 at 06:38:33PM -0400, Willem de Bruijn wrote:
> On Wed, Oct 7, 2026 at 6:03 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Oct 07, 2026 at 12:36:59PM -0400, Willem de Bruijn wrote:
> > > From: Willem de Bruijn <willemb@google.com>
> > >
> > > __virtio_net_hdr_to_skb() validates hdr->csum_start against nh_min_len:
> > >
> > >     if (skb_transport_offset(skb) < nh_min_len)
> > >         return -EINVAL;
> > >
> > > Extend the check to account for the link layer header including VLAN
> > > tags, IPv4 options, and IPv6 other than VIRTIO_NET_HDR_GSO_TCPV6.
> > >
> > > Payload, gso_type and skb->protocol can come from userspace, so cannot
> > > be trusted to be consistent, or correct.
> > >
> > > Therefore:
> > > - For Ethernet packets (ARPHRD_ETHER), parse from ETH_HLEN and
> > >   eth_hdr(skb)->h_proto, advancing past any VLAN tags with
> > >   __vlan_get_protocol().
> > > - For non-Ethernet packets, use skb_network_offset(skb) as nhoff and
> > >   infer the L3 protocol from iph->version at skb->data + nhoff.
> > > - If skb->protocol is set and disagrees with the protocol parsed from
> > >   the packet, enforce the minimum header length of both.
> > > - For non-IP protocols, require only nhoff + nh_min_len. No in-tree
> > >   non-IP protocol generates CHECKSUM_PARTIAL itself. They only carry it
> > >   when encapsulating IP (e.g., MPLS), in which case csum_start lies
> > >   beyond an inner IP header.
> > >
> > > Reported-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > Link: https://lore.kernel.org/netdev/20260922030310.8684-2-habte.yibelo@gmail.com/
> > > Fixes: 49d14b54a527 ("net: test for not too small csum_start in virtio_net_hdr_to_skb()")
> > > Co-developed-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > Signed-off-by: Paulos Yibelo <habte.yibelo@gmail.com>
> > > Signed-off-by: Willem de Bruijn <willemb@google.com>
> >
> > This doesn't fix all the issues, or does it?
> 
> It does not.
> 
> > If not, I am confused why we
> > are doing this piecemeal approach. I thought we agreed to
> >
> > a. validate some basic things about the checksum in the core ip stack
> >
> > b. in virtio, validate specific checksum values
> >    and for anything else, fill in the checksum and fragment then
> >    and there
> 
> I understood differently. That the fixes are cumulative, addressing
> different packet types.
> 
> I like your approach of doing checksum calculation early, in
> __virtio_net_hdr_to_skb. I figured that was a follow-up targeting
> net-next, eventually.
> 
> I can drop this if you want to send that instead.

Yes it's net-next material I feel. I don't think I can implement that
today if that is the question. But we can backport later.

But I also am not excited about very clearly UAPI-visible changes
landing in net at the last moment like this.

To me, it seems highly likely someone has a minor
bug in userspace and previously it would just make a bad checksum
and it is dropped, and now it is suddenly failing.

And if we pile up change upon change then what? Do we commit to erroring
out on bad packets? On specific bad packets but not others?  It's UAPI.
If we are not in a terrible rush, I'd rather we did it carefully and
fully.

-- 
MST