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

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

[Willem de Bruijn]

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>
---
 include/linux/virtio_net.h | 48 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
index d6466f96cdd0..6a30f58d9d65 100644
--- a/include/linux/virtio_net.h
+++ b/include/linux/virtio_net.h
@@ -48,6 +48,52 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
 	return 0;
 }
 
+static inline bool virtio_net_hdr_thoff_valid(const struct sk_buff *skb,
+					      unsigned int nh_min_len)
+{
+	int thoff = skb_transport_offset(skb);
+	const struct iphdr *iph;
+	__be16 proto = 0;
+	int nhoff;
+
+	DEBUG_NET_WARN_ON_ONCE(skb->mac_len);
+
+	if (skb->dev->type == ARPHRD_ETHER) {
+		if (unlikely(thoff < ETH_HLEN))
+			return false;
+		nhoff = ETH_HLEN;
+		proto = eth_hdr(skb)->h_proto;
+		if (eth_type_vlan(proto)) {
+			proto = __vlan_get_protocol(skb, proto, &nhoff);
+			if (!proto)
+				return false;
+		}
+	} else {
+		nhoff = skb_network_offset(skb);
+	}
+
+	if (unlikely(thoff < nhoff + nh_min_len))
+		return false;
+
+	iph = (const void *)(skb->data + nhoff);
+	if (!proto) {
+		if (iph->version == 4)
+			proto = htons(ETH_P_IP);
+		else if (iph->version == 6)
+			proto = htons(ETH_P_IPV6);
+	}
+
+	if (proto == htons(ETH_P_IP) || skb->protocol == htons(ETH_P_IP)) {
+		if (unlikely(iph->ihl < 5))
+			return false;
+		nh_min_len = max_t(u32, iph->ihl * 4, nh_min_len);
+	}
+	if (proto == htons(ETH_P_IPV6) || skb->protocol == htons(ETH_P_IPV6))
+		nh_min_len = max_t(u32, sizeof(struct ipv6hdr), nh_min_len);
+
+	return thoff >= nhoff + nh_min_len;
+}
+
 static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 					  const struct virtio_net_hdr *hdr,
 					  bool little_endian, u8 hdr_gso_type)
@@ -104,7 +150,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
 
 		if (!skb_partial_csum_set(skb, start, off))
 			return -EINVAL;
-		if (skb_transport_offset(skb) < nh_min_len)
+		if (!virtio_net_hdr_thoff_valid(skb, nh_min_len))
 			return -EINVAL;
 
 		nh_min_len = skb_transport_offset(skb);
-- 
2.56.0.360.g66cac248cb-goog

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Willem de Bruijn]

netdev-bot+sinfo@ wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:

Oh right, I need to learn that.

> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

External report, see Link:. That does not say exactly how they found
this.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

Created and verified with a reproducer. Not sent as an upstream patch,
because the code complexity vs added coverage ratio is too low imho.

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

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

[Michael S. Tsirkin]

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? 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


> ---
>  include/linux/virtio_net.h | 48 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd0..6a30f58d9d65 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,6 +48,52 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static inline bool virtio_net_hdr_thoff_valid(const struct sk_buff *skb,
> +					      unsigned int nh_min_len)
> +{
> +	int thoff = skb_transport_offset(skb);
> +	const struct iphdr *iph;
> +	__be16 proto = 0;
> +	int nhoff;
> +
> +	DEBUG_NET_WARN_ON_ONCE(skb->mac_len);
> +
> +	if (skb->dev->type == ARPHRD_ETHER) {
> +		if (unlikely(thoff < ETH_HLEN))
> +			return false;
> +		nhoff = ETH_HLEN;
> +		proto = eth_hdr(skb)->h_proto;
> +		if (eth_type_vlan(proto)) {
> +			proto = __vlan_get_protocol(skb, proto, &nhoff);
> +			if (!proto)
> +				return false;
> +		}
> +	} else {
> +		nhoff = skb_network_offset(skb);
> +	}
> +
> +	if (unlikely(thoff < nhoff + nh_min_len))
> +		return false;
> +
> +	iph = (const void *)(skb->data + nhoff);
> +	if (!proto) {
> +		if (iph->version == 4)
> +			proto = htons(ETH_P_IP);
> +		else if (iph->version == 6)
> +			proto = htons(ETH_P_IPV6);
> +	}
> +
> +	if (proto == htons(ETH_P_IP) || skb->protocol == htons(ETH_P_IP)) {
> +		if (unlikely(iph->ihl < 5))
> +			return false;
> +		nh_min_len = max_t(u32, iph->ihl * 4, nh_min_len);
> +	}
> +	if (proto == htons(ETH_P_IPV6) || skb->protocol == htons(ETH_P_IPV6))
> +		nh_min_len = max_t(u32, sizeof(struct ipv6hdr), nh_min_len);
> +
> +	return thoff >= nhoff + nh_min_len;
> +}
> +
>  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  					  const struct virtio_net_hdr *hdr,
>  					  bool little_endian, u8 hdr_gso_type)
> @@ -104,7 +150,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  
>  		if (!skb_partial_csum_set(skb, start, off))
>  			return -EINVAL;
> -		if (skb_transport_offset(skb) < nh_min_len)
> +		if (!virtio_net_hdr_thoff_valid(skb, nh_min_len))
>  			return -EINVAL;
>  
>  		nh_min_len = skb_transport_offset(skb);
> -- 
> 2.56.0.360.g66cac248cb-goog

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

[Willem de Bruijn]

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.

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

[Michael S. Tsirkin]

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

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

[Willem de Bruijn]

On Wed, Oct 7, 2026 at 6:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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.

Would it be better if we pass the packets along, but strip the
checksum offload request?

That is not feasible for GSO packets, only, as checksum offload
there is implied.

In many cases, it may be that the software gso stack only validates
the packets, then drops SKB_GSO_DODGY and still sends them
on for HW TSO (see NETIF_F_GSO_ROBUST in tcp_gso_segment).
But we cannot rely on that always being the case (e.g., if TSO is off).

> 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.

My fingers _are_ itching to drop all of them.. but you're right: I
understand why we need to be lenient.





> If we are not in a terrible rush, I'd rather we did it carefully and
> fully.
>
> --
> MST
>

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix leaves the UDP-tunnel path, virtio_net_hdr_tnl_to_skb()
  in include/linux/virtio_net.h, with the same weakness it removes from…
- [High] virtio_net_hdr_thoff_valid() decides whether skb->data starts
  with an Ethernet header only from `skb->dev->type == ARPHRD_ETHER`…
- [Medium] On the non-ARPHRD_ETHER branch, virtio_net_hdr_thoff_valid()
  leaves proto = 0 and then guesses the L3 protocol from the version…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but UML's
  raw_verify_header() (arch/um/drivers/vector_transports.c:213) discards…

--

net: extend virtio_net_hdr csum_start checks to VLAN, IPv6 and IP options

This patch replaces the fixed nh_min_len check on csum_start in
__virtio_net_hdr_to_skb() with a new helper, virtio_net_hdr_thoff_valid().
On ARPHRD_ETHER devices the helper parses the Ethernet header and any VLAN
tags. On other devices it uses skb_network_offset(). It then checks the
transport offset against the IPv4 IHL and the IPv6 header length.

Link: https://lore.kernel.org/netdev/20260922030310.8684-2-habte.yibelo@gmail.com/

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..6a30f58d9d65f 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,6 +48,52 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
>  	return 0;
>  }
>  
> +static inline bool virtio_net_hdr_thoff_valid(const struct sk_buff *skb,
> +					      unsigned int nh_min_len)
> +{

[ ... ]

> +	if (skb->dev->type == ARPHRD_ETHER) {
> +		if (unlikely(thoff < ETH_HLEN))
> +			return false;
> +		nhoff = ETH_HLEN;
> +		proto = eth_hdr(skb)->h_proto;
> +		if (eth_type_vlan(proto)) {
> +			proto = __vlan_get_protocol(skb, proto, &nhoff);
> +			if (!proto)
> +				return false;
> +		}
> +	} else {
> +		nhoff = skb_network_offset(skb);
> +	}

[Severity: High]
Does skb->dev->type reliably tell us what framing is at skb->data?

For tun, IFF_TUN/IFF_TAP decides the framing in tun_get_user(). TUNSETLINK
lets the fd owner set dev->type to any value while the device is down:

drivers/net/tun.c:__tun_chr_ioctl() {
    ...
	case TUNSETLINK:
    ...
			tun->dev->type = (int) arg;
    ...
}

That seems to allow two cases.

Case 1: an IFF_TAP device with dev->type != ARPHRD_ETHER. tun_get_user()
calls skb_set_network_header(skb, ETH_HLEN). skb->protocol is still 0,
because eth_type_trans() only runs after tun_vnet_hdr_tnl_to_skb().

The helper then takes the else branch with nhoff = 14 and does no VLAN
parsing. For an 802.1Q frame, data[14] is the TCI, and its top nibble is
the PCP/DEI bits chosen by the sender.

If that nibble is not 4 or 6, only thoff >= 34 is enforced. The real IPv4
header starts at offset 18 and can run to offset 78 with options. Doesn't
that leave the VLAN case open?

Case 2: an IFF_TUN device with dev->type == ARPHRD_ETHER. Here skb->data
starts at the L3 header (skb_reset_network_header()). The helper still
parses it as Ethernet, with nhoff = ETH_HLEN and h_proto taken from bytes
12-13 of the IP header, which is the source address.

A valid IPv4/TCP packet with csum_start = 20 now fails
thoff < nhoff + nh_min_len (20 < 34). An IPv6 packet with csum_start = 40
also fails (40 < 54). The old check accepted both. Is this a regression for
such setups?

Could the caller pass in the framing, instead of the helper relying on
dev->type?

> +
> +	if (unlikely(thoff < nhoff + nh_min_len))
> +		return false;
> +
> +	iph = (const void *)(skb->data + nhoff);
> +	if (!proto) {
> +		if (iph->version == 4)
> +			proto = htons(ETH_P_IP);
> +		else if (iph->version == 6)
> +			proto = htons(ETH_P_IPV6);
> +	}
> +
> +	if (proto == htons(ETH_P_IP) || skb->protocol == htons(ETH_P_IP)) {
> +		if (unlikely(iph->ihl < 5))
> +			return false;
> +		nh_min_len = max_t(u32, iph->ihl * 4, nh_min_len);
> +	}

[Severity: Medium]
Should the version nibble guess be skipped when skb->protocol already
names a non-IP protocol?

The commit message says:

  - 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.

Take IFF_TUN without IFF_NO_PI and with pi.proto = ETH_P_MPLS_UC.
tun_get_user() calls skb_reset_network_header() and sets
skb->protocol = pi.proto. So nhoff = 0, and data[0] is the top byte of
the MPLS label.

For labels 0x40000-0x4FFFF, iph->version reads as 4 and iph->ihl reads as
label bits 15..12. ihl < 5 is rejected outright. ihl >= 7 requires
thoff >= 28, so a valid MPLS + IPv4/TCP packet with csum_start = 24 is
rejected.

For labels 0x60000-0x6FFFF, the version reads as 6 and nh_min_len becomes
40, so csum_start = 24 fails as well.

The old check accepted these packets. Should proto be guessed from the
version only when skb->protocol is 0, ETH_P_IP or ETH_P_IPV6?

[ ... ]

> @@ -104,7 +150,7 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  
>  		if (!skb_partial_csum_set(skb, start, off))
>  			return -EINVAL;
> -		if (skb_transport_offset(skb) < nh_min_len)
> +		if (!virtio_net_hdr_thoff_valid(skb, nh_min_len))
>  			return -EINVAL;

[Severity: High]
Does the UDP tunnel path get the same coverage? Before it calls into here,
virtio_net_hdr_tnl_to_skb() still checks its offsets only against fixed
lengths:

include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
    ...
	outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);

	inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
	inner_nh = le16_to_cpu(vhdr->inner_nh_offset);
	outer_th = le16_to_cpu(vhdr->outer_th_offset);
	if (outer_th < outer_l3min ||
	    inner_nh < outer_th + sizeof(struct udphdr) ||
	    inner_th < inner_nh + inner_l3min)
		return -EINVAL;

	/* Let the basic parsing deal with plain GSO features. */
	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
				      hdr->gso_type & ~gso_tunnel_type);
    ...
	skb->transport_header = outer_th + skb_headroom(skb);
    ...
}

For tunnel packets, csum_start is inner_th. virtio_net_hdr_thoff_valid()
checks it only against the outer L2/L3 headers, so inner_th can still point
inside inner IPv4 options.

After that, skb->transport_header is overwritten with outer_th, which was
only checked against the fixed 34/54. Two examples:

  - an 802.1Q tagged outer frame, with outer IPv4 at bytes 18..38
  - an untagged outer IPv4 header with ihl = 15, at bytes 14..74

In both, outer_th = 34 passes and the outer transport header ends up
inside the outer IP header.

There are two ways to reach this:

  tun_get_user()->tun_vnet_hdr_tnl_to_skb()->virtio_net_hdr_tnl_to_skb()
  when vnet_hdr_sz >= TUN_VNET_TNL_SIZE

  virtnet_receive_done()->virtio_net_hdr_tnl_to_skb()
  when rx_tnl is negotiated

Should the tunnel path get the same VLAN and IP option handling? If not,
should the commit message say that this path is not covered?

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch.
raw_verify_header() in arch/um/drivers/vector_transports.c ignores the
return value of virtio_net_hdr_to_skb():

	skb_set_network_header(skb, ETH_HLEN);
	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
	return 0;

vector_legacy_rx() and vector_mmsg_rx() only drop the packet on a negative
return. So if skb_partial_csum_set() succeeds and
virtio_net_hdr_thoff_valid() then fails, the skb is still delivered. It
arrives as CHECKSUM_PARTIAL with a transport header inside L3.

The old nh_min_len check was ignored in the same way. The vnet header here
comes from the host kernel's packet socket, not from untrusted input.

Should raw_verify_header() pass the error up so the new check applies to
this caller?

>  
>  		nh_min_len = skb_transport_offset(skb);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007163819.3041710-1-willemdebruijn.kernel%40gmail.com

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

[Willem de Bruijn]

netdev-bot+sashiko@ wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The fix leaves the UDP-tunnel path, virtio_net_hdr_tnl_to_skb()
>   in include/linux/virtio_net.h, with the same weakness it removes from…
> - [High] virtio_net_hdr_thoff_valid() decides whether skb->data starts
>   with an Ethernet header only from `skb->dev->type == ARPHRD_ETHER`…
> - [Medium] On the non-ARPHRD_ETHER branch, virtio_net_hdr_thoff_valid()
>   leaves proto = 0 and then guesses the L3 protocol from the version…

All comments look valid and require a respin. Any revision will take
them into account.

Based also on the previous conversation in this thread, will review the
approach. Updated in patchwork to changes requested. 

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

[Michael S. Tsirkin]

On Thu, Oct 08, 2026 at 04:04:16PM -0400, Willem de Bruijn wrote:
> netdev-bot+sashiko@ wrote:
> > Thank you for your contribution! Sashiko AI review found 3 potential
> > issue(s) to consider.
> > 
> > Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)
> > 
> > If you believe none of the issues are legitimate, please reply to each
> > issue of Medium severity or higher and explain why the AI is wrong.
> > If any of them are legitimate and you plan to respin, please let patchwork
> > know by including "pw-bot: cr" as a separate line at the end of your reply
> > (one such reply per series is enough).
> > 
> > - [High] The fix leaves the UDP-tunnel path, virtio_net_hdr_tnl_to_skb()
> >   in include/linux/virtio_net.h, with the same weakness it removes from…
> > - [High] virtio_net_hdr_thoff_valid() decides whether skb->data starts
> >   with an Ethernet header only from `skb->dev->type == ARPHRD_ETHER`…
> > - [Medium] On the non-ARPHRD_ETHER branch, virtio_net_hdr_thoff_valid()
> >   leaves proto = 0 and then guesses the L3 protocol from the version…
> 
> All comments look valid and require a respin. Any revision will take
> them into account.
> 
> Based also on the previous conversation in this thread, will review the
> approach. Updated in patchwork to changes requested. 


But let's agree who works on this, please. Have a bunch of stuff on my
plate so if it is you, I'm very happy and will just criticise :)


-- 
MST

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

[Willem de Bruijn]

On Thu, Oct 8, 2026 at 4:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Oct 08, 2026 at 04:04:16PM -0400, Willem de Bruijn wrote:
> > netdev-bot+sashiko@ wrote:
> > > Thank you for your contribution! Sashiko AI review found 3 potential
> > > issue(s) to consider.
> > >
> > > Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)
> > >
> > > If you believe none of the issues are legitimate, please reply to each
> > > issue of Medium severity or higher and explain why the AI is wrong.
> > > If any of them are legitimate and you plan to respin, please let patchwork
> > > know by including "pw-bot: cr" as a separate line at the end of your reply
> > > (one such reply per series is enough).
> > >
> > > - [High] The fix leaves the UDP-tunnel path, virtio_net_hdr_tnl_to_skb()
> > >   in include/linux/virtio_net.h, with the same weakness it removes from…
> > > - [High] virtio_net_hdr_thoff_valid() decides whether skb->data starts
> > >   with an Ethernet header only from `skb->dev->type == ARPHRD_ETHER`…
> > > - [Medium] On the non-ARPHRD_ETHER branch, virtio_net_hdr_thoff_valid()
> > >   leaves proto = 0 and then guesses the L3 protocol from the version…
> >
> > All comments look valid and require a respin. Any revision will take
> > them into account.
> >
> > Based also on the previous conversation in this thread, will review the
> > approach. Updated in patchwork to changes requested.
>
>
> But let's agree who works on this, please. Have a bunch of stuff on my
> plate so if it is you, I'm very happy and will just criticise :)

Do you agree with my suggested revision that just drops the checksum
offload request for weird packets?

Or do you want to investigate your suggestion to skb_checksum_help those here?

I can't estimate yet how easy/hard that will prove to be. Can
prototype, but no guarantee that it's a fruitful approach.

The next few days I'll be OOO, so next week at the earliest.

We agree that this is not urgent, correct. So can do some prototyping.

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

[Michael S. Tsirkin]

On Thu, Oct 08, 2026 at 05:06:30PM -0400, Willem de Bruijn wrote:
> On Thu, Oct 8, 2026 at 4:55 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Oct 08, 2026 at 04:04:16PM -0400, Willem de Bruijn wrote:
> > > netdev-bot+sashiko@ wrote:
> > > > Thank you for your contribution! Sashiko AI review found 3 potential
> > > > issue(s) to consider.
> > > >
> > > > Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)
> > > >
> > > > If you believe none of the issues are legitimate, please reply to each
> > > > issue of Medium severity or higher and explain why the AI is wrong.
> > > > If any of them are legitimate and you plan to respin, please let patchwork
> > > > know by including "pw-bot: cr" as a separate line at the end of your reply
> > > > (one such reply per series is enough).
> > > >
> > > > - [High] The fix leaves the UDP-tunnel path, virtio_net_hdr_tnl_to_skb()
> > > >   in include/linux/virtio_net.h, with the same weakness it removes from…
> > > > - [High] virtio_net_hdr_thoff_valid() decides whether skb->data starts
> > > >   with an Ethernet header only from `skb->dev->type == ARPHRD_ETHER`…
> > > > - [Medium] On the non-ARPHRD_ETHER branch, virtio_net_hdr_thoff_valid()
> > > >   leaves proto = 0 and then guesses the L3 protocol from the version…
> > >
> > > All comments look valid and require a respin. Any revision will take
> > > them into account.
> > >
> > > Based also on the previous conversation in this thread, will review the
> > > approach. Updated in patchwork to changes requested.
> >
> >
> > But let's agree who works on this, please. Have a bunch of stuff on my
> > plate so if it is you, I'm very happy and will just criticise :)
> 
> Do you agree with my suggested revision that just drops the checksum
> offload request for weird packets?
> 
> Or do you want to investigate your suggestion to skb_checksum_help those here?
> 
> I can't estimate yet how easy/hard that will prove to be. Can
> prototype, but no guarantee that it's a fruitful approach.
> 
> The next few days I'll be OOO, so next week at the earliest.
> 
> We agree that this is not urgent, correct. So can do some prototyping.

Oh I have a prototype. Quite ugly atm.

-- 
MST