[PATCH] s390/crypto: fix aes ctr concurrency issue

[PATCH] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

The aes-ctr mode used one preallocated page without any concurrency
protection. When multiple threads run aes-ctr encryption or decryption
this could lead to data corruption.

The patch introduces locking for the preallocated page and alternatively
allocating and freeing of an temp page in concurrency situations.

Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>

Harald Freudenberger (1):
  s390/crypto: fix aes ctr concurrency issue

 arch/s390/crypto/aes_s390.c |   55 ++++++++++++++++++++++++++++++++----------
 1 files changed, 42 insertions(+), 13 deletions(-)

[PATCH] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

The aes-ctr mode used one preallocated page without any concurrency
protection. When multiple threads run aes-ctr encryption or decryption
this could lead to data corruption.

The patch introduces locking for the preallocated page and alternatively
allocating and freeing of an temp page in concurrency situations.
---
 arch/s390/crypto/aes_s390.c |   55 ++++++++++++++++++++++++++++++++----------
 1 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/arch/s390/crypto/aes_s390.c b/arch/s390/crypto/aes_s390.c
index 4363528..fcb5297 100644
--- a/arch/s390/crypto/aes_s390.c
+++ b/arch/s390/crypto/aes_s390.c
@@ -25,6 +25,7 @@
 #include <linux/err.h>
 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/mutex.h>
 #include "crypt_s390.h"
 
 #define AES_KEYLEN_128		1
@@ -32,6 +33,7 @@
 #define AES_KEYLEN_256		4
 
 static u8 *ctrblk;
+static DEFINE_MUTEX(ctrblk_lock);
 static char keylen_flag;
 
 struct s390_aes_ctx {
@@ -762,11 +764,25 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, long func,
 	unsigned int i, n, nbytes;
 	u8 buf[AES_BLOCK_SIZE];
 	u8 *out, *in;
+	u8 *ctrpage;
 
 	if (!walk->nbytes)
 		return ret;
 
-	memcpy(ctrblk, walk->iv, AES_BLOCK_SIZE);
+	if (mutex_trylock(&ctrblk_lock)) {
+		/* ctrblk is now reserved for us */
+		ctrpage = ctrblk;
+	} else {
+		/* ctrblk is in use by someone else, alloc our own page */
+		ctrpage = (u8*) __get_free_page(GFP_ATOMIC);
+		if (!ctrpage) {
+			/* gfp failed, wait until ctrblk becomes available */
+			mutex_lock(&ctrblk_lock);
+			ctrpage = ctrblk;
+		}
+	}
+
+	memcpy(ctrpage, walk->iv, AES_BLOCK_SIZE);
 	while ((nbytes = walk->nbytes) >= AES_BLOCK_SIZE) {
 		out = walk->dst.virt.addr;
 		in = walk->src.virt.addr;
@@ -775,17 +791,19 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, long func,
 			n = (nbytes > PAGE_SIZE) ? PAGE_SIZE :
 						 nbytes & ~(AES_BLOCK_SIZE - 1);
 			for (i = AES_BLOCK_SIZE; i < n; i += AES_BLOCK_SIZE) {
-				memcpy(ctrblk + i, ctrblk + i - AES_BLOCK_SIZE,
+				memcpy(ctrpage + i, ctrpage + i - AES_BLOCK_SIZE,
 				       AES_BLOCK_SIZE);
-				crypto_inc(ctrblk + i, AES_BLOCK_SIZE);
+				crypto_inc(ctrpage + i, AES_BLOCK_SIZE);
+			}
+			ret = crypt_s390_kmctr(func, sctx->key, out, in, n, ctrpage);
+			if (ret < 0 || ret != n) {
+				ret = -EIO;
+				goto out;
 			}
-			ret = crypt_s390_kmctr(func, sctx->key, out, in, n, ctrblk);
-			if (ret < 0 || ret != n)
-				return -EIO;
 			if (n > AES_BLOCK_SIZE)
-				memcpy(ctrblk, ctrblk + n - AES_BLOCK_SIZE,
+				memcpy(ctrpage, ctrpage + n - AES_BLOCK_SIZE,
 				       AES_BLOCK_SIZE);
-			crypto_inc(ctrblk, AES_BLOCK_SIZE);
+			crypto_inc(ctrpage, AES_BLOCK_SIZE);
 			out += n;
 			in += n;
 			nbytes -= n;
@@ -799,14 +817,25 @@ static int ctr_aes_crypt(struct blkcipher_desc *desc, long func,
 		out = walk->dst.virt.addr;
 		in = walk->src.virt.addr;
 		ret = crypt_s390_kmctr(func, sctx->key, buf, in,
-				       AES_BLOCK_SIZE, ctrblk);
-		if (ret < 0 || ret != AES_BLOCK_SIZE)
-			return -EIO;
+				       AES_BLOCK_SIZE, ctrpage);
+		if (ret < 0 || ret != AES_BLOCK_SIZE) {
+			ret = -EIO;
+			goto out;
+		}
 		memcpy(out, buf, nbytes);
-		crypto_inc(ctrblk, AES_BLOCK_SIZE);
+		crypto_inc(ctrpage, AES_BLOCK_SIZE);
 		ret = blkcipher_walk_done(desc, walk, 0);
 	}
-	memcpy(walk->iv, ctrblk, AES_BLOCK_SIZE);
+	memcpy(walk->iv, ctrpage, AES_BLOCK_SIZE);
+
+out:
+	if (ctrpage == ctrblk) {
+		/* free the reservation for ctrblk now */
+		mutex_unlock(&ctrblk_lock);
+	} else {
+		/* free the page allocated above */
+		free_page((unsigned long) ctrpage);
+	}
 	return ret;
 }
 
-- 
1.7.0.4

Re: [PATCH] s390/crypto: fix aes ctr concurrency issue

[Gerald Schaefer]

On Tue, 19 Nov 2013 11:22:11 +0100
Harald Freudenberger <freude@linux.vnet.ibm.com> wrote:

> The aes-ctr mode used one preallocated page without any concurrency
> protection. When multiple threads run aes-ctr encryption or decryption
> this could lead to data corruption.
> 
> The patch introduces locking for the preallocated page and
> alternatively allocating and freeing of an temp page in concurrency
> situations.
> 
> Signed-off-by: Harald Freudenberger <freude@linux.vnet.ibm.com>
> 
> Harald Freudenberger (1):
>   s390/crypto: fix aes ctr concurrency issue
> 
>  arch/s390/crypto/aes_s390.c |   55
> ++++++++++++++++++++++++++++++++---------- 1 files changed, 42
> insertions(+), 13 deletions(-)

Herbert, could you please add Cc: stable@vger.kernel.org to this?

Re: [PATCH] s390/crypto: fix aes ctr concurrency issue

[Herbert Xu]

On Tue, Nov 19, 2013 at 11:22:12AM +0100, Harald Freudenberger wrote:
> The aes-ctr mode used one preallocated page without any concurrency
> protection. When multiple threads run aes-ctr encryption or decryption
> this could lead to data corruption.
> 
> The patch introduces locking for the preallocated page and alternatively
> allocating and freeing of an temp page in concurrency situations.

You can't use mutex_lock because you may be in a non-sleepable
context.  Perhaps just fall back to doing it block-by-block, like
we do in aesni-intel on x86?

I have to say that your hardware has a funny way of doing CTR.
Somebody was generalising out of their backside :)

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

On Thu, 2013-11-28 at 22:00 +0800, Herbert Xu wrote:
> On Tue, Nov 19, 2013 at 11:22:12AM +0100, Harald Freudenberger wrote:
> > The aes-ctr mode used one preallocated page without any concurrency
> > protection. When multiple threads run aes-ctr encryption or decryption
> > this could lead to data corruption.
> > 
> > The patch introduces locking for the preallocated page and alternatively
> > allocating and freeing of an temp page in concurrency situations.
> 
> You can't use mutex_lock because you may be in a non-sleepable
> context.  Perhaps just fall back to doing it block-by-block, like
> we do in aesni-intel on x86?

The first attempt to lock the mutex is done with mutex_trylock() which
should be safe for non-sleepable context. If this fails, an attempt is
made to allocate a fresh page __get_free_page(GFP_ATOMIC). If this also
fails, well what could be done then ? I think, it is valid to wait for
the preallocated page to get released with an mutex_lock(). Should I
really add code here for handling the 3rd level of the exceptional
path ?

> 
> I have to say that your hardware has a funny way of doing CTR.
> Somebody was generalising out of their backside :)
> 
> Thanks,

Re: [PATCH] s390/crypto: fix aes ctr concurrency issue

[Herbert Xu]

On Thu, Nov 28, 2013 at 04:39:43PM +0100, Harald Freudenberger wrote:
>
> > You can't use mutex_lock because you may be in a non-sleepable
> > context.  Perhaps just fall back to doing it block-by-block, like
> > we do in aesni-intel on x86?
> 
> The first attempt to lock the mutex is done with mutex_trylock() which
> should be safe for non-sleepable context. If this fails, an attempt is
> made to allocate a fresh page __get_free_page(GFP_ATOMIC). If this also
> fails, well what could be done then ? I think, it is valid to wait for
> the preallocated page to get released with an mutex_lock(). Should I
> really add code here for handling the 3rd level of the exceptional
> path ?

If it's wrong per se, how does hiding it behind two if's make it
OK?
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] s390/crypto: fix aes ctr concurrency issue

[Harald Freudenberger]

On Fri, 2013-11-29 at 09:50 +0800, Herbert Xu wrote:
> On Thu, Nov 28, 2013 at 04:39:43PM +0100, Harald Freudenberger wrote:
> >
> > > You can't use mutex_lock because you may be in a non-sleepable
> > > context.  Perhaps just fall back to doing it block-by-block, like
> > > we do in aesni-intel on x86?
> > 
> > The first attempt to lock the mutex is done with mutex_trylock() which
> > should be safe for non-sleepable context. If this fails, an attempt is
> > made to allocate a fresh page __get_free_page(GFP_ATOMIC). If this also
> > fails, well what could be done then ? I think, it is valid to wait for
> > the preallocated page to get released with an mutex_lock(). Should I
> > really add code here for handling the 3rd level of the exceptional
> > path ?
> 
> If it's wrong per se, how does hiding it behind two if's make it
> OK?
Sorry, I got the point now. Will do a rework of the patch according
to your hints. Thanks