Re: kern/58543: NPF rule with multiple addresses in "from" disregards source address constraint

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

The following reply was made to PR kern/58543; it has been noted by GNATS.

From: mlelstv%serpens.de@localhost (Michael van Elst)
To: gnats-bugs%netbsd.org@localhost
Cc: 
Subject: Re: kern/58543: NPF rule with multiple addresses in "from" disregards source address constraint
Date: Sat, 3 Aug 2024 05:34:38 -0000 (UTC)

 tnn%nygren.pp.se@localhost writes:
 
 >Syntactically valid NPF rule does not behave as expected and disregards the configured source address constraint.
 >        pass in final family inet6 proto tcp from {fd42:dead:beef::1, fd42:dead:beef::2} to any port 9002 apply "log"
 
 The compiler generates bad BPF code for IPv6 (see bin/55403).
 
 Quick workaround is to use a table instead of a list of addresses.
 
 I am using this patch that compiles working, but maybe not
 optimal BPF code. Please check if that helps in your case:
 
 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[] = {