Module Name: src
Committed By: martin
Date: Sun Nov 17 13:58:11 UTC 2024
Modified Files:
src/usr.sbin/npf/npfctl [netbsd-9]: npf_bpf_comp.c
src/usr.sbin/npf/npftest [netbsd-9]: npftest.conf
src/usr.sbin/npf/npftest/libnpftest [netbsd-9]: npf_rule_test.c
Log Message:
Pull up following revision(s) (requested by riastradh in ticket #1918):
usr.sbin/npf/npftest/npftest.conf: revision 1.10
usr.sbin/npf/npftest/npftest.conf: revision 1.11
usr.sbin/npf/npftest/npftest.conf: revision 1.12
usr.sbin/npf/npfctl/npf_bpf_comp.c: revision 1.17
usr.sbin/npf/npftest/libnpftest/npf_rule_test.c: revision 1.20
usr.sbin/npf/npftest/libnpftest/npf_rule_test.c: revision 1.21
usr.sbin/npf/npftest/libnpftest/npf_rule_test.c: revision 1.22
usr.sbin/npf/npftest/libnpftest/npf_rule_test.c: revision 1.23
tests/net/npf/t_npf.sh: revision 1.5
tests/net/npf/t_npf.sh: revision 1.6
tests/net/npf/t_npf.sh: revision 1.7
npftest: Add AF_* parameter to test cases.
No functional change intended.
Preparation to add test cases for:
PR bin/55403: npfctl miscompiles IPv6 rules
npftest: Add a test to match groups of IPv6 addresses.
The npf_rule test group is now an xfail. (npftest doesn't have a way
to mark individual cases in a test group as xfail, so this will have
to do for now.)
PR bin/55403: npfctl miscompiles IPv6 rules
npftest: Fix newly added test.
- Adapt new test to actually exercise new rules.
- Mark the right test xfail.
PR bin/55403: npfctl miscompiles IPv6 rules
npftest: Expand test cases to cover more compiler paths.
Cover masked ranges with full- and partial-word sizes.
PR bin/55403: npfctl miscompiles IPv6 rules
npfctl(8): Fix compiling multiword comparisons, i.e., IPv6 addrs.
PR bin/55403: npfctl miscompiles IPv6 rules
To generate a diff of this commit:
cvs rdiff -u -r1.13.2.3 -r1.13.2.4 src/usr.sbin/npf/npfctl/npf_bpf_comp.c
cvs rdiff -u -r1.7.2.2 -r1.7.2.3 src/usr.sbin/npf/npftest/npftest.conf
cvs rdiff -u -r1.17.2.2 -r1.17.2.3 \
src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/usr.sbin/npf/npfctl/npf_bpf_comp.c
diff -u src/usr.sbin/npf/npfctl/npf_bpf_comp.c:1.13.2.3 src/usr.sbin/npf/npfctl/npf_bpf_comp.c:1.13.2.4
--- src/usr.sbin/npf/npfctl/npf_bpf_comp.c:1.13.2.3 Sat Jun 20 15:46:48 2020
+++ src/usr.sbin/npf/npfctl/npf_bpf_comp.c Sun Nov 17 13:58:11 2024
@@ -79,10 +79,27 @@
* out of the group; if false, then jump "next". At the end of the
* group, an addition failure path is appended and the JUMP_MAGIC
* uses within the group are patched to jump past the said path.
+ *
+ * For multi-word comparisons (IPv6 addresses), there is another
+ * layer of grouping:
+ *
+ * A and B and ((C and D) or (E and F))
+ *
+ * This strains the simple-minded JUMP_MAGIC logic, so for now,
+ * when generating the jump-if-false targets for (C and D), we
+ * simply count the number of instructions left to skip over.
+ *
+ * A better architecture might be to create asm-type labels for
+ * the jt and jf continuations in the first pass, and then, once
+ * their offsets are determined, go back and fill them in in the
+ * second pass. This would simplify the logic (no need to compute
+ * exactly how many instructions we're about to generate in a
+ * chain of conditionals) and eliminate redundant RET #0
+ * instructions which are currently generated after some groups.
*/
#include <sys/cdefs.h>
-__RCSID("$NetBSD: npf_bpf_comp.c,v 1.13.2.3 2020/06/20 15:46:48 martin Exp $");
+__RCSID("$NetBSD: npf_bpf_comp.c,v 1.13.2.4 2024/11/17 13:58:11 martin Exp $");
#include <stdlib.h>
#include <stdbool.h>
@@ -134,6 +151,7 @@ struct npf_bpf {
*/
unsigned ingroup;
bool invert;
+ bool multiword;
unsigned goff;
unsigned gblock;
@@ -322,6 +340,7 @@ npfctl_bpf_group_enter(npf_bpf_t *ctx, b
ctx->goff = bp->bf_len;
ctx->gblock = ctx->nblocks;
ctx->invert = invert;
+ ctx->multiword = false;
ctx->ingroup++;
}
@@ -334,8 +353,14 @@ npfctl_bpf_group_exit(npf_bpf_t *ctx)
assert(ctx->ingroup);
ctx->ingroup--;
- /* If there are no blocks or only one - nothing to do. */
- if (!ctx->invert && (ctx->nblocks - ctx->gblock) <= 1) {
+ /*
+ * If we're not inverting, there were only zero or one options,
+ * and the last comparison was not a multi-word comparison
+ * requiring a fallthrough failure -- nothing to do.
+ */
+ if (!ctx->invert &&
+ (ctx->nblocks - ctx->gblock) <= 1 &&
+ !ctx->multiword) {
ctx->goff = ctx->gblock = 0;
return;
}
@@ -502,7 +527,7 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
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, origlength, length, maxmask, off;
assert(((opts & MATCH_SRC) != 0) ^ ((opts & MATCH_DST) != 0));
assert((mask && mask <= NPF_MAX_NETMASK) || mask == NPF_NO_NETMASK);
@@ -529,7 +554,7 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
/* Ensure address family. */
fetch_l3(ctx, af, 0);
- length = (mask == NPF_NO_NETMASK) ? maxmask : mask;
+ length = origlength = (mask == NPF_NO_NETMASK) ? maxmask : mask;
/* CAUTION: BPF operates in host byte-order. */
for (unsigned i = 0; i < nwords; i++) {
@@ -563,13 +588,49 @@ npfctl_bpf_cidr(npf_bpf_t *ctx, unsigned
add_insns(ctx, insns_mask, __arraycount(insns_mask));
}
+ /*
+ * Determine how many instructions we have to jump
+ * ahead if the match fails.
+ *
+ * - If this is the last word, we jump to the final
+ * failure, JUMP_MAGIC.
+ *
+ * - If this is not the last word, we jump past the
+ * remaining instructions to match this sequence.
+ * Each 32-bit word in the sequence takes two
+ * instructions (BPF_LD and BPF_JMP). If there is a
+ * partial-word mask ahead, there will be one
+ * additional instruction (BPF_ALU).
+ */
+ uint8_t jf;
+ if (i + 1 == (origlength + 31)/32) {
+ jf = JUMP_MAGIC;
+ } else {
+ jf = 2*((origlength + 31)/32 - i - 1);
+ if (origlength % 32 != 0 && wordmask == 0)
+ jf += 1;
+ }
+
/* A == expected-IP-word ? */
struct bpf_insn insns_cmp[] = {
- BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, word, 0, JUMP_MAGIC),
+ BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, word, 0, jf),
};
add_insns(ctx, insns_cmp, __arraycount(insns_cmp));
}
+ /*
+ * If we checked a chain of words in sequence, mark this as a
+ * multi-word comparison so if this is in a group there will be
+ * a fallthrough case.
+ *
+ * XXX This is a little silly; the compiler should really just
+ * record holes where conditional jumps need success/failure
+ * continuations, and go back to fill in the holes when the
+ * locations of the continuations are determined later. But
+ * that requires restructuring this code a little more.
+ */
+ ctx->multiword = (origlength + 31)/32 > 1;
+
uint32_t mwords[] = {
(opts & MATCH_SRC) ? BM_SRC_CIDR: BM_DST_CIDR, 6,
af, mask, awords[0], awords[1], awords[2], awords[3],
Index: src/usr.sbin/npf/npftest/npftest.conf
diff -u src/usr.sbin/npf/npftest/npftest.conf:1.7.2.2 src/usr.sbin/npf/npftest/npftest.conf:1.7.2.3
--- src/usr.sbin/npf/npftest/npftest.conf:1.7.2.2 Sat Jun 20 15:46:48 2020
+++ src/usr.sbin/npf/npftest/npftest.conf Sun Nov 17 13:58:11 2024
@@ -1,4 +1,4 @@
-# $NetBSD: npftest.conf,v 1.7.2.2 2020/06/20 15:46:48 martin Exp $
+# $NetBSD: npftest.conf,v 1.7.2.3 2024/11/17 13:58:11 martin Exp $
$ext_if = "npftest0"
$int_if = "npftest1"
@@ -30,6 +30,10 @@ map $ext_if dynamic $local_ip1 port 6000
$net6_inner = fd01:203:405::/48
$net6_outer = 2001:db8:1::/48
+# Example of multiple addresses with a common 32-bit word, taken from
+# PR bin/55403: npfctl miscompiles IPv6 rules.
+$net6_pr55403 = { fe80::1, fe80::1000:0:0/95, fe80::2, fe80::2000:0:0/96, fe80::3, fe80::3000:0:0/97 }
+
$net_a = 10.100.0.0/16
$net_b = 10.255.0.0/16
@@ -51,6 +55,7 @@ group "ext" on $ext_if {
pass stateful out final from $local_net
pass stateful in final to any port $ports
pass stateful in final proto icmp all
+
block all
}
@@ -59,6 +64,9 @@ group "int" on $int_if {
pass stateful out final to $local_ip2
pass out final to $local_ip3
block final to $local_ip4
+
+ pass in final family inet6 proto udp from $net6_pr55403
+ pass in final family inet6 proto udp from ! $net6_pr55403 to $net6_pr55403
}
group default {
Index: src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c
diff -u src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c:1.17.2.2 src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c:1.17.2.3
--- src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c:1.17.2.2 Sun Sep 1 13:21:39 2019
+++ src/usr.sbin/npf/npftest/libnpftest/npf_rule_test.c Sun Nov 17 13:58:11 2024
@@ -15,6 +15,7 @@
#define RESULT_BLOCK ENETUNREACH
static const struct test_case {
+ int af;
const char * src;
const char * dst;
const char * ifname;
@@ -25,11 +26,13 @@ static const struct test_case {
/* Stateful pass. */
{
+ .af = AF_INET,
.src = "10.1.1.1", .dst = "10.1.1.2",
.ifname = IFNAME_INT, .di = PFIL_OUT,
.stateful_ret = RESULT_PASS, .ret = RESULT_PASS
},
{
+ .af = AF_INET,
.src = "10.1.1.2", .dst = "10.1.1.1",
.ifname = IFNAME_INT, .di = PFIL_IN,
.stateful_ret = RESULT_PASS, .ret = RESULT_BLOCK
@@ -37,18 +40,153 @@ static const struct test_case {
/* Pass forwards stream only. */
{
+ .af = AF_INET,
.src = "10.1.1.1", .dst = "10.1.1.3",
.ifname = IFNAME_INT, .di = PFIL_OUT,
.stateful_ret = RESULT_PASS, .ret = RESULT_PASS
},
{
+ .af = AF_INET,
.src = "10.1.1.3", .dst = "10.1.1.1",
.ifname = IFNAME_INT, .di = PFIL_IN,
.stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
},
+ /*
+ * Pass in from any of the { fe80::1, fe80:1000:0:0/95,
+ * fe80::2, fe80::2000:0:0/96, fe80::3, fe80::3000:0:0/97 }
+ * group.
+ */
+ { /* fe80::1 */
+ .af = AF_INET6,
+ .src = "fe80::1", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::1000:0:0/95 */
+ .af = AF_INET6,
+ .src = "fe80::1001:0:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::1000:0:0/95, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::1003:0:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ { /* fe80::2 */
+ .af = AF_INET6,
+ .src = "fe80::2", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::2000:0:0/96 */
+ .af = AF_INET6,
+ .src = "fe80::2000:8000:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::2000:0:0/96, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::2001:8000:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ { /* fe80::3 */
+ .af = AF_INET6,
+ .src = "fe80::3", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::3000:0:0/97 */
+ .af = AF_INET6,
+ .src = "fe80::3000:7fff:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::3000:0:0/97, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::3000:ffff:0", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ {
+ .af = AF_INET6,
+ .src = "fe80::4", .dst = "fe80::adec:c91c:d116:7592",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+
+ /*
+ * Pass in from anywhere _not_ in that group, as long as it is
+ * to that group.
+ */
+ { /* fe80::1 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::1",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::1000:0:0/95 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::1001:0:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::1000:0:0/95, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::1003:0:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ { /* fe80::2 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::2",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::2000:0:0/96 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::2000:8000:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::2000:0:0/96, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::2001:8000:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ { /* fe80::3 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::3",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::3000:0:0/97 */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::3000:7fff:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_PASS, .ret = RESULT_PASS
+ },
+ { /* fe80::3000:0:0/97, one bit off */
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::3000:ffff:0",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+ {
+ .af = AF_INET6,
+ .src = "fe80::adec:c91c:d116:7592", .dst = "fe80::4",
+ .ifname = IFNAME_INT, .di = PFIL_IN,
+ .stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
+ },
+
/* Block. */
- { .src = "10.1.1.1", .dst = "10.1.1.4",
+ {
+ .af = AF_INET,
+ .src = "10.1.1.1", .dst = "10.1.1.4",
.ifname = IFNAME_INT, .di = PFIL_OUT,
.stateful_ret = RESULT_BLOCK, .ret = RESULT_BLOCK
},
@@ -65,7 +203,7 @@ run_raw_testcase(unsigned i)
npf_rule_t *rl;
int slock, error;
- m = mbuf_get_pkt(AF_INET, IPPROTO_UDP, t->src, t->dst, 9000, 9000);
+ m = mbuf_get_pkt(t->af, IPPROTO_UDP, t->src, t->dst, 9000, 9000);
npc = get_cached_pkt(m, t->ifname);
slock = npf_config_read_enter(npf);
@@ -91,7 +229,7 @@ run_handler_testcase(unsigned i)
struct mbuf *m;
int error;
- m = mbuf_get_pkt(AF_INET, IPPROTO_UDP, t->src, t->dst, 9000, 9000);
+ m = mbuf_get_pkt(t->af, IPPROTO_UDP, t->src, t->dst, 9000, 9000);
error = npfk_packet_handler(npf, &m, ifp, t->di);
if (m) {
m_freem(m);
