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

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

[Roland Hieber]

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.

The problem is solved by introducing a temporary buffer in the auxiliary
struct ahash_edesc, which is kmalloc'ed and can be DMA-mapped safely to
receive the result from hardware. Then the result is copied back into
req->result in the respective done callbacks that are called when the
CAAM has finished the request.

Other hardware crypto drivers which use DMA also solve it this way, see
for example atmel_sha_copy_ready_hash() in drivers/crypto/atmel-sha.c.

Supplementary for the above stack trace, fix the issue also in other
code paths which show the same usage pattern of req->request.

Cc: stable@vger.kernel.org
Signed-off-by: Roland Hieber <rhi@pengutronix.de>
---
 drivers/crypto/caam/caamhash.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/crypto/caam/caamhash.c b/drivers/crypto/caam/caamhash.c
index b65e2e54c5625..3f425d3bf6972 100644
--- a/drivers/crypto/caam/caamhash.c
+++ b/drivers/crypto/caam/caamhash.c
@@ -426,7 +426,8 @@ static int ahash_setkey(struct crypto_ahash *ahash,
 
 /*
  * ahash_edesc - s/w-extended ahash descriptor
- * @dst_dma: physical mapped address of req->result
+ * @result: temporary heap buffer to hold req->result
+ * @dst_dma: physical mapped address of 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,6 +435,7 @@ static int ahash_setkey(struct crypto_ahash *ahash,
  * @sec4_sg: h/w link table
  */
 struct ahash_edesc {
+	u8 result[CAAM_MAX_HASH_KEY_SIZE];
 	dma_addr_t dst_dma;
 	dma_addr_t sec4_sg_dma;
 	int src_nents;
@@ -498,6 +500,7 @@ static void ahash_done(struct device *jrdev, u32 *desc, u32 err,
 		caam_jr_strstatus(jrdev, err);
 
 	ahash_unmap(jrdev, edesc, req, digestsize);
+	memcpy(req->result, edesc->result, digestsize);
 	kfree(edesc);
 
 #ifdef DEBUG
@@ -567,6 +570,7 @@ static void ahash_done_ctx_src(struct device *jrdev, u32 *desc, u32 err,
 		caam_jr_strstatus(jrdev, err);
 
 	ahash_unmap_ctx(jrdev, edesc, req, digestsize, DMA_TO_DEVICE);
+	memcpy(req->result, edesc->result, digestsize);
 	kfree(edesc);
 
 #ifdef DEBUG
@@ -858,7 +862,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,
+	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, edesc->result,
 						digestsize);
 	if (dma_mapping_error(jrdev, edesc->dst_dma)) {
 		dev_err(jrdev, "unable to map dst\n");
@@ -945,7 +949,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,
+	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, edesc->result,
 						digestsize);
 	if (dma_mapping_error(jrdev, edesc->dst_dma)) {
 		dev_err(jrdev, "unable to map dst\n");
@@ -1023,7 +1027,7 @@ static int ahash_digest(struct ahash_request *req)
 
 	desc = edesc->hw_desc;
 
-	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
+	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, edesc->result,
 						digestsize);
 	if (dma_mapping_error(jrdev, edesc->dst_dma)) {
 		dev_err(jrdev, "unable to map dst\n");
@@ -1083,7 +1087,7 @@ 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,
+	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, edesc->result,
 						digestsize);
 	if (dma_mapping_error(jrdev, edesc->dst_dma)) {
 		dev_err(jrdev, "unable to map dst\n");
@@ -1298,7 +1302,7 @@ static int ahash_finup_no_ctx(struct ahash_request *req)
 		goto unmap;
 	}
 
-	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, req->result,
+	edesc->dst_dma = map_seq_out_ptr_result(desc, jrdev, edesc->result,
 						digestsize);
 	if (dma_mapping_error(jrdev, edesc->dst_dma)) {
 		dev_err(jrdev, "unable to map dst\n");
-- 
2.20.1

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

[Horia Geanta]

On 1/22/2019 5:27 PM, Roland Hieber wrote:
> 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.
> 
> The problem is solved by introducing a temporary buffer in the auxiliary
> struct ahash_edesc, which is kmalloc'ed and can be DMA-mapped safely to
> receive the result from hardware. Then the result is copied back into
> req->result in the respective done callbacks that are called when the
> CAAM has finished the request.
> 
Roland, thanks for the accurate analysis and the fix!

Instead of adding a new buffer, I would prefer re-using the partial hash buffer
(state->caam_ctx) for storing also the final hash.
I'll shortly send a v2 using this approach.

> Other hardware crypto drivers which use DMA also solve it this way, see
> for example atmel_sha_copy_ready_hash() in drivers/crypto/atmel-sha.c.
> 
Indeed.
Unfortunately the crypto API does not guarantee req->result is DMAable (we've
gone through this also for the IV).

I've skimmed through the crypto engine drivers (drivers/crypto/*), out of
curiosity - to see how many are affected by this.

At least two other drivers seem to incorrectly DMA map req->result: amcc and axis.

Many other drivers are affected performance-wise - since they are forced to
memcpy the data: atmel-sha.c, ccree, chelsio, img-hash.c, inside-secure,
marvell, mxs-dcp.c, qce, sahara.c, stm32, ux500.

Herbert, is there something we could do to avoid this?

Thanks,
Horia

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

[Herbert Xu]

On Fri, Jan 25, 2019 at 12:43:21PM +0000, Horia Geanta wrote:
>
> Herbert, is there something we could do to avoid this?

Async crypto requests cannot be allocated off the stack.  So
whoever is doing this needs to stop.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Horia Geanta]

On 1/25/2019 3:48 PM, Herbert Xu wrote:
> On Fri, Jan 25, 2019 at 12:43:21PM +0000, Horia Geanta wrote:
>>
>> Herbert, is there something we could do to avoid this?
> 
> Async crypto requests cannot be allocated off the stack.  So
> whoever is doing this needs to stop.
> 
IIUC, the crypto request is allocated in ima_calc_file_hash_atfm() using
ahash_request_alloc(), so this seems to be fine.

However, further below the req->result pointer is set to a location on stack:
out2:
	if (!rc) {
		ahash_request_set_crypt(req, NULL, hash->digest, 0);

More exactly, the call sequence is:
ima_collect_measurement() -> ima_calc_file_hash() -> ima_calc_file_ahash() ->
ima_calc_file_hash_atfm()
and "hash" is allocated on stack in ima_collect_measurement().

Thus, it seems the problem lies in the fact that ahash_request structure defines
the digest buffer as "u8 *result" - thus the memory area might not be DMAable.

Thanks,
Horia

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

[Herbert Xu]

On Fri, Jan 25, 2019 at 02:26:27PM +0000, Horia Geanta wrote:
>
> Thus, it seems the problem lies in the fact that ahash_request structure defines
> the digest buffer as "u8 *result" - thus the memory area might not be DMAable.

Oh OK, I misunderstood.  Anything that's in the form of a kernel
pointer must be assumed to be not DMAable.  So yes this needs to
be copied.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Horia Geantă]

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

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

[Roland Hieber]

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 |

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

[Horia Geanta]

On 1/28/2019 9:00 PM, Roland Hieber wrote:
> Hi Horia,
> 
> I didn't understand your patch thoroughly yet, but I tested it and it
> gets rid of my DMA-API warning, so:
> 
As mentioned in the commit message, the main idea is to use the buffer holding
the partial hash (state->caam_ctx) also for the final hash.
So now:
-crypto engine DMA writes final hash to state->caam_ctx
-driver copies hash from state->caam_ctx to req->result

There are some implications of this change, mainly the change of DMA mapping
direction for state->caam_ctx, clean-up code (DMA unmapping) and updates in how
crypto engine job descriptors are generated.

> Tested-by: Roland Hieber <rhi@pengutronix.de>
> 
Thanks!Horia

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

[Herbert Xu]

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(-)

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

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

[Herbert Xu]

On Tue, Jan 22, 2019 at 04:26:51PM +0100, Roland Hieber wrote:
>
> 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.

Then IMA needs to be fixed.  Asynchronous crypto requests must be
allocated with kmalloc so this is an API violation.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt