[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
kern/47892: bad mutex handling in /src/sys/opencrypto/crypto.c
>Synopsis: bad mutex handling in /src/sys/opencrypto/crypto.c
>Arrival-Date: Wed Jun 05 10:50:00 +0000 2013
>Originator: Dr. Wolfgang Stukenbrock
>Release: NetBSD 6.1
Dr. Nagler & Company GmbH
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
In function crypto_invoke() there are two calls to the crypto session
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:
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
(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
found while validating/integrating private changes/patches into 6.1
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_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
Someone with deeper knowledge about the sence of the mutexes in
opencrypto should have a look at this.
Main Index |
Thread Index |