[v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets
diff mbox series

Message ID 1573630441-13937-1-git-send-email-lirongqing@baidu.com
State Not Applicable
Delegated to: Pablo Neira
Headers show
Series
  • [v2] netfilter: only call csum_tcpudp_magic for TCP/UDP packets
Related show

Commit Message

Li,Rongqing Nov. 13, 2019, 7:34 a.m. UTC
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>
---

Comments

Pablo Neira Ayuso Nov. 29, 2019, 8:10 a.m. UTC | #1
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.
Li,Rongqing Dec. 3, 2019, 2:42 a.m. UTC | #2
> > -				       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

Patch
diff mbox series

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 */