Skip to contentSkip to conversation

Fixes for the Atmel AES crypto module

PATCH In review Romain Izard 3 participants 6 messages Oct 31, 2017 – Nov 6, 2017

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover Fixes for the Atmel AES crypto module

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(-)
Signature · 1 line
2.14.1
#1 lore raw

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

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.

---
 drivers/crypto/atmel-aes.c | 40 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 37 insertions(+), 3 deletions(-)
drivers/crypto/atmel-aes.c +37 -3
index 29e20c37f3a6..53432ab97d7e 100644
@@ -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)
Signature · 1 line
2.14.1
#2 lore raw

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

In reply to 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.

---
 drivers/crypto/atmel-aes.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)
drivers/crypto/atmel-aes.c +3 -7
index 53432ab97d7e..024914e82734 100644
@@ -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;
 }
Signature · 1 line
2.14.1
#3 lore raw

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

Hi, Romain,

Quoted 5 lines · 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.

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

#4 lore raw

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

2017-11-06 16:45 GMT+01:00 Tudor Ambarus <[email protected]>:

Quoted 11 lines

Hi, Romain,

Quoted 6 lines · 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.

Quoted 7 lines

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.

Signature · 1 line
Romain Izard
maintainer
#5 lore raw
In reply to Romain Izard
Quoted 14 lines · 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.

Quoted 6 lines

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.

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt