diff mbox series

crypto: arm64/aes-gcm-ce - fix scatterwalk API violation

Message ID 20180820145834.5916-1-ard.biesheuvel@linaro.org
State Accepted
Commit c2b24c36e0a30ebd8fc7d068da7f0451f2c05c76
Headers show
Series crypto: arm64/aes-gcm-ce - fix scatterwalk API violation | expand

Commit Message

Ard Biesheuvel Aug. 20, 2018, 2:58 p.m. UTC 
Commit 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on
two input blocks at a time") modified the granularity at which
the AES/GCM code processes its input to allow subsequent changes
to be applied that improve performance by using aggregation to
process multiple input blocks at once.

For this reason, it doubled the algorithm's 'chunksize' property
to 2 x AES_BLOCK_SIZE, but retained the non-SIMD fallback path that
processes a single block at a time. In some cases, this violates the
skcipher scatterwalk API, by calling skcipher_walk_done() with a
non-zero residue value for a chunk that is expected to be handled
in its entirety. This results in a WARN_ON() to be hit by the TLS
self test code, but is likely to break other user cases as well.
Unfortunately, none of the current test cases exercises this exact
code path at the moment.

Fixes: 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on two ...")
Reported-by: Vakul Garg <vakul.garg@nxp.com>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
 arch/arm64/crypto/ghash-ce-glue.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

-- 
2.11.0

Comments

Vakul Garg Aug. 20, 2018, 5:05 p.m. UTC | #1 
> -----Original Message-----

> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> Sent: Monday, August 20, 2018 8:29 PM

> To: linux-crypto@vger.kernel.org

> Cc: herbert@gondor.apana.org.au; Vakul Garg <vakul.garg@nxp.com>;

> davejwatson@fb.com; Peter Doliwa <peter.doliwa@nxp.com>; Ard

> Biesheuvel <ard.biesheuvel@linaro.org>

> Subject: [PATCH] crypto: arm64/aes-gcm-ce - fix scatterwalk API violation

> 

> Commit 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on

> two input blocks at a time") modified the granularity at which

> the AES/GCM code processes its input to allow subsequent changes

> to be applied that improve performance by using aggregation to

> process multiple input blocks at once.

> 

> For this reason, it doubled the algorithm's 'chunksize' property

> to 2 x AES_BLOCK_SIZE, but retained the non-SIMD fallback path that

> processes a single block at a time. In some cases, this violates the

> skcipher scatterwalk API, by calling skcipher_walk_done() with a

> non-zero residue value for a chunk that is expected to be handled

> in its entirety. This results in a WARN_ON() to be hit by the TLS

> self test code, but is likely to break other user cases as well.

> Unfortunately, none of the current test cases exercises this exact

> code path at the moment.

> 

> Fixes: 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on two ...")

> Reported-by: Vakul Garg <vakul.garg@nxp.com>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---

>  arch/arm64/crypto/ghash-ce-glue.c | 29 +++++++++++++++++++++++------

>  1 file changed, 23 insertions(+), 6 deletions(-)

> 

> diff --git a/arch/arm64/crypto/ghash-ce-glue.c b/arch/arm64/crypto/ghash-

> ce-glue.c

> index 6e9f33d14930..067d8937d5af 100644

> --- a/arch/arm64/crypto/ghash-ce-glue.c

> +++ b/arch/arm64/crypto/ghash-ce-glue.c

> @@ -417,7 +417,7 @@ static int gcm_encrypt(struct aead_request *req)

>  		__aes_arm64_encrypt(ctx->aes_key.key_enc, tag, iv,

> nrounds);

>  		put_unaligned_be32(2, iv + GCM_IV_SIZE);

> 

> -		while (walk.nbytes >= AES_BLOCK_SIZE) {

> +		while (walk.nbytes >= (2 * AES_BLOCK_SIZE)) {

>  			int blocks = walk.nbytes / AES_BLOCK_SIZE;

>  			u8 *dst = walk.dst.virt.addr;

>  			u8 *src = walk.src.virt.addr;

> @@ -437,11 +437,18 @@ static int gcm_encrypt(struct aead_request *req)

>  					NULL);

> 

>  			err = skcipher_walk_done(&walk,

> -						 walk.nbytes %

> AES_BLOCK_SIZE);

> +						 walk.nbytes % (2 *

> AES_BLOCK_SIZE));

>  		}

> -		if (walk.nbytes)

> +		if (walk.nbytes) {

>  			__aes_arm64_encrypt(ctx->aes_key.key_enc, ks, iv,

>  					    nrounds);

> +			if (walk.nbytes > AES_BLOCK_SIZE) {

> +				crypto_inc(iv, AES_BLOCK_SIZE);

> +				__aes_arm64_encrypt(ctx-

> >aes_key.key_enc,

> +					            ks + AES_BLOCK_SIZE, iv,

> +						    nrounds);

> +			}

> +		}

>  	}

> 

>  	/* handle the tail */

> @@ -545,7 +552,7 @@ static int gcm_decrypt(struct aead_request *req)

>  		__aes_arm64_encrypt(ctx->aes_key.key_enc, tag, iv,

> nrounds);

>  		put_unaligned_be32(2, iv + GCM_IV_SIZE);

> 

> -		while (walk.nbytes >= AES_BLOCK_SIZE) {

> +		while (walk.nbytes >= (2 * AES_BLOCK_SIZE)) {

>  			int blocks = walk.nbytes / AES_BLOCK_SIZE;

>  			u8 *dst = walk.dst.virt.addr;

>  			u8 *src = walk.src.virt.addr;

> @@ -564,11 +571,21 @@ static int gcm_decrypt(struct aead_request *req)

>  			} while (--blocks > 0);

> 

>  			err = skcipher_walk_done(&walk,

> -						 walk.nbytes %

> AES_BLOCK_SIZE);

> +						 walk.nbytes % (2 *

> AES_BLOCK_SIZE));

>  		}

> -		if (walk.nbytes)

> +		if (walk.nbytes) {

> +			if (walk.nbytes > AES_BLOCK_SIZE) {

> +				u8 *iv2 = iv + AES_BLOCK_SIZE;

> +

> +				memcpy(iv2, iv, AES_BLOCK_SIZE);

> +				crypto_inc(iv2, AES_BLOCK_SIZE);

> +

> +				__aes_arm64_encrypt(ctx-

> >aes_key.key_enc, iv2,

> +						    iv2, nrounds);

> +			}

>  			__aes_arm64_encrypt(ctx->aes_key.key_enc, iv, iv,

>  					    nrounds);

> +		}

>  	}

> 

>  	/* handle the tail */

> --

> 2.11.0


All tls tests pass now.

Tested-by: Vakul Garg <vakul.garg@nxp.com>
Herbert Xu Aug. 25, 2018, 1:28 p.m. UTC | #2 
On Mon, Aug 20, 2018 at 04:58:34PM +0200, Ard Biesheuvel wrote:
> Commit 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on

> two input blocks at a time") modified the granularity at which

> the AES/GCM code processes its input to allow subsequent changes

> to be applied that improve performance by using aggregation to

> process multiple input blocks at once.

> 

> For this reason, it doubled the algorithm's 'chunksize' property

> to 2 x AES_BLOCK_SIZE, but retained the non-SIMD fallback path that

> processes a single block at a time. In some cases, this violates the

> skcipher scatterwalk API, by calling skcipher_walk_done() with a

> non-zero residue value for a chunk that is expected to be handled

> in its entirety. This results in a WARN_ON() to be hit by the TLS

> self test code, but is likely to break other user cases as well.

> Unfortunately, none of the current test cases exercises this exact

> code path at the moment.

> 

> Fixes: 71e52c278c54 ("crypto: arm64/aes-ce-gcm - operate on two ...")

> Reported-by: Vakul Garg <vakul.garg@nxp.com>

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


Patch applied.  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
diff mbox series

Patch

diff --git a/arch/arm64/crypto/ghash-ce-glue.c b/arch/arm64/crypto/ghash-ce-glue.c
index 6e9f33d14930..067d8937d5af 100644
--- a/arch/arm64/crypto/ghash-ce-glue.c
+++ b/arch/arm64/crypto/ghash-ce-glue.c
@@ -417,7 +417,7 @@  static int gcm_encrypt(struct aead_request *req)
 		__aes_arm64_encrypt(ctx->aes_key.key_enc, tag, iv, nrounds);
 		put_unaligned_be32(2, iv + GCM_IV_SIZE);
 
-		while (walk.nbytes >= AES_BLOCK_SIZE) {
+		while (walk.nbytes >= (2 * AES_BLOCK_SIZE)) {
 			int blocks = walk.nbytes / AES_BLOCK_SIZE;
 			u8 *dst = walk.dst.virt.addr;
 			u8 *src = walk.src.virt.addr;
@@ -437,11 +437,18 @@  static int gcm_encrypt(struct aead_request *req)
 					NULL);
 
 			err = skcipher_walk_done(&walk,
-						 walk.nbytes % AES_BLOCK_SIZE);
+						 walk.nbytes % (2 * AES_BLOCK_SIZE));
 		}
-		if (walk.nbytes)
+		if (walk.nbytes) {
 			__aes_arm64_encrypt(ctx->aes_key.key_enc, ks, iv,
 					    nrounds);
+			if (walk.nbytes > AES_BLOCK_SIZE) {
+				crypto_inc(iv, AES_BLOCK_SIZE);
+				__aes_arm64_encrypt(ctx->aes_key.key_enc,
+					            ks + AES_BLOCK_SIZE, iv,
+						    nrounds);
+			}
+		}
 	}
 
 	/* handle the tail */
@@ -545,7 +552,7 @@  static int gcm_decrypt(struct aead_request *req)
 		__aes_arm64_encrypt(ctx->aes_key.key_enc, tag, iv, nrounds);
 		put_unaligned_be32(2, iv + GCM_IV_SIZE);
 
-		while (walk.nbytes >= AES_BLOCK_SIZE) {
+		while (walk.nbytes >= (2 * AES_BLOCK_SIZE)) {
 			int blocks = walk.nbytes / AES_BLOCK_SIZE;
 			u8 *dst = walk.dst.virt.addr;
 			u8 *src = walk.src.virt.addr;
@@ -564,11 +571,21 @@  static int gcm_decrypt(struct aead_request *req)
 			} while (--blocks > 0);
 
 			err = skcipher_walk_done(&walk,
-						 walk.nbytes % AES_BLOCK_SIZE);
+						 walk.nbytes % (2 * AES_BLOCK_SIZE));
 		}
-		if (walk.nbytes)
+		if (walk.nbytes) {
+			if (walk.nbytes > AES_BLOCK_SIZE) {
+				u8 *iv2 = iv + AES_BLOCK_SIZE;
+
+				memcpy(iv2, iv, AES_BLOCK_SIZE);
+				crypto_inc(iv2, AES_BLOCK_SIZE);
+
+				__aes_arm64_encrypt(ctx->aes_key.key_enc, iv2,
+						    iv2, nrounds);
+			}
 			__aes_arm64_encrypt(ctx->aes_key.key_enc, iv, iv,
 					    nrounds);
+		}
 	}
 
 	/* handle the tail */