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
