netdev - Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <671bb179d039d_34060c294fd@willemb.c.googlers.com.notmuch>
Date: Fri, 25 Oct 2024 10:55:53 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Benoît Monin <benoit.monin@....fr>, 
 "David S. Miller" <davem@...emloft.net>, 
 Eric Dumazet <edumazet@...gle.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Paolo Abeni <pabeni@...hat.com>, 
 Jiri Pirko <jiri@...nulli.us>, 
 Sebastian Andrzej Siewior <bigeasy@...utronix.de>, 
 Lorenzo Bianconi <lorenzo@...nel.org>
Cc: netdev@...r.kernel.org, 
 linux-kernel@...r.kernel.org, 
 Benoît Monin <benoit.monin@....fr>
Subject: Re: [PATCH v2 net] net: skip offload for NETIF_F_IPV6_CSUM if ipv6
 header contains extension

Benoît Monin wrote:
> As documented in skbuff.h, devices with NETIF_F_IPV6_CSUM capability
> can only checksum TCP and UDP over IPv6 if the IP header does not
> contains extension.
> 
> This is enforced for UDP packets emitted from user-space to an IPv6
> address as they go through ip6_make_skb(), which calls
> __ip6_append_data() where a check is done on the header size before
> setting CHECKSUM_PARTIAL.
> 
> But the introduction of UDP encapsulation with fou6 added a code-path
> where it is possible to get an skb with a partial UDP checksum and an
> IPv6 header with extension:
> * fou6 adds a UDP header with a partial checksum if the inner packet
> does not contains a valid checksum.
> * ip6_tunnel adds an IPv6 header with a destination option extension
> header if encap_limit is non-zero (the default value is 4).
> 
> The thread linked below describes in more details how to reproduce the
> problem with GRE-in-UDP tunnel.
> 
> Add a check on the network header size in skb_csum_hwoffload_help() to
> make sure no IPv6 packet with extension header is handed to a network
> device with NETIF_F_IPV6_CSUM capability.
> 
> Link: https://lore.kernel.org/netdev/26548921.1r3eYUQgxm@benoit.monin/T/#u
> Fixes: aa3463d65e7b ("fou: Add encap ops for IPv6 tunnels")
> Signed-off-by: Benoît Monin <benoit.monin@....fr>

Reviewed-by: Willem de Bruijn <willemb@...gle.com>

> ---
> changelog
> * v2:
>     - patch against net instead of net-next
>     - clarify documentation of NETIF_F_IPV6_CSUM
>     - add link to thread describing the problem
>     - add fixes tag
>     - use vlan_get_protocol to check for IPv6
> * v1:
>     - https://lore.kernel.org/netdev/0dc0c2af98e96b1df20bd36aeaed4eb4e27d507e.1728056028.git.benoit.monin@gmx.fr/T/#u
> ---
>  net/core/dev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ea5fbcd133ae..8453e14d301b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3639,6 +3639,9 @@ int skb_csum_hwoffload_help(struct sk_buff *skb,
>  		return 0;
> 
>  	if (features & (NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM)) {
> +		if (vlan_get_protocol(skb) == htons(ETH_P_IPV6) &&
> +		    skb_network_header_len(skb) != sizeof(struct ipv6hdr))
> +			goto sw_checksum;

skb_network_header_len requires skb->transport_header to be set.

This is not true for all egress packets. See for instance commit
d2aa125d6290 ("net: Don't set transport offset to invalid value").

But it should be true for all CHECKSUM_PARTIAL packets. See for
instance skb_partial_csum_set. So LGTM.

Just calling this out as it is not obvious and in case someone
does know a counter example of CHECKSUM_PARTIAL and
!skb_transport_header_was_set.