bin/54169: correct bpf code is not generated when multiple conditions including range specification are specified for the port variable in npf.conf

[ryo%nerv.org@localhost]

>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:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon May 06 19:55:00 +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;