Hi Horia,

I didn't understand your patch thoroughly yet, but I tested it and it
gets rid of my DMA-API warning, so:

Tested-by: Roland Hieber <r...@pengutronix.de>

Thanks! :)

 - Roland

On Sat, Jan 26, 2019 at 08:02:15PM +0200, Horia Geantă wrote:
> Roland reports the following issue and provides a root cause analysis:
> 
> "On a v4.19 i.MX6 system with IMA and CONFIG_DMA_API_DEBUG enabled, a
> warning is generated when accessing files on a filesystem for which IMA
> measurement is enabled:
> 
>     ------------[ cut here ]------------
>     WARNING: CPU: 0 PID: 1 at kernel/dma/debug.c:1181 
> check_for_stack.part.9+0xd0/0x120
>     caam_jr 2101000.jr0: DMA-API: device driver maps memory from stack 
> [addr=b668049e]
>     Modules linked in:
>     CPU: 0 PID: 1 Comm: switch_root Not tainted 4.19.0-20181214-1 #2
>     Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
>     Backtrace:
>     [<c010efb8>] (dump_backtrace) from [<c010f2d0>] (show_stack+0x20/0x24)
>     [<c010f2b0>] (show_stack) from [<c08b04f4>] (dump_stack+0xa0/0xcc)
>     [<c08b0454>] (dump_stack) from [<c012b610>] (__warn+0xf0/0x108)
>     [<c012b520>] (__warn) from [<c012b680>] (warn_slowpath_fmt+0x58/0x74)
>     [<c012b62c>] (warn_slowpath_fmt) from [<c0199acc>] 
> (check_for_stack.part.9+0xd0/0x120)
>     [<c01999fc>] (check_for_stack.part.9) from [<c019a040>] 
> (debug_dma_map_page+0x144/0x174)
>     [<c0199efc>] (debug_dma_map_page) from [<c065f7f4>] 
> (ahash_final_ctx+0x5b4/0xcf0)
>     [<c065f240>] (ahash_final_ctx) from [<c065b3c4>] (ahash_final+0x1c/0x20)
>     [<c065b3a8>] (ahash_final) from [<c03fe278>] (crypto_ahash_op+0x38/0x80)
>     [<c03fe240>] (crypto_ahash_op) from [<c03fe2e0>] 
> (crypto_ahash_final+0x20/0x24)
>     [<c03fe2c0>] (crypto_ahash_final) from [<c03f19a8>] 
> (ima_calc_file_hash+0x29c/0xa40)
>     [<c03f170c>] (ima_calc_file_hash) from [<c03f2b24>] 
> (ima_collect_measurement+0x1dc/0x240)
>     [<c03f2948>] (ima_collect_measurement) from [<c03f0a60>] 
> (process_measurement+0x4c4/0x6b8)
>     [<c03f059c>] (process_measurement) from [<c03f0cdc>] 
> (ima_file_check+0x88/0xa4)
>     [<c03f0c54>] (ima_file_check) from [<c02d8adc>] (path_openat+0x5d8/0x1364)
>     [<c02d8504>] (path_openat) from [<c02dad24>] (do_filp_open+0x84/0xf0)
>     [<c02daca0>] (do_filp_open) from [<c02cf50c>] (do_open_execat+0x84/0x1b0)
>     [<c02cf488>] (do_open_execat) from [<c02d1058>] 
> (__do_execve_file+0x43c/0x890)
>     [<c02d0c1c>] (__do_execve_file) from [<c02d1770>] (sys_execve+0x44/0x4c)
>     [<c02d172c>] (sys_execve) from [<c0101000>] (ret_fast_syscall+0x0/0x28)
>     ---[ end trace 3455789a10e3aefd ]---
> 
> The cause is that the struct ahash_request *req is created as a
> stack-local variable up in the stack (presumably somewhere in the IMA
> implementation), then passed down into the CAAM driver, which tries to
> dma_single_map the req->result (indirectly via map_seq_out_ptr_result)
> in order to make that buffer available for the CAAM to store the result
> of the following hash operation.
> 
> The calling code doesn't know how req will be used by the CAAM driver,
> and there could be other such occurrences where stack memory is passed
> down to the CAAM driver. Therefore we should rather fix this issue in
> the CAAM driver where the requirements are known."
> 
> Fix this problem by:
> -instructing the crypto engine to write the final hash in state->caam_ctx
> -subsequently memcpy-ing the final hash into req->result
> 
> Cc: <sta...@vger.kernel.org> # v4.19+
> Reported-by: Roland Hieber <r...@pengutronix.de>
> Signed-off-by: Horia Geantă <horia.gea...@nxp.com>
> ---
>  drivers/crypto/caam/caamhash.c | 85 
> +++++++++++-------------------------------
>  1 file changed, 21 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
> index b65e2e54c562..89ecda28f87b 100644
> --- a/drivers/crypto/caam/caamhash.c
> +++ b/drivers/crypto/caam/caamhash.c
> @@ -178,18 +178,6 @@ static inline int map_seq_out_ptr_ctx(u32 *desc, struct 
> device *jrdev,
>       return 0;
>  }
>  
> -/* Map req->result, and append seq_out_ptr command that points to it */
> -static inline dma_addr_t map_seq_out_ptr_result(u32 *desc, struct device 
> *jrdev,
> -                                             u8 *result, int digestsize)
> -{
> -     dma_addr_t dst_dma;
> -
> -     dst_dma = dma_map_single(jrdev, result, digestsize, DMA_FROM_DEVICE);
> -     append_seq_out_ptr(desc, dst_dma, digestsize, 0);
> -
> -     return dst_dma;
> -}
> -
>  /* Map current buffer in state (if length > 0) and put it in link table */
>  static inline int buf_map_to_sec4_sg(struct device *jrdev,
>                                    struct sec4_sg_entry *sec4_sg,
> @@ -426,7 +414,6 @@ static int ahash_setkey(struct crypto_ahash *ahash,
>  
>  /*
>   * ahash_edesc - s/w-extended ahash descriptor
> - * @dst_dma: physical mapped address of req->result
>   * @sec4_sg_dma: physical mapped address of h/w link table
>   * @src_nents: number of segments in input scatterlist
>   * @sec4_sg_bytes: length of dma mapped sec4_sg space
> @@ -434,7 +421,6 @@ static int ahash_setkey(struct crypto_ahash *ahash,
>   * @sec4_sg: h/w link table
>   */
>  struct ahash_edesc {
> -     dma_addr_t dst_dma;
>       dma_addr_t sec4_sg_dma;
>       int src_nents;
>       int sec4_sg_bytes;
> @@ -450,8 +436,6 @@ static inline void ahash_unmap(struct device *dev,
>  
>       if (edesc->src_nents)
>               dma_unmap_sg(dev, req->src, edesc->src_nents, DMA_TO_DEVICE);
> -     if (edesc->dst_dma)
> -             dma_unmap_single(dev, edesc->dst_dma, dst_len, DMA_FROM_DEVICE);
>  
>       if (edesc->sec4_sg_bytes)
>               dma_unmap_single(dev, edesc->sec4_sg_dma,
> @@ -486,9 +470,9 @@ static void ahash_done(struct device *jrdev, u32 *desc, 
> u32 err,
>       struct ahash_edesc *edesc;
>       struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
>       int digestsize = crypto_ahash_digestsize(ahash);
> +     struct caam_hash_state *state = ahash_request_ctx(req);
>  #ifdef DEBUG
>       struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
> -     struct caam_hash_state *state = ahash_request_ctx(req);
>  
>       dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
>  #endif
> @@ -497,17 +481,14 @@ static void ahash_done(struct device *jrdev, u32 *desc, 
> u32 err,
>       if (err)
>               caam_jr_strstatus(jrdev, err);
>  
> -     ahash_unmap(jrdev, edesc, req, digestsize);
> +     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
> +     memcpy(req->result, state->caam_ctx, digestsize);
>       kfree(edesc);
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "ctx@"__stringify(__LINE__)": ",
>                      DUMP_PREFIX_ADDRESS, 16, 4, state->caam_ctx,
>                      ctx->ctx_len, 1);
> -     if (req->result)
> -             print_hex_dump(KERN_ERR, "result@"__stringify(__LINE__)": ",
> -                            DUMP_PREFIX_ADDRESS, 16, 4, req->result,
> -                            digestsize, 1);
>  #endif
>  
>       req->base.complete(&req->base, err);
> @@ -555,9 +536,9 @@ static void ahash_done_ctx_src(struct device *jrdev, u32 
> *desc, u32 err,
>       struct ahash_edesc *edesc;
>       struct crypto_ahash *ahash = crypto_ahash_reqtfm(req);
>       int digestsize = crypto_ahash_digestsize(ahash);
> +     struct caam_hash_state *state = ahash_request_ctx(req);
>  #ifdef DEBUG
>       struct caam_hash_ctx *ctx = crypto_ahash_ctx(ahash);
> -     struct caam_hash_state *state = ahash_request_ctx(req);
>  
>       dev_err(jrdev, "%s %d: err 0x%x\n", __func__, __LINE__, err);
>  #endif
> @@ -566,17 +547,14 @@ static void ahash_done_ctx_src(struct device *jrdev, 
> u32 *desc, u32 err,
>       if (err)
>               caam_jr_strstatus(jrdev, err);
>  
> -     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_TO_DEVICE);
> +     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_BIDIRECTIONAL);
> +     memcpy(req->result, state->caam_ctx, digestsize);
>       kfree(edesc);
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "ctx@"__stringify(__LINE__)": ",
>                      DUMP_PREFIX_ADDRESS, 16, 4, state->caam_ctx,
>                      ctx->ctx_len, 1);
> -     if (req->result)
> -             print_hex_dump(KERN_ERR, "result@"__stringify(__LINE__)": ",
> -                            DUMP_PREFIX_ADDRESS, 16, 4, req->result,
> -                            digestsize, 1);
>  #endif
>  
>       req->base.complete(&req->base, err);
> @@ -837,7 +815,7 @@ static int ahash_final_ctx(struct ahash_request *req)
>       edesc->sec4_sg_bytes = sec4_sg_bytes;
>  
>       ret = ctx_map_to_sec4_sg(jrdev, state, ctx->ctx_len,
> -                              edesc->sec4_sg, DMA_TO_DEVICE);
> +                              edesc->sec4_sg, DMA_BIDIRECTIONAL);
>       if (ret)
>               goto unmap_ctx;
>  
> @@ -857,14 +835,7 @@ static int ahash_final_ctx(struct ahash_request *req)
>  
>       append_seq_in_ptr(desc, edesc->sec4_sg_dma, ctx->ctx_len + buflen,
>                         LDST_SGF);
> -
> -     edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
> -                                             digestsize);
> -     if (dma_mapping_error(jrdev, edesc->dst_dma)) {
> -             dev_err(jrdev, "unable to map dst\n");
> -             ret = -ENOMEM;
> -             goto unmap_ctx;
> -     }
> +     append_seq_out_ptr(desc, state->ctx_dma, digestsize, 0);
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "jobdesc@"__stringify(__LINE__)": ",
> @@ -877,7 +848,7 @@ static int ahash_final_ctx(struct ahash_request *req)
>  
>       return -EINPROGRESS;
>   unmap_ctx:
> -     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
> +     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_BIDIRECTIONAL);
>       kfree(edesc);
>       return ret;
>  }
> @@ -931,7 +902,7 @@ static int ahash_finup_ctx(struct ahash_request *req)
>       edesc->src_nents = src_nents;
>  
>       ret = ctx_map_to_sec4_sg(jrdev, state, ctx->ctx_len,
> -                              edesc->sec4_sg, DMA_TO_DEVICE);
> +                              edesc->sec4_sg, DMA_BIDIRECTIONAL);
>       if (ret)
>               goto unmap_ctx;
>  
> @@ -945,13 +916,7 @@ static int ahash_finup_ctx(struct ahash_request *req)
>       if (ret)
>               goto unmap_ctx;
>  
> -     edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
> -                                             digestsize);
> -     if (dma_mapping_error(jrdev, edesc->dst_dma)) {
> -             dev_err(jrdev, "unable to map dst\n");
> -             ret = -ENOMEM;
> -             goto unmap_ctx;
> -     }
> +     append_seq_out_ptr(desc, state->ctx_dma, digestsize, 0);
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "jobdesc@"__stringify(__LINE__)": ",
> @@ -964,7 +929,7 @@ static int ahash_finup_ctx(struct ahash_request *req)
>  
>       return -EINPROGRESS;
>   unmap_ctx:
> -     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
> +     ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_BIDIRECTIONAL);
>       kfree(edesc);
>       return ret;
>  }
> @@ -1023,10 +988,8 @@ static int ahash_digest(struct ahash_request *req)
>  
>       desc = edesc->hw_desc;
>  
> -     edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
> -                                             digestsize);
> -     if (dma_mapping_error(jrdev, edesc->dst_dma)) {
> -             dev_err(jrdev, "unable to map dst\n");
> +     ret = map_seq_out_ptr_ctx(desc, jrdev, state, digestsize);
> +     if (ret) {
>               ahash_unmap(jrdev, edesc, req, digestsize);
>               kfree(edesc);
>               return -ENOMEM;
> @@ -1041,7 +1004,7 @@ static int ahash_digest(struct ahash_request *req)
>       if (!ret) {
>               ret = -EINPROGRESS;
>       } else {
> -             ahash_unmap(jrdev, edesc, req, digestsize);
> +             ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
>               kfree(edesc);
>       }
>  
> @@ -1083,12 +1046,9 @@ static int ahash_final_no_ctx(struct ahash_request 
> *req)
>               append_seq_in_ptr(desc, state->buf_dma, buflen, 0);
>       }
>  
> -     edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
> -                                             digestsize);
> -     if (dma_mapping_error(jrdev, edesc->dst_dma)) {
> -             dev_err(jrdev, "unable to map dst\n");
> +     ret = map_seq_out_ptr_ctx(desc, jrdev, state, digestsize);
> +     if (ret)
>               goto unmap;
> -     }
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "jobdesc@"__stringify(__LINE__)": ",
> @@ -1099,7 +1059,7 @@ static int ahash_final_no_ctx(struct ahash_request *req)
>       if (!ret) {
>               ret = -EINPROGRESS;
>       } else {
> -             ahash_unmap(jrdev, edesc, req, digestsize);
> +             ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
>               kfree(edesc);
>       }
>  
> @@ -1298,12 +1258,9 @@ static int ahash_finup_no_ctx(struct ahash_request 
> *req)
>               goto unmap;
>       }
>  
> -     edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
> -                                             digestsize);
> -     if (dma_mapping_error(jrdev, edesc->dst_dma)) {
> -             dev_err(jrdev, "unable to map dst\n");
> +     ret = map_seq_out_ptr_ctx(desc, jrdev, state, digestsize);
> +     if (ret)
>               goto unmap;
> -     }
>  
>  #ifdef DEBUG
>       print_hex_dump(KERN_ERR, "jobdesc@"__stringify(__LINE__)": ",
> @@ -1314,7 +1271,7 @@ static int ahash_finup_no_ctx(struct ahash_request *req)
>       if (!ret) {
>               ret = -EINPROGRESS;
>       } else {
> -             ahash_unmap(jrdev, edesc, req, digestsize);
> +             ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_FROM_DEVICE);
>               kfree(edesc);
>       }
>  
> -- 
> 2.16.2
> 
> 

-- 
Roland Hieber                     | r.hie...@pengutronix.de     |
Pengutronix e.K.                  | https://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim | Phone: +49-5121-206917-5086 |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

Reply via email to