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

[Romain Izard]

2017-10-26 14:34 GMT+02:00 Tudor Ambarus <tudor.ambarus at 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)) {
>> +               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 also be an aead_request. The ciphertail flag somehow worked
around this issue, but I need to rewrite the patch to take this into account.

Best regards,
-- 
Romain Izard