kern/47892: bad mutex handling in /src/sys/opencrypto/crypto.c

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

>Number:         47892
>Category:       kern
>Synopsis:       bad mutex handling in /src/sys/opencrypto/crypto.c
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 05 10:50:00 +0000 2013
>Originator:     Dr. Wolfgang Stukenbrock
>Release:        NetBSD 6.1
>Organization:
Dr. Nagler & Company GmbH
>Environment:
        
        
System: NetBSD test-s0 5.1.2 NetBSD 5.1.2 (NSW-WS) #3: Fri Dec 21 15:15:43 CET 
2012 wgstuken@test-s0:/usr/src/sys/arch/amd64/compile/NSW-WS amd64
Architecture: x86_64
Machine: amd64
>Description:
        In function crypto_invoke() there are two calls to the crypto session 
managment functions.
        These functions now (since 6.x I think) allocates the crypto_mtx 
themself so that it may not
        be aquired when calling crypto_freesession() and/or 
crypto_newsession(). OK so far.

        But in crypto_invoke() the only mutex-handling is the following:
          mutex_exit(&crypto_mtx);
          crypto_freesession(crp->crp_sid);
          mutex_enter(&crypto_mtx);
        So when calling crypto_freesession() the "crypto_mtx" mutex is 
released, but not when calling crypto_newsession().

        This cannot be OK !!!!

        I think crypto_invoke() is currently called without any of the crypto 
mutexes aquired, so the "mutex_exit(&crypto_mtx);"
        would crash the kernel. This may have been overlooked, because the 
CRYPTOCAP_F_CLEANUP case is very rare ...

        On the other hand crypto_invoke() accesses the global variable 
crypto_drivers_num and that one may change when an
        additional driver registers via crypto_get_driverid(). Same for the 
cc_flags field inside the driver itself on unregister calls.
        Therefore the "crypto_mtx" mutex need to be aquired - as far as I think.
        I'm not shure if a driver may ever (un-)register at runtime, but if not 
that should be stated somewhere in a comment and
        a security assertion or something like this should be added to the 
register function.
        (and then the whole unregister support makes no sence ...)

        If a driver unregister and all CAP are gone, the whole CAP-strucure is 
cleared with memset(...).
        This will kill cc_process and cc_arg fields, so if CRYPTOCAP_F_CLEANUP 
is ever set, theese fields are NULL (or 0)
        and the current implementation will end up by callein NULL as function.

        By the way: the comment "Driver has unregistered; ..." is wrong, 
because the variable crypto_drivers_num never gets
        lower as before.
        So either we have a very strange HID here (derived from the crp_sid) or 
we will always fall into the case
        "hid < crypto_drivers_num)". This looks like historic code that should 
be cleaned up.

        remark: the korresponding function "crypto_kinvoke" allocates the 
crypto_mtx mutex. So it looks like that this is simply
        missing here
>How-To-Repeat:
        found while validating/integrating private changes/patches into 6.1 
release
>Fix:
        Not realy know at the moment, because I'm not 100% confirm with the 
locking here till now.
        As long as no crypto-driver registers or unregisters at runtime of the 
system, CRYPTOCAP_F_CLEANUP is never set
        and no problems should came up.

        It may be a sollution (as quick shot) to just remove the mutext 
operations from the 
        "mutex_exit(&crypto_mtx); crypto_freesession(crp->crp_sid);  
mutex_enter(&crypto_mtx);" sequence, if no mutex is required.
        But this does not honor the other mentioned things.

        I think the mutex is needed and it should be handled as in 
crypto_kinvoke().

        Someone with deeper knowledge about the sence of the mutexes in 
opencrypto should have a look at this.

>Unformatted: