Hi tech@,
Syzkaller found a few crashes in pf_anchor_global_RB_REMOVE.
https://syzkaller.appspot.com/bug?id=a97f712331903ce38b8c084a489818b9bb5c6fcb
and also
https://syzkaller.appspot.com/text?tag=CrashLog&x=15ace9aaf00000
The call stack is something like this:
pf_anchor_global_RB_REMOVE
pf_remove_if_empty_ruleset
pfi_dynaddr_setup
pfioctl
I believe this is caused by two calls to pf_remove_if_empty_ruleset
in pfi_dynaddr_setup and a possibly pfr_destroy_ktable.
I think it is a problem that pfr_attach_table is being called
with 1 (intr) because the introducing commit says:
commit 4b3977248902c22d96aaebdb5784840debc2631c
Author: mikeb <mi...@openbsd.org>
Date: Mon Nov 24 13:22:09 2008 +0000
Fix splasserts seen in pr 5987 by propagating a flag that discribes
whether we're called from the interrupt context to the functions
performing allocations.
Looked at by mpf@ and henning@, tested by mpf@ and Antti Harri,
the pr originator.
ok tedu
And we are not in interrupt context. A detailed analysis follows.
The pfioctl cd60441a is DIOCADDRULE.
The call stack fails in pf_addr_setup calls from pf_ioctl.c:
1647 if (pf_addr_setup(ruleset, &newrule->src.addr,
1648 newrule->af))
similar calls below.
One of these calls might do the following:
424 pfi_dynaddr_setup(struct pf_addr_wrap *aw, sa_family_t af)
...
462 if ((ruleset = pf_find_or_create_ruleset(PF_RESERVED_ANCHOR)) ==
NULL) {
assume false.
...
467 if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 1)) == NULL)
might evaluate to true, I will share my thoughts on it.
468 rv = 1;
469 goto _bad;
...
481 _bad:
482 if (dyn->pfid_kt != NULL)
483 pfr_detach_table(dyn->pfid_kt);
484 if (ruleset != NULL)
485 pf_remove_if_empty_ruleset(ruleset);
remember this call. I will show a case where we don't want this.
due to the following in pf_table.c:
2424 pfr_attach_table(struct pf_ruleset *rs, char *name, int intr)
...
2434 kt = pfr_lookup_table(&tbl);
2435 if (kt == NULL) {
assume true.
2436 kt = pfr_create_ktable(&tbl, gettime(), 1, intr);
2437 if (kt == NULL)
2438 return (NULL);
2439 if (ac != NULL) {
assume true.
this means kt != NULL and kt->pfrkt_rs = our ruleset.
2440 bzero(tbl.pfrt_anchor, sizeof(tbl.pfrt_anchor));
2441 rt = pfr_lookup_table(&tbl);
2442 if (rt == NULL) {
assume true.
pfr_ktable_compares pfrkt_name an pfrkt_anchor if previous was same.
pfrkt_name should be '\0', pfrkt_anchor is PF_RESERVED_ANCHOR vs '\0'
2443 rt = pfr_create_ktable(&tbl, 0, 1, intr);
let's look into this call next just to be sure.
2444 if (rt == NULL) {
assume true.
2445 pfr_destroy_ktable(kt, 0);
remember this call is possible. We look at this afterwards.
2446 return (NULL);
...
looking into create_ktable, I want to see if we can reach pfr_destroy_ktable,
still in pf_table.c:
2202 pfr_create_ktable(struct pfr_table *tbl, time_t tzero, int attachruleset
...
2208 if (intr)
2209 kt = pool_get(&pfr_ktable_pl,
PR_NOWAIT|PR_ZERO|PR_LIMITFAIL);
This can fail. Returning NULL afterwards.
Note PR_ZERO.
...
2216 if (attachruleset) {
2217 rs = pf_find_or_create_ruleset(tbl->pfrt_anchor);
2218 if (!rs) {
not reachable because pfrt_anchor comes from l.2433.
But a weird indirection for our current call stack anyway. Might be necessary.
2219 pfr_destroy_ktable(kt, 0);
2220 return (NULL);
2221 }
2222 kt->pfrkt_rs = rs;
2223 rs->tables++;
2224 }
2225
2226 if (!rn_inithead((void **)&kt->pfrkt_ip4,
2227 offsetof(struct sockaddr_in, sin_addr)) ||
2228 !rn_inithead((void **)&kt->pfrkt_ip6,
2229 offsetof(struct sockaddr_in6, sin6_addr))) {
unreachable because &kt->pfrkt_ip{4,6} are NULL.
2230 pfr_destroy_ktable(kt, 0);
2231 return (NULL);
2232 }
Back to pfr_destroy_ktable, still in pf_table.c:
2253 pfr_destroy_ktable(struct pfr_ktable *kt, int flushaddr)
2254 {
...
2268 if (kt->pfrkt_rs != NULL) {
assume true.
2269 kt->pfrkt_rs->tables--;
2270 pf_remove_if_empty_ruleset(kt->pfrkt_rs);
our first call to pf_remove_if_empty_ruleset.
The second one follows from pfi_dynaddr_setup.
If this is sound, then the only reason why pfr_destroy_ktable was called
is that pool_get is called with PR_NOWAIT. And then the following diff
would help.
mbuhl
Index: pf_if.c
===================================================================
RCS file: /cvs/src/sys/net/pf_if.c,v
retrieving revision 1.104
diff -u -p -r1.104 pf_if.c
--- pf_if.c 29 Apr 2022 09:55:43 -0000 1.104
+++ pf_if.c 3 May 2022 16:01:23 -0000
@@ -464,7 +464,7 @@ pfi_dynaddr_setup(struct pf_addr_wrap *a
goto _bad;
}
- if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 1)) == NULL) {
+ if ((dyn->pfid_kt = pfr_attach_table(ruleset, tblname, 0)) == NULL) {
rv = 1;
goto _bad;
}
