|
Hello,
On Thu, 9 Jul 2026, Julian Anastasov wrote:
> TCP/UDP checksum validation for CHECKSUM_COMPLETE is broken
> before the git history.
>
> Expecting skb->csum to cover data starting from the protocol
> header is wrong. As IPVS works at the IP layer, the csum for
> the IP header is not subtracted yet.
>
> ip_vs_in_icmp_v6() is missing checksum validation for ICMPv6
> packets from clients.
>
> Also, Sashiko points out that handle_response_icmp() being
> common for IPv4 and IPv6 is missing the pseudo-header
> calculation while validating ICMPv6 messages from real
> servers which is a problem if checksum is not validated
> by the hardware.
>
> Fix the problems by creating ip_vs_checksum_common_check()
> helper and use it for TCP/UDP/ICMP both for IPv4 and IPv6.
>
> Also, ip_vs_checksum_complete() can be marked static.
>
> Fixes: 2a3b791e6e11 ("IPVS: Add/adjust Netfilter hook functions and helpers
> for v6")
> Link: https://sashiko.dev/#/patchset/20260708180315.77413-1-ja%40ssi.bg
> Signed-off-by: Julian Anastasov <ja@xxxxxx>
This patch needs changes, will send v2.
pw-bot: changes-requested
> ---
> include/net/ip_vs.h | 5 ++-
> net/netfilter/ipvs/ip_vs_core.c | 63 ++++++++++++++++++++++++++--
> net/netfilter/ipvs/ip_vs_proto_tcp.c | 35 ++--------------
> net/netfilter/ipvs/ip_vs_proto_udp.c | 39 +++--------------
> 4 files changed, 71 insertions(+), 71 deletions(-)
>
> diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
> index 49297fec448a..fd18b1cd6471 100644
> --- a/include/net/ip_vs.h
> +++ b/include/net/ip_vs.h
> @@ -2065,8 +2065,6 @@ void ip_vs_nat_icmp_v6(struct sk_buff *skb, struct
> ip_vs_protocol *pp,
> struct ip_vs_conn *cp, int dir);
> #endif
>
> -__sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset);
> -
> static inline __wsum ip_vs_check_diff4(__be32 old, __be32 new, __wsum oldsum)
> {
> __be32 diff[2] = { ~old, new };
> @@ -2092,6 +2090,9 @@ static inline __wsum ip_vs_check_diff2(__be16 old,
> __be16 new, __wsum oldsum)
> return csum_partial(diff, sizeof(diff), oldsum);
> }
>
> +bool ip_vs_checksum_common_check(int af, struct sk_buff *skb, int proto,
> + int offset);
> +
> /* 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 c9c88c99d07b..ceb1cbf33dd7 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -867,11 +867,56 @@ static int sysctl_nat_icmp_send(struct netns_ipvs
> *ipvs) { return 0; }
>
> #endif
>
> -__sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset)
> +static __sum16 ip_vs_checksum_complete(struct sk_buff *skb, int offset)
> {
> return csum_fold(skb_checksum(skb, offset, skb->len - offset, 0));
> }
>
> +/**
> + * ip_vs_checksum_common_check - validate checksum for TCP/UDP/ICMP
> + * @af: AF_INET/AF_INET6
> + * @skb: socket buffer
> + * @proto: IPPROTO_xxx
> + * @offset: offset of protocol header
> + */
> +bool ip_vs_checksum_common_check(int af, struct sk_buff *skb, int proto,
> + int offset)
> +{
> + __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) {
> + /* IPVS works at IP layer, so skb->csum covers data from
> + * IP header, strip it up to the protocol header
> + */
> + csum = csum_sub(skb->csum, skb_checksum(skb, 0, offset, 0));
> + } else {
> + /* No need to checksum. */
> + return true;
> + }
> +#ifdef CONFIG_IP_VS_IPV6
> + if (af == AF_INET6) {
> + if (csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> + &ipv6_hdr(skb)->daddr,
> + skb->len - offset, proto,
> + csum))
> + return false;
> + } else
> +#endif
> + if (proto == IPPROTO_ICMP)
> + return !csum_fold(csum);
> + else if (csum_tcpudp_magic(ip_hdr(skb)->saddr,
> + ip_hdr(skb)->daddr,
> + skb->len - offset, proto,
> + csum))
> + return false;
> +
> + return true;
> +}
> +
> static inline enum ip_defrag_users ip_vs_defrag_user(unsigned int hooknum)
> {
> if (NF_INET_LOCAL_IN == hooknum)
> @@ -1039,12 +1084,13 @@ static int handle_response_icmp(int af, struct
> sk_buff *skb,
> unsigned int hooknum)
> {
> unsigned int verdict = NF_DROP;
> + int iproto = af == AF_INET6 ? IPPROTO_ICMPV6 : IPPROTO_ICMP;
>
> if (IP_VS_FWD_METHOD(cp) != IP_VS_CONN_F_MASQ)
> goto after_nat;
>
> /* Ensure the checksum is correct */
> - if (!skb_csum_unnecessary(skb) && ip_vs_checksum_complete(skb, ihl)) {
> + if (!ip_vs_checksum_common_check(af, skb, iproto, ihl)) {
> /* Failed checksum! */
> IP_VS_DBG_BUF(1, "Forward ICMP: failed checksum from %s!\n",
> IP_VS_DBG_ADDR(af, snet));
> @@ -1898,7 +1944,7 @@ ip_vs_in_icmp(struct netns_ipvs *ipvs, struct sk_buff
> *skb, int *related,
> verdict = NF_DROP;
>
> /* Ensure the checksum is correct */
> - if (!skb_csum_unnecessary(skb) && ip_vs_checksum_complete(skb, ihl)) {
> + if (!ip_vs_checksum_common_check(AF_INET, skb, IPPROTO_ICMP, ihl)) {
> /* Failed checksum! */
> IP_VS_DBG(1, "Incoming ICMP: failed checksum from %pI4!\n",
> &iph->saddr);
> @@ -2064,6 +2110,17 @@ static int ip_vs_in_icmp_v6(struct netns_ipvs *ipvs,
> struct sk_buff *skb,
> goto out;
> }
>
> + verdict = NF_DROP;
> +
> + /* Ensure the checksum is correct */
> + if (!ip_vs_checksum_common_check(AF_INET6, skb, IPPROTO_ICMPV6,
> + iph->len)) {
> + /* Failed checksum! */
> + IP_VS_DBG(1, "Incoming ICMPv6: failed checksum from %pI6c!\n",
> + &iph->saddr);
> + goto out;
> + }
> +
> /* do the statistics and put it back */
> ip_vs_in_stats(cp, skb);
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> index 8cc0a8ce6241..147cf01708ff 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_tcp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_tcp.c
> @@ -304,39 +304,10 @@ static int
> tcp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp,
> unsigned int tcphoff)
> {
> - switch (skb->ip_summed) {
> - case CHECKSUM_NONE:
> - skb->csum = skb_checksum(skb, tcphoff, skb->len - tcphoff, 0);
> - fallthrough;
> - case CHECKSUM_COMPLETE:
> -#ifdef CONFIG_IP_VS_IPV6
> - if (af == AF_INET6) {
> - if (csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> - &ipv6_hdr(skb)->daddr,
> - skb->len - tcphoff,
> - IPPROTO_TCP,
> - skb->csum)) {
> - IP_VS_DBG_RL_PKT(0, af, pp, skb, 0,
> - "Failed checksum for");
> - return 0;
> - }
> - } else
> -#endif
> - if (csum_tcpudp_magic(ip_hdr(skb)->saddr,
> - ip_hdr(skb)->daddr,
> - skb->len - tcphoff,
> - ip_hdr(skb)->protocol,
> - skb->csum)) {
> - IP_VS_DBG_RL_PKT(0, af, pp, skb, 0,
> - "Failed checksum for");
> - return 0;
> - }
> - break;
> - default:
> - /* No need to checksum. */
> - break;
> + if (!ip_vs_checksum_common_check(af, skb, IPPROTO_TCP, tcphoff)) {
> + IP_VS_DBG_RL_PKT(0, af, pp, skb, 0, "Failed checksum for");
> + return 0;
> }
> -
> return 1;
> }
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_udp.c
> b/net/netfilter/ipvs/ip_vs_proto_udp.c
> index f9de632e38cd..d10713ca74f7 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_udp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_udp.c
> @@ -306,40 +306,11 @@ udp_csum_check(int af, struct sk_buff *skb, struct
> ip_vs_protocol *pp,
> if (uh == NULL)
> return 0;
>
> - if (uh->check != 0) {
> - switch (skb->ip_summed) {
> - case CHECKSUM_NONE:
> - skb->csum = skb_checksum(skb, udphoff,
> - skb->len - udphoff, 0);
> - fallthrough;
> - case CHECKSUM_COMPLETE:
> -#ifdef CONFIG_IP_VS_IPV6
> - if (af == AF_INET6) {
> - if (csum_ipv6_magic(&ipv6_hdr(skb)->saddr,
> - &ipv6_hdr(skb)->daddr,
> - skb->len - udphoff,
> - IPPROTO_UDP,
> - skb->csum)) {
> - IP_VS_DBG_RL_PKT(0, af, pp, skb, 0,
> - "Failed checksum for");
> - return 0;
> - }
> - } else
> -#endif
> - if (csum_tcpudp_magic(ip_hdr(skb)->saddr,
> - ip_hdr(skb)->daddr,
> - skb->len - udphoff,
> - ip_hdr(skb)->protocol,
> - skb->csum)) {
> - IP_VS_DBG_RL_PKT(0, af, pp, skb, 0,
> - "Failed checksum for");
> - return 0;
> - }
> - break;
> - default:
> - /* No need to checksum. */
> - break;
> - }
> + if (!uh->check)
> + return 1;
> + if (!ip_vs_checksum_common_check(af, skb, IPPROTO_UDP, udphoff)) {
> + IP_VS_DBG_RL_PKT(0, af, pp, skb, 0, "Failed checksum for");
> + return 0;
> }
> return 1;
> }
> --
> 2.55.0
Regards
--
Julian Anastasov <ja@xxxxxx>
|
