Hi Herbert,
Changes are working for me.
Thanks,
Meenakshi
> -----Original Message-----
> From: Herbert Xu <herbert@xxxxxxxxxxxxxxxxxxx>
> Sent: Monday, February 27, 2023 1:56 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: [v2 PATCH] crypto: caam - Fix edesc/iv ordering mixup
>
> On Mon, Feb 27, 2023 at 05:20:32AM +0000, Meenakshi Aggarwal wrote:
> > Hi Herbert,
> >
> > I have tested your changes, not facing a kernel crash now but still kernel
> warning messages are coming:
>
> Thanks for testing! Indeed, I forgot to update the IV calculations on the way out:
>
> ---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..12b1c8346243 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
> @@ -1000,6 +1004,13 @@ static void aead_crypt_done(struct device *jrdev,
> u32 *desc, u32 err,
> crypto_finalize_aead_request(jrp->engine, req, ecode); }
>
> +static inline u8 *skcipher_edesc_iv(struct skcipher_edesc *edesc) {
> +
> + return PTR_ALIGN((u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
> + dma_get_cache_alignment());
> +}
> +
> static void skcipher_crypt_done(struct device *jrdev, u32 *desc, u32 err,
> void *context)
> {
> @@ -1027,8 +1038,7 @@ static void skcipher_crypt_done(struct device *jrdev,
> u32 *desc, u32 err,
> * This is used e.g. by the CTS mode.
> */
> if (ivsize && !ecode) {
> - memcpy(req->iv, (u8 *)edesc->sec4_sg + edesc->sec4_sg_bytes,
> - ivsize);
> + memcpy(req->iv, skcipher_edesc_iv(edesc), ivsize);
>
> print_hex_dump_debug("dstiv @" __stringify(__LINE__)": ",
> DUMP_PREFIX_ADDRESS, 16, 4, req->iv, @@
> -1683,18 +1693,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 +1717,7
> @@ static struct skcipher_edesc *skcipher_edesc_alloc(struct skcipher_request
> *req,
>
> /* Make sure IV is located in a DMAable area */
> if (ivsize) {
> + iv = skcipher_edesc_iv(edesc);
> 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..743ce50c14f2 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
> @@ -1204,6 +1207,12 @@ static int ipsec_gcm_decrypt(struct aead_request
> *req)
> false);
> }
>
> +static inline u8 *skcipher_edesc_iv(struct skcipher_edesc *edesc) {
> + return PTR_ALIGN((u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> + dma_get_cache_alignment());
> +}
> +
> static void skcipher_done(struct caam_drv_req *drv_req, u32 status) {
> struct skcipher_edesc *edesc;
> @@ -1236,8 +1245,7 @@ static void skcipher_done(struct caam_drv_req
> *drv_req, u32 status)
> * This is used e.g. by the CTS mode.
> */
> if (!ecode)
> - memcpy(req->iv, (u8 *)&edesc->sgt[0] + edesc->qm_sg_bytes,
> - ivsize);
> + memcpy(req->iv, skcipher_edesc_iv(edesc), ivsize);
>
> qi_cache_free(edesc);
> skcipher_request_complete(req, ecode); @@ -1259,6 +1267,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 +1328,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 +1342,24 @@ 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)));
> + edesc->src_nents = src_nents;
> + edesc->dst_nents = dst_nents;
> + edesc->qm_sg_bytes = qm_sg_bytes;
> + edesc->drv_req.app_ctx = req;
> + edesc->drv_req.cbk = skcipher_done;
> + edesc->drv_req.drv_ctx = drv_ctx;
>
> /* Make sure IV is located in a DMAable area */
> sg_table = &edesc->sgt[0];
> + iv = skcipher_edesc_iv(edesc);
> memcpy(iv, req->iv, ivsize);
>
> iv_dma = dma_map_single(qidev, iv, ivsize, DMA_BIDIRECTIONAL); @@
> -1353,13 +1371,7 @@ static struct skcipher_edesc *skcipher_edesc_alloc(struct
> skcipher_request *req,
> return ERR_PTR(-ENOMEM);
> }
>
> - edesc->src_nents = src_nents;
> - edesc->dst_nents = dst_nents;
> edesc->iv_dma = iv_dma;
> - edesc->qm_sg_bytes = qm_sg_bytes;
> - edesc->drv_req.app_ctx = req;
> - edesc->drv_req.cbk = skcipher_done;
> - edesc->drv_req.drv_ctx = drv_ctx;
>
> dma_to_qm_sg_one(sg_table, iv_dma, ivsize, 0);
> sg_to_qm_sg(req->src, req->cryptlen, sg_table + 1, 0); 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%7Cfa91dc99b1b147f0b13008db189c4c71%7C686ea1d3bc2b4c6fa92cd99c5c
> 301635%7C0%7C0%7C638130831812121500%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C3000%7C%7C%7C&sdata=UScavImC4%2FcLjGiqTDANWb%2FG1cOg4J18Ijfoicl
> %2FRSg%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%7Cfa91dc99b1b147f0b13008db189c4c71%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C638130831812121500%7CUnknown%7CTWFp
> bGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&sdata=ub4VeaCOKFt4iDVcxy3jnN0EXJBPJZEs%2
> B1CrnK5gimg%3D&reserved=0
