NetBSD Problem Report #44472

From Wolfgang.Stukenbrock@nagler-company.com  Thu Jan 27 11:32:31 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 7E01A63B873
	for <gnats-bugs@gnats.NetBSD.org>; Thu, 27 Jan 2011 11:32:31 +0000 (UTC)
Message-Id: <20110127113222.1A7771E80CE@test-s0.nagler-company.com>
Date: Thu, 27 Jan 2011 12:32:22 +0100 (CET)
From: Wolfgang.Stukenbrock@nagler-company.com
Reply-To: Wolfgang.Stukenbrock@nagler-company.com
To: gnats-bugs@gnats.NetBSD.org
Subject: opencrypto compression MP-problem - value gets overwriten
X-Send-Pr-Version: 3.95

>Number:         44472
>Category:       kern
>Synopsis:       opencrypto compression MP-problem - value gets overwriten
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 27 11:35:01 +0000 2011
>Closed-Date:    Thu Feb 10 21:28:56 +0000 2011
>Last-Modified:  Thu Feb 10 21:28:56 +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:
	The software implementation of opencrypto uses sessions to store prepared data - e.g.
	prepared key-information. This is stored in a "struct swcr_data" for each step fot the
	procession.
	Now it is possible, that the same session is called multiple times with the same
	"struct swcr_data" structures on different CPU's.
	In the network (FAST_)IPSEC processing session based information is stored in the SAV-entries
	and if more than one packet is gooing to use the same SAV it may be used at the same
	time by the interrupt-driver routine, the network-soft-interrupt  and a socket send call.
	For ESP and AH this is OK, because the prepared data in "struct swcr_data" is read-only
	during work.
	But for compression/decompression the resulting size is stored in the field "SW_size" of this
	structure and taken from there a few statements later again by an upper lever routine.
	If now two CPU's are dooing this at the same time, the extracted result is wrong from one of
	them.
>How-To-Repeat:
	not easy - under very heavy load with forwarding and local traffic that is
	routed through an ipsec tunnel with compression (esp is optional for this problem)
	you will get sometimes a "strange" crash with a NULL pointer and/or missing
	data in packets - e.g. in ip_forward() while extracting the first 68 bytes.
	I've got best reproducability by sending data from a Solaris10 system via an NetBSD 4.0
	system that is one tunnel endpoint to the NetBSD 5.1 system. But it still runs for a
	while before crashing.
>Fix:
This problem was hard to find but is easy to fix ...
	Simply to not use any data field in "struct swcr_data" for storing data while processing
	a request. This field is used only by swcr_compdec() to return the resulting length and it
	is static to cryptosoft.c. So we can add another parameter to return the value and eleminate
	SW_size from the "struct swcr_data".

	remark: the field SW_crc is completly unused and is removed by this patch from the struct
		definition too.

	The following patch will fix for /usr/src/sys/opencrypto/cryptosoft.[ch] this problem.

===================================================================
RCS file: RCS/cryptosoft.c,v
retrieving revision 1.1
diff -u -r1.1 cryptosoft.c
--- cryptosoft.c        2011/01/27 09:40:49     1.1
+++ cryptosoft.c        2011/01/27 09:47:24
@@ -61,7 +61,7 @@
        : cuio_copydata((struct uio *)a,b,c,d)

 static int swcr_encdec(struct cryptodesc *, struct swcr_data *, void *, int);
-static int swcr_compdec(struct cryptodesc *, struct swcr_data *, void *, int);
+static int swcr_compdec(struct cryptodesc *, struct swcr_data *, void *, int, int *);
 static int swcr_process(void *, struct cryptop *, int);
 static int swcr_newsession(void *, u_int32_t *, struct cryptoini *);
 static int swcr_freesession(void *, u_int64_t);
@@ -513,7 +513,7 @@
  */
 static int
 swcr_compdec(struct cryptodesc *crd, struct swcr_data *sw,
-    void *buf, int outtype)
+    void *buf, int outtype, int *res_size)
 {
        u_int8_t *data, *out;
        const struct swcr_comp_algo *cxf;
@@ -544,7 +544,7 @@
        /* Copy back the (de)compressed data. m_copyback is
         * extending the mbuf as necessary.
         */
-       sw->sw_size = result;
+       *res_size = (int)result;
        /* Check the compressed size when doing compression */
        if (crd->crd_flags & CRD_F_COMP) {
                if (result > crd->crd_len) {
@@ -986,10 +986,8 @@
                case CRYPTO_GZIP_COMP:
                        DPRINTF(("swcr_process: compdec for %d\n", sw->sw_alg));
                        if ((crp->crp_etype = swcr_compdec(crd, sw,
-                           crp->crp_buf, type)) != 0)
+                           crp->crp_buf, type, &crp->crp_olen)) != 0)
                                goto done;
-                       else
-                               crp->crp_olen = (int)sw->sw_size;
                        break;

                default:
===================================================================
RCS file: RCS/cryptosoft.h,v
retrieving revision 1.1
diff -u -r1.1 cryptosoft.h
--- cryptosoft.h        2011/01/27 09:40:49     1.1
+++ cryptosoft.h        2011/01/27 09:42:40
@@ -40,8 +40,6 @@
                        const struct swcr_enc_xform *SW_exf;
                } SWCR_ENC;
                struct {
-                       u_int32_t        SW_size;
-                       u_int32_t        SW_crc;
                        const struct swcr_comp_algo *SW_cxf;
                } SWCR_COMP;
        } SWCR_UN;
@@ -52,7 +50,6 @@
 #define sw_axf         SWCR_UN.SWCR_AUTH.SW_axf
 #define sw_kschedule   SWCR_UN.SWCR_ENC.SW_kschedule
 #define sw_exf         SWCR_UN.SWCR_ENC.SW_exf
-#define sw_size                SWCR_UN.SWCR_COMP.SW_size
 #define sw_cxf         SWCR_UN.SWCR_COMP.SW_cxf

        struct swcr_data *sw_next;

>Release-Note:

>Audit-Trail:
From: "Matthias Drochner" <drochner@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/44472 CVS commit: src/sys/opencrypto
Date: Thu, 10 Feb 2011 21:00:42 +0000

 Module Name:	src
 Committed By:	drochner
 Date:		Thu Feb 10 21:00:42 UTC 2011

 Modified Files:
 	src/sys/opencrypto: cryptosoft.c cryptosoft.h

 Log Message:
 Don't store temporary values in the opencrypto session data struct which
 can be shared by multiple threads -- pass them on the stack instead.
 Add some "const" to document this. (One _could_ use the session struct
 for temporary stuff with proper locking, but it seems unnecessary here.)
 Also remove the unused SW_crc member in the session struct.
 From Wolfgang Stukenbrock per PR kern/44472.


 To generate a diff of this commit:
 cvs rdiff -u -r1.26 -r1.27 src/sys/opencrypto/cryptosoft.c
 cvs rdiff -u -r1.6 -r1.7 src/sys/opencrypto/cryptosoft.h

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

State-Changed-From-To: open->closed
State-Changed-By: drochner@NetBSD.org
State-Changed-When: Thu, 10 Feb 2011 21:28:56 +0000
State-Changed-Why:
applied, thanks


>Unformatted:

Home	PR Database Search