On Sat, Apr 11, 2020 at 03:35:45PM +0200, Martin Pieuchot wrote:
> On 11/04/20(Sat) 23:09, David Gwynne wrote:
> > On Sat, Apr 11, 2020 at 03:21:49AM +0000, Visa Hankala wrote:
> > > On Fri, Apr 10, 2020 at 01:30:47PM -0600, Theo de Raadt wrote:
> > > > Why did it take almost a year to find this?
> > > >
> > > > Or is this bug due to ioctl(2) becoming UNLOCKED on 2020/02/22?
> > >
> > > This is not related to ioctl(2) becoming UNLOCKED. Lower-layer ioctl
> > > code, soo_ioctl() included, lock the kernel when needed. However, most
> > > .if_ioctl backends need NET_LOCK() in addition to KERNEL_LOCK(). In
> > > most cases, that is satisfied by ifioctl() which acquires the lock
> > > before invoking .if_ioctl(). bridge_ioctl() nullifies this by
> > > releasing NET_LOCK().
> >
> > yes.
> >
> > i came up with the following diff before i read the thread here. it's
> > largely identical to what you (visa) already came up with, but it adds
> > some extra checks to ifpromisc based on the doco in around struct ifnet
> > members in src/sys/net/if_var.h. i audited the rest of the ifpromisc
> > calls and found another one in if_aggr that i was able to trigger.
>
> The documentation says `if_pcount' is protected by the KERNEL_LOCK() but
> in fact it is only read & modified in ifpromisc().
>
> So I'd suggest fixing the documentation and not add another assert there.
Can do.
> > i think the only other call to ifpromisc outside src/sys/net is in carp,
> > and i managed to convinced myself that all those calls hold NET_LOCK
> > already.
Index: if_var.h
===================================================================
RCS file: /cvs/src/sys/net/if_var.h,v
retrieving revision 1.103
diff -u -p -r1.103 if_var.h
--- if_var.h 8 Nov 2019 07:16:29 -0000 1.103
+++ if_var.h 11 Apr 2020 13:38:10 -0000
@@ -130,7 +130,7 @@ struct ifnet { /* and the
entries */
/* [I] check or clean routes (+ or -)'d */
void (*if_rtrequest)(struct ifnet *, int, struct rtentry *);
char if_xname[IFNAMSIZ]; /* [I] external name (name + unit) */
- int if_pcount; /* [k] # of promiscuous listeners */
+ int if_pcount; /* [N] # of promiscuous listeners */
unsigned int if_bridgeidx; /* [k] used by bridge ports */
caddr_t if_bpf; /* packet filter structure */
caddr_t if_switchport; /* used by switch ports */
Index: if.c
===================================================================
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.601
diff -u -p -r1.601 if.c
--- if.c 10 Mar 2020 09:11:55 -0000 1.601
+++ if.c 11 Apr 2020 13:38:10 -0000
@@ -3031,6 +3031,8 @@ ifpromisc(struct ifnet *ifp, int pswitch
unsigned short oif_flags;
int oif_pcount, error;
+ NET_ASSERT_LOCKED(); /* modifying if_flags and if_pcount */
+
oif_flags = ifp->if_flags;
oif_pcount = ifp->if_pcount;
if (pswitch) {
Index: if_aggr.c
===================================================================
RCS file: /cvs/src/sys/net/if_aggr.c,v
retrieving revision 1.28
diff -u -p -r1.28 if_aggr.c
--- if_aggr.c 11 Mar 2020 07:01:42 -0000 1.28
+++ if_aggr.c 11 Apr 2020 13:38:10 -0000
@@ -589,8 +589,10 @@ aggr_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
/* last ref, no need to lock. aggr_p_dtor locks anyway */
+ NET_LOCK();
while ((p = TAILQ_FIRST(&sc->sc_ports)) != NULL)
aggr_p_dtor(sc, p, "destroy");
+ NET_UNLOCK();
free(sc, M_DEVBUF, sizeof(*sc));
Index: if_bridge.c
===================================================================
RCS file: /cvs/src/sys/net/if_bridge.c,v
retrieving revision 1.338
diff -u -p -r1.338 if_bridge.c
--- if_bridge.c 6 Nov 2019 03:51:26 -0000 1.338
+++ if_bridge.c 11 Apr 2020 13:38:10 -0000
@@ -313,7 +313,9 @@ bridge_ioctl(struct ifnet *ifp, u_long c
break;
}
+ NET_LOCK();
error = ifpromisc(ifs, 1);
+ NET_UNLOCK();
if (error != 0) {
free(bif, M_DEVBUF, sizeof(*bif));
break;
@@ -558,7 +560,9 @@ bridge_ifremove(struct bridge_iflist *bi
}
bif->ifp->if_bridgeidx = 0;
+ NET_LOCK();
error = ifpromisc(bif->ifp, 0);
+ NET_UNLOCK();
bridge_rtdelete(sc, bif->ifp, 0);
bridge_flushrule(bif);
Index: if_tpmr.c
===================================================================
RCS file: /cvs/src/sys/net/if_tpmr.c,v
retrieving revision 1.9
diff -u -p -r1.9 if_tpmr.c
--- if_tpmr.c 11 Apr 2020 11:01:03 -0000 1.9
+++ if_tpmr.c 11 Apr 2020 13:38:10 -0000
@@ -201,12 +201,14 @@ tpmr_clone_destroy(struct ifnet *ifp)
if_detach(ifp);
+ NET_LOCK();
for (i = 0; i < nitems(sc->sc_ports); i++) {
struct tpmr_port *p = SMR_PTR_GET_LOCKED(&sc->sc_ports[i]);
if (p == NULL)
continue;
tpmr_p_dtor(sc, p, "destroy");
}
+ NET_UNLOCK();
free(sc, M_DEVBUF, sizeof(*sc));
