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.
> 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.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:08:46 -0000
> @@ -3031,7 +3031,9 @@ ifpromisc(struct ifnet *ifp, int pswitch
> unsigned short oif_flags;
> int oif_pcount, error;
>
> + NET_ASSERT_LOCKED(); /* modifying if_flags */
> oif_flags = ifp->if_flags;
> + KERNEL_ASSERT_LOCKED(); /* modifying if_pcount */
> oif_pcount = ifp->if_pcount;
> if (pswitch) {
> if (ifp->if_pcount++ != 0)
> 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:08:46 -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:08:46 -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:08:46 -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));
>
>
