[PATCH][v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets

[PATCH][v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets

[Li RongQing]

csum_tcpudp_magic should not be called to compute checksum
for non-TCP/UDP packets, like ICMP with wrong checksum

Fixes: 5d1549847c76 ("netfilter: Fix remainder of pseudo-header protocol 0")
Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
diff with v1:
rewrite the code as suggested
add fixes tag

 net/netfilter/utils.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/utils.c b/net/netfilter/utils.c
index 51b454d8fa9c..0f78416566fa 100644
--- a/net/netfilter/utils.c
+++ b/net/netfilter/utils.c
@@ -17,12 +17,21 @@ __sum16 nf_ip_checksum(struct sk_buff *skb, unsigned int hook,
 	case CHECKSUM_COMPLETE:
 		if (hook != NF_INET_PRE_ROUTING && hook != NF_INET_LOCAL_IN)
 			break;
-		if ((protocol != IPPROTO_TCP && protocol != IPPROTO_UDP &&
-		    !csum_fold(skb->csum)) ||
-		    !csum_tcpudp_magic(iph->saddr, iph->daddr,
-				       skb->len - dataoff, protocol,
-				       skb->csum)) {
-			skb->ip_summed = CHECKSUM_UNNECESSARY;
+		switch (protocol) {
+		case IPPROTO_TCP:
+		case IPPROTO_UDP:
+			if (!csum_tcpudp_magic(iph->saddr, iph->daddr,
+					    skb->len - dataoff,
+					    protocol, skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				return 0;
+			}
+			break;
+		default:
+			if (!csum_fold(skb->csum)) {
+				skb->ip_summed = CHECKSUM_UNNECESSARY;
+				return 0;
+			}
 			break;
 		}
 		/* fall through */
-- 
2.16.2

Re: [PATCH][v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets

[Pablo Neira Ayuso]

Hi,

On Wed, Nov 13, 2019 at 03:34:01PM +0800, Li RongQing wrote:
> csum_tcpudp_magic should not be called to compute checksum
> for non-TCP/UDP packets, like ICMP with wrong checksum
> 
> Fixes: 5d1549847c76 ("netfilter: Fix remainder of pseudo-header protocol 0")
> Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> diff with v1:
> rewrite the code as suggested
> add fixes tag
> 
>  net/netfilter/utils.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/net/netfilter/utils.c b/net/netfilter/utils.c
> index 51b454d8fa9c..0f78416566fa 100644
> --- a/net/netfilter/utils.c
> +++ b/net/netfilter/utils.c
> @@ -17,12 +17,21 @@ __sum16 nf_ip_checksum(struct sk_buff *skb, unsigned int hook,
>  	case CHECKSUM_COMPLETE:
>  		if (hook != NF_INET_PRE_ROUTING && hook != NF_INET_LOCAL_IN)
>  			break;
> -		if ((protocol != IPPROTO_TCP && protocol != IPPROTO_UDP &&
> -		    !csum_fold(skb->csum)) ||
> -		    !csum_tcpudp_magic(iph->saddr, iph->daddr,
> -				       skb->len - dataoff, protocol,
> -				       skb->csum)) {

Could you describe what you observe there to tag this patch as a Fix?

Thanks.

答复: [PATCH][v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets

[Li,Rongqing]

> > -				       skb->len - dataoff, protocol,
> > -				       skb->csum)) {
> 
> Could you describe what you observe there to tag this patch as a Fix?
> 

nothing,  this fix tag can be dropped

I find this unreasonable codes when I read, it should have little negative effect.

thanks

-Li