kern/44472: opencrypto compression MP-problem - value gets overwriten

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

>Number:         44472
>Category:       kern
>Synopsis:       opencrypto compression MP-problem - value gets overwriten
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    kern-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jan 27 11:35:01 +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;

>Unformatted: