Re: [PATCH v3] crypto: AF_ALG - remove locking in async callback
2017-11-10 13:20 GMT+01:00 Stephan Müller <smuel...@chronox.de>:
> The code paths protected by the socket-lock do not use or modify the
> socket in a non-atomic fashion. The actions pertaining the socket do not
> even need to be handled as an atomic operation. Thus, the socket-lock
> can be safely ignored.
>
> This fixes a bug regarding scheduling in atomic as the callback function
> may be invoked in interrupt context.
>
> In addition, the sock_hold is moved before the AIO encrypt/decrypt
> operation to ensure that the socket is always present. This avoids a
> tiny race window where the socket is unprotected and yet used by the AIO
> operation.
>
> Finally, the release of resources for a crypto operation is moved into a
> common function of af_alg_free_resources.
>
> Fixes: e870456d8e7c8 ("crypto: algif_skcipher - overhaul memory management")
> Fixes: d887c52d6ae43 ("crypto: algif_aead - overhaul memory management")
> Reported-by: Romain Izard <romain.izard....@gmail.com>
> Signed-off-by: Stephan Mueller <smuel...@chronox.de>
Tested-by: Romain Izard <romain.izard@gmail.com>
On 4.14-rc8, with some fuzzing when applying the patch.
The deterministic crash is not reproduced.
> ---
> crypto/af_alg.c | 21 ++---
> crypto/algif_aead.c | 23 ---
> crypto/algif_skcipher.c | 23 ---
> include/crypto/if_alg.h | 1 +
> 4 files changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 85cea9de324a..358749c38894 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -1021,6 +1021,18 @@ ssize_t af_alg_sendpage(struct socket *sock, struct
> page *page,
> EXPORT_SYMBOL_GPL(af_alg_sendpage);
>
> /**
> + * af_alg_free_resources - release resources required for crypto request
> + */
> +void af_alg_free_resources(struct af_alg_async_req *areq)
> +{
> + struct sock *sk = areq->sk;
> +
> + af_alg_free_areq_sgls(areq);
> + sock_kfree_s(sk, areq, areq->areqlen);
> +}
> +EXPORT_SYMBOL_GPL(af_alg_free_resources);
> +
> +/**
> * af_alg_async_cb - AIO callback handler
> *
> * This handler cleans up the struct af_alg_async_req upon completion of the
> @@ -1036,18 +1048,13 @@ void af_alg_async_cb(struct crypto_async_request
> *_req, int err)
> struct kiocb *iocb = areq->iocb;
> unsigned int resultlen;
>
> - lock_sock(sk);
> -
> /* Buffer size written by crypto operation. */
> resultlen = areq->outlen;
>
> - af_alg_free_areq_sgls(areq);
> - sock_kfree_s(sk, areq, areq->areqlen);
> - __sock_put(sk);
> + af_alg_free_resources(areq);
> + sock_put(sk);
>
> iocb->ki_complete(iocb, err ? err : resultlen, 0);
> -
> - release_sock(sk);
> }
> EXPORT_SYMBOL_GPL(af_alg_async_cb);
>
> diff --git a/crypto/algif_aead.c b/crypto/algif_aead.c
> index aacae0837aff..db9378a45296 100644
> --- a/crypto/algif_aead.c
> +++ b/crypto/algif_aead.c
> @@ -268,12 +268,23 @@ static int _aead_recvmsg(struct socket *sock, struct
> msghdr *msg,
>
> if (msg->msg_iocb && !is_sync_kiocb(msg->msg_iocb)) {
> /* AIO operation */
> + sock_hold(sk);
> areq->iocb = msg->msg_iocb;
> aead_request_set_callback(>cra_u.aead_req,
> CRYPTO_TFM_REQ_MAY_BACKLOG,
> af_alg_async_cb, areq);
> err = ctx->enc ? crypto_aead_encrypt(>cra_u.aead_req) :
> crypto_aead_decrypt(>cra_u.aead_req);
> +
> + /* AIO operation in progress */
> + if (err == -EINPROGRESS || err == -EBUSY) {
> + /* Remember output size that will be generated. */
> + areq->outlen = outlen;
> +
> + return -EIOCBQUEUED;
> + }
> +
> + sock_put(sk);
> } else {
> /* Synchronous operation */
> aead_request_set_callback(>cra_u.aead_req,
> @@ -285,19 +296,9 @@ static int _aead_recvmsg(struct socket *sock, struct
> msghdr *msg,
> >wait);
> }
>
> - /* AIO operation in progress */
> - if (err == -EINPROGRESS) {
> - sock_hold(sk);
> -
> - /* Remember output size that will be generated. */
> - areq->outlen = outlen;
> -
> - return -EIOCBQUEUED;
> - }
>
> free:
> -
Re: [PATCH 2/2] crypto: atmel-aes - Reset the controller before each use
2017-11-06 16:45 GMT+01:00 Tudor Ambarus <tudor.amba...@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
[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.
Signed-off-by: Romain Izard <romain.izard@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;
+ boolis_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, >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
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@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
[PATCH 0/2] 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(-)
--
2.14.1
[PATCH] crypto: ccm - preserve the IV buffer
The IV buffer used during CCM operations is used twice, during both the hashing step and the ciphering step. When using a hardware accelerator that updates the contents of the IV buffer at the end of ciphering operations, the value will be modified. In the decryption case, the subsequent setup of the hashing algorithm will interpret the updated IV instead of the original value, which can lead to out-of-bounds writes. Reuse the idata buffer, only used in the hashing step, to preserve the IV's value during the ciphering step in the decryption case. Signed-off-by: Romain Izard <romain.izard@gmail.com> --- crypto/ccm.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crypto/ccm.c b/crypto/ccm.c index 1ce37ae0ce56..0a083342ec8c 100644 --- a/crypto/ccm.c +++ b/crypto/ccm.c @@ -363,7 +363,7 @@ static int crypto_ccm_decrypt(struct aead_request *req) unsigned int cryptlen = req->cryptlen; u8 *authtag = pctx->auth_tag; u8 *odata = pctx->odata; - u8 *iv = req->iv; + u8 *iv = pctx->idata; int err; cryptlen -= authsize; @@ -379,6 +379,8 @@ static int crypto_ccm_decrypt(struct aead_request *req) if (req->src != req->dst) dst = pctx->dst; + memcpy(iv, req->iv, 16); + skcipher_request_set_tfm(skreq, ctx->ctr); skcipher_request_set_callback(skreq, pctx->flags, crypto_ccm_decrypt_done, req); -- 2.14.1
Re: [PATCH] crypto: AF_ALG - remove locking in async callback
2017-10-29 21:39 GMT+01:00 Stephan Müller <smuel...@chronox.de>:
> Am Mittwoch, 25. Oktober 2017, 17:26:31 CET schrieb Romain Izard:
>
> Hi Romain,
>
> the patch below should cover the issue you see. Would you mind testing it?
>
> Thanks
> Stephan
>
> ---8<---
>
> The code paths protected by the socket-lock do not use or modify the
> socket in a non-atomic fashion. The actions pertaining the socket do not
> even need to be handled as an atomic operation. Thus, the socket-lock
> can be safely ignored.
>
> This fixes a bug regarding scheduling in atomic as the callback function
> may be invoked in interrupt context.
>
> Fixes: 2d97591ef43d0 ("crypto: af_alg - consolidation of duplicate code")
> Reported-by: Romain Izard <romain.izard@gmail.com>
> Signed-off-by: Stephan Mueller <smuel...@chronox.de>
Tested-by: Romain Izard <romain.izard@gmail.com>
The issue observed with atmel-aes is not reproduced anymore.
> ---
> crypto/af_alg.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 337cf382718e..a41f08642eee 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -1063,8 +1063,6 @@ void af_alg_async_cb(struct crypto_async_request *_req,
> int err)
> struct kiocb *iocb = areq->iocb;
> unsigned int resultlen;
>
> - lock_sock(sk);
> -
> /* Buffer size written by crypto operation. */
> resultlen = areq->outlen;
>
> @@ -1073,8 +1071,6 @@ void af_alg_async_cb(struct crypto_async_request *_req,
> int err)
> __sock_put(sk);
>
> iocb->ki_complete(iocb, err ? err : resultlen, 0);
> -
> - release_sock(sk);
> }
> EXPORT_SYMBOL_GPL(af_alg_async_cb);
>
> --
> 2.13.6
>
>
Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator
2017-10-26 14:34 GMT+02:00 Tudor Ambarus <tudor.amba...@microchip.com>:
> Hi, Romain,
>
> On 10/18/2017 04:32 PM, Romain Izard wrote:
>>
>> diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
>> index 29e20c37f3a6..f3eabe1f1490 100644
>> --- a/drivers/crypto/atmel-aes.c
>> +++ b/drivers/crypto/atmel-aes.c
>> @@ -80,6 +80,7 @@
>> #define AES_FLAGS_BUSY BIT(3)
>> #define AES_FLAGS_DUMP_REG BIT(4)
>> #define AES_FLAGS_OWN_SHA BIT(5)
>> +#define AES_FLAGS_CIPHERTAIL BIT(6)
>
>
> not really needed, see below.
>
>>
>> #define AES_FLAGS_PERSISTENT (AES_FLAGS_INIT | AES_FLAGS_BUSY)
>>
>> @@ -156,6 +157,7 @@ struct atmel_aes_authenc_ctx {
>>
>> struct atmel_aes_reqctx {
>> unsigned long mode;
>> + u32 ciphertail[AES_BLOCK_SIZE / sizeof(u32)];
>
> How about renaming this variable to "lastc"? Ci, i=1..n is the usual
> notation for the ciphertext blocks.
>
OK
> The assumption that the last ciphertext block is of AES_BLOCK_SIZE size
> is correct, this driver is meant just for AES.
>
>> };
>>
>> #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
>> @@ -496,6 +498,11 @@ 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)
>> {
>> + struct ablkcipher_request *req =
>> ablkcipher_request_cast(dd->areq);
>> + struct crypto_ablkcipher *ablkcipher =
>> crypto_ablkcipher_reqtfm(req);
>> + struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
>> + int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>> +
>> #ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
>> atmel_aes_authenc_complete(dd, err);
>> #endif
>> @@ -503,6 +510,17 @@ static inline int atmel_aes_complete(struct
>> atmel_aes_dev *dd, int err)
>> clk_disable(dd->iclk);
>> dd->flags &= ~AES_FLAGS_BUSY;
>>
>> + if (rctx->mode & AES_FLAGS_CIPHERTAIL) {
>> + if (rctx->mode & AES_FLAGS_ENCRYPT) {
>> + scatterwalk_map_and_copy(req->info, req->dst,
>> +req->nbytes - ivsize,
>> +ivsize, 0);
>> + } else {
>> + memcpy(req->info, rctx->ciphertail, ivsize);
>
>
> why don't we make the same assumption and replace ivsize with
> AES_BLOCK_SIZE? Is there any chance that req->info was allocated with
> less than AES_BLOCK_SIZE size?
>
I do not really know. I'm just getting used to the crypto framework, and the
fact that the iv buffer size is implicit...
>> + memset(rctx->ciphertail, 0, ivsize);
>
>
> memset to zero is not necessary.
>
OK
>> + }
>> + }
>> +
>> if (dd->is_async)
>> dd->areq->complete(dd->areq, err);
>>
>> @@ -1071,11 +1089,13 @@ static int atmel_aes_ctr_start(struct
>> atmel_aes_dev *dd)
>>
>> static int atmel_aes_crypt(struct ablkcipher_request *req, unsigned long
>> mode)
>> {
>> + struct crypto_ablkcipher *ablkcipher;
>> struct atmel_aes_base_ctx *ctx;
>> struct atmel_aes_reqctx *rctx;
>> struct atmel_aes_dev *dd;
>>
>> - ctx = crypto_ablkcipher_ctx(crypto_ablkcipher_reqtfm(req));
>> + ablkcipher = crypto_ablkcipher_reqtfm(req);
>> + ctx = crypto_ablkcipher_ctx(ablkcipher);
>
>
> you can initialize these variables at declaration.
>
OK
>> switch (mode & AES_FLAGS_OPMODE_MASK) {
>> case AES_FLAGS_CFB8:
>> ctx->block_size = CFB8_BLOCK_SIZE;
>> @@ -1103,7 +1123,15 @@ static int atmel_aes_crypt(struct
>> ablkcipher_request *req, unsigned long mode)
>> return -ENODEV;
>>
>> rctx = ablkcipher_request_ctx(req);
>
>
> while here, please initialize this at declaration.
>
> Also, this one:
> '''
> dd = atmel_aes_find_dev(ctx);
> if (!dd)
> return -ENODEV;
>
> '''
> should be moved at the beginning of the function. If an initialization
> might fail, let's check it as soon as we can, so that we don't waste
> resources.
>
Most of these initializations are in fact structure casts.
>> - rctx->mode = mode;
>> +
>> + if (!(mode & AES_FLAGS_ENCRYPT)) {
>> +
"BUG: scheduling while atomic" in atmel-aes on Linux v4.14-rc6
Hello, While running the kcapi test suite on a SAMA5D2 Xplained board with a v4.14-rc6 kernel, I encountered the following error: # kcapi -x 9 -e -c "cbc(aes)" -i -k 000 0 -p 1b077a6af4b7f98229de786d7516b639 BUG: scheduling while atomic: kcapi/926/0x0100 CPU: 0 PID: 926 Comm: kcapi Not tainted 4.14.0-rc6 #2 Hardware name: Atmel SAMA5 [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [] (show_stack) from [] (__schedule_bug+0x60/0x80) [] (__schedule_bug) from [] (__schedule+0x368/0x3fc) [] (__schedule) from [] (schedule+0x40/0xa0) [] (schedule) from [] (__lock_sock+0x78/0xb0) [] (__lock_sock) from [] (lock_sock_nested+0x48/0x50) [] (lock_sock_nested) from [] (af_alg_async_cb+0x20/0x80) [] (af_alg_async_cb) from [] (atmel_aes_transfer_complete+0x38/0x68) [] (atmel_aes_transfer_complete) from [] (tasklet_action+0x68/0xb4) [] (tasklet_action) from [] (__do_softirq+0xc4/0x250) [] (__do_softirq) from [] (irq_exit+0xfc/0x130) [] (irq_exit) from [] (__handle_domain_irq+0x58/0xa8) [] (__handle_domain_irq) from [] (__irq_svc+0x6c/0x90) [] (__irq_svc) from [] (skcipher_recvmsg+0x2d8/0x318) [] (skcipher_recvmsg) from [] (sock_read_iter+0x88/0xc8) [] (sock_read_iter) from [] (aio_read.constprop.3+0xcc/0x178) [] (aio_read.constprop.3) from [] (SyS_io_submit+0x540/0x644) [] (SyS_io_submit) from [] (ret_fast_syscall+0x0/0x48) After bisecting, I determined that it appeared during the 4.14 merge window, with the following commit: e870456d8e7c crypto: algif_skcipher - overhaul memory management Best regards, -- Romain Izard
Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator
2017-10-24 5:20 GMT+02:00 Herbert Xu <herb...@gondor.apana.org.au>: > On Mon, Oct 23, 2017 at 03:38:59PM +0300, Tudor Ambarus wrote: >> >> I will propose a fix, but I'm taking my time to better understand why >> CTR requires to overwrite the iv with the last ciphertext block. > > That's an API requirement. So we should fix ccm. > Where is the documentation for this API requirement? I tried to find it in the kernel, but I only found a few comments in the commit messages or in the implementations, but not an explicit requirement. Moreover, as it seems to be a common mistake in the crypto accelerators, I believe that the algorithms' self-test should also check the IV at the end of a request. In the decryption case, the code should probably be shared for most implementations: we need to save the input data before decryption in case of in-place decoding, and restore it into the IV buffer before returning to the caller. -- Romain Izard
Kernel panic when using ccm(aes) with the Atmel AES HW accelerator
ctx->odata; - u8 *iv = req->iv; + u8 *iv = pctx->iv; int err; - err = crypto_ccm_init_crypt(req, odata); + err = crypto_ccm_init_crypt(req, odata, iv); if (err) return err; @@ -363,12 +354,12 @@ static int crypto_ccm_decrypt(struct aead_request *req) unsigned int cryptlen = req->cryptlen; u8 *authtag = pctx->auth_tag; u8 *odata = pctx->odata; - u8 *iv = req->iv; + u8 *iv = pctx->iv; int err; cryptlen -= authsize; - err = crypto_ccm_init_crypt(req, authtag); + err = crypto_ccm_init_crypt(req, authtag, iv); if (err) return err; 8<-- My problem is that I do not understand why it works. It ensures that in both encryption and decryption cases, the IV buffer is available and 16 bytes wide. But normally the IV buffer provided by the crypto request is already 16 bytes wide, as the algorithm is registered with ivsize=16. As I am not very familiar with the crypto subsystem, I fear that I missed something. I would gladly appreciate the feedback of more experienced developers regarding this issue. Best regards, -- Romain Izard
Re: [PATCH] crypto: atmel-aes - properly set IV after {en,de}crypt
2017-10-10 16:16 GMT+02:00 Boris Brezillon <boris.brezil...@free-electrons.com>: > Hi Romain, > > May I ask why you're sending this patch to the MTD ML? > This is related to an issue reported by Nicolas Feignon with UBIFS and fscrypt, which was reported on both mtd and fscrypt mailing lists. The file names are encrypted with cts(cbc(aes)), and this patch tried to fix a SAMA5D2 issue where the file names are not correctly encrypted when hardware acceleration is enabled. > While I'm here, can you have a look at this patch [1] and add you > Reviewed-by/Tested-by? > > [1]http://patchwork.ozlabs.org/patch/821959/ I'll try it. -- Romain Izard
Re: [PATCH] crypto: atmel-aes - properly set IV after {en,de}crypt
2017-10-06 17:51 GMT+02:00 Romain Izard <romain.izard@gmail.com>:
>
> 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.
>
> To handle the case of in-place decryption, copy the ciphertext in an
> intermediate buffer before decryption.
>
Unfortunately this does not seem to be enough. The tcrypt module's tests
pass, but I encounter more issues. If I run the libkcapi test suite, I
end up randomly with the following type of panic:
8< --
Unable to handle kernel paging request at virtual address 7ffc
pgd = dee9c000
[7ffc] *pgd=
Internal error: Oops: 5 [#1] ARM
Modules linked in:
CPU: 0 PID: 2187 Comm: kcapi Not tainted 4.13.4+ #16
Hardware name: Atmel SAMA5
task: dec7f280 task.stack: dee82000
PC is at memcpy+0x114/0x330
LR is at atmel_aes_transfer_complete+0x64/0xe8
pc : []lr : []psr: 2013
sp : dee83bcc ip : 0003 fp : dee83bfc
r10: r9 : df638940 r8 : df638874
r7 : 0010 r6 : r5 : df638940 r4 : dec68110
r3 : 4004 r2 : 000c r1 : 7ffc r0 : df638afc
Flags: nzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c53c7d Table: 3ee9c059 DAC: 0051
Process kcapi (pid: 2187, stack limit = 0xdee82208)
Stack: (0xdee83bcc to 0xdee84000)
3bc0:df638afc dec68110 c05e419c
3be0: 0030 dec68110 df557040 0030 dee83c3c dee83c00 c05e61cc c05e4144
3c00: 10031000 dec68110 df6388a4 0030 df6388a4 dec68110 df6388a4 0030
3c20: 0030 df638874 df557070 dee83c6c dee83c40 c05e488c c05e6064
3c40: df6388a4 df638874 dee83c6c dec68110 0030 0030 df6388a4 df638874
3c60: dee83c94 dee83c70 c05e4998 c05e471c 0030 dec68110 df557040
3c80: df638874 df557070 dee83cd4 dee83c98 c05e6198 c05e48d4 c05e6058
3ca0: dee83cbc df6388a4 c041ab04 dec68110 df557040 df638940 ff8d
3cc0: a013 0004 dee83d04 dee83cd8 c05e62f8 c05e6064 dee83d3c dee83ce8
3ce0: c01dbe9c dec68110 df638940 df638940 ff8d dee83d2c dee83d08
3d00: c05e4ac8 c05e61f8 df638940 4000 df638800 df557000 0020 df638860
3d20: dee83d44 dee83d30 c05e4b50 c05e4a14 df638874 0400 dee83d54 dee83d48
3d40: c05e4ba0 c05e4af0 dee83d74 dee83d58 c034997c c05e4b90 df638840 df638800
3d60: dec3e4c0 dee83db4 dee83d78 c0368f70 c0349918 c03bfab0
3d80: df6388a4 df638afc dfff1bc2 df63898c df638988 df608800 0040 df701000
3da0: df63898c dee83e28 dee83e1c dee83db8 c0393514 c0368e88 df701000 df608800
3dc0: 0020 df638afc 030c 0188 df63898c df638800 0044 014000c0
3de0: df638ae8 0040 dee83e20 df701000 0040 dee83e80 c0392d48 df61ff00
3e00: df241200 dee83e80 0004a150 dee83e6c dee83e20 c0641d8c c0392d54
3e20: dee83ea0 c0101254
3e40: dee4f400 df61ff00 dee4f400
3e60: dee83efc dee83e70 c0242950 c0641cfc dee83e80 c07edac4 8013
3e80: 0040 dee83e98 0001 c0101254 0004a298 0040
3ea0: f7f27003 0055 b6f27000 df644000 00f6 c0108ea4 0004a150
3ec0: e000 c021636c dee83ee4 dee83ed8 c021636c c02162d8 dee83efc 00049148
3ee0: dee4f400 df61ff00 df557200 dee83fa4 dee83f00 c0243db8 c024285c
3f00: dee83f1c c0d05f40 c0d98a98 014080c0 c0d9ad5c 0001 e000
3f20: dee83f20 dee83f20 dee83f28 dee83f28 dee83f30 dee83f30
3f40: 0007 0004a298 0040
3f60: 0001 0006 011d b6f2bce8
3f80: 00f6 c0108ea4 dee82000 dee83fa8
3fa0: c0108ce0 c0243734 b6f2bce8 b6f27000 0001 0004a150 00049188
3fc0: b6f2bce8 00f6 0001 000490b8 000490d4
3fe0: bee3d838 bee3d828 b6ee63bc b6e73810 6010 b6f27000
[] (memcpy) from [] (atmel_aes_transfer_complete+0x64/0xe8)
[] (atmel_aes_transfer_complete) from []
(atmel_aes_ctr_transfer+0x174/0x194)
[] (atmel_aes_ctr_transfer) from []
(atmel_aes_cpu_transfer+0x17c/0x1b8)
[] (atmel_aes_cpu_transfer) from []
(atmel_aes_cpu_start+0xd0/0xd4)
[] (atmel_aes_cpu_start) from []
(atmel_aes_ctr_transfer+0x140/0x194)
[] (atmel_aes_ctr_transfer) from []
(atmel_aes_ctr_start+0x10c/0x15c)
[] (atmel_aes_ctr_start) from []
(atmel_aes_handle_queue+0xc0/0xdc)
[] (atmel_aes_handle_queue) from []
(atmel_aes_crypt+0x6c/0xa0)
[] (atmel_aes_crypt) from []
(atmel_aes_ctr_decrypt+0x1c/0x20)
[] (atmel_aes_ctr_decrypt) from []
(skcipher_decrypt_ablkcipher+0x70/0x74)
[] (skcipher_decrypt_ab
[PATCH] 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.
To handle the case of in-place decryption, copy the ciphertext in an
intermediate buffer before decryption.
Signed-off-by: Romain Izard <romain.izard@gmail.com>
---
drivers/crypto/atmel-aes.c | 28
1 file changed, 28 insertions(+)
diff --git a/drivers/crypto/atmel-aes.c b/drivers/crypto/atmel-aes.c
index 29e20c37f3a6..f22300babb45 100644
--- a/drivers/crypto/atmel-aes.c
+++ b/drivers/crypto/atmel-aes.c
@@ -156,6 +156,7 @@ struct atmel_aes_authenc_ctx {
struct atmel_aes_reqctx {
unsigned long mode;
+ u8 *backup_info;
};
#ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
@@ -496,6 +497,12 @@ 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)
{
+ struct ablkcipher_request *req = ablkcipher_request_cast(dd->areq);
+ struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
+ struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
+ int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+ bool enc = atmel_aes_is_encrypt(dd);
+
#ifdef CONFIG_CRYPTO_DEV_ATMEL_AUTHENC
atmel_aes_authenc_complete(dd, err);
#endif
@@ -503,6 +510,15 @@ static inline int atmel_aes_complete(struct atmel_aes_dev
*dd, int err)
clk_disable(dd->iclk);
dd->flags &= ~AES_FLAGS_BUSY;
+ if (enc) {
+ scatterwalk_map_and_copy(req->info, req->dst,
+req->nbytes - ivsize, ivsize, 0);
+ } else if (rctx->backup_info) {
+ memcpy(req->info, rctx->backup_info, ivsize);
+ kfree(rctx->backup_info);
+ rctx->backup_info = NULL;
+ }
+
if (dd->is_async)
dd->areq->complete(dd->areq, err);
@@ -959,13 +975,25 @@ static int atmel_aes_transfer_complete(struct
atmel_aes_dev *dd)
static int atmel_aes_start(struct atmel_aes_dev *dd)
{
struct ablkcipher_request *req = ablkcipher_request_cast(dd->areq);
+ struct crypto_ablkcipher *ablkcipher = crypto_ablkcipher_reqtfm(req);
struct atmel_aes_reqctx *rctx = ablkcipher_request_ctx(req);
+ int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+ bool enc = atmel_aes_is_encrypt(dd);
bool use_dma = (req->nbytes >= ATMEL_AES_DMA_THRESHOLD ||
dd->ctx->block_size != AES_BLOCK_SIZE);
int err;
atmel_aes_set_mode(dd, rctx);
+ if (!enc) {
+ rctx->backup_info = kzalloc(ivsize, GFP_KERNEL);
+ if (rctx->backup_info == NULL)
+ return atmel_aes_complete(dd, -ENOMEM);
+
+ scatterwalk_map_and_copy(rctx->backup_info, req->src,
+(req->nbytes - ivsize), ivsize, 0);
+ }
+
err = atmel_aes_hw_init(dd);
if (err)
return atmel_aes_complete(dd, err);
--
2.11.0
