Re: Dynamic & static address grouping in NPF

[Christoph Badura <bad%bsd.de@localhost>]

On Thu, Jul 24, 2025 at 09:25:30AM +0000, Emmanuel Nyarko wrote:
> In an attempt to fix this PR reported by Josh Mayor: https://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=59507
> 
> the crash ( from the PR )  was here    "$clientgroup={ $self, $otherip }”
> $self =. Dynamic, $otherip = raw IP
> 
> and this is because he was listing a dynamic address first and a raw IP second on the same rule.
>  
> the basic issue I found was 
> if you have a rule
> pass from { ifaddrs(wm0), 192.164.64.7 } on the same rule,
> 
> an assertion failure.
> 
> 		/*
> 		 * L3 block cannot be inserted in the middle of a group.
> 		 * In fact, it never is.  Check and start the group after.
> 		 */
> 		if (ingroup) {
> 			assert(ctx->nblocks == ctx->gblock);
> 			npfctl_bpf_group_exit(ctx);
> 		}
> 
> 
> This is a comment rmind@ left in a function that fetches the address family.
> 
> Group here means : a group of addresses ( not the group keyword )
> { 192.168.65.7, 192.168.64.8, 192.168.64.9 } => a group 

No, group here means a grouping on filter conditions as explained here:
https://nxr.netbsd.org/xref/src/usr.sbin/npf/npfctl/npf_bpf_comp.c?r=1.19#70

> What he means:  always load the address family first.

Sort of.  What this actually means is that he wants to load the address
family *once* and before the first static address is filtered.

> and when you list a static address, NPF extracts the address family and uses first when evaluating a group.
> 
> (000) ld M[0]     // 4 bytes offset the packet header to get the version
> (001) jeq #0x04 // the address family fetched on the rule ( version 4 here )
> 
> /// before you load and compare the addresses.

Yes.

> But NPF doesn’t extract and check the version to use in filtering when you enter a dynamic address,
> 
> when you put { Ifaddrs(wm0), 192.168.64.7 } in the same group, 

Pretty much.  That is because "ifaddrs(wm0)" is internally implemented as
an IFADDR table and it does a BPF_COP call to do the table lookup.  And the
table lookup doesn't care about the BPF code having loaded the address
family into the BPF memory.

> now when it is now about to evaluate the static IP on rule
> 
> doesn’t allow fetching the address family because NPF thinks it should be the first thing checked.

Correct.

> So it tries to exit the group,

Where does it try to exit the group?  I don't see that.

> With this behaviour , NPF tries to tie each rule to one address family. 

> based on this comment by rmind, 
>   /*
>  * L3 block cannot be inserted in the middle of a group.
>  * In fact, it never is.  Check and start the group after.
>  */
> if this is a statement we agree with, then I make it a compile time error
> when you try to insert a raw IP in the middle of a group that begins with
> a dynamic address.

You misunderstand the statement.  It's also a bit misleading.  What it says
is that the L3 block of statements should be the first thing done in such a
group, if it is necessary.  And certainly it shouldn't be done multiple
times in the group.

But that comment was added in r1.1 of npf_bpf_comp.c and the filt_addr_list
stuff was added in r1.55 of npf_parse.y, i.e. much, much later.  So this
comment doesn't reflect current reality.

> you are free to list only dynamic addresses in one group.(that doesn’t
> seg fault because no address family check )

That's also not accurate.

The following example asserts:
----------------------------------------------------------------------------
group default {
        pass from { ifaddrs(wm0), 192.164.64.7 } 
}
----------------------------------------------------------------------------

This is because, leaving out unimported details, it gets translated into the
following group

    ifnet_table(wm0) or bpf_cidr(192.164.64.8)

and bpf_cidr() tries to add fetch_l3(AF_INET).  Which, btw doesn't do any harm.

But the following doesn't.

----------------------------------------------------------------------------
group default {
	pass from { 192.164.64.7, ifaddrs(wm0) } 
}
----------------------------------------------------------------------------

Because it gets translated into:

    bpf_cidr(192.164.64.8/32) or ifnet_table(wm0)

Here bpf_cidr() can add the fetch_l3(AF_INET) but if we're dealing with a
IPv6 address, the code generated by fetch_l3() will jump passt the
ifnet_table(wm0) too, I think, which is wrong.

Please add test cases. ;-)

But that seems wrong to me.  It should get translated into

    ( bpf_cidr(192.164.64.8/32) ) or ifnet_table(wm0)

That is, the bpf_cidr() should be put into it's own group.

Consider this:

----------------------------------------------------------------------------
group default {
	pass from { 192.164.64.7, 2001:db8::1, ifaddrs(wm0),
	3fff:1334::/64, 192.0.2.0/24, ifaddrs(wm1) } 
}
----------------------------------------------------------------------------

This could be translated into

   ( bpf_cidr(192.164.64.7/32) or bpf_cidr(192.0.2.0/24) ) or
   ( bpf_cidr(2001:db8::1/128) or bpf_cidr(3fff:1334::/64) or
   ( ifnet_table(wm0) or ifnet_table(wm1) )

And then bpf_cidr() could add the fetch_l3() at the beginning of each group
and it would work.

Note that the order of the terms doesn't matter.  Except for performance, of
course.

--chris