netdev - Re: [PATCH] net: fix csum calculation for encapsulated packets

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: <0540a49d-40e2-45a7-a068-fd14b75584f0@redhat.com>
Date: Thu, 22 Aug 2024 12:05:17 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: 沈安琪(凛玥) <amy.saq@...group.com>,
 netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: fix csum calculation for encapsulated packets



On 8/19/24 13:17, 沈安琪(凛玥) wrote:
> This commit fixes the issue that when a packet is encapsulated, such as
> sending through a UDP tunnel, the outer TCP/UDP checksum is not
> correctly recalculated if (1) checksum has been offloaded to hardware
> and (2) encapsulated packet has been NAT-ed again, which causes the
> packet being dropped due to the invalid outer checksum.
> 
> Previously, when an encapsulated packet met some NAT rules and its
> src/dst ip and/or src/dst port has been modified,
> inet_proto_csum_replace4 will be invoked to recalculated the outer
> checksum. However, if the packet is under the following condition: (1)
> checksum offloaded to hardware and (2) NAT rule has changed the src/dst
> port, its outer checksum will not be recalculated, since (1)
> skb->ip_summed is set to CHECKSUM_PARTIAL due to csum offload and (2)
> pseudohdr is set to false since port number is not part of pseudo
> header. 

I don't see where nat is calling inet_proto_csum_replace4() with 
pseudohdr == false: please include more detailed description of the 
relevant setup (ideally a self-test) or at least a backtrace leading to 
the issue.

> This leads to the outer TCP/UDP checksum invalid since it does
> not change along with the port number change.
> 
> In this commit, another condition has been added to recalculate outer
> checksum: if (1) the packet is encapsulated, (2) checksum has been
> offloaded, (3) the encapsulated packet has been NAT-ed to change port
> number and (4) outer checksum is needed, the outer checksum for
> encapsulated packet will be recalculated to make sure it is valid.

Please add a suitable fix tag.

> Signed-off-by: Anqi Shen <amy.saq@...group.com>
> ---
>   net/core/utils.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/utils.c b/net/core/utils.c
> index c994e95172ac..d9de60e9b347 100644
> --- a/net/core/utils.c
> +++ b/net/core/utils.c
> @@ -435,6 +435,8 @@ void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
>   		*sum = ~csum_fold(csum_add(csum_sub(csum_unfold(*sum),
>   						    (__force __wsum)from),
>   					   (__force __wsum)to));
> +	else if (skb->encapsulation && !!(*sum))
> +		csum_replace4(sum, from, to);

This looks incorrect for a csum partial value, and AFAICS the nat caller 
has already checked for !!(*sum).

Thanks,

Paolo