On 2022-05-03, Moritz Buhl wrote: > 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.
It's been a while, but iirc the flag is not necessarily whether this call, happening right now, is interrupt or not, but whether the paired alloc or free was or will be. So user code may need to set the intr flag if later an interrupt context will be touching this. (Interrupts are still not ever allowed to use user context allocators.) It may be more complicated than just flipping the flag, because there may be other ways to reach this free that require it. Or it could just be a bug. Just make sure we never free anything allocated with intr in this path.
