netdev - Re: [PATCH net v2 2/2] bpf: Fix L4 csum update on IPv6 in CHECKSUM

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <040e674d-7854-426f-b466-63dc36cccb98@iogearbox.net>
Date: Tue, 27 May 2025 17:14:14 +0200
From: Daniel Borkmann <daniel@...earbox.net>
To: Paul Chaignon <paul.chaignon@...il.com>, netdev@...r.kernel.org,
 bpf@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, David Ahern <dsahern@...nel.org>,
 Tom Herbert <tom@...bertland.com>, Alexei Starovoitov <ast@...nel.org>,
 Andrii Nakryiko <andrii@...nel.org>
Subject: Re: [PATCH net v2 2/2] bpf: Fix L4 csum update on IPv6 in
 CHECKSUM_COMPLETE

On 5/27/25 11:48 AM, Paul Chaignon wrote:
> In Cilium, we use bpf_csum_diff + bpf_l4_csum_replace to, among other
> things, update the L4 checksum after reverse SNATing IPv6 packets. That
> use case is however not currently supported and leads to invalid
> skb->csum values in some cases. This patch adds support for IPv6 address
> changes in bpf_l4_csum_update via a new flag.
> 
> When calling bpf_l4_csum_replace in Cilium, it ends up calling
> inet_proto_csum_replace_by_diff:
> 
>      1:  void inet_proto_csum_replace_by_diff(__sum16 *sum, struct sk_buff *skb,
>      2:                                       __wsum diff, bool pseudohdr)
>      3:  {
>      4:      if (skb->ip_summed != CHECKSUM_PARTIAL) {
>      5:          csum_replace_by_diff(sum, diff);
>      6:          if (skb->ip_summed == CHECKSUM_COMPLETE && pseudohdr)
>      7:              skb->csum = ~csum_sub(diff, skb->csum);
>      8:      } else if (pseudohdr) {
>      9:          *sum = ~csum_fold(csum_add(diff, csum_unfold(*sum)));
>      10:     }
>      11: }
> 
> The bug happens when we're in the CHECKSUM_COMPLETE state. We've just
> updated one of the IPv6 addresses. The helper now updates the L4 header
> checksum on line 5. Next, it updates skb->csum on line 7. It shouldn't.
> 
> For an IPv6 packet, the updates of the IPv6 address and of the L4
> checksum will cancel each other. The checksums are set such that
> computing a checksum over the packet including its checksum will result
> in a sum of 0. So the same is true here when we update the L4 checksum
> on line 5. We'll update it as to cancel the previous IPv6 address
> update. Hence skb->csum should remain untouched in this case.
> 
> The same bug doesn't affect IPv4 packets because, in that case, three
> fields are updated: the IPv4 address, the IP checksum, and the L4
> checksum. The change to the IPv4 address and one of the checksums still
> cancel each other in skb->csum, but we're left with one checksum update
> and should therefore update skb->csum accordingly. That's exactly what
> inet_proto_csum_replace_by_diff does.
> 
> This special case for IPv6 L4 checksums is also described atop
> inet_proto_csum_replace16, the function we should be using in this case.
> 
> This patch introduces a new bpf_l4_csum_replace flag, BPF_F_IPV6,
> to indicate that we're updating the L4 checksum of an IPv6 packet. When
> the flag is set, inet_proto_csum_replace_by_diff will skip the
> skb->csum update.
> 
> Fixes: 7d672345ed295 ("bpf: add generic bpf_csum_diff helper")
> Signed-off-by: Paul Chaignon <paul.chaignon@...il.com>

Great catch!

Acked-by: Daniel Borkmann <daniel@...earbox.net>