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 |
