NetBSD Problem Report #52609

From stix@stix.id.au  Tue Oct 10 13:14:58 2017
Return-Path: <stix@stix.id.au>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id ABEDD7A1AE
	for <gnats-bugs@gnats.NetBSD.org>; Tue, 10 Oct 2017 13:14:58 +0000 (UTC)
Message-Id: <20171010131451.5269D19E75@stix.id.au>
Date: Wed, 11 Oct 2017 00:14:51 +1100 (AEDT)
From: stix@stix.id.au
Reply-To: stix@stix.id.au
To: gnats-bugs@NetBSD.org
Subject: npf generates bad bytecode for port ranges
X-Send-Pr-Version: 3.95

>Number:         52609
>Category:       bin
>Synopsis:       npf generates bad bytecode for port ranges
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue Oct 10 13:15:00 +0000 2017
>Closed-Date:    Thu Aug 08 21:36:03 +0000 2019
>Last-Modified:  Sun Aug 11 10:15:01 +0000 2019
>Originator:     Paul Ripke
>Release:        NetBSD 8.0_BETA
>Organization:
Paul Ripke
"Great minds discuss ideas, average minds discuss events, small minds
 discuss people."
-- Disputed: Often attributed to Eleanor Roosevelt. 1948.
>Environment:
System: NetBSD slave 8.0_BETA NetBSD 8.0_BETA (SLAVE) #0: Sat Sep 9 12:02:11 AEST 2017 stix@slave:/home/netbsd/netbsd-8/obj.amd64/home/netbsd/netbsd-8/src/sys/arch/amd64/compile/SLAVE amd64
Architecture: x86_64
Machine: amd64
>Description:

Given rules containing a port list, where one of the elements is a port
range, the generated bytecode is incorrect.

>How-To-Repeat:

Given a sample config:

group "fubar" on wi0 {
        pass in final proto tcp to any port { 1, 4-5, 7 }
        pass in final proto tcp to any port { 4-5, 7 }
        pass in final proto tcp to any port { 1, 4-5 }
        pass in final proto tcp to any port { 4-5 }
}

The generated bytecode is:

RULE AT LINE 2
(000) ld       M[0]
(001) jeq      #0x0             jt 14   jf 2
(002) ld       M[2]
(003) jeq      #0x6             jt 4    jf 14
(004) ldx      M[1]
(005) ldh      [x + 2]
(006) jeq      #0x1             jt 13   jf 7
(007) ldh      [x + 2]
(008) jge      #0x4             jt 13   jf 9  <- should be: jt 9  jf 10
(009) jgt      #0x5             jt 10   jf 13
(010) ldh      [x + 2]
(011) jeq      #0x7             jt 13   jf 12
(012) ret      #0
(013) ret      #-1
(014) ret      #0

RULE AT LINE 3
(000) ld       M[0]
(001) jeq      #0x0             jt 12   jf 2
(002) ld       M[2]
(003) jeq      #0x6             jt 4    jf 12
(004) ldx      M[1]
(005) ldh      [x + 2]
(006) jge      #0x4             jt 11   jf 7  <- should be: jt 7  jf 8
(007) jgt      #0x5             jt 8    jf 11
(008) ldh      [x + 2]
(009) jeq      #0x7             jt 11   jf 10
(010) ret      #0
(011) ret      #-1
(012) ret      #0

RULE AT LINE 4
(000) ld       M[0]
(001) jeq      #0x0             jt 12   jf 2
(002) ld       M[2]
(003) jeq      #0x6             jt 4    jf 12
(004) ldx      M[1]
(005) ldh      [x + 2]
(006) jeq      #0x1             jt 11   jf 7
(007) ldh      [x + 2]
(008) jge      #0x4             jt 11   jf 9  <- should be: jt 9  jf 10 (or 12)
(009) jgt      #0x5             jt 10   jf 11
(010) ret      #0
(011) ret      #-1
(012) ret      #0

RULE AT LINE 5
(000) ld       M[0]
(001) jeq      #0x0             jt 9    jf 2
(002) ld       M[2]
(003) jeq      #0x6             jt 4    jf 9
(004) ldx      M[1]
(005) ldh      [x + 2]
(006) jge      #0x4             jt 7    jf 9  <- correct!
(007) jgt      #0x5             jt 9    jf 8
(008) ret      #-1
(009) ret      #0

Workaround: don't use a port range as a list element.

>Fix:

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->feedback
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Fri, 23 Mar 2018 09:52:18 +0000
State-Changed-Why:
I don't understand. How did you dump/reconstruct this bytecode? I
did test the other day and everything was fine.


From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/52609 (npf generates bad bytecode for port ranges)
Date: Mon, 26 Mar 2018 17:32:31 +1100

 This was on an amd64, NetBSD 8.0_BETA as of Sat Sep  9 12:02:11 AEST 2017.

 It's pretty easy to dump out the bytecode:

 slave:ksh$ cat /tmp/npf.conf 
 group "fubar" on wi0 {
         pass in final proto tcp to any port { 1, 4-5, 7 }
         pass in final proto tcp to any port { 4-5, 7 }
         pass in final proto tcp to any port { 1, 4-5 }
         pass in final proto tcp to any port { 4-5 }
 }
 slave:ksh$ npfctl debug /tmp/npf.conf /tmp/zz
 npfctl: warning - unknown interface 'wi0'
 npfctl: default group was not defined

 RULE AT LINE 2
 (000) ld       M[0]
 (001) jeq      #0x0             jt 14   jf 2
 (002) ld       M[2]
 (003) jeq      #0x6             jt 4    jf 14
 (004) ldx      M[1]
 (005) ldh      [x + 2]
 (006) jeq      #0x1             jt 13   jf 7
 ...

 I'm building -current, and I'll double check its behaviour when I get it
 installed somewhere.

From: Paul Ripke <stix@stix.id.au>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: bin/52609 (npf generates bad bytecode for port ranges)
Date: Fri, 13 Apr 2018 11:58:23 +1000

 Generated bytecode for netbsd-current as of 2018-03-26 is as above;
 and will result in unexpected behaviour.

 I think the generated bytecode could be cleaned up somewhat; the
 way it currently tries to reuse the ret's at the tail and fudge the
 generated jump offsets is rather messy.

State-Changed-From-To: feedback->open
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Fri, 13 Apr 2018 16:46:26 +0000
State-Changed-Why:
feedback received.


State-Changed-From-To: open->analyzed
State-Changed-By: maxv@NetBSD.org
State-Changed-When: Fri, 13 Apr 2018 17:41:35 +0000
State-Changed-Why:
After some investigation, it turns out that basically NPF's BPF generator
does not support the syntax { 4-5, 7 }.

This syntax generates:

(004) ldx      M[1]
(005) ldh      [x + 2]
(006) jge      #0x4             jt 11   jf 7
(007) jgt      #0x5             jt 8    jf 11
(008) ldh      [x + 2]
(009) jeq      #0x7             jt 11   jf 10
(010) ret      #0
(011) ret      #-1

Instruction 006 has inverted fields, because of fixup_jumps(). It is
rather clear that this one is a bug.

But even if you swap the fields back to their correct form, the
instruction will still be:

(006) jge      #0x4             jt 7    jf 11

which is incorrect, it should logically be 'jf 8' (as you said).

But NPF doesn't handle that logic. NPF creates a group, from instruction
005 to instruction 010 (included on both sides), and can either continue
in that group or skip the group entirely.

So in instruction 006, it can only do two things, continue in 007, or
leave the group in 011. You can't ask to jump to 008, in the middle of
the group.

Obviously, it would be nice to fix it, but I don't really know how to
achieve that. We can't replace the instruction by

	BPF_JUMP(BPF_JMP+BPF_JGE+BPF_K, from, 0, 1)

because if there is no other port in the syntax, then there's a fall-
through in the "success" path, and we don't want that to happen.


From: "Mindaugas Rasiukevicius" <rmind@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52609 CVS commit: src/usr.sbin/npf/npfctl
Date: Thu, 8 Aug 2019 21:29:16 +0000

 Module Name:	src
 Committed By:	rmind
 Date:		Thu Aug  8 21:29:16 UTC 2019

 Modified Files:
 	src/usr.sbin/npf/npfctl: npf_bpf_comp.c npf_build.c npfctl.h

 Log Message:
 NPF: fix BPF byte-code generation for a port-range used in a group.
 Resolved PR/52609 and PR/54169.


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.14 src/usr.sbin/npf/npfctl/npf_bpf_comp.c
 cvs rdiff -u -r1.50 -r1.51 src/usr.sbin/npf/npfctl/npf_build.c
 cvs rdiff -u -r1.48 -r1.49 src/usr.sbin/npf/npfctl/npfctl.h

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

State-Changed-From-To: analyzed->closed
State-Changed-By: rmind@NetBSD.org
State-Changed-When: Thu, 08 Aug 2019 21:36:03 +0000
State-Changed-Why:
Fixed.


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/52609 CVS commit: [netbsd-9] src/usr.sbin/npf/npfctl
Date: Sun, 11 Aug 2019 10:10:23 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sun Aug 11 10:10:23 UTC 2019

 Modified Files:
 	src/usr.sbin/npf/npfctl [netbsd-9]: npf_bpf_comp.c npf_build.c npfctl.h

 Log Message:
 Pull up following revision(s) (requested by rmind in ticket #44):

 	usr.sbin/npf/npfctl/npfctl.h: revision 1.49
 	usr.sbin/npf/npfctl/npf_build.c: revision 1.51
 	usr.sbin/npf/npfctl/npf_bpf_comp.c: revision 1.14

 NPF: fix BPF byte-code generation for a port-range used in a group.
 Resolved PR/52609 and PR/54169.


 To generate a diff of this commit:
 cvs rdiff -u -r1.13 -r1.13.2.1 src/usr.sbin/npf/npfctl/npf_bpf_comp.c
 cvs rdiff -u -r1.50 -r1.50.2.1 src/usr.sbin/npf/npfctl/npf_build.c
 cvs rdiff -u -r1.48 -r1.48.2.1 src/usr.sbin/npf/npfctl/npfctl.h

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

>Unformatted:

Home	PR Database Search