pf: prevent SCTP-based NULL dereference in pfi_kkif_match()

[franco_opnsense.org]

Summary

It appears that 0fe663b2a815 can cause a panic when being run
as it reaches for IFG_ALL with a NULL pointer for the pfik_ifp
inside packet_kif. The stack trace as presented by the reporter:

• trap 0xc, rip = 0xffffffff821ab744, rsp = 0xfffffe00625cef50, rbp = 0xfffffe00625cef50 --- pfi_kkif_match() at pfi_kkif_match+0x24/frame 0xfffffe00625cef50 pf_test_rule() at pf_test_rule+0xe6b/frame 0xfffffe00625cf3a0 pf_sctp_multihome_delayed() at pf_sctp_multihome_delayed+0x30e/frame 0xfffffe00625cf4d0 pf_test() at pf_test+0xd1a/frame 0xfffffe00625cf680 pf_check_in() at pf_check_in+0x27/frame 0xfffffe00625cf6a0 pfil_mbuf_in() at pfil_mbuf_in+0x38/frame 0xfffffe00625cf6d0 enc_hhook() at enc_hhook+0x28a/frame 0xfffffe00625cf710 hhook_run_hooks() at hhook_run_hooks+0x61/frame 0xfffffe00625cf780 ipsec_run_hhooks() at ipsec_run_hhooks+0x6d/frame 0xfffffe00625cf7a0 ipsec4_common_input_cb() at ipsec4_common_input_cb+0x32a/frame 0xfffffe00625cf830 esp_input_cb() at esp_input_cb+0x430/frame 0xfffffe00625cf8e0 swcr_process() at swcr_process+0x25/frame 0xfffffe00625cf900 crypto_dispatch() at crypto_dispatch+0x60/frame 0xfffffe00625cf920 esp_input() at esp_input+0x4d8/frame 0xfffffe00625cf9f0 udp_ipsec_input() at udp_ipsec_input+0x17b/frame 0xfffffe00625cfa50 ipsec_kmod_udp_input() at ipsec_kmod_udp_input+0x2d/frame 0xfffffe00625cfa70 udp_append() at udp_append+0xe4/frame 0xfffffe00625cfae0 udp_input() at udp_input+0x803/frame 0xfffffe00625cfbc0 ip_input() at ip_input+0x268/frame 0xfffffe00625cfc20 netisr_dispatch_src() at netisr_dispatch_src+0x9e/frame 0xfffffe00625cfc70 ether_demux() at ether_demux+0x149/frame 0xfffffe00625cfca0 ether_nh_input() at ether_nh_input+0x36a/frame 0xfffffe00625cfd00 netisr_dispatch_src() at netisr_dispatch_src+0x9e/frame 0xfffffe00625cfd50 ether_input() at ether_input+0x56/frame 0xfffffe00625cfda0 re_rxeof() at re_rxeof+0x547/frame 0xfffffe00625cfe20 re_intr_msi() at re_intr_msi+0xf3/frame 0xfffffe00625cfe60 ithread_loop() at ithread_loop+0x257/frame 0xfffffe00625cfef0 fork_exit() at fork_exit+0x7f/frame 0xfffffe00625cff30 fork_trampoline() at fork_trampoline+0xe/frame 0xfffffe00625cff30
• trap 0, rip = 0, rsp = 0, rbp = 0 ---

PR: https://github.com/opnsense/src/issues/227
Sponsored by: OPNsense

[kp]

The backtrace appears to have gotten mangled here.
Also, a gdb backtrace would be more useful as it'll decode to line numbers rather than to addresses.

This strikes me as overly broad. "We hit a NULL pointer here, so let's just check for that in this exact location" isn't an ideal approach to a fix.

We're not usually going to hit a 'packet_kif' that doesn't have a real interface associated with it (i.e. it's expected to always have pfik_ifp set). The SCTP multi home path is somewhat unusual there. Either that needs to be changed (although then we'd probably end up having to add special case flags to pf_test_rule(), which isn't ideal), or pfi_kkif_match() needs to check packet_kif against V_pfi_all. That in turn raises an interesting question: what is the expected behaviour of an interface matching rule if the interface isn't know when the rule is evaluated?
Perhaps this does suggest that the former option needs to be explored.

Finally, this really wants a test case. Presumably the crash scenario is when there's a multi homed SCTP connection (test examples already exist) and the ruleset contains a rule that matches on interface (which I believe is not something that's currently tested).

[franco_opnsense.org]

Sure. For context:

(kgdb) frame 18
#18 pfi_kkif_match (rule_kif=0xfffff8001830a100, packet_kif=packet_kif@entry=0xfffff80005b1ee00) at /usr/src/sys/netpfil/pf/pf_if.c:433
433			CK_STAILQ_FOREACH(p, &packet_kif->pfik_ifp->if_groups, ifgl_next)
(kgdb) p *packet_kif
$1 = {pfik_name = "all", '\000' <repeats 12 times>, _pfik_glue = {_pfik_tree = {rbe_link = {0xfffff80018427900, 0x0, 0x0}}, _pfik_list = {le_next = 0xfffff80018427900, le_prev = 0x0}}, 
  pfik_packets = {{{{counter = 0xfffffe00e7046d70}, {counter = 0xfffffe00e7046d60}}, {{counter = 0xfffffe00e7046d50}, {counter = 0xfffffe00e7046d40}}}, {{{counter = 0xfffffe00e7046d30}, {
          counter = 0xfffffe00e7046d20}}, {{counter = 0xfffffe00e7046d10}, {counter = 0xfffffe00e7046d00}}}}, pfik_bytes = {{{{counter = 0xfffffe00e7046d68}, {counter = 0xfffffe00e7046d58}}, {{
          counter = 0xfffffe00e7046d48}, {counter = 0xfffffe00e7046d38}}}, {{{counter = 0xfffffe00e7046d28}, {counter = 0xfffffe00e7046d18}}, {{counter = 0xfffffe00e7046d08}, {
          counter = 0xfffffe00e7046cf8}}}}, pfik_tzero = 1731419937, pfik_flags = 1, pfik_ifp = 0x0, pfik_group = 0xfffff8000397c340, pfik_rulerefs = 37, pfik_dynaddrs = {
    tqh_first = 0xfffff80018214e00, tqh_last = 0xfffff800187c4d00}}
(kgdb) frame 19
#19 0xffffffff8238a953 in pf_test_rule (rm=rm@entry=0xfffffe006259b358, sm=sm@entry=0xfffffe006259b388, kif=0xfffff80005b1ee00, m=0xfffff800ca08e400, off=off@entry=20, 
    pd=pd@entry=0xfffff8001835a210, am=0xfffffe006259b338, rsm=0xfffffe006259b340, inp=0x0) at /usr/src/sys/netpfil/pf/pf.c:4651
4651			if (pfi_kkif_match(r->kif, kif) == r->ifnot)
(kgdb) list
4646		}
4647	
4648		SLIST_INIT(&match_rules);
4649		while (r != NULL) {
4650			pf_counter_u64_add(&r->evaluations, 1);
4651			if (pfi_kkif_match(r->kif, kif) == r->ifnot)
4652				r = r->skip[PF_SKIP_IFP].ptr;
4653			else if (r->direction && r->direction != pd->dir)
4654				r = r->skip[PF_SKIP_DIR].ptr;
4655			else if (r->af && r->af != af)

If IFG_ALL is not supposed to have a pfik_ifp set why are we passing it downwards to create floating states as per 0fe663b2a815 that eventually runs into this? At which layer should this be prevented from happening?

[kp]

In D47658#1086624, @franco_opnsense.org wrote:

Sure. For context:

<snip>
The entire backtrace would have been nice.
Still, that's not the most important thing right now. Concentrate on the test case, which will mean we can trivially reproduce backtraces and we can circle back to improving the commit message once we have both the test case and an actual fix.

If IFG_ALL is not supposed to have a pfik_ifp set why are we passing it downwards to create floating states as per 0fe663b2a815 that eventually runs into this? At which layer should this be prevented from happening?

The pd->kif is set pretty early on in the pf flow, via pf_setup_pdesc(): https://cgit.freebsd.org/src/tree/sys/netpfil/pf/pf.c#n8649
That code got reshuffled a little, but not in ways that matter to this discussion.

There's no separate layer that handles this later. The issue is one of mismatches assumptions between the SCTP multihome code and pfi_kkif_match().

To reiterate, the priorities now are, in this order:

• a test case
• Answering "what is the expected behaviour of an interface matching rule if the interface isn't know when the rule is evaluated?", so we can figure out how to best fix this
• actually fixing the problem
• writing up a useful commit message, explaining both the problem and the chosen solution, with any tradeoffs that may be involved.

[franco_opnsense.org]

In D47658#1086724, @kp wrote:

The entire backtrace would have been nice.

Just let me know what you need? Just "bt" or more?

• a test case

I'll see what I can do. Thanks so far.