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

[Romain Izard]

2017-10-10 16:16 GMT+02:00 Boris Brezillon :
> 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

[Boris Brezillon]

Hi Romain,

May I ask why you're sending this patch to the MTD ML?

While I'm here, can you have a look at this patch [1] and add you
Reviewed-by/Tested-by?

Thanks,

Boris

[1]http://patchwork.ozlabs.org/patch/821959/

On Fri,  6 Oct 2017 17:51:08 +0200
Romain Izard  wrote:

> 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 
> ---
>  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);

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

[Romain Izard]

2017-10-06 17:51 GMT+02:00 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.
>
> 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_ablkcipher) from []
(crypto_ccm_decrypt+0xf4/0x13c)
[] (crypto_ccm_decrypt) from [] 

[PATCH] 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.

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

Signed-off-by: Romain Izard 
---
 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