From 9e6a8db20158ea7acf9160b34fc2865ab9606eb6 Mon Sep 17 00:00:00 2001
From: angus19 <angus19@users.noreply.github.com>
Date: Tue, 29 Oct 2024 17:02:00 +0800
Subject: [PATCH] Improve zero checksum handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As per the suggestion by @ayourtch:

Well setting zero_csum_pass to 1 is not much of a fix, is it - it rather masks the problem in the code. Also it is a change of defaults, which i would be a bit hesitant to do…

Here’s an idea, what if we rewrite that bit as follows:

if (!udp->check) {
if (zero_csum_pass) {
// do nothing
break;
}
// recalculate the IPv6 checksum before adjusting
ip6_update_csum(…)
}
// here we will have nonzero checksum, unmagic + remagic

so, if it’s UDP and the checksum is zero and we don’t pass the zero checksum, recalculate it ?

It will still be broken if we don’t have full payload since we can’t do the full calculation in this case, but should take care of other cases ?

what do you think ?
---
 nat46/modules/nat46-core.c | 23 ++++++++++++++++++-----
 nat46/modules/nat46-core.h |  2 ++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/nat46/modules/nat46-core.c b/nat46/modules/nat46-core.c
index 9e4f5fc..259f114 100644
--- a/nat46/modules/nat46-core.c
+++ b/nat46/modules/nat46-core.c
@@ -1007,6 +1007,7 @@ int xlate_payload6_to4(nat46_instance_t *nat46, void *pv6, void *ptrans_hdr, int
         /* zero checksum and the config to pass it is set - do nothing with it */
         break;
       }
+      /* pretend checksum is correct not null */
       sum1 = csum_ipv6_unmagic(nat46, &ip6h->saddr, &ip6h->daddr, infrag_payload_len, NEXTHDR_UDP, udp->check);
       sum2 = csum_tcpudp_remagic(v4saddr, v4daddr, infrag_payload_len, NEXTHDR_UDP, sum1); /* add pseudoheader */
       if(ul_sum) {
@@ -1671,9 +1672,12 @@ int nat46_ipv6_input(struct sk_buff *old_skb) {
       case NEXTHDR_UDP: {
         struct udphdr *udp = add_offset(ip6h, v6packet_l3size);
         u16 sum1, sum2;
-        if ((udp->check == 0) && zero_csum_pass) {
-          /* zero checksum and the config to pass it is set - do nothing with it */
-          break;
+        if (udp->check == 0) {
+          if (zero_csum_pass) {
+            /* zero checksum and the config to pass it is set - do nothing with it */
+            break;
+          }
+          ip6_update_csum(old_skb, ip6h, ip6h->nexthdr == NEXTHDR_FRAGMENT); /* recalculate checksum before adjusting */
         }
         sum1 = csum_ipv6_unmagic(nat46, &ip6h->saddr, &ip6h->daddr, l3_infrag_payload_len, NEXTHDR_UDP, udp->check);
         sum2 = csum_tcpudp_remagic(v4saddr, v4daddr, l3_infrag_payload_len, NEXTHDR_UDP, sum1);
@@ -1767,8 +1771,17 @@ void ip6_update_csum(struct sk_buff * skb, struct ipv6hdr * ip6hdr, int do_atomi
       unsigned udplen = ntohs(ip6hdr->payload_len) - (do_atomic_frag?8:0); /* UDP hdr + payload */
 
       if ((udp->check == 0) && zero_csum_pass) {
-         /* zero checksum and the config to pass it is set - do nothing with it */
-         break;
+        /* zero checksum and the config to pass it is set - do nothing with it */
+        break;
+      }
+
+      if (do_atomic_frag) {
+        if ((udp->check == 0) && !zero_csum_pass) {
+          /*
+           * Checksum will still be broken if we don't have full payload
+           * since we can't do the full calculation in this case.
+           */
+        }
       }
 
       oldsum = udp->check;
diff --git a/nat46/modules/nat46-core.h b/nat46/modules/nat46-core.h
index aca331b..184ef86 100644
--- a/nat46/modules/nat46-core.h
+++ b/nat46/modules/nat46-core.h
@@ -82,4 +82,6 @@ nat46_instance_t *get_nat46_instance(struct sk_buff *sk);
 nat46_instance_t *alloc_nat46_instance(int npairs, nat46_instance_t *old, int from_ipair, int to_ipair, int remove_ipair);
 void release_nat46_instance(nat46_instance_t *nat46);
 
+void ip6_update_csum(struct sk_buff * skb, struct ipv6hdr * ip6hdr, int do_atomic_frag);
+
 #endif