Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

[Andrew Morton]

(switched to email.  Please respond via emailed reply-to-all, not via the
bugzilla web interface).

On Thu, 2 Sep 2010 09:25:12 GMT
bugzilla-daemon@bugzilla.kernel.org wrote:

> https://bugzilla.kernel.org/show_bug.cgi?id=17622
> 
>            Summary: snmp trap ALG issue
>            Product: Networking
>            Version: 2.5
>     Kernel Version: 2.6.35.4
>           Platform: All
>         OS/Version: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: IPV4
>         AssignedTo: shemminger@linux-foundation.org
>         ReportedBy: wtweeker@163.com
>         Regression: No
> 
> 
> Symptom:
> SNMP manager can't show trap when SNMP agent set trap message to  version1.
> 
> steps to reproduce:
> (1)SNMP agent-----linux device(NAT)-----SNMP manager.
> (2)Set SNMP agent trap message to version 1, destination IP as SNMP   manger's
> IP.
> (3)Do some operation to generate trap message, such as make one port of SNMP
> agent up and down. But SNMP manger can't accept trap message.
> 
> I have capured the packet by Ethereal software, and check the SNMP trap packet,
> found that the UDP checksum is incorrect. 
> I think that the function fast_csum()(nf_nat_snmp_basic.c) have some problem.
> 

and


> I have changed this function by refering to other checksum algorithm.
> And tested it, it can work. the checksum is correct. 
> 
> static void fast_csum(__sum16 *csum,
>               const unsigned char *optr,
>               const unsigned char *nptr,
>               int offset)
> {
>     unsigned char s[4];
> 
>     if (offset & 1) {
>         s[0] = s[2] = 0;
>         s[0] = ~s[0]; //this line is add by me
>         s[1] = ~*optr;
>         s[3] = *nptr;
>     } else {
>         s[1] = s[3] = 0;
>         s[1] = ~s[1];//this line is add by me
>         s[0] = ~*optr;
>         s[2] = *nptr;
>     }
> 
>     *csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
> }

Great.  Please prepare a kernel patch as per
Documentation/SubmittingPatches and send it via a reply-to-all to this
email?

Thanks.

Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

[Stephen Hemminger]

I think the bug should be fixed by removing the potentially buggy
fast_csum() in nf_nat_snmp_basic and just using the existing
generic code. The following is compile tested only..

Subject: [PATCH] nf_nat_snmp: use existing checksum update code

The fast_csum() in NAT code for processing SNMP trap is buggy 
(see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
Replace it by using the existing checksum replacement code;
it means adding a new csum_replace1() inline wrapper.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 include/net/checksum.h                 |    5 +++++
 net/ipv4/netfilter/nf_nat_snmp_basic.c |   31 ++-----------------------------
 2 files changed, 7 insertions(+), 29 deletions(-)

--- a/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-16 22:17:21.660806917 -0700
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-16 22:32:52.084075112 -0700
@@ -882,30 +882,6 @@ static unsigned char snmp_request_decode
 }
 
 /*
- * Fast checksum update for possibly oddly-aligned UDP byte, from the
- * code example in the draft.
- */
-static void fast_csum(__sum16 *csum,
-		      const unsigned char *optr,
-		      const unsigned char *nptr,
-		      int offset)
-{
-	unsigned char s[4];
-
-	if (offset & 1) {
-		s[0] = s[2] = 0;
-		s[1] = ~*optr;
-		s[3] = *nptr;
-	} else {
-		s[1] = s[3] = 0;
-		s[0] = ~*optr;
-		s[2] = *nptr;
-	}
-
-	*csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
-}
-
-/*
  * Mangle IP address.
  * 	- begin points to the start of the snmp messgae
  *      - addr points to the start of the address
@@ -924,11 +900,8 @@ static inline void mangle_address(unsign
 		*addr = map->to;
 
 		/* Update UDP checksum if being used */
-		if (*check) {
-			fast_csum(check,
-				  &map->from, &map->to, addr - begin);
-
-		}
+		if (*check)
+			csum_replace1(check, map->from, map->to);
 
 		if (debug)
 			printk(KERN_DEBUG "bsalg: mapped %pI4 to %pI4\n",
--- a/include/net/checksum.h	2010-09-16 22:31:27.524503074 -0700
+++ b/include/net/checksum.h	2010-09-16 22:32:09.934282263 -0700
@@ -106,6 +106,11 @@ static inline void csum_replace2(__sum16
 	csum_replace4(sum, (__force __be32)from, (__force __be32)to);
 }
 
+static inline void csum_replace1(__sum16 *sum, __u8 from, __u8 to)
+{
+	csum_replace4(sum, (__force __be32)from, (__force __be32)to);
+}
+
 struct sk_buff;
 extern void inet_proto_csum_replace4(__sum16 *sum, struct sk_buff *skb,
 				     __be32 from, __be32 to, int pseudohdr);

Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

[Patrick McHardy]

Am 17.09.2010 07:39, schrieb Stephen Hemminger:
> nf_nat_snmp: use existing checksum update code
> 
> The fast_csum() in NAT code for processing SNMP trap is buggy 
> (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
> Replace it by using the existing checksum replacement code;
> it means adding a new csum_replace1() inline wrapper.

Applied, thanks Stephen.

Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

[Stephen Hemminger]

On Fri, 17 Sep 2010 14:08:56 +0200
Patrick McHardy <kaber@trash.net> wrote:

> Am 17.09.2010 07:39, schrieb Stephen Hemminger:
> > nf_nat_snmp: use existing checksum update code
> > 
> > The fast_csum() in NAT code for processing SNMP trap is buggy 
> > (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
> > Replace it by using the existing checksum replacement code;
> > it means adding a new csum_replace1() inline wrapper.
> 
> Applied, thanks Stephen.

As I said in the patch, this was compile tested only. It would
be good if the original Bug submitter validated that this
fixed the problem.

-- 

Re: [Bugme-new] [Bug 17622] New: snmp trap ALG issue

[Patrick McHardy]

Am 17.09.2010 17:31, schrieb Stephen Hemminger:
> On Fri, 17 Sep 2010 14:08:56 +0200
> Patrick McHardy <kaber@trash.net> wrote:
> 
>> Am 17.09.2010 07:39, schrieb Stephen Hemminger:
>>> nf_nat_snmp: use existing checksum update code
>>>
>>> The fast_csum() in NAT code for processing SNMP trap is buggy 
>>> (see https://bugzilla.kernel.org/show_bug.cgi?id=17622)
>>> Replace it by using the existing checksum replacement code;
>>> it means adding a new csum_replace1() inline wrapper.
>>
>> Applied, thanks Stephen.
> 
> As I said in the patch, this was compile tested only. It would
> be good if the original Bug submitter validated that this
> fixed the problem.

Thanks, that slipped past my eyes. I'll make sure to wait for
confirmation from the reporter or try to test this myself before
sending it upstream.

[PATCH] nf_nat_snmp: fix checksum calculation (v3)

[Stephen Hemminger]

Revised version of the original patch in the bug
  https://bugzilla.kernel.org/show_bug.cgi?id=17622
from clark wang <wtweeker@163.com>

I took the opportunity to do some cleanup here.
 * reorder the assignment to make the byte order clear
 * get rid of unnecessary ref/deref and just pass the bytes
 * use sizeof() instead of hard coding size

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---


--- a/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-20 09:05:11.752067965 -0700
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c	2010-09-20 09:38:48.518661840 -0700
@@ -885,24 +885,23 @@ static unsigned char snmp_request_decode
  * Fast checksum update for possibly oddly-aligned UDP byte, from the
  * code example in the draft.
  */
-static void fast_csum(__sum16 *csum,
-		      const unsigned char *optr,
-		      const unsigned char *nptr,
-		      int offset)
+static void fast_csum(__sum16 *csum, __u8 from, __u8 to, int offset)
 {
-	unsigned char s[4];
+	__be16 diff[2];
 
 	if (offset & 1) {
-		s[0] = s[2] = 0;
-		s[1] = ~*optr;
-		s[3] = *nptr;
+		s[0] = ~0;
+		s[1] = ~from;
+		s[2] = 0;
+		s[3] = to;
 	} else {
-		s[1] = s[3] = 0;
-		s[0] = ~*optr;
-		s[2] = *nptr;
+		s[0] = ~from;
+		s[1] = ~0;
+		s[2] = to;
+		s[3] = 0;
 	}
 
-	*csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));
+	*csum = csum_fold(csum_partial(s, sizeof(s), ~csum_unfold(*csum)));
 }
 
 /*
@@ -924,11 +923,8 @@ static inline void mangle_address(unsign
 		*addr = map->to;
 
 		/* Update UDP checksum if being used */
-		if (*check) {
-			fast_csum(check,
-				  &map->from, &map->to, addr - begin);
-
-		}
+		if (*check)
+			fast_csum(check, map->from, map->to, addr - begin);
 
 		if (debug)
 			printk(KERN_DEBUG "bsalg: mapped %pI4 to %pI4\n",

Re: [PATCH] nf_nat_snmp: fix checksum calculation (v3)

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

On 20.09.2010 18:44, Stephen Hemminger wrote:
> Revised version of the original patch in the bug
>   https://bugzilla.kernel.org/show_bug.cgi?id=17622
> from clark wang <wtweeker@163.com>
> 
> I took the opportunity to do some cleanup here.
>  * reorder the assignment to make the byte order clear
>  * get rid of unnecessary ref/deref and just pass the bytes
>  * use sizeof() instead of hard coding size
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

Thanks Stephen, but this patch didn't compile:

net/ipv4/netfilter/nf_nat_snmp_basic.c: In function 'fast_csum':
net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: 's' undeclared (first
use in this function)
net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: (Each undeclared
identifier is reported only once
net/ipv4/netfilter/nf_nat_snmp_basic.c:893: error: for each function it
appears in.)
net/ipv4/netfilter/nf_nat_snmp_basic.c:890: warning: unused variable 'diff'

Since I prefer to keep this fix to the minimal size at this point,
I've committed this patch based on Clark's and your patches:


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1031 bytes --]

commit 8d70d82cdcc6da0a77440c6d860957a1f50b8089
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Sep 20 19:29:40 2010 +0200

    netfilter: nf_nat_snmp: fix checksum calculation (v4)
    
    Fix checksum calculation in nf_nat_snmp_basic.
    
    Based on patches by Clark Wang <wtweeker@163.com> and
    Stephen Hemminger <shemminger@vyatta.com>.
    
    https://bugzilla.kernel.org/show_bug.cgi?id=17622
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/ipv4/netfilter/nf_nat_snmp_basic.c b/net/ipv4/netfilter/nf_nat_snmp_basic.c
index 1679e2c..ee5f419 100644
--- a/net/ipv4/netfilter/nf_nat_snmp_basic.c
+++ b/net/ipv4/netfilter/nf_nat_snmp_basic.c
@@ -893,13 +893,15 @@ static void fast_csum(__sum16 *csum,
 	unsigned char s[4];
 
 	if (offset & 1) {
-		s[0] = s[2] = 0;
+		s[0] = ~0;
 		s[1] = ~*optr;
+		s[2] = 0;
 		s[3] = *nptr;
 	} else {
-		s[1] = s[3] = 0;
 		s[0] = ~*optr;
+		s[1] = ~0;
 		s[2] = *nptr;
+		s[3] = 0;
 	}
 
 	*csum = csum_fold(csum_partial(s, 4, ~csum_unfold(*csum)));