Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Romain Izard]

2017-10-26 14:34 GMT+02:00 Tudor Ambarus :
> 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)) {
>> +   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
>> +
>> +   scatterwalk_map_and_copy(rctx->ciphertail, req->src,
>> +   (req->nbytes - ivsize), ivsize, 0);
>> +   }
>> +   rctx->mode = mode | AES_FLAGS_CIPHERTAIL;
>
>
> saving the last ciphertext block is a requirement for skcipher. This
> flag is redundant, there's no need to use it.
>

The idea regarding the ciphertail flag was because atmel_aes_complete
is used by for regular block crypto (ecb, cbc, ctr, etc.) but also for
aead transfers in gcm and authenc cases, which triggered panics in
my code.

I now realize that casting the areq value to an ablkcipher_req is wrong,
as it can 

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

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.

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?


+   memset(rctx->ciphertail, 0, ivsize);


memset to zero is not necessary.


+   }
+   }
+
 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.


 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.


-   rctx->mode = mode;
+
+   if (!(mode & AES_FLAGS_ENCRYPT)) {
+   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
+   scatterwalk_map_and_copy(rctx->ciphertail, req->src,
+   (req->nbytes - ivsize), ivsize, 0);
+   }
+   rctx->mode = mode | AES_FLAGS_CIPHERTAIL;


saving the last ciphertext block is a requirement for skcipher. This
flag is redundant, there's no need to use it.

Thank you, Romain!
ta

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

Hi, Romain,

On 10/18/2017 04:32 PM, Romain Izard wrote:

diff --git a/crypto/ccm.c b/crypto/ccm.c
index 1ce37ae0ce56..e7c2121a3ab2 100644
--- a/crypto/ccm.c
+++ b/crypto/ccm.c
@@ -47,6 +47,7 @@ struct crypto_ccm_req_priv_ctx {
 u8 odata[16];
 u8 idata[16];
 u8 auth_tag[16];
+   u8 iv[16];
 u32 flags;
 struct scatterlist src[3];
 struct scatterlist dst[3];
@@ -248,32 +249,22 @@ static void crypto_ccm_encrypt_done(struct
crypto_async_request *areq, int err)
 aead_request_complete(req, err);
  }

-static inline int crypto_ccm_check_iv(const u8 *iv)
-{
-   /* 2 <= L <= 8, so 1 <= L' <= 7. */
-   if (1 > iv[0] || iv[0] > 7)
-   return -EINVAL;
-
-   return 0;
-}
-
-static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag)
+static int crypto_ccm_init_crypt(struct aead_request *req, u8 *tag, u8* iv)
  {
 struct crypto_ccm_req_priv_ctx *pctx = crypto_ccm_reqctx(req);
 struct scatterlist *sg;
-   u8 *iv = req->iv;
-   int err;
+   u8 L = req->iv[0] + 1;

-   err = crypto_ccm_check_iv(iv);
-   if (err)
-   return err;
-
-   pctx->flags = aead_request_flags(req);
+   if (2 > L || L > 8)
+   return -EINVAL;

  /* Note: rfc 3610 and NIST 800-38C require counter of
  * zero to encrypt auth tag.
  */
-   memset(iv + 15 - iv[0], 0, iv[0] + 1);
+   memcpy(iv, req->iv, 16 - L);
+   memset(iv + 16 - L, 0, L);
+
+   pctx->flags = aead_request_flags(req);

 sg_init_table(pctx->src, 3);
 sg_set_buf(pctx->src, tag, 16);
@@ -301,10 +292,10 @@ static int crypto_ccm_encrypt(struct aead_request *req)
 struct scatterlist *dst;
 unsigned int cryptlen = req->cryptlen;
 u8 *odata = pctx->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;


Looks good. Can you please submit with a commit message?

Thanks,
ta

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Romain Izard]

2017-10-24 5:20 GMT+02:00 Herbert Xu :
> 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

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Herbert Xu]

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.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Tudor Ambarus]

Hi, Romain,

On 10/18/2017 04:32 PM, Romain Izard wrote:

my fix also led to a
systematic oops when running the ccm(aes) test case.


The NULL deference appears because of a memory corruption issue.

atmel-aes does not implement ccm(aes), so the algorithm will be in the
following form: ccm_base(atmel-ctr-aes,cbcmac(aes-generic)).

ccm auth uses the first byte of the IV as length and eventually will
memset memory to zero based on that length (see set_msg_len()). CTR
overwrites the iv with the last ciphertext block and the length will be
wrong.

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.

Cheers,
ta

Kernel panic when using ccm(aes) with the Atmel AES HW accelerator

[Romain Izard]

Hello,

For some time I have been trying to fix an issue with the Atmel AES hardware
accelerator available on SAMA5D2 chips. The ciphertext stealing mode did not
work, and this led to problems when using the cts(cbc(aes)) crypto engine
for fscrypt with Linux 4.13.

(see also
I have updated the driver in atmel-aes.c to fix this issue, taking care of
the corner case where the source and destination are the same in decryption
mode by saving the last 16 bytes of ciphertext in memory, and restoring them
into the 'info' member of the encryption request. This made it possible to
pass the cts(cbc(aes)) test in tcrypt.ko, which was failing before.

Here are my modifications:

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

 #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)];
 };

 #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);
+   memset(rctx->ciphertail, 0, ivsize);
+   }
+   }
+
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);
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);
-   rctx->mode = mode;
+
+   if (!(mode & AES_FLAGS_ENCRYPT)) {
+   int ivsize = crypto_ablkcipher_ivsize(ablkcipher);
+
+   scatterwalk_map_and_copy(rctx->ciphertail, req->src,
+   (req->nbytes - ivsize), ivsize, 0);
+   }
+   rctx->mode = mode | AES_FLAGS_CIPHERTAIL;
+

return atmel_aes_handle_queue(dd, >base);
 }

8<--


But as I wanted to test my code more thoroughly, I also ran the kcapi test
suite on my test platform. Some tests cases were now passing, some others
did not pass both before and after my fix, but my fix also led to a
systematic oops when running the ccm(aes) test case. Here is an example of
the kernel logs observed:

# kcapi -x 2 -c 'ccm(aes)' -i 010003020100a0a1a2a3a4a5 -k c0c1c2c3c4c5c6
c7c8c9cacbcccdcecf -q 588c979a61c663d2f066d0c2c0f989806d5f6b61dac38417e8d12cfdf9
26e0 -t 0001020304050607
[   92.34] atmel_aes f002c000.aes: saving ciphertext tail (16b)
[   92.35] atmel_aes f002c000.aes: copy ciphertext tail to IV
EBADMSG[   92.36] Unable to handle kernel NULL pointer dereference
at virtual address 0020
[   92.37] pgd = deebc000
[   92.37] [0020] *pgd=3ec40831, *pte=, *ppte=
[   92.37] Internal error: Oops: 17 [#1] ARM
[   92.37] Modules linked in:
[   92.37] CPU: 0 PID: 839 Comm: kcapi Not tainted 4.13.4+ #38
[   92.37] Hardware name: Atmel SAMA5
[