On Tue, Sep 07, 2021 at 02:06:33PM +0200, Anton Lindqvist wrote:
> On Tue, Sep 07, 2021 at 03:56:00AM -0600, Vitaliy Makkoveev wrote:
> > CVSROOT: /cvs
> > Module name: src
> > Changes by: m...@cvs.openbsd.org 2021/09/07 03:56:00
> >
> > Modified files:
> > sys/net : rtsock.c
> >
> > Log message:
> > Fix the race between if_detach() and rtm_output().
> >
> > When the dying network interface descriptor has if_get(9) obtained
> > reference owned by foreign thread, the if_detach() thread will sleep
> > just after it removed this interface from the interface index map.
> >
> > The data related to this interface is still in routing table, so
> > if_get(9) called by concurrent rtm_output() thread will return NULL and
> > the following "ifp != NULL" assertion will be triggered.
> >
> > So remove the "ifp != NULL" assertions from rtm_output() and try to grab
> > `ifp' as early as possible then hold it until we finish the work. In the
> > case we won the race and we have `ifp' non NULL, concurrent if_detach()
> > thread will wait us. In the case we lost we just return ESRCH.
> >
> > The problem reported by danj@.
> > Diff tested by danj@.
> >
> > ok mpi@
> >
>
> syzkaller just found what looks like a NULL pointer derefence in
> rtm_output(). Regression introduced in this commit?
>
> https://syzkaller.appspot.com/bug?extid=684597dbbb9b516e76ae
Unfortunately yes. `rt' could be set to NULL before I call if_get(9) in
'RTM_CHANGE' path. I have a fix for this:
Index: sys/net/rtsock.c
===================================================================
RCS file: /cvs/src/sys/net/rtsock.c,v
retrieving revision 1.320
diff -u -p -r1.320 rtsock.c
--- sys/net/rtsock.c 7 Sep 2021 09:56:00 -0000 1.320
+++ sys/net/rtsock.c 7 Sep 2021 13:21:01 -0000
@@ -1032,14 +1032,6 @@ rtm_output(struct rt_msghdr *rtm, struct
rt = NULL;
}
- ifp = if_get(rt->rt_ifidx);
- if (ifp == NULL) {
- rtfree(rt);
- rt = NULL;
- error = ESRCH;
- break;
- }
-
/*
* If RTAX_GATEWAY is the argument we're trying to
* change, try to find a compatible route.
@@ -1065,6 +1057,14 @@ rtm_output(struct rt_msghdr *rtm, struct
*/
if (ISSET(rt->rt_flags, RTF_LOCAL|RTF_BROADCAST)) {
error = EINVAL;
+ break;
+ }
+
+ ifp = if_get(rt->rt_ifidx);
+ if (ifp == NULL) {
+ rtfree(rt);
+ rt = NULL;
+ error = ESRCH;
break;
}
