Sashiko notes that playing games with the skb dst and rt flags instead of providing hooknum is not a good idea when validating the checksums. Also, skipping checksum validation for FORWARD packets risk silent data corruption, even if the only user is the FTP-CMD packets coming from the real server. Sashiko also noticed that by using common checksum helper in the previous commit we actually fixed old bug where the TCP/UDP checksum for IPv6 on CHECKSUM_COMPLETE was not validated correctly. Fixes: e876b75b9020 ("ipvs: fix the checksum validations") Link: https://sashiko.dev/#/patchset/20260722211420.153933-1-pablo%40netfilter.org Link: https://sashiko.dev/#/patchset/20260727185024.67534-1-ja%40ssi.bg Signed-off-by: Julian Anastasov --- include/net/ip_vs.h | 30 ++------------------------- net/netfilter/ipvs/ip_vs_core.c | 27 ++++++++++++++++++++++++ net/netfilter/ipvs/ip_vs_proto_sctp.c | 2 +- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h index 1235f1934e94..10f5ce76a0ae 100644 --- a/include/net/ip_vs.h +++ b/include/net/ip_vs.h @@ -25,9 +25,7 @@ #include /* for union nf_inet_addr */ #include #include /* for struct ipv6hdr */ -#include #include -#include #if IS_ENABLED(CONFIG_NF_CONNTRACK) #include #endif @@ -2095,32 +2093,8 @@ static inline __wsum ip_vs_check_diff2(__be16 old, __be16 new, __wsum oldsum) return csum_partial(diff, sizeof(diff), oldsum); } -static inline bool ip_vs_checksum_needed(struct sk_buff *skb, int af) -{ - /* Checksum unnecessary or already validated? */ - if (skb_csum_unnecessary(skb)) - return false; - /* LOCAL_OUT ? */ - if (!skb->dev || skb->dev->flags & IFF_LOOPBACK) - return false; - /* !LOCAL_IN (FORWARD) ? */ - if (af == AF_INET6) { - if (!(dst_rt6_info(skb_dst(skb))->rt6i_flags & RTF_LOCAL)) - return false; - } else { - if (!(skb_rtable(skb)->rt_flags & RTCF_LOCAL)) - return false; - } - return true; -} - -static inline bool ip_vs_checksum_common_check(struct sk_buff *skb, - int offset, int proto, int af) -{ - if (!ip_vs_checksum_needed(skb, af)) - return true; - return !nf_checksum(skb, NF_INET_LOCAL_IN, offset, proto, af); -} +bool ip_vs_checksum_common_check(struct sk_buff *skb, int offset, int proto, + int af); /* Forget current conntrack (unconfirmed) and attach notrack entry */ static inline void ip_vs_notrack(struct sk_buff *skb) diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c index 0bdaeb4ed61e..a51418a449cc 100644 --- a/net/netfilter/ipvs/ip_vs_core.c +++ b/net/netfilter/ipvs/ip_vs_core.c @@ -872,6 +872,33 @@ static __sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset) return csum_fold(skb_checksum(skb, offset, skb->len - offset, 0)); } +bool ip_vs_checksum_common_check(struct sk_buff *skb, int offset, int proto, + int af) +{ + __wsum csum; + + if (skb_csum_unnecessary(skb)) + return true; + if (skb->ip_summed == CHECKSUM_NONE) + csum = skb_checksum(skb, offset, skb->len - offset, 0); + else if (skb->ip_summed == CHECKSUM_COMPLETE) + csum = csum_sub(skb->csum, skb_checksum(skb, 0, offset, 0)); + else + return true; + +#ifdef CONFIG_IP_VS_IPV6 + if (af == AF_INET6) + return !csum_ipv6_magic(&ipv6_hdr(skb)->saddr, + &ipv6_hdr(skb)->daddr, + skb->len - offset, + proto, csum); +#endif + if (proto == IPPROTO_ICMP) + return !csum_fold(csum); + return !csum_tcpudp_magic(ip_hdr(skb)->saddr, ip_hdr(skb)->daddr, + skb->len - offset, proto, csum); +} + static inline enum ip_defrag_users ip_vs_defrag_user(unsigned int hooknum) { if (NF_INET_LOCAL_IN == hooknum) diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c index 3dbd3096e163..dd54a37dee29 100644 --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c @@ -193,7 +193,7 @@ sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp, struct sctphdr *sh; __le32 cmp, val; - if (!ip_vs_checksum_needed(skb, af)) + if (skb_csum_unnecessary(skb)) return 1; sh = (struct sctphdr *)(skb->data + sctphoff); cmp = sh->checksum; -- 2.55.0