Hi Herbert,
I have tested your changes, not facing a kernel crash now but still kernel warning messages are coming:
[ 6.359772] alg: skcipher: ctr-aes-caam encryption test failed (wrong output IV) on test vector 0, cfg="in-place (one sglist)"
[ 6.371179] 00000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 6.377645] alg: self-tests for ctr(aes) using ctr-aes-caam failed (rc=-22)
[ 6.377649] ------------[ cut here ]------------
[ 6.389248] alg: self-tests for ctr(aes) using ctr-aes-caam failed (rc=-22)
[ 6.389269] WARNING: CPU: 0 PID: 246 at crypto/testmgr.c:6101 alg_test.part.0+0x3c8/0x3d0
[ 6.404400] Modules linked in:
[ 6.407446] CPU: 0 PID: 246 Comm: cryptomgr_test Not tainted 6.2.0-rc4-next-20230119-04770-gb8b0d08d8447 #3
[ 6.417181] Hardware name: LS1046A RDB Board (DT)
[ 6.421876] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 6.428833] pc : alg_test.part.0+0x3c8/0x3d0
[ 6.433095] lr : alg_test.part.0+0x3c8/0x3d0
[ 6.437358] sp : ffff80000a51bd40
[ 6.440662] x29: ffff80000a51bd40 x28: 0000000000000000 x27: 0000000000000000
[ 6.447795] x26: 00000000ffffffff x25: 0000000000000400 x24: ffff125a1d7ad280
[ 6.454926] x23: ffff125a1d7ad200 x22: ffff125a1e24d100 x21: 0000000000011085
[ 6.462057] x20: 00000000ffffffea x19: ffffb57461018490 x18: 0000000000000001
[ 6.469188] x17: 74757074756f2067 x16: 6e6f727728206465 x15: ffff125a1e24d568
[ 6.476319] x14: 0000000000000000 x13: ffffb57462e8e300 x12: 00000000ffffefff
[ 6.483450] x11: 0000000000000003 x10: ffffb5746298ecd8 x9 : ffffb5745f902b34
[ 6.490581] x8 : 0000000000017fe8 x7 : c0000000ffffefff x6 : 000000000000bff4
[ 6.497712] x5 : 0000000000057fa8 x4 : 0000000000000000 x3 : ffff80000a51bb08
[ 6.504842] x2 : ffffb57462936a60 x1 : 78bd64a75e317900 x0 : 0000000000000000
[ 6.511973] Call trace:
[ 6.514409] alg_test.part.0+0x3c8/0x3d0
[ 6.518325] alg_test+0x24/0x68
[ 6.521458] cryptomgr_test+0x28/0x48
[ 6.525112] kthread+0x114/0x120
[ 6.528334] ret_from_fork+0x10/0x20
[ 6.531903] ---[ end trace 0000000000000000 ]---
[ 6.536608] alg: skcipher: cbc-des-caam encryption test failed (wrong output IV) on test vector 0, cfg="in-place (one sglist)"
[ 6.548010] 00000000: 00 00 00 00 00 00 00 00
[ 6.552370] alg: self-tests for cbc(des) using cbc-des-caam failed (rc=-22)
Thanks,
Meenakshi
> -----Original Message-----
> From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Sent: Friday, February 24, 2023 3:48 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@xxxxxxx>
> Cc: Linux Crypto Mailing List <linux-crypto@xxxxxxxxxxxxxxx>; Christoph Hellwig
> <hch@xxxxxxxxxxxxx>; Horia Geanta <horia.geanta@xxxxxxx>; Pankaj Gupta
> <pankaj.gupta@xxxxxxx>; Gaurav Jain <gaurav.jain@xxxxxxx>
> Subject: [PATCH] crypto: caam - Fix edesc/iv ordering mixup
>
> Hi Meenakshi:
>
> On Fri, Feb 24, 2023 at 06:23:15AM +0000, Meenakshi Aggarwal wrote:
> >
> > with this change, edesc is following IV but we need IV to follow edesc.
> > Also, we are freeing edesc pointer in function, returning edesc
> > pointer and calling function also free edesc pointer but you have allocated IV .
> So we are facing kernel crash.
> >
> > We need to fix this, please share why are you allocating IV in place of edesc ?
>
> Sorry, my patch was completely broken. I was trying to place the IV at the front
> in a vain effort to reduce the total allocation size.
>
> Anyhow, please let me know if this patch fixes the problem, and I will push it to
> Linus.
>
> BTW, should we add you to the list of maintainers for caam? Perhaps next time
> we can spot the problem earlier. Thanks!
>
> ---8<---
> The attempt to add DMA alignment padding by moving IV to the front of edesc
> was completely broken as it didn't change the places where edesc was freed.
>
> It's also wrong as the IV may still share a cache-line with the edesc.
>
> Fix this by restoring the original layout and simply reserving enough memmory
> so that the IV is on a DMA cache-line by itself.
>
> Reported-by: Meenakshi Aggarwal <meenakshi.aggarwal@xxxxxxx>
> Fixes: 199354d7fb6e ("crypto: caam - Remove GFP_DMA and add DMA
> alignment padding")
> Signed-off-by: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
>
> diff --git a/drivers/crypto/caam/caamalg.c b/drivers/crypto/caam/caamalg.c
> index 4a9b998a8d26..c71955ac2252 100644
> --- a/drivers/crypto/caam/caamalg.c
> +++ b/drivers/crypto/caam/caamalg.c
> @@ -60,7 +60,11 @@
> #include <crypto/xts.h>
> #include <asm/unaligned.h>
> #include <linux/dma-mapping.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> #include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
>
> /*
> * crypto alg
> @@ -1683,18 +1687,19 @@ static struct skcipher_edesc
> *skcipher_edesc_alloc(struct skcipher_request *req,
> /*
> * allocate space for base edesc and hw desc commands, link tables, IV
> */
> - aligned_size = ALIGN(ivsize, __alignof__(*edesc));
> - aligned_size += sizeof(*edesc) + desc_bytes + sec4_sg_bytes;
> + aligned_size = sizeof(*edesc) + desc_bytes + sec4_sg_bytes;
> aligned_size = ALIGN(aligned_size, dma_get_cache_alignment());
> - iv = kzalloc(aligned_size, flags);
> - if (!iv) {
> + aligned_size += ~(ARCH_KMALLOC_MINALIGN - 1) &
> + (dma_get_cache_alignment() - 1);
> + aligned_size += ALIGN(ivsize, dma_get_cache_alignment());
> + edesc = kzalloc(aligned_size, flags);
> + if (!edesc) {
> dev_err(jrdev, "could not allocate extended descriptor\n");
> caam_unmap(jrdev, req->src, req->dst, src_nents, dst_nents, 0,
> 0, 0, 0);
> return ERR_PTR(-ENOMEM);
> }
>
> - edesc = (void *)(iv + ALIGN(ivsize, __alignof__(*edesc)));
> edesc->src_nents = src_nents;
> edesc->dst_nents = dst_nents;
> edesc->mapped_src_nents = mapped_src_nents; @@ -1706,6 +1711,8
> @@ static struct skcipher_edesc *skcipher_edesc_alloc(struct skcipher_request
> *req,
>
> /* Make sure IV is located in a DMAable area */
> if (ivsize) {
> + iv = (u8 *)edesc->sec4_sg + sec4_sg_bytes;
> + iv = PTR_ALIGN(iv, dma_get_cache_alignment());
> memcpy(iv, req->iv, ivsize);
>
> iv_dma = dma_map_single(jrdev, iv, ivsize,
> DMA_BIDIRECTIONAL); diff --git a/drivers/crypto/caam/caamalg_qi.c
> b/drivers/crypto/caam/caamalg_qi.c
> index 5e218bf20d5b..5d17f5862b93 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -20,8 +20,11 @@
> #include "caamalg_desc.h"
> #include <crypto/xts.h>
> #include <asm/unaligned.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> #include <linux/dma-mapping.h>
> #include <linux/kernel.h>
> +#include <linux/string.h>
>
> /*
> * crypto alg
> @@ -1259,6 +1262,7 @@ static struct skcipher_edesc
> *skcipher_edesc_alloc(struct skcipher_request *req,
> int dst_sg_idx, qm_sg_ents, qm_sg_bytes;
> struct qm_sg_entry *sg_table, *fd_sgt;
> struct caam_drv_ctx *drv_ctx;
> + unsigned int len;
>
> drv_ctx = get_drv_ctx(ctx, encrypt ? ENCRYPT : DECRYPT);
> if (IS_ERR(drv_ctx))
> @@ -1319,9 +1323,12 @@ static struct skcipher_edesc
> *skcipher_edesc_alloc(struct skcipher_request *req,
> qm_sg_ents = 1 + pad_sg_nents(qm_sg_ents);
>
> qm_sg_bytes = qm_sg_ents * sizeof(struct qm_sg_entry);
> - if (unlikely(ALIGN(ivsize, __alignof__(*edesc)) +
> - offsetof(struct skcipher_edesc, sgt) + qm_sg_bytes >
> - CAAM_QI_MEMCACHE_SIZE)) {
> +
> + len = offsetof(struct skcipher_edesc, sgt) + qm_sg_bytes;
> + len = ALIGN(len, dma_get_cache_alignment());
> + len += ivsize;
> +
> + if (unlikely(len > CAAM_QI_MEMCACHE_SIZE)) {
> dev_err(qidev, "No space for %d S/G entries and/or %dB IV\n",
> qm_sg_ents, ivsize);
> caam_unmap(qidev, req->src, req->dst, src_nents, dst_nents, 0,
> @@ -1330,18 +1337,18 @@ static struct skcipher_edesc
> *skcipher_edesc_alloc(struct skcipher_request *req,
> }
>
> /* allocate space for base edesc, link tables and IV */
> - iv = qi_cache_alloc(flags);
> - if (unlikely(!iv)) {
> + edesc = qi_cache_alloc(flags);
> + if (unlikely(!edesc)) {
> dev_err(qidev, "could not allocate extended descriptor\n");
> caam_unmap(qidev, req->src, req->dst, src_nents, dst_nents, 0,
> 0, DMA_NONE, 0, 0);
> return ERR_PTR(-ENOMEM);
> }
>
> - edesc = (void *)(iv + ALIGN(ivsize, __alignof__(*edesc)));
> -
> /* Make sure IV is located in a DMAable area */
> sg_table = &edesc->sgt[0];
> + iv = (u8 *)(sg_table + qm_sg_ents);
> + iv = PTR_ALIGN(iv, dma_get_cache_alignment());
> memcpy(iv, req->iv, ivsize);
>
> iv_dma = dma_map_single(qidev, iv, ivsize, DMA_BIDIRECTIONAL); diff -
> -git a/drivers/crypto/caam/qi.c b/drivers/crypto/caam/qi.c index
> 4c52c9365558..2ad2c1035856 100644
> --- a/drivers/crypto/caam/qi.c
> +++ b/drivers/crypto/caam/qi.c
> @@ -8,7 +8,13 @@
> */
>
> #include <linux/cpumask.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/kernel.h>
> #include <linux/kthread.h>
> +#include <linux/netdevice.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> #include <soc/fsl/qman.h>
>
> #include "debugfs.h"
> @@ -755,8 +761,8 @@ int caam_qi_init(struct platform_device *caam_pdev)
> napi_enable(irqtask);
> }
>
> - qi_cache = kmem_cache_create("caamqicache",
> CAAM_QI_MEMCACHE_SIZE, 0,
> - 0, NULL);
> + qi_cache = kmem_cache_create("caamqicache",
> CAAM_QI_MEMCACHE_SIZE,
> + dma_get_cache_alignment(), 0, NULL);
> if (!qi_cache) {
> dev_err(qidev, "Can't allocate CAAM cache\n");
> free_rsp_fqs();
> --
> Email: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&data=05%7C01%7Cmeenakshi.aggarwal%40nxp.co
> m%7C02a5c1fbb5f54a893c0a08db165073cb%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C638128307024161720%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C3000%7C%7C%7C&sdata=51qldsOV%2FNUO5VEMSd4kKbQdqIOJdEWF99Us%2
> FR01sUA%3D&reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&data=05%7C01%7Cmeenakshi.aggarwal
> %40nxp.com%7C02a5c1fbb5f54a893c0a08db165073cb%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638128307024161720%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=ME%2BROVEsFMovZ1P4Y3KOONWHwA
> ezgFgxOOe0y5fToCA%3D&reserved=0
