Re: [PATCH net v3 6/8] net: add a sysctl to opt out of virtio net hdr checksum validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: virtio_net_csum_partial_ok() fails to bypass virtio_net_l3_start()
--

commit 559fb10ce506a1c7276bf12adfdeed16a12a4f0c
Author: Michael S. Tsirkin <mst@redhat.com>

net: add a sysctl to opt out of virtio net hdr checksum validation

This commit adds a per-netns sysctl net.core.virtio_net_hdr_csum_check
guarded by a static key to allow opting out of ingress virtio_net_hdr
checksum metadata validation.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 97b554ef7123f..25aa95c11e31a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -179,6 +193,12 @@ virtio_net_csum_partial_ok_at_offset(struct sk_buff *skb, u8 hdr_gso_type,
>  	unsigned int thoff;
>  	int ip_proto;
>  
> +	/* Failsafe: a namespace opted out of the validation,
> +	 * trust the checksum metadata as before.
> +	 */
> +	if (virtio_net_hdr_csum_check_disabled(skb))
> +		return true;
> +
>  	if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto))
>  		return false;
>  
> @@ -516,6 +536,9 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
>  	unsigned int thoff;
>  	int ip_proto;
>  
> +	if (virtio_net_hdr_csum_check_disabled(skb))
> +		return true;
> +
>  	if (!virtio_net_l3_start(skb, &nhoff, &proto))
>  		return false;
>  	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))

[Severity: Medium]
In virtio_net_tnl_offsets_ok(), virtio_net_hdr_csum_check_disabled(skb) is
checked before calling virtio_net_l3_start(). However, in the non-tunnel
path, virtio_net_hdr_to_skb() calls virtio_net_csum_partial_ok(), which
calls virtio_net_l3_start() before reaching the check in
virtio_net_csum_partial_ok_at_offset():

include/linux/virtio_net.h:virtio_net_csum_partial_ok() {
	int nhoff;
	__be16 proto;

	if (!virtio_net_l3_start(skb, &nhoff, &proto))
		return false;

	return virtio_net_csum_partial_ok_at_offset(skb, hdr_gso_type, nhoff,
						   proto);
}

When net.core.virtio_net_hdr_csum_check is set to 0 and a packet with
VIRTIO_NET_HDR_F_NEEDS_CSUM enters virtio_net_hdr_to_skb(),
virtio_net_l3_start() still runs pskb_may_pull() and __vlan_get_protocol()
to compute nhoff and proto, which virtio_net_csum_partial_ok_at_offset()
then immediately discards.

Furthermore, if virtio_net_l3_start() returns false (for example, when
__vlan_get_protocol() returns 0 on VLAN tags exceeding VLAN_MAX_DEPTH, or
on a non-ARPHRD_ETHER device where skb_network_offset(skb) is unset or out
of bounds), virtio_net_csum_partial_ok() returns false without consulting
virtio_net_hdr_csum_check_disabled(skb). virtio_net_hdr_to_skb() then falls
through to virtio_net_resolve_csum_partial_mismatch(), dropping GSO packets
or completing non-GSO checksums in software despite the sysctl opt-out.

Should virtio_net_csum_partial_ok() check
virtio_net_hdr_csum_check_disabled(skb) before calling
virtio_net_l3_start()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792411304.git.mst@redhat.com?part=6