NetBSD Problem Report #47892

From Wolfgang.Stukenbrock@nagler-company.com  Wed Jun  5 10:49:34 2013
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1C56671AE4
	for <gnats-bugs@gnats.NetBSD.org>; Wed,  5 Jun 2013 10:49:34 +0000 (UTC)
Message-Id: <20130605104924.50A91123B93@test-s0.nagler-company.com>
Date: Wed,  5 Jun 2013 12:49:24 +0200 (CEST)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: bad mutex handling in /src/sys/opencrypto/crypto.c
X-Send-Pr-Version: 3.95

>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:

Home	PR Database Search