NetBSD Problem Report #44470

From Wolfgang.Stukenbrock@nagler-company.com  Wed Jan 26 16:19:32 2011
Return-Path: <Wolfgang.Stukenbrock@nagler-company.com>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by www.NetBSD.org (Postfix) with ESMTP id DC70463B873
	for <gnats-bugs@gnats.NetBSD.org>; Wed, 26 Jan 2011 16:19:31 +0000 (UTC)
Message-Id: <20110126161923.18B761E80CE@test-s0.nagler-company.com>
Date: Wed, 26 Jan 2011 17:19:23 +0100 (CET)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: opencrypto kernel implementation may pass outdated argument to worker
X-Send-Pr-Version: 3.95

>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:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Wed Jan 26 16:20:01 +0000 2011
>Closed-Date:    Mon Feb 07 09:23:58 +0000 2011
>Last-Modified:  Mon Feb 07 09:23:58 +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);
        }
 }

>Release-Note:

>Audit-Trail:
From: "Christos Zoulas" <christos@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44470 CVS commit: src/sys/opencrypto
Date: Wed, 26 Jan 2011 14:52:16 -0500

 Module Name:	src
 Committed By:	christos
 Date:		Wed Jan 26 19:52:16 UTC 2011

 Modified Files:
 	src/sys/opencrypto: crypto.c

 Log Message:
 PR/44470: Dr. Wolfgang Stukenbrock: opencrypto kernel implementation may pass
 outdated argument to worker


 To generate a diff of this commit:
 cvs rdiff -u -r1.36 -r1.37 src/sys/opencrypto/crypto.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: open->feedback
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Thu, 27 Jan 2011 00:28:28 +0000
State-Changed-Why:
Christos committed it, I think. Ok to close?


From: Wolfgang Stukenbrock <Wolfgang.Stukenbrock@nagler-company.com>
To: gnats-bugs@NetBSD.org
Cc: kern-bug-people@NetBSD.org, gnats-admin@NetBSD.org, netbsd-bugs@NetBSD.org,
        Wolfgang.Stukenbrock@nagler-company.com
Subject: Re: PR/44470 CVS commit: src/sys/opencrypto
Date: Mon, 07 Feb 2011 10:15:55 +0100

 Hi - sorry for the delay

 The patch is integrated and I agree to close the PR.
 I've validated it by a look into the sources.

 Thank,

 W. Stukenbrock

 Christos Zoulas wrote:

 > The following reply was made to PR kern/44470; it has been noted by GNATS.
 > 
 > From: "Christos Zoulas" <christos@netbsd.org>
 > To: gnats-bugs@gnats.NetBSD.org
 > Cc: 
 > Subject: PR/44470 CVS commit: src/sys/opencrypto
 > Date: Wed, 26 Jan 2011 14:52:16 -0500
 > 
 >  Module Name:	src
 >  Committed By:	christos
 >  Date:		Wed Jan 26 19:52:16 UTC 2011
 >  
 >  Modified Files:
 >  	src/sys/opencrypto: crypto.c
 >  
 >  Log Message:
 >  PR/44470: Dr. Wolfgang Stukenbrock: opencrypto kernel implementation may pass
 >  outdated argument to worker
 >  
 >  
 >  To generate a diff of this commit:
 >  cvs rdiff -u -r1.36 -r1.37 src/sys/opencrypto/crypto.c
 >  
 >  Please note that diffs are not public domain; they are subject to the
 >  copyright notices on the relevant files.
 >  
 > 


 -- 


 Dr. Nagler & Company GmbH
 Hauptstraße 9
 92253 Schnaittenbach

 Tel. +49 9622/71 97-42
 Fax +49 9622/71 97-50

 Wolfgang.Stukenbrock@nagler-company.com
 http://www.nagler-company.com


 Hauptsitz: Schnaittenbach
 Handelregister: Amberg HRB
 Gerichtsstand: Amberg
 Steuernummer: 201/118/51825
 USt.-ID-Nummer: DE 273143997
 Geschäftsführer: Dr. Martin Nagler, Dr. Dr. Karl-Kuno Kunze


State-Changed-From-To: feedback->closed
State-Changed-By: wiz@NetBSD.org
State-Changed-When: Mon, 07 Feb 2011 09:23:58 +0000
State-Changed-Why:
Confirmed fixed, thanks.


>Unformatted:

Home	PR Database Search