NPF Compiler problems

[mlelstv%serpens.de@localhost (Michael van Elst)]

Hello,

the npf compiler (part of the npfctl command) miscompiles IPv6 matches,
the resulting BPF code is broken. Please see my orignal PR bin/55403 
and kern/58543 for details.

The following patch fixes this for my configuration, although not optimal.
The compiler in npfctl is mostly hand-crafted code, so it's unclear to
me if the patch is actually correct or what other bugs are present in
the compiler.

Please, someone with more experience in compiler writing, review the
patch. As a stopgap it's probably good enough to increase reliablity
of npf.

Index: usr.sbin/npf/npfctl/npf_bpf_comp.c
===================================================================
RCS file: /cvsroot/src/usr.sbin/npf/npfctl/npf_bpf_comp.c,v
retrieving revision 1.16
diff -p -u -r1.16 npf_bpf_comp.c
--- usr.sbin/npf/npfctl/npf_bpf_comp.c	30 May 2020 14:16:56 -0000	1.16
+++ usr.sbin/npf/npfctl/npf_bpf_comp.c	3 Aug 2024 05:31:50 -0000
@@ -206,12 +206,22 @@ fixup_jumps(npf_bpf_t *ctx, u_int start,
 		 * Fixup the "magic" value.  Swap only the "magic" jumps.
 		 */
 
-		if (insn->jt == JUMP_MAGIC) {
-			insn->jt = fail_off;
+		if (insn->jf == JUMP_MAGIC) {
+			if (swap && insn->jt) {
+				insn->jf = 0;
+			} else {
+				insn->jf = fail_off;
+				insn->jt = 0;
+			}
 			seen_magic = true;
 		}
-		if (insn->jf == JUMP_MAGIC) {
-			insn->jf = fail_off;
+		if (insn->jt == JUMP_MAGIC) {
+			if (swap && insn->jf) {
+				insn->jt = 0;
+			} else {
+				insn->jt = fail_off;
+				insn->jf = 0;
+			}
 			seen_magic = true;
 		}
 
@@ -496,13 +506,14 @@ npfctl_bpf_proto(npf_bpf_t *ctx, unsigne
  * npfctl_bpf_cidr: code block to match IPv4 or IPv6 CIDR.
  *
  * => IP address shall be in the network byte order.
+ *
  */
 void
 npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned opts, sa_family_t af,
     const npf_addr_t *addr, const npf_netmask_t mask)
 {
 	const uint32_t *awords = (const uint32_t *)addr;
-	unsigned nwords, length, maxmask, off;
+	unsigned nwords, length, maxmask, off, fail_off;
 
 	assert(((opts & MATCH_SRC) != 0) ^ ((opts & MATCH_DST) != 0));
 	assert((mask && mask <= NPF_MAX_NETMASK) || mask == NPF_NO_NETMASK);
@@ -513,14 +524,12 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
 		off = (opts & MATCH_SRC) ?
 		    offsetof(struct ip, ip_src) :
 		    offsetof(struct ip, ip_dst);
-		nwords = sizeof(struct in_addr) / sizeof(uint32_t);
 		break;
 	case AF_INET6:
 		maxmask = 128;
 		off = (opts & MATCH_SRC) ?
 		    offsetof(struct ip6_hdr, ip6_src) :
 		    offsetof(struct ip6_hdr, ip6_dst);
-		nwords = sizeof(struct in6_addr) / sizeof(uint32_t);
 		break;
 	default:
 		abort();
@@ -531,6 +540,14 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
 
 	length = (mask == NPF_NO_NETMASK) ? maxmask : mask;
 
+	/* Compute number of words to check */
+	nwords = howmany(length, 32);
+	/* Compute offset to failure path */
+	fail_off = length / 32 * 2 + (length % 32 ? 3 : 0) - 1;
+
+	if (nwords * 32 > maxmask)
+		abort();
+
 	/* CAUTION: BPF operates in host byte-order. */
 	for (unsigned i = 0; i < nwords; i++) {
 		const unsigned woff = i * sizeof(uint32_t);
@@ -545,8 +562,8 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
 			wordmask = 0xffffffff << (32 - length);
 			length = 0;
 		} else {
-			/* The mask became zero - skip the rest. */
-			break;
+			/* The mask became zero - this cannot happen */
+			abort();
 		}
 
 		/* A <- IP address (or one word of it) */
@@ -554,6 +571,7 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
 			BPF_STMT(BPF_LD+BPF_W+BPF_ABS, off + woff),
 		};
 		add_insns(ctx, insns_ip, __arraycount(insns_ip));
+		fail_off--;
 
 		/* A <- (A & MASK) */
 		if (wordmask) {
@@ -561,13 +579,17 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
 				BPF_STMT(BPF_ALU+BPF_AND+BPF_K, wordmask),
 			};
 			add_insns(ctx, insns_mask, __arraycount(insns_mask));
+			fail_off--;
 		}
 
 		/* A == expected-IP-word ? */
 		struct bpf_insn insns_cmp[] = {
 			BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, word, 0, JUMP_MAGIC),
 		};
+		if (i < nwords-1)
+			insns_cmp[0].jt = fail_off;
 		add_insns(ctx, insns_cmp, __arraycount(insns_cmp));
+		fail_off--;
 	}
 
 	uint32_t mwords[] = {


Greetings,