303334 - freebl symmetric ciphers need to be able to use preallocated contexts

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

In our early performance work on NSS, we found that a huge amount of time was
spent allocating and deallocating memory.  Using NSPR's "Zone allocator" 
helps a lot, but the amount of allocation is still wasteful.  

Every time one wants to do encryption, one must get a context for that 
operation, and in NSS 3.10, the only way to get that is to ask freebl to 
allocate a new one.  There's no way to have one preallocated (say, as part 
of a PKCS11 context) and have the freebl code use that context instead of 
allocating a new one.  

So, we desire a new set of functions for initializing contexts, one per 
algorithm, that take the context memory as input, rather than as output.  

While we're working on new context initialization functions, it also would
be good if all the new functions for initializing symmetric encryption 
contexts shared a common signature, with a common set of arguments.  

So, on the performance branch, I've added two new functions for every 
symmetric cipher:
XXX_AllocateContext(void)      - just allocates, nothing more.
XXX_InitContext(context, key, keylen, iv, mode, arg1, arg2)
         - initializes according to args.  Has two alg-dependent args.

I have reimplemented all of the old XXX_CreateContext() functions as calls
to their newer Allocate and Init brethren.

The hash functions can use a pre-allocated context, but always allocate
a new one when asked to copy a context (e.g. for partially completed hashes).
We should be able to copy them without allocation.  So for each hash 
algorithm, I've added a new function
XXX_Clone( dest_cx, source_cx)
      - which knows just how to do that copy.

I can prepare a patch that shows this work, but you can see the diffs now
at this URL:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=NSS&branch=NSS_PERFORMANCE_HACKS_BRANCH&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Ffreebl&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-04-06+18%3A00&maxdate=2005-04-15&cvsroot=%2Fcvsroot

I'd like to ask you (Bob and/or Wan-Teh) to review those diffs in that form, 
if you will.  If you really want me to attach a patch, please request that 
here.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Obviously, in addition to these changes in freebl, there must be changes 
to the callers of these functions in order to benefit from them.
That will be the subject of a subsequent RFE and patch.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

The URL I gave above was incomplete.  It included some, but not all, of the 
relevant changes.  The rest are seen at this URL
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=NSS&branch=NSS_PERFORMANCE_HACKS_BRANCH&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib%2Ffreebl&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-07+21%3A45&maxdate=2005-06-07+21%3A50&cvsroot=%2Fcvsroot

I will work on producing a patch that includes all these changes.

Comment 3

[Robert Relyea]

r=relyea

Just a couple of clean up things (I've ignored most of the stuff that was
obviously copied from the previous implementation).

1. The Assert in the AES_ContextInit() should probably be followed by an if that
bails out (I doubt we can trigger it with our current code but...).

2. The copied comment in the header file has RIJNDAEL_NUM_ROUNDS twice. The
second occurance should be RIJNDAEL_MAX_STATE_SIZE.

Neither should prevent the patch from landing..

bob

/* RIJNDAEL_NUM_ROUNDS                                                          
 *                                                                              
 * Number of rounds per execution                                               
 * Nk - number of key bytes                                                     
 * Nb - blocksize (in bytes)                                                    
 */                                                                            
                                                                              

#define RIJNDAEL_NUM_ROUNDS(Nk, Nb) \                                          
                                                                           
    (PR_MAX(Nk, Nb) + 6)                                                        
                                                                          

/* RIJNDAEL_NUM_ROUNDS                                                          

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch matches the above URLs

This patch was created with a shell script that cvs diffed each source file,
specifying the starting and ending version numbers explicitly for each file.

This patch depends on the prior application of the patch for another bug, 
bug 285932.

Comment 5

[Robert Relyea]

Comment on attachment 191657 [details] [diff] [review]
patch matches the above URLs

officially Making patch r+ so it's clear that it good to go in.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Checking in aeskeywrap.c; new revision: 1.4;  previous revision: 1.3
Checking in alg2268.c;    new revision: 1.7;  previous revision: 1.6
Checking in arcfour.c;    new revision: 1.16; previous revision: 1.15
Checking in blapi.h;      new revision: 1.19; previous revision: 1.18
Checking in blapit.h;     new revision: 1.16; previous revision: 1.15
Checking in desblapi.c;   new revision: 1.6;  previous revision: 1.5
Checking in ldvector.c;   new revision: 1.9;  previous revision: 1.8
Checking in loader.c;     new revision: 1.19; previous revision: 1.18
Checking in loader.h;     new revision: 1.13; previous revision: 1.12
Checking in md2.c;        new revision: 1.4;  previous revision: 1.3
Checking in md5.c;        new revision: 1.11; previous revision: 1.10
Checking in rijndael.c;   new revision: 1.19; previous revision: 1.18
Checking in rijndael.h;   new revision: 1.11; previous revision: 1.10
Checking in sha512.c;     new revision: 1.8;  previous revision: 1.7


Checked in on trunk.  
Leaving bug open pending correction of issues from review feedback.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to test after assert

Bob, please review this patch to see if it satisfies your 
review comments.  I fixed the comment in the header file 
before checkin, so it's already fixed.

Comment 8

[Robert Relyea]

Comment on attachment 191863 [details] [diff] [review]
patch to test after assert

Looks good to me.

bob

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Checking in rijndael.c;  new revision: 1.20; previous revision: 1.19

Thanks for the reviews.

Comment 10

[Wan-Teh Chang]

Comment on attachment 191657 [details] [diff] [review]
patch matches the above URLs

I also reviewed this patch, but didn't have time to write
the review comments until now.

When responding to my comments below, you don't need to answer
the questions marked with "Just curious", or the make the suggested
changes marked with "Not a big deal".

1. In blapit.h, we have

>+typedef void * (*BLapiAllocateFunc)(void);
>+typedef void (*BLapiDestroyContextFunc)(void *cx, PRBool freeit);
>+typedef SECStatus (*BLapiInitContextFunc)(void *cx, 
>+				   const unsigned char *key, 
>+				   unsigned int keylen,
>+				   const unsigned char *, 
>+				   int, 
>+				   unsigned int ,
>+				   unsigned int );
>+typedef SECStatus (*BLapiEncrypt)(void *cx, unsigned char *output,
>+				unsigned int *outputLen, 
>+				unsigned int maxOutputLen,
>+				const unsigned char *input, 
>+				unsigned int inputLen);

I suggest that we rename BLapiAllocateFunc as BLapiAllocateContextFunc
and BLapiEncrypt as BLapiEncryptFunc to be consistent with the other
two function pointer typedefs.

The "BLapi" prefix looks strange.  I think it can be just "BL".
Not a big deal.

What code uses these function pointer typedefs?  No other code in
this patch uses them.

Why are some of the function parameters unnamed?  Just curious.

2. In blapi.h and some other files, the "src" parameter of the various
XXX_Clone functions should be declared with "const".  I know you don't
want to fix this now.  That's fine.

3. Why are the XXX_InitContext and XXX_AllocateContext functions
only used for symmetric ciphers and the XXX_Clone functions only
used for hash functions?  Just curious.

4. In rijndael.c, we have

>+SECStatus   
>+AES_InitContext(AESContext *cx, const unsigned char *key, unsigned int keysize, 
>+	        const unsigned char *iv, int mode, unsigned int encrypt,
>+	        unsigned int blocksize)
> {
...
> cleanup:
>-    if (cx->expandedKey)
>-	PORT_ZFree(cx->expandedKey, cx->Nb * (cx->Nr + 1));
>-    PORT_ZFree(cx, sizeof *cx);
>-    return NULL;
>+    return SECFailure;
>+}

Are you sure we don't need to zeroize cx here?	This is the error
path so it can't be performance critical.

5. 

>+AESContext *
>+AES_CreateContext(const unsigned char *key, const unsigned char *iv, 
>+                  int mode, int encrypt,
>+                  unsigned int keysize, unsigned int blocksize)
>+{
>+    AESContext *cx = AES_AllocateContext();
>+    if (cx) {
>+	SECStatus rv = AES_InitContext(cx, key, keysize, iv, mode, encrypt,
>+				       blocksize);

AES_CreateContext and AES_InitContext have an unfortunate inconsistency
in the ordering of their parameters, in particular "keysize".  I'm
wondering if we can fix this.  Not a big deal.

6. 

> void 
> AES_DestroyContext(AESContext *cx, PRBool freeit)
> {
>-    PORT_ZFree(cx->expandedKey, cx->Nb * (cx->Nr + 1));
>-    memset(cx, 0, sizeof *cx);
>+/*  memset(cx, 0, sizeof *cx); */
>     if (freeit)
> 	PORT_Free(cx);
> }

Is it safe to not zeroize cx here?

7. In aeskeywrap.c, we have

>-/* $Id: aeskeywrap.c,v 1.3 2004/04/27 23:04:36 gerv%gerv.net Exp $ */
>+/* $Id: aeskeywrap.c,v 1.3.2.2 2005/06/17 02:08:50 julien.pierre.bugs%sun.com Exp $ */
> 
>-/* $Id: aeskeywrap.c,v 1.3 2004/04/27 23:04:36 gerv%gerv.net Exp $ */
>+/* $Id: aeskeywrap.c,v 1.3.2.2 2005/06/17 02:08:50 julien.pierre.bugs%sun.com Exp $ */

Please remove one of the duplicated RCS strings.

8.

> AESKeyWrap_CreateContext(const unsigned char *key, const unsigned char *iv, 
>                          int encrypt, unsigned int keylen)
> {
...
>+    rv = AESKeyWrap_InitContext(cx, key, keylen, iv, 0, encrypt, 0);
>+    if (rv != SECSuccess) {
>         PORT_Free(cx);
>-	return NULL; 	/* error should already be set */
>-    }

Why don't we call AESKeyWrap_DestroyContext here?

9. In alg2268.c, we have

>+SECStatus   
>+RC2_InitContext(RC2Context *cx, const unsigned char *key, unsigned int len,
>+	        const unsigned char *input, int mode, unsigned int efLen8, 
>+		unsigned int unused)

Please rename "input" as "iv".

10. In arcfour.c, we have:

>+RC4Context *
>+RC4_CreateContext(const unsigned char *key, int len)
>+{
...
>+	SECStatus rv = RC4_InitContext(cx, key, len, NULL, 0, 0, 0);
>+	if (rv != SECSuccess) {
>+	    PORT_ZFree(cx, sizeof(*cx));
>+	    cx = NULL;
>+	}

Why don't we call RC4_DestroyContext here?

11. In desblapi.c, we have

>+DESContext *
>+DES_CreateContext(const BYTE * key, const BYTE *iv, int mode, PRBool encrypt)
>+{
>+    DESContext *cx = PORT_ZNew(DESContext);
>+    SECStatus rv   = DES_InitContext(cx, key, 0, iv, mode, encrypt, 0);

We should test for cx == NULL before passing cx to DES_InitContext.
Although DES_InitContext(NULL, ...) won't crash, it will change the
error code from SEC_ERROR_NO_MEMORY to SEC_ERROR_INVALID_ARGS, which
is confusing.

>+    if (rv != SECSuccess) {
>+    	PORT_ZFree(cx, sizeof *cx);
>+	cx = NULL;
>     }

Same question: why not DES_DestroyContext here?

12. In md5.c, there are some commented-out memset calls or
change from PORT_ZFree to PORT_Free.   These should be
handled the same way as their counterparts in sha_fast.c.
I trust that you will do the right thing about zeroization.

13. Global comment: I hope you can write all the XXX_InitContext
functions in the same way.  Right now I've seen two or three
styles.  They should all look like this:

    XXXContext *cx = XXX_AllocateContext();
    if (cx) {
	SECStatus rv = XXX_InitContext(cx, ...);
	if (rv != SECSuccess) {
	    XXX_DestroyContext(cx, PR_TRUE);
	    // or PORT_ZFree(cx, sizeof *cx); pick one.
	    cx = NULL;
	}
    }
    return cx;