Skip to contentSkip to conversation

New: snmp trap ALG issue

Bugme-new In review Andrew Morton 3 participants 7 messages Sep 14, 2010 – Sep 20, 2010 3 messages not in the archive

Subsystems (3)
Networking Network drivers netfilter
lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]/

Andrew Morton

author
#1 lore raw

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

Quoted 28 lines · On Thu, 2 Sep 2010 09:25:12 GMT [email protected] 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: [email protected]
        ReportedBy: [email protected]
        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.

Quoted 4 lines

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

Quoted 16 lines

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 {
Quoted 8 lines
        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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw

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 <[email protected]>
---
 include/net/checksum.h                 |    5 +++++
 net/ipv4/netfilter/nf_nat_snmp_basic.c |   31 ++-----------------------------
 2 files changed, 7 insertions(+), 29 deletions(-)
net/ipv4/netfilter/nf_nat_snmp_basic.c +2 -29
@@ -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",
include/net/checksum.h +5 -0
@@ -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);
Applied
#4 lore raw

Am 17.09.2010 07:39, schrieb Stephen Hemminger:

Quoted 6 lines

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.

#5 lore raw
Quoted 9 lines · On Fri, 17 Sep 2010 14:08:56 +0200 Patrick McHardy <[email protected]> wrote:

Am 17.09.2010 07:39, schrieb Stephen Hemminger:

Quoted 6 lines

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.

#6 lore raw

Am 17.09.2010 17:31, schrieb Stephen Hemminger:

Quoted 11 lines
Quoted 8 lines · On Fri, 17 Sep 2010 14:08:56 +0200 Patrick McHardy <[email protected]> wrote:

Am 17.09.2010 07:39, schrieb Stephen Hemminger:

Quoted 6 lines

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.

Quoted 5 lines
Quoted 1 line

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

PATCH 1/1 nf_nat_snmp: fix checksum calculation (v3)

Revised version of the original patch in the bug
https://bugzilla.kernel.org/show_bug.cgi?id=17622
from clark wang <[email protected]>

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 <[email protected]>

---

net/ipv4/netfilter/nf_nat_snmp_basic.c +13 -17
@@ -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",
#9 lore raw

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

Quoted 10 lines · Stephen Hemminger

Revised version of the original patch in the bug
https://bugzilla.kernel.org/show_bug.cgi?id=17622
from clark wang <[email protected]>

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 <[email protected]>

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:

commit 8d70d82cdcc6da0a77440c6d860957a1f50b8089

Author: Patrick McHardy <[email protected]>
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 <[email protected]> and Stephen Hemminger <[email protected]>.

https://bugzilla.kernel.org/show_bug.cgi?id=17622

Signed-off-by Patrick McHardy <[email protected]>
net/ipv4/netfilter/nf_nat_snmp_basic.c +4 -2
index 1679e2c..ee5f419 100644
@@ -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)));