[PATCH 0/2] Fixes for the Atmel AES crypto module

[PATCH 0/2] Fixes for the Atmel AES crypto module

[Romain Izard]

After encountering an issue with cts(cbc(aes)) in the Atmel AES module,
I have used tcrypt and libkcapi's test suite to validate my fix. This led
me to observe some other issues.

This series includes the IV issue correction for the Atmel AES crypto
engine, as well as a secondary issue observed when running
'insmod tcrypt.ko mode=10' and 'insmod tcrypt.ko mode=152' on a SAMA5D2
board.

The libkcapi test suite still reports some problems, for example when the
input data is too large to fit into an intermediate buffer in unaligned
cases. And it seems that with the v4.14 updates, new asynchronous tests
are enabled and report new issues.

Romain Izard (2):
  crypto: atmel-aes - properly set IV after {en,de}crypt
  crypto: atmel-aes - Reset the controller before each use

 drivers/crypto/atmel-aes.c | 50 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 10 deletions(-)

-- 
2.14.1

[PATCH 1/2] crypto: atmel-aes - properly set IV after {en,de}crypt

[Romain Izard]

Certain cipher modes like CTS expect the IV (req->info) of
ablkcipher_request (or equivalently req->iv of skcipher_request) to
contain the last ciphertext block when the {en,de}crypt operation is done.

Fix this issue for the Atmel AES hardware engine. The tcrypt test
case for cts(cbc(aes)) is now correctly passed.

In the case of in-place decryption, copy the ciphertext in an
intermediate buffer before decryption.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/crypto/atmel-aes.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 29e20c37f3a6..53432ab97d7e 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -110,6 +110,7 @@ struct atmel_aes_base_ctx {
 	int			keylen;
 	u32			key[AES_KEYSIZE_256 / sizeof(u32)];
 	u16			block_size;
+	bool			is_aead;
 };
 
 struct atmel_aes_ctx {
@@ -156,6 +157,7 @@ struct atmel_aes_authenc_ctx {
 
 struct atmel_aes_reqctx {
 	unsigned long		mode;
+	u32			lastc[AES_BLOCK_SIZE / sizeof(u32)];
 };
 
 #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
@@ -497,12 +499,34 @@ static void atmel_aes_authenc_complete(struct atmel_aes_dev *dd, int err);
 static inline int atmel_aes_complete(struct atmel_aes_dev *dd, int err)
 {
 #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
-	atmel_aes_authenc_complete(dd, err);
+	if (dd->ctx->is_aead)
+		atmel_aes_authenc_complete(dd, err);
 #endif
 
 	clk_disable(dd->iclk);
 	dd->flags &= ~AES_FLAGS_BUSY;
 
+	if (!dd->ctx->is_aead) {
+		struct ablkcipher_request *req =
+			ablkcipher_request_cast(dd->areq);
+		struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
+		struct crypto_ablkcipher *ablkcipher =
+			crypto_ablkcipher_reqtfm(req);
+		int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
+		if (rctx->mode & AES_FLAGS_ENCRYPT) {
+			scatterwalk_map_and_copy(req->info, req->dst,
+				req->nbytes - ivsize, ivsize, 0);
+		} else {
+			if (req->src == req->dst) {
+				memcpy(req->info, rctx->lastc, ivsize);
+			} else {
+				scatterwalk_map_and_copy(req->info, req->src,
+					req->nbytes - ivsize, ivsize, 0);
+			}
+		}
+	}
+
 	if (dd->is_async)
 		dd->areq->complete(dd->areq, err);
 
@@ -1071,11 +1095,11 @@ static int atmel_aes_ctr_start(struct atmel_aes_dev *dd)
 
 static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
 {
-	struct atmel_aes_base_ctx *ctx;
+	struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
+	struct atmel_aes_base_ctx *ctx = crypto_ablkcipher_ctx(ablkcipher);
 	struct atmel_aes_reqctx *rctx;
 	struct atmel_aes_dev *dd;
 
-	ctx = crypto_ablkcipher_ctx(crypto_ablkcipher_reqtfm(req));
 	switch (mode & AES_FLAGS_OPMODE_MASK) {
 	case AES_FLAGS_CFB8:
 		ctx->block_size = CFB8_BLOCK_SIZE;
@@ -1097,6 +1121,7 @@ static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
 		ctx->block_size = AES_BLOCK_SIZE;
 		break;
 	}
+	ctx->is_aead = false;
 
 	dd = atmel_aes_find_dev(ctx);
 	if (!dd)
@@ -1105,6 +1130,13 @@ static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long mode)
 	rctx = ablkcipher_request_ctx(req);
 	rctx->mode = mode;
 
+	if (!(mode & AES_FLAGS_ENCRYPT) && (req->src == req->dst)) {
+		int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
+		scatterwalk_map_and_copy(rctx->lastc, req->src,
+			(req->nbytes - ivsize), ivsize, 0);
+	}
+
 	return atmel_aes_handle_queue(dd, &req->base);
 }
 
@@ -1739,6 +1771,7 @@ static int atmel_aes_gcm_crypt(struct aead_request *req,
 
 	ctx = crypto_aead_ctx(crypto_aead_reqtfm(req));
 	ctx->block_size = AES_BLOCK_SIZE;
+	ctx->is_aead = true;
 
 	dd = atmel_aes_find_dev(ctx);
 	if (!dd)
@@ -2223,6 +2256,7 @@ static int atmel_aes_authenc_crypt(struct aead_request *req,
 
 	rctx->base.mode = mode;
 	ctx->block_size = AES_BLOCK_SIZE;
+	ctx->is_aead = true;
 
 	dd = atmel_aes_find_dev(ctx);
 	if (!dd)
-- 
2.14.1

[PATCH 2/2] crypto: atmel-aes - Reset the controller before each use

[Romain Izard]

When using the rfc4543(gcm(aes))) mode, the registers of the hardware
engine are not empty after use. If the engine is not reset before its
next use, the following results will be invalid.

Always reset the hardware engine.

Signed-off-by: Romain Izard <romain.izard.pro@gmail.com>
---
 drivers/crypto/atmel-aes.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 53432ab97d7e..024914e82734 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -76,12 +76,11 @@
 				 AES_FLAGS_ENCRYPT |		\
 				 AES_FLAGS_GTAGEN)
 
-#define AES_FLAGS_INIT		BIT(2)
 #define AES_FLAGS_BUSY		BIT(3)
 #define AES_FLAGS_DUMP_REG	BIT(4)
 #define AES_FLAGS_OWN_SHA	BIT(5)
 
-#define AES_FLAGS_PERSISTENT	(AES_FLAGS_INIT | AES_FLAGS_BUSY)
+#define AES_FLAGS_PERSISTENT	AES_FLAGS_BUSY
 
 #define ATMEL_AES_QUEUE_LENGTH	50
 
@@ -450,11 +449,8 @@ static int atmel_aes_hw_init(struct atmel_aes_dev *dd)
 	if (err)
 		return err;
 
-	if (!(dd->flags & AES_FLAGS_INIT)) {
-		atmel_aes_write(dd, AES_CR, AES_CR_SWRST);
-		atmel_aes_write(dd, AES_MR, 0xE << AES_MR_CKEY_OFFSET);
-		dd->flags |= AES_FLAGS_INIT;
-	}
+	atmel_aes_write(dd, AES_CR, AES_CR_SWRST);
+	atmel_aes_write(dd, AES_MR, 0xE << AES_MR_CKEY_OFFSET);
 
 	return 0;
 }
-- 
2.14.1

Re: [PATCH 2/2] crypto: atmel-aes - Reset the controller before each use

[Tudor Ambarus]

Hi, Romain,

On 10/31/2017 05:25 PM, Romain Izard wrote:
> When using the rfc4543(gcm(aes))) mode, the registers of the hardware
> engine are not empty after use. If the engine is not reset before its
> next use, the following results will be invalid.
> 
> Always reset the hardware engine.

Thanks for the fix! I could reproduce the issue only when running
rfc4543(gcm(aes))) and then, immediately after, ecb(aes).

Have you encountered this bug with other combination of algorithms?

I'm trying to isolate the bug so that we can have a more fine-grained
fix.

Cheers,
ta

Re: [PATCH 2/2] crypto: atmel-aes - Reset the controller before each use

[Romain Izard]

2017-11-06 16:45 GMT+01:00 Tudor Ambarus <tudor.ambarus@microchip.com>:
> Hi, Romain,
>
> On 10/31/2017 05:25 PM, Romain Izard wrote:
>>
>> When using the rfc4543(gcm(aes))) mode, the registers of the hardware
>> engine are not empty after use. If the engine is not reset before its
>> next use, the following results will be invalid.
>>
>> Always reset the hardware engine.
>
>
> Thanks for the fix! I could reproduce the issue only when running
> rfc4543(gcm(aes))) and then, immediately after, ecb(aes).
>
> Have you encountered this bug with other combination of algorithms?
>
> I'm trying to isolate the bug so that we can have a more fine-grained
> fix.

I just ran the tcrypt tests because they were failing on the
cts(cbc(aes)) transform and I observed this issue when the ecb
test failed only on the second run.

For me, the issue looks like the rfc4543 mode does not read
all the registers from the AES engine, and the following operation
fails because the registers are reused directly in the ECB mode.

As the ECB mode is a rare case where we do not use an IV, this
may be the reason why other modes do not display the issue.

-- 
Romain Izard

Re: [PATCH 0/2] Fixes for the Atmel AES crypto module

[Herbert Xu]

On Tue, Oct 31, 2017 at 04:25:22PM +0100, Romain Izard wrote:
> After encountering an issue with cts(cbc(aes)) in the Atmel AES module,
> I have used tcrypt and libkcapi's test suite to validate my fix. This led
> me to observe some other issues.
> 
> This series includes the IV issue correction for the Atmel AES crypto
> engine, as well as a secondary issue observed when running
> 'insmod tcrypt.ko mode=10' and 'insmod tcrypt.ko mode=152' on a SAMA5D2
> board.
> 
> The libkcapi test suite still reports some problems, for example when the
> input data is too large to fit into an intermediate buffer in unaligned
> cases. And it seems that with the v4.14 updates, new asynchronous tests
> are enabled and report new issues.
> 
> Romain Izard (2):
>   crypto: atmel-aes - properly set IV after {en,de}crypt
>   crypto: atmel-aes - Reset the controller before each use
> 
>  drivers/crypto/atmel-aes.c | 50 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 10 deletions(-)

All 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