kern/47538: races in uatp(4)

[Taylor R Campbell <campbell+netbsd%mumble.net@localhost>]

>Number:         47538
>Category:       kern
>Synopsis:       races in uatp(4)
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Feb 06 21:05:00 +0000 2013
>Originator:     Taylor R Campbell <campbell+netbsd%mumble.net@localhost>
>Release:        NetBSD-current
>Organization:
>Environment:
>Description:

        uatp_detach does nothing to ensure the callout is stopped
        before destroying it or to ensure that uatp_intr will not run
        before clobbering the softc structure.  sc_status, sc_input,
        sc_input_index, and sc_input_size are not protected by any
        mutex.  Also, geyser34_enable_raw_mode has a window between the
        UR_GET_REPORT and UR_SET_REPORT requests with nothing to
        prevent intervening updates to the report.  It's not clear that
        this is a problem, but it makes me nervous.

        The hardware should perhaps be reset when enabling the uhidev,
        not just when attaching the device -- that might help to fix
        the interrupt storms I have seen when starting X which the
        heuristic in uatp_intr doesn't detect.

>How-To-Repeat:

        Code inspection.

>Fix:

        Coming!

        I think that it should suffice to

        (a) extend sc_tap_mutex to cover all the mutable fields (and
        rename it to sc_lock), and

        (b) add some logic to uatp_detach to wait for the callout to
        stop running.

        In particular, I believe calling uhidev_close is enough to
        block further uatp_intr, so it is not necessary to add any
        logic to uatp_intr to check for sc_status & ENABLED.

        Eliminating the window in geyser34_enable_raw_mode might be
        best done with an additional flag in sc_status, say RESETTING,
        and an associated condvar:

static int
uatp_enter(struct uatp_softc *sc)
{
        int error;

        mutex_enter(&sc->sc_lock);
        while (ISSET(sc->sc_status, UATP_RESETTING)) {
                error = cv_timedwait_sig(&sc->sc_cv, &sc->sc_lock,
                    mstohz(1000));
                if (error)
                        return error;
        }

        return 0;
}

static void
uatp_exit(struct uatp_softc *sc)
{

        mutex_exit(&sc->sc_lock);
}

static void
geyser34_enable_raw_mode(struct uatp_softc *sc)
{

        if (uatp_enter(sc) != 0)
                return;
        sc->sc_status |= UATP_RESETTING;
        uatp_exit(sc);

        ...do the USB requests...

out:    mutex_enter(&sc->sc_lock);
        sc->sc_status &=~ UATP_RESETTING;
        cv_broadcast(&sc->sc_cv);
        mutex_exit(&sc->sc_lock);
}