Re: [PATCH v2 2/2] crypto: caam - fix DMA mapping of stack memory

[Roland Hieber <rhi@pengutronix.de>]

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 <rhi@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: <stable@vger.kernel.org> # v4.19+
> Reported-by: Roland Hieber <rhi@pengutronix.de>
> Signed-off-by: Horia Geantă <horia.geanta@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.hieber@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 |