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

[Horia Geanta <horia.geanta@nxp.com>]

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