kern/44470: opencrypto kernel implementation may pass outdated argument to worker

[Wolfgang.Stukenbrock%nagler-company.com@localhost]

>Number:         44470
>Category:       kern
>Synopsis:       opencrypto kernel implementation may pass outdated argument to 
>worker
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jan 26 16:20:01 +0000 2011
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 5.1
>Organization:
Dr. Nagler & Company GmbH
>Environment:
        
        
System: NetBSD test-s0 4.0 NetBSD 4.0 (NSW-WS) #0: Tue Aug 17 17:28:09 CEST 
2010 wgstuken@test-s0:/usr/src/sys/arch/amd64/compile/NSW-WS amd64
Architecture: x86_64
Machine: amd64
>Description:
        In /usr/src/sys/opencrypto/crypto.c the crypto-requests are scheduled 
to the different
        sessions (for e.g. the software implementation in cryptosoft.c).
        This is done in crypto_kinvoke() without the crypto_mtx and in 
crypto_invoke() only
        with parts of the access to the common structures with crypto_mtx 
locked.
        If another call modifies the list of drivers while a request is 
scheduled, the data in
        the selected entry may get invalid and so either garbage is passes in 
crypto_invoke() to the
        process routine or garbage may be called and/or passed in 
crypto_kinvoke().
>How-To-Repeat:
        Found by a look into the sources.
>Fix:
        The following patch will fix this problem.
        For crypto_kinvoke() the mutex is allocated while the driver list is 
searched and the
        required pointers are transfered into local variables prior releasing 
the mutex again.
        For crypto_invoke() also the args pointer is copied into a local 
variable.

        For performance reason someone should think about an other strategie 
for calling the invoke routines
        as it is at the moment. Currently the crypto_mtx is released prio 
calling the routine and
        the routine gets it back after only a few security checks. Perhaps it 
would be faster to hold
        the mutex when calling the invoke() routine and return with mutex 
released.
        I'm not shure if this may conflict with the "crypto_timing" part in 
crypto_invoke(), so I do not change
        this.

--- crypto.c    2011/01/26 15:47:51     1.1
+++ crypto.c    2011/01/26 16:05:52
@@ -833,6 +833,8 @@
 {
        u_int32_t hid;
        int error;
+       int (*process)(void*, struct cryptkop *, int);
+       void *arg;
 
        /* Sanity checks. */
        if (krp == NULL)
@@ -843,6 +845,7 @@
                return EINVAL;
        }
 
+       mutex_spin_enter(&crypto_mtx);
        for (hid = 0; hid < crypto_drivers_num; hid++) {
                if ((crypto_drivers[hid].cc_flags & CRYPTOCAP_F_SOFTWARE) &&
                    crypto_devallowsoft == 0)
@@ -855,10 +858,13 @@
                break;
        }
        if (hid < crypto_drivers_num) {
+               process = crypto_drivers[hid].cc_kprocess;
+               arg = crypto_drivers[hid].cc_karg;
+               mutex_spin_exit(&crypto_mtx);
                krp->krp_hid = hid;
-               error = crypto_drivers[hid].cc_kprocess(
-                               crypto_drivers[hid].cc_karg, krp, hint);
+               error = (*process)(arg, krp, hint);
        } else {
+               mutex_spin_exit(&crypto_mtx);
                error = ENODEV;
        }
 
@@ -901,6 +907,7 @@
 {
        u_int32_t hid;
        int (*process)(void*, struct cryptop *, int);
+       void *arg;
 
 #ifdef CRYPTO_TIMING
        if (crypto_timing)
@@ -924,6 +931,7 @@
                if (crypto_drivers[hid].cc_flags & CRYPTOCAP_F_CLEANUP)
                        crypto_freesession(crp->crp_sid);
                process = crypto_drivers[hid].cc_process;
+               arg = crypto_drivers[hid].cc_arg;
                mutex_spin_exit(&crypto_mtx);
        } else {
                process = NULL;
@@ -954,7 +962,7 @@
                 * Invoke the driver to process the request.
                 */
                DPRINTF(("calling process for %08x\n", (uint32_t)crp));
-               return (*process)(crypto_drivers[hid].cc_arg, crp, hint);
+               return (*process)(arg, crp, hint);
        }
 }

>Unformatted: