NetBSD Problem Report #43541

From www@NetBSD.org  Mon Jun 28 16:31:07 2010
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id DEEB563BA69
	for <gnats-bugs@gnats.NetBSD.org>; Mon, 28 Jun 2010 16:31:06 +0000 (UTC)
Message-Id: <20100628163106.71C6A63BA68@www.NetBSD.org>
Date: Mon, 28 Jun 2010 16:31:06 +0000 (UTC)
From: gandersen@cradlepoint.com
Reply-To: gandersen@cradlepoint.com
To: gnats-bugs@NetBSD.org
Subject: Unaligned access in pf_normalize_tcpopt() 
X-Send-Pr-Version: www-1.0

>Number:         43541
>Category:       kern
>Synopsis:       Unaligned access in pf_normalize_tcpopt()
>Confidential:   no
>Severity:       serious
>Priority:       low
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Jun 28 16:35:00 +0000 2010
>Last-Modified:  Sun Jul 31 23:10:02 +0000 2016
>Originator:     Gregory T. Andersen
>Release:        5.0.2
>Organization:
CradlePoint Technology
>Environment:
NetBSD  5.0.2 NetBSD 5.0.2 (PUCK) #0: Fri Jun 25 11:18:40 MDT 2010  andersen@andersen:/build/sys/arch/evbmips/compile/obj/PUCK evbmips
>Description:
Ran into a unaligned access kernel crasher on a MIPS32 architecture when certain PF rules are applied that manipulate tcp options (mss).
>How-To-Repeat:
Receive TCP packets where the MSS TCP option is unaligned on architecture that doesn't handle unaligned access with mss scrub rule in PF.
>Fix:
Potential patch:

Index: sys/dist/pf/net/pf_norm.c
===================================================================
--- sys/dist/pf/net/pf_norm.c
+++ sys/dist/pf/net/pf_norm.c	(working copy)
@@ -1878,7 +1878,7 @@
 pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
     int off)
 {
-	u_int16_t	*mss;
+	u_int16_t	 mss;
 	int		 thoff;
 	int		 opt, cnt, optlen = 0;
 	int		 rewrite = 0;
@@ -1903,11 +1903,12 @@
 		}
 		switch (opt) {
 		case TCPOPT_MAXSEG:
-			mss = (u_int16_t *)(optp + 2);
-			if ((ntohs(*mss)) > r->max_mss) {
+			mss = (optp[2] << 8) | optp[3];
+			if (mss > r->max_mss) {
 				th->th_sum = pf_cksum_fixup(th->th_sum,
-				    *mss, htons(r->max_mss), 0);
-				*mss = htons(r->max_mss);
+				    htons(mss), htons(r->max_mss), 0);
+				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
+				optp[3] = (u_char)(r->max_mss) & 0xff;
 				rewrite = 1;
 			}
 			break;

>Audit-Trail:
From: matthew green <mrg@eterna.com.au>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@netbsd.org, gnats-admin@netbsd.org,
    netbsd-bugs@netbsd.org, Hisashi T Fujinaka <htodd@twofifty.com>
Subject: re: kern/43541: Unaligned access in pf_normalize_tcpopt() 
Date: Tue, 29 Jun 2010 04:15:00 +1000

 i wonder if this is the same problem as reported in:

   port-sparc/40629: kernel panic under high network load using pf (and ipv6)

 hisashi, can you try the patch in this PR for your problem?
 (included below again.)


 .mrg.


 Index: sys/dist/pf/net/pf_norm.c
 ===================================================================
 --- sys/dist/pf/net/pf_norm.c
 +++ sys/dist/pf/net/pf_norm.c	(working copy)
 @@ -1878,7 +1878,7 @@
  pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
      int off)
  {
 -	u_int16_t	*mss;
 +	u_int16_t	 mss;
  	int		 thoff;
  	int		 opt, cnt, optlen = 0;
  	int		 rewrite = 0;
 @@ -1903,11 +1903,12 @@
  		}
  		switch (opt) {
  		case TCPOPT_MAXSEG:
 -			mss = (u_int16_t *)(optp + 2);
 -			if ((ntohs(*mss)) > r->max_mss) {
 +			mss = (optp[2] << 8) | optp[3];
 +			if (mss > r->max_mss) {
  				th->th_sum = pf_cksum_fixup(th->th_sum,
 -				    *mss, htons(r->max_mss), 0);
 -				*mss = htons(r->max_mss);
 +				    htons(mss), htons(r->max_mss), 0);
 +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
 +				optp[3] = (u_char)(r->max_mss) & 0xff;
  				rewrite = 1;
  			}
  			break;

From: Hisashi T Fujinaka <htodd@twofifty.com>
To: matthew green <mrg@eterna.com.au>
Cc: gnats-bugs@NetBSD.org, kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org,
        netbsd-bugs@NetBSD.org
Subject: re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 28 Jun 2010 16:01:12 -0700 (PDT)

 I've switched to a sparc64 system and had a hard time causing the hang,
 so I think I better ask for this to be closed. I still have the old
 system, but I'm not sure how I'd trigger this.

 Thanks.

 On Tue, 29 Jun 2010, matthew green wrote:

 >
 > i wonder if this is the same problem as reported in:
 >
 >  port-sparc/40629: kernel panic under high network load using pf (and ipv6)
 >
 > hisashi, can you try the patch in this PR for your problem?
 > (included below again.)
 >
 >
 > .mrg.
 >
 >
 > Index: sys/dist/pf/net/pf_norm.c
 > ===================================================================
 > --- sys/dist/pf/net/pf_norm.c
 > +++ sys/dist/pf/net/pf_norm.c	(working copy)
 > @@ -1878,7 +1878,7 @@
 > pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
 >     int off)
 > {
 > -	u_int16_t	*mss;
 > +	u_int16_t	 mss;
 > 	int		 thoff;
 > 	int		 opt, cnt, optlen = 0;
 > 	int		 rewrite = 0;
 > @@ -1903,11 +1903,12 @@
 > 		}
 > 		switch (opt) {
 > 		case TCPOPT_MAXSEG:
 > -			mss = (u_int16_t *)(optp + 2);
 > -			if ((ntohs(*mss)) > r->max_mss) {
 > +			mss = (optp[2] << 8) | optp[3];
 > +			if (mss > r->max_mss) {
 > 				th->th_sum = pf_cksum_fixup(th->th_sum,
 > -				    *mss, htons(r->max_mss), 0);
 > -				*mss = htons(r->max_mss);
 > +				    htons(mss), htons(r->max_mss), 0);
 > +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
 > +				optp[3] = (u_char)(r->max_mss) & 0xff;
 > 				rewrite = 1;
 > 			}
 > 			break;
 >

 -- 
 Hisashi T Fujinaka - htodd@twofifty.com
 BSEE(6/86) + BSChem(3/95) + BAEnglish(8/95) + MSCS(8/03) + $2.50 = latte

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Tue, 29 Jun 2010 20:12:11 +0000

 On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
  > +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
  > +				optp[3] = (u_char)(r->max_mss) & 0xff;

 Do those casts serve any purpose?

 otherwise, seems like a desirable patch...

 -- 
 David A. Holland
 dholland@netbsd.org

From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Wed, 30 Jun 2010 00:21:05 +0100

 On Tue, Jun 29, 2010 at 08:15:05PM +0000, David Holland wrote:
 > The following reply was made to PR kern/43541; it has been noted by GNATS.
 > 
 > From: David Holland <dholland-bugs@netbsd.org>
 > To: gnats-bugs@NetBSD.org
 > Cc: 
 > Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
 > Date: Tue, 29 Jun 2010 20:12:11 +0000
 > 
 >  On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
 >   > +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
 >   > +				optp[3] = (u_char)(r->max_mss) & 0xff;
 >  
 >  Do those casts serve any purpose?

 Do the '& 0xff' serve any purpose ?

 The compiler may well generate code that 'and's the value with 0xff twice
 before storing the low 8 bits away!

 OTOH reversing the two lines may well save a temporary register.
 Hmmm... actually you want to cache r->max_rss in a local, the compiler
 is most likely required to re-read r->max_rss between the two statements.

 	David

 -- 
 David Laight: david@l8s.co.uk

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Tue, 29 Jun 2010 23:35:13 +0000

 On Tue, Jun 29, 2010 at 11:20:05PM +0000, David Laight wrote:
  >>> +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
  >>> +				optp[3] = (u_char)(r->max_mss) & 0xff;
  >>  
  >>  Do those casts serve any purpose?
  >  
  >  Do the '& 0xff' serve any purpose ?

 Arguably not, but they at least can't mask other bugs.

  >  The compiler may well generate code that 'and's the value with 0xff twice
  >  before storing the low 8 bits away!

 Not very likely, except perhaps with gcc -O0, which is so stupid
 anyway that there's no point worrying about it.

  >  OTOH reversing the two lines may well save a temporary register.
  >  Hmmm... actually you want to cache r->max_rss in a local, the compiler
  >  is most likely required to re-read r->max_rss between the two statements.

 Why would it be? It's not IIRC marked volatile.

 -- 
 David A. Holland
 dholland@netbsd.org

From: David Laight <david@l8s.co.uk>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 5 Jul 2010 21:22:46 +0100

 On Tue, Jun 29, 2010 at 11:40:05PM +0000, David Holland wrote:
 > 
 >  On Tue, Jun 29, 2010 at 11:20:05PM +0000, David Laight wrote:
 >   >>> +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
 >   >>> +				optp[3] = (u_char)(r->max_mss) & 0xff;
 >   >>  
 >   >>  Do those casts serve any purpose?
 >   >  
 >   >  Do the '& 0xff' serve any purpose ?
 >  
 >  Arguably not, but they at least can't mask other bugs.
 >  
 >   >  The compiler may well generate code that 'and's the value with 0xff twice
 >   >  before storing the low 8 bits away!
 >  
 >  Not very likely, except perhaps with gcc -O0, which is so stupid
 >  anyway that there's no point worrying about it.

 Not in my experience!
 I've certainly seen compilers mask with 0xff then save the low bits.

 >   >  OTOH reversing the two lines may well save a temporary register.
 >   >  Hmmm... actually you want to cache r->max_rss in a local, the compiler
 >   >  is most likely required to re-read r->max_rss between the two statements.
 >  
 >  Why would it be? It's not IIRC marked volatile.

 The aliasing rules ...

 	David

 -- 
 David Laight: david@l8s.co.uk

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Mon, 5 Jul 2010 21:07:27 +0000

 On Mon, Jul 05, 2010 at 08:20:03PM +0000, David Laight wrote:
  >>>  The compiler may well generate code that 'and's the value with 0xff
  >>>  twice before storing the low 8 bits away!
  >>  
  >>  Not very likely, except perhaps with gcc -O0, which is so stupid
  >>  anyway that there's no point worrying about it.
  >  
  >  Not in my experience!
  >  I've certainly seen compilers mask with 0xff then save the low bits.

 Well, I've also *seen* compilers store temporary values to scratch
 slots in the stack and immediately read them back. The question is
 whether a reasonably sane and modernish compiler that's been at least
 somewhat debugged is going to do anything so silly.

 However, it's also moot because such micro-optimizations are not worth
 spending time thinking about.

  >>>  OTOH reversing the two lines may well save a temporary register.
  >>>  Hmmm... actually you want to cache r->max_rss in a local, the compiler
  >>>  is most likely required to re-read r->max_rss between the two statements.
  >>  
  >>  Why would it be? It's not IIRC marked volatile.
  >  
  >  The aliasing rules ...

 Hrm, perhaps, I guess proving that optp[2] and r->max_rss don't
 overlap would require global analysis.

 -- 
 David A. Holland
 dholland@netbsd.org

From: David Holland <dholland-bugs@netbsd.org>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: kern/43541: Unaligned access in pf_normalize_tcpopt()
Date: Sun, 31 Jul 2016 23:08:15 +0000

 On Mon, Jun 28, 2010 at 04:35:00PM +0000, gandersen@cradlepoint.com wrote:
  > Index: sys/dist/pf/net/pf_norm.c
  > ===================================================================
  > --- sys/dist/pf/net/pf_norm.c
  > +++ sys/dist/pf/net/pf_norm.c	(working copy)
  > @@ -1878,7 +1878,7 @@
  >  pf_normalize_tcpopt(struct pf_rule *r, struct mbuf *m, struct tcphdr *th,
  >      int off)
  >  {
  > -	u_int16_t	*mss;
  > +	u_int16_t	 mss;
  >  	int		 thoff;
  >  	int		 opt, cnt, optlen = 0;
  >  	int		 rewrite = 0;
  > @@ -1903,11 +1903,12 @@
  >  		}
  >  		switch (opt) {
  >  		case TCPOPT_MAXSEG:
  > -			mss = (u_int16_t *)(optp + 2);
  > -			if ((ntohs(*mss)) > r->max_mss) {
  > +			mss = (optp[2] << 8) | optp[3];
  > +			if (mss > r->max_mss) {
  >  				th->th_sum = pf_cksum_fixup(th->th_sum,
  > -				    *mss, htons(r->max_mss), 0);
  > -				*mss = htons(r->max_mss);
  > +				    htons(mss), htons(r->max_mss), 0);
  > +				optp[2] = (u_char)(r->max_mss >> 8) & 0xff;
  > +				optp[3] = (u_char)(r->max_mss) & 0xff;
  >  				rewrite = 1;
  >  			}
  >  			break;
  > 

 Is there any reason this patch hasn't been committed?

 -- 
 David A. Holland
 dholland@netbsd.org

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.