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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Kernel]     [Gnu Classpath]     [Gnu Crypto]     [DM Crypt]     [Netfilter]     [Bugtraq]

  Powered by Linux