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

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, &req->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 0100000003020100a0a1a2a3a4a50000 -k c0c1c2c3c4c5c6
c7c8c9cacbcccdcecf -q 588c979a61c663d2f066d0c2c0f989806d5f6b61dac38417e8d12cfdf9
26e0 -t 0001020304050607
[   92.340000] atmel_aes f002c000.aes: saving ciphertext tail (16b)
[   92.350000] atmel_aes f002c000.aes: copy ciphertext tail to IV
EBADMSG[   92.360000] Unable to handle kernel NULL pointer dereference
at virtual address 00000020
[   92.370000] pgd = deebc000
[   92.370000] [00000020] *pgd=3ec40831, *pte=00000000, *ppte=00000000
[   92.370000] Internal error: Oops: 17 [#1] ARM
[   92.370000] Modules linked in:
[   92.370000] CPU: 0 PID: 839 Comm: kcapi Not tainted 4.13.4+ #38
[   92.370000] Hardware name: Atmel SAMA5
[   92.370000] task: dec0f580 task.stack: df730000
[   92.370000] PC is at aead_sock_destruct+0x28/0x7c
[   92.370000] LR is at __sk_destruct+0x30/0x168
[   92.370000] pc : [<c0388e6c>]    lr : [<c063df5c>]    psr: 600b0013
[   92.370000] sp : df731e78  ip : df731e98  fp : df731e94
[   92.370000] r10: 00000008  r9 : df241220  r8 : 00000000
[   92.370000] r7 : df427710  r6 : df221908  r5 : df6a9800  r4 : dee6e400
[   92.370000] r3 : 00000000  r2 : 00000000  r1 : 00000026  r0 : dee6e400
[   92.370000] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   92.370000] Control: 10c53c7d  Table: 3eebc059  DAC: 00000051
[   92.370000] Process kcapi (pid: 839, stack limit = 0xdf730208)
[   92.370000] Stack: (0xdf731e78 to 0xdf732000)
[   92.370000] 1e60:
    dee6e5c0 dee6e400
[   92.370000] 1e80: df221908 df427710 df731eac df731e98 c063df5c
c0388e50 dee6e400 00000000
[   92.370000] 1ea0: df731ebc df731eb0 c063fd5c c063df38 df731ed4
df731ec0 c063fdcc c063fd40
[   92.370000] 1ec0: df241200 00000000 df731ee4 df731ed8 c063fe78
c063fd7c df731ef4 df731ee8
[   92.370000] 1ee0: c0385a5c c063fe48 df731f0c df731ef8 c063a1fc
c0385a18 dee7fb40 df241220
[   92.370000] 1f00: df731f1c df731f10 c063a294 c063a1d8 df731f5c
df731f20 c01f10d8 c063a284
[   92.370000] 1f20: 00000000 00000000 dee7fb40 dee7fb48 df731f54
dec0f8bc dec0f580 c0d8a22c
[   92.370000] 1f40: 00000000 c0108ea4 df730000 00000000 df731f6c
df731f60 c01f1284 c01f1050
[   92.370000] 1f60: df731f8c df731f70 c0134cec c01f1278 df730000
c0108ea4 df731fb0 00000006
[   92.370000] 1f80: df731fac df731f90 c010c378 c0134c78 0004a070
0004a130 00000000 00000006
[   92.370000] 1fa0: 00000000 df731fb0 c0108d34 c010c2f0 00000000
00000000 00000001 00000000
[   92.370000] 1fc0: 0004a070 0004a130 00000000 00000006 00000027
ffffffb6 00000002 0004a130
[   92.370000] 1fe0: 00000000 bef01844 b6f43a1c b6ec5040 400b0010
00000006 00000000 00000000
[   92.370000] [<c0388e6c>] (aead_sock_destruct) from [<c063df5c>]
(__sk_destruct+0x30/0x168)
[   92.370000] [<c063df5c>] (__sk_destruct) from [<c063fd5c>]
(sk_destruct+0x28/0x3c)
[   92.370000] [<c063fd5c>] (sk_destruct) from [<c063fdcc>]
(__sk_free+0x5c/0xcc)
[   92.370000] [<c063fdcc>] (__sk_free) from [<c063fe78>] (sk_free+0x3c/0x40)
[   92.370000] [<c063fe78>] (sk_free) from [<c0385a5c>]
(af_alg_release+0x50/0x58)
[   92.370000] [<c0385a5c>] (af_alg_release) from [<c063a1fc>]
(sock_release+0x30/0xac)
[   92.370000] [<c063a1fc>] (sock_release) from [<c063a294>]
(sock_close+0x1c/0x24)
[   92.370000] [<c063a294>] (sock_close) from [<c01f10d8>] (__fput+0x94/0x1d8)
[   92.370000] [<c01f10d8>] (__fput) from [<c01f1284>] (____fput+0x18/0x1c)
[   92.370000] [<c01f1284>] (____fput) from [<c0134cec>]
(task_work_run+0x80/0xa4)
[   92.370000] [<c0134cec>] (task_work_run) from [<c010c378>]
(do_work_pending+0x94/0xbc)
[   92.370000] [<c010c378>] (do_work_pending) from [<c0108d34>]
(slow_work_pending+0xc/0x20)
[   92.370000] Code: e5902064 e1a04000 e59532d0 e3520000 (e5933020)
[   92.660000] ---[ end trace cce00d600b8ad985 ]---

Segmentation fault


As the changes in the hardware driver were related to the IV buffer, the
symptoms were coherent with an out-of-bounds memory access when updating the
IV. I applied the following patch, and this was enough to avoid the panic:

8<----------------------------------------------------------------------

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;

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

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, &req->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 0100000003020100a0a1a2a3a4a50000 -k c0c1c2c3c4c5c6
c7c8c9cacbcccdcecf -q 588c979a61c663d2f066d0c2c0f989806d5f6b61dac38417e8d12cfdf9
26e0 -t 0001020304050607
[   92.340000] atmel_aes f002c000.aes: saving ciphertext tail (16b)
[   92.350000] atmel_aes f002c000.aes: copy ciphertext tail to IV
EBADMSG[   92.360000] Unable to handle kernel NULL pointer dereference
at virtual address 00000020
[   92.370000] pgd = deebc000
[   92.370000] [00000020] *pgd=3ec40831, *pte=00000000, *ppte=00000000
[   92.370000] Internal error: Oops: 17 [#1] ARM
[   92.370000] Modules linked in:
[   92.370000] CPU: 0 PID: 839 Comm: kcapi Not tainted 4.13.4+ #38
[   92.370000] Hardware name: Atmel SAMA5
[   92.370000] task: dec0f580 task.stack: df730000
[   92.370000] PC is at aead_sock_destruct+0x28/0x7c
[   92.370000] LR is@__sk_destruct+0x30/0x168
[   92.370000] pc : [<c0388e6c>]    lr : [<c063df5c>]    psr: 600b0013
[   92.370000] sp : df731e78  ip : df731e98  fp : df731e94
[   92.370000] r10: 00000008  r9 : df241220  r8 : 00000000
[   92.370000] r7 : df427710  r6 : df221908  r5 : df6a9800  r4 : dee6e400
[   92.370000] r3 : 00000000  r2 : 00000000  r1 : 00000026  r0 : dee6e400
[   92.370000] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
[   92.370000] Control: 10c53c7d  Table: 3eebc059  DAC: 00000051
[   92.370000] Process kcapi (pid: 839, stack limit = 0xdf730208)
[   92.370000] Stack: (0xdf731e78 to 0xdf732000)
[   92.370000] 1e60:
    dee6e5c0 dee6e400
[   92.370000] 1e80: df221908 df427710 df731eac df731e98 c063df5c
c0388e50 dee6e400 00000000
[   92.370000] 1ea0: df731ebc df731eb0 c063fd5c c063df38 df731ed4
df731ec0 c063fdcc c063fd40
[   92.370000] 1ec0: df241200 00000000 df731ee4 df731ed8 c063fe78
c063fd7c df731ef4 df731ee8
[   92.370000] 1ee0: c0385a5c c063fe48 df731f0c df731ef8 c063a1fc
c0385a18 dee7fb40 df241220
[   92.370000] 1f00: df731f1c df731f10 c063a294 c063a1d8 df731f5c
df731f20 c01f10d8 c063a284
[   92.370000] 1f20: 00000000 00000000 dee7fb40 dee7fb48 df731f54
dec0f8bc dec0f580 c0d8a22c
[   92.370000] 1f40: 00000000 c0108ea4 df730000 00000000 df731f6c
df731f60 c01f1284 c01f1050
[   92.370000] 1f60: df731f8c df731f70 c0134cec c01f1278 df730000
c0108ea4 df731fb0 00000006
[   92.370000] 1f80: df731fac df731f90 c010c378 c0134c78 0004a070
0004a130 00000000 00000006
[   92.370000] 1fa0: 00000000 df731fb0 c0108d34 c010c2f0 00000000
00000000 00000001 00000000
[   92.370000] 1fc0: 0004a070 0004a130 00000000 00000006 00000027
ffffffb6 00000002 0004a130
[   92.370000] 1fe0: 00000000 bef01844 b6f43a1c b6ec5040 400b0010
00000006 00000000 00000000
[   92.370000] [<c0388e6c>] (aead_sock_destruct) from [<c063df5c>]
(__sk_destruct+0x30/0x168)
[   92.370000] [<c063df5c>] (__sk_destruct) from [<c063fd5c>]
(sk_destruct+0x28/0x3c)
[   92.370000] [<c063fd5c>] (sk_destruct) from [<c063fdcc>]
(__sk_free+0x5c/0xcc)
[   92.370000] [<c063fdcc>] (__sk_free) from [<c063fe78>] (sk_free+0x3c/0x40)
[   92.370000] [<c063fe78>] (sk_free) from [<c0385a5c>]
(af_alg_release+0x50/0x58)
[   92.370000] [<c0385a5c>] (af_alg_release) from [<c063a1fc>]
(sock_release+0x30/0xac)
[   92.370000] [<c063a1fc>] (sock_release) from [<c063a294>]
(sock_close+0x1c/0x24)
[   92.370000] [<c063a294>] (sock_close) from [<c01f10d8>] (__fput+0x94/0x1d8)
[   92.370000] [<c01f10d8>] (__fput) from [<c01f1284>] (____fput+0x18/0x1c)
[   92.370000] [<c01f1284>] (____fput) from [<c0134cec>]
(task_work_run+0x80/0xa4)
[   92.370000] [<c0134cec>] (task_work_run) from [<c010c378>]
(do_work_pending+0x94/0xbc)
[   92.370000] [<c010c378>] (do_work_pending) from [<c0108d34>]
(slow_work_pending+0xc/0x20)
[   92.370000] Code: e5902064 e1a04000 e59532d0 e3520000 (e5933020)
[   92.660000] ---[ end trace cce00d600b8ad985 ]---

Segmentation fault


As the changes in the hardware driver were related to the IV buffer, the
symptoms were coherent with an out-of-bounds memory access when updating the
IV. I applied the following patch, and this was enough to avoid the panic:

8<----------------------------------------------------------------------

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;

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: 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

[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

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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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 <herbert@gondor.apana.org.au>
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

[Romain Izard]

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

[Romain Izard]

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

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

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

[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

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

[Romain Izard]

2017-10-26 14:34 GMT+02:00 Tudor Ambarus <tudor.ambarus@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

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@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