NetBSD Problem Report #44059

From www@NetBSD.org  Sun Nov  7 17:39:50 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 332B063BAC6
	for <gnats-bugs@gnats.NetBSD.org>; Sun,  7 Nov 2010 17:39:50 +0000 (UTC)
Message-Id: <20101107173950.117DB63BABA@www.NetBSD.org>
Date: Sun,  7 Nov 2010 17:39:50 +0000 (UTC)
From: l.illanes@gmx.de
Reply-To: l.illanes@gmx.de
To: gnats-bugs@NetBSD.org
Subject: panic in pf_modulate_sack after receipt of an TCP segment specifying invalid options
X-Send-Pr-Version: www-1.0

>Number:         44059
>Category:       kern
>Synopsis:       panic in pf_modulate_sack after receipt of an TCP segment specifying invalid options
>Confidential:   no
>Severity:       critical
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 07 17:40:00 +0000 2010
>Closed-Date:    Sat Feb 10 06:35:53 +0000 2018
>Last-Modified:  Sat Feb 10 06:35:53 +0000 2018
>Originator:     Lucio Albornoz
>Release:        NetBSD 5.0.1-RELEASE
>Organization:
>Environment:
NetBSD amal.arabs.ps 5.0.1 NetBSD 5.0.1 (AMAL) #0: Fri Nov  5 23:54:02 CET 2010  toor@amal.arabs.ps:/usr/src/sys/arch/i386/compile/AMAL i386

>Description:
	pf_modulate_sack may cause a panic(9) given pf(4) and receipt of certain TCP
segments amending, at least as exhibited in this case, the unassigned TCP option 81
(pertaining to the Unassigned range as per[1]) alongside a length octet disagreeing
with the total header size as per the Data offset field.  The resulting backtrace
analysed post-mortem appears, on the surface, quite similar to[2]; unfortunately
however, no further details relating to the immediate circumstance were included and
thus may or may not serve as reference.

#0  0xc03592dd in cpu_reboot (howto=260, bootstr=0x0) at ../../../../arch/i386/i386/machdep.c:928
#1  0xc02bce9b in panic (fmt=0xc0417f7b "trap") at ../../../../kern/subr_prf.c:253
#2  0xc035bc76 in trap (frame=0xcb07d774) at ../../../../arch/i386/i386/trap.c:367
#3  0xc010cb60 in calltrap ()
#4  0xc03549f6 in db_read_bytes (addr=4, size=4, data=0xcb07d7dc "\200!g\n<&#9618;\a&#9618;<&#9618;\a&#9618;\004") at ../../../../arch/i386/i386/db_memrw.c:91
#5  0xc019f552 in db_get_value (addr=4, size=4, is_signed=false) at ../../../../ddb/db_access.c:62
#6  0xc0355337 in db_stack_trace_print (addr=-888678188, have_addr=true, count=65535, modif=0xc0403e6c "", pr=0xc02bcc8c <printf>)
    at ../../../../arch/i386/i386/db_trace.c:147
#7  0xc02bce6c in panic (fmt=0xc0417f7b "trap") at ../../../../kern/subr_prf.c:242
#8  0xc035bc76 in trap (frame=0xcb07d998) at ../../../../arch/i386/i386/trap.c:367
#9  0xc010cb60 in calltrap ()
#10 0xc017f9e9 in pf_modulate_sack (m=0xc2221800, off=20, pd=0xcb07dbb4, th=0xcb07dc2c, dst=0xc2706e60) at ../../../../dist/pf/net/pf.c:1600
#11 0xc0188261 in pf_test_state_tcp (state=0xcb07dc48, direction=1, kif=0xc19ad400, m=0xc2221800, off=20, h=0xcac82016, pd=0xcb07dbb4,
    reason=0xcb07dc52) at ../../../../dist/pf/net/pf.c:4060
#12 0xc018a86f in pf_test (dir=1, ifp=0xc19e0000, m0=0xcb07dcc0, eh=0x0) at ../../../../dist/pf/net/pf.c:5818
#13 0xc018d4ba in pfil4_wrapper (arg=0x0, mp=0xcb07dcc0, ifp=0xc19e0000, dir=1) at ../../../../dist/pf/net/pf_ioctl.c:3184
#14 0xc0322fd1 in pfil_run_hooks (ph=0xc0466640, mp=0xcb07dd28, ifp=0xc19e0000, dir=1) at ../../../../net/pfil.c:70
#15 0xc0125e9a in ip_input (m=0xc2221800) at ../../../../netinet/ip_input.c:662
#16 0xc0126702 in ipintr () at ../../../../netinet/ip_input.c:476
#17 0xc02a32fd in softint_dispatch (pinned=0xcab07c80, s=4) at ../../../../kern/kern_softint.c:534
#18 0xc0100e4d in Xsoftintr ()
#19 0x00000000 in ?? ()

	The immediate culprit as visible from the core dump appears to comprise
the following condition:

	[ ... ]
	#10 0xc017f9e9 in pf_modulate_sack (m=0xc2221800, off=20, pd=0xcb07dbb4, th=0xcb07dc2c, dst=0xc2706e60) at ../../../../dist/pf/net/pf.c:1600
	1600                    olen = opt[1];
	(gdb) print opt
	$28 = (u_int8_t *) 0xcb099000 <Address 0xcb099000 out of bounds>
	[ ... ]

	A tcpdump (8) of the relevant headers [bar IP addresses and TCP ports] of the
offending TCP segment as contained within the supplied mbuf structure for quick
reference follows:

ip: (tos 0x0, ttl 116, id 23046, offset 0, flags [DF], proto TCP (6), length 1480) 0.0.0.0.0 > 0.0.0.0.0: FP, cksum 0x3f64 (correct), 3560130094:3560131502(1408) ack 1812274157 win 58486 <[bad opt]>
        0x0000:  4500 05c8 5a06 4000 7406 c56d 0000 0000  E...Z.@.t..m....
        0x0010:  0000 0000 0000 0000 d433 462e 6c05 1bed  .........3F.l...
        0x0020:  d019 e476 3f64 0000 519d 4575 76d9 92a4  ...v?d..Q.Euv...
        0x0030:  941f b737 f62a aa11 0bbe 9ff6 7102 e8fe  ...7.*......q...
        0x0040:  d06a 15aa b470 5ef6 4254 172c 6f85 1be0  .j...p^.BT.,o...

	I can provide the full TCP segment, core file, kernel, kernel configuration,
	etc. if necessary.

N.B.	The above behaviour has presented itself on numerous occasions over the
	timeframe of at least half a year, spanning NetBSD 5.0-RELEASE up until
	until 5.0.1-RELEASE, exhibiting an irregularity that may have one be led
	to presume either recurring hardware failure or a bug in the network stack
	either exploited or inadvertently triggered; I have thus far not
	investigated this occurrence up until now and do therefore unfortunately
	find myself incapable of providing coredumps or backtraces applying to
	prior incidents of the same nature.


[1] TCP Option Kind Numbers - <http://www.iana.org/assignments/tcp-parameters/tcp-parameters.xml>
[2] pf(4) panic in pf_modulate_sack() on netbsd-5 - <http://mail-index.netbsd.org/netbsd-users/2009/12/01/msg004975.html>
>How-To-Repeat:

>Fix:

>Release-Note:

>Audit-Trail:
From: "Maxime Villard" <maxv@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: src/sys/dist/pf/net
Date: Fri, 9 Feb 2018 21:25:04 +0000

 Module Name:	src
 Committed By:	maxv
 Date:		Fri Feb  9 21:25:04 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net: pf.c

 Log Message:
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.

 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.

 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.77 -r1.78 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->pending-pullups
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Fri, 09 Feb 2018 21:42:04 +0000
State-Changed-Why:
The issue is fixed in NetBSD-current. I've sent pullups.

Thank you very much for the report. It is a shame that it got ignored
for so long.


From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-8] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:12:17 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:12:17 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-8]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #540):
 	sys/dist/pf/net/pf.c: 1.77-1.78
 PR/52682: David Binderman: Fix wrong assignment (in the !__NetBSD__ code)
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.76 -r1.76.6.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-7-0] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:21:15 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:21:15 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-7-0]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1565):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.72 -r1.72.6.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-7-1] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:21:17 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:21:17 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-7-1]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1565):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.72 -r1.72.10.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-7] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:21:19 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:21:19 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-7]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1565):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.72 -r1.72.2.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-6-0] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:25:34 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:25:34 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-6-0]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1527):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.68 -r1.68.6.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-6-1] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:25:36 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:25:36 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-6-1]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1527):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.68 -r1.68.8.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44059 CVS commit: [netbsd-6] src/sys/dist/pf/net
Date: Sat, 10 Feb 2018 04:25:38 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Sat Feb 10 04:25:38 UTC 2018

 Modified Files:
 	src/sys/dist/pf/net [netbsd-6]: pf.c

 Log Message:
 Pull up following revision(s) (requested by maxv in ticket #1527):
 	sys/dist/pf/net/pf.c: revision 1.78 via patch
 Oh, what is this. Fix a remotely-triggerable integer overflow: the way we
 define TCPOLEN_SACK makes it unsigned, and the comparison in the while()
 is unsigned too. That's not the expected behavior, the original code
 wanted a signed comparison.
 It's pretty easy to make 'hlen' go negative and trigger a buffer overflow.
 This bug was reported 8 years ago by Lucio Albornoz in PR/44059.


 To generate a diff of this commit:
 cvs rdiff -u -r1.68 -r1.68.2.1 src/sys/dist/pf/net/pf.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Sat, 10 Feb 2018 06:35:53 +0000
State-Changed-Why:
The fix was applied in 6/7/8. I'm closing this PR.

Thanks again for the report.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.