NetBSD Problem Report #54169

From ryo@nerv.org  Mon May  6 19:51:02 2019
Return-Path: <ryo@nerv.org>
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 9906B7A149
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  6 May 2019 19:51:02 +0000 (UTC)
Message-Id: <20190506195059.D907E1CC0D2@yaml.nerv.org>
Date: Tue,  7 May 2019 04:50:59 +0900 (JST)
From: ryo@nerv.org
Reply-To: ryo@nerv.org
To: gnats-bugs@NetBSD.org
Subject: correct bpf code is not generated when multiple conditions including range specification are specified for the port variable in npf.conf
X-Send-Pr-Version: 3.95

>Number:         54169
>Category:       bin
>Synopsis:       correct bpf code is not generated when multiple conditions including range specification are specified for the port variable in npf.conf
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    bin-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May 06 19:55:00 +0000 2019
>Closed-Date:    Thu Aug 08 21:35:35 +0000 2019
>Last-Modified:  Sun Aug 11 10:15:02 +0000 2019
>Originator:     Ryo Shimizu
>Release:        NetBSD 8.99.37
>Organization:
>Environment:
System: NetBSD netbsd 8.99.37 NetBSD 8.99.37 (GENERIC) #3: Fri Apr 19 02:08:38 JST 2019 ryo@moveq:/usr/src/sys/arch/amd64/compile/GENERIC amd64
Architecture: x86_64
Machine: amd64
>Description:

npfctl generated incorrect bpf code for the LINE 6 of the following npf.conf.
There is no problem for LINE 5.

	% cat -n npf.conf
	     1  $var1 = { 137, 138, 139, 445 }
	     2  $var2 = { 137-139, 445 }
	     3  
	     4  group default {
	     5  	pass in from any port $var1
	     6  	pass in from any port $var2
	     7  }
	     8  

	# npfctl debug npf.conf /dev/null

	RULE AT LINE 5
	(000) ld       M[0]
	(001) jeq      #0x0             jt 18   jf 2
	(002) ld       M[2]
	(003) jeq      #0x6             jt 7    jf 4
	(004) ld       M[2]
	(005) jeq      #0x11            jt 7    jf 6
	(006) ret      #0
	(007) ldx      M[1]
	(008) ldh      [x + 0]
	(009) jeq      #0x89            jt 17   jf 10	# if (port == 137) goto match(17); else goto 10;
	(010) ldh      [x + 0]
	(011) jeq      #0x8a            jt 17   jf 12	# if (port == 138) goto match(17); else goto 12;
	(012) ldh      [x + 0]
	(013) jeq      #0x8b            jt 17   jf 14	# if (port == 139) goto match(17); else goto 14;
	(014) ldh      [x + 0]
	(015) jeq      #0x1bd           jt 17   jf 16	# if (port == 445) goto match(17); else goto 16;
	(016) ret      #0
	(017) ret      #-1
	(018) ret      #0

	RULE AT LINE 6
	(000) ld       M[0]
	(001) jeq      #0x0             jt 15   jf 2
	(002) ld       M[2]
	(003) jeq      #0x6             jt 7    jf 4
	(004) ld       M[2]
	(005) jeq      #0x11            jt 7    jf 6
	(006) ret      #0
	(007) ldx      M[1]
	(008) ldh      [x + 0]
	(009) jge      #0x89            jt 14   jf 10	# if (port >= 137) goto match(14); else goto 10;	/* incorrect! */
	(010) jgt      #0x8b            jt 11   jf 14	# if (port > 139) goto 11; else goto match(14);
	(011) ldh      [x + 0]
	(012) jeq      #0x1bd           jt 14   jf 13
	(013) ret      #0
	(014) ret      #-1
	(015) ret      #0


bpf code for LINE 6 should be

	RULE AT LINE 6
	(000) ld       M[0]
	(001) jeq      #0x0             jt 15   jf 2
	(002) ld       M[2]
	(003) jeq      #0x6             jt 7    jf 4
	(004) ld       M[2]
	(005) jeq      #0x11            jt 7    jf 6
	(006) ret      #0
	(007) ldx      M[1]
	(008) ldh      [x + 0]
	(009) jge      #0x89            jt 10   jf 11	# if (port >= 137) goto 10; else goto 11;
	(010) jgt      #0x8b            jt 11   jf 14	# if (port > 139) goto 11; else goto match(14);
	(011) ldh      [x + 0]
	(012) jeq      #0x1bd           jt 14   jf 13
	(013) ret      #0
	(014) ret      #-1
	(015) ret      #0


This happens because fixup_jumps() only take into account a single bpf jump code
when converting a condition enumeration to "or".


>How-To-Repeat:

>Fix:

It's not elegant code :-(

cvs -q diff -aup npf_bpf_comp.c
Index: npf_bpf_comp.c
===================================================================
RCS file: /src/cvs/cvsroot-netbsd/src/usr.sbin/npf/npfctl/npf_bpf_comp.c,v
retrieving revision 1.12
diff -a -u -p -r1.12 npf_bpf_comp.c
--- npf_bpf_comp.c	17 Apr 2019 20:41:58 -0000	1.12
+++ npf_bpf_comp.c	6 May 2019 11:55:43 -0000
@@ -129,9 +129,34 @@ fixup_jumps(npf_bpf_t *ctx, u_int start,
 			continue;
 		}
 		if (swap) {
-			uint8_t jt = insn->jt;
-			insn->jt = insn->jf;
-			insn->jf = jt;
+			/*
+			 * XXX: depend on the bpf code generated by
+			 *      npfctl_bpf_ports().
+			 */
+			if ((i + 1 < end) &&
+			    (insn[0].code == (BPF_JMP+BPF_JGE+BPF_K)) &&
+			    (insn[0].jt == 0) && (insn[0].jf == JUMP_MAGIC) &&
+			    (insn[1].code == (BPF_JMP+BPF_JGT+BPF_K)) &&
+			    (insn[1].jt == JUMP_MAGIC) && (insn[1].jf == 0)) {
+				/*
+				 * <begin>-<end> range check insns
+				 *
+				 *   JGE #<begin> jt 0 jf JUMP_MAGIC
+				 *   JGT #<end>   jt JUMP_MAGIC jf 0
+				 *    :
+				 *
+				 * should be changed to
+				 *
+				 *   JGE #<begin> jt 0 jt 1
+				 *   JGT #<end>   jt 0 jt JUMP_MAGIC
+				 *    :
+				 */
+				insn->jf = 1;
+			} else {
+				uint8_t jt = insn->jt;
+				insn->jt = insn->jf;
+				insn->jf = jt;
+			}
 		}
 		if (insn->jt == JUMP_MAGIC)
 			insn->jt = fail_off;

>Release-Note:

>Audit-Trail:
From: "Mindaugas Rasiukevicius" <rmind@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54169 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: open->closed
State-Changed-By: rmind@NetBSD.org
State-Changed-When: Thu, 08 Aug 2019 21:35:35 +0000
State-Changed-Why:
Fixed.


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/54169 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