From:   Horia Geanta <horia.geanta@nxp.com>
To:     Roland Hieber <rhi@pengutronix.de>,
        Aymen Sghaier <aymen.sghaier@nxp.com>,
        "linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        Jesper Nilsson <jesper.nilsson@axis.com>,
        Lars Persson <lars.persson@axis.com>,
        Christian Lamparter <chunkeey@gmail.com>
CC:     Roland Hieber <r.hieber@pengutronix.de>,
        "David S. Miller" <davem@davemloft.net>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: Re: [PATCH 2/2] crypto: caam - fix DMA mapping of stack memory
Thread-Topic: [PATCH 2/2] crypto: caam - fix DMA mapping of stack memory
Thread-Index: AQHUsmbwIdQhV/qgPk+dqH8znmHDkA==
Date:   Fri, 25 Jan 2019 12:43:21 +0000
Message-ID: <VI1PR0402MB348525A6FEFD55F6896C9249989B0@VI1PR0402MB3485.eurprd04.prod.outlook.com>
References: <20190122152651.1150-1-rhi@pengutronix.de>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f0ac7306-f9f6-4746-077c-08d682c2b041
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Jan 2019 12:43:21.0571
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR0402MB3438
Sender: linux-crypto-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

On 1/22/2019 5:27 PM, Roland Hieber wrote:=0A=
> On a v4.19 i.MX6 system with IMA and CONFIG_DMA_API_DEBUG enabled, a=0A=
> warning is generated when accessing files on a filesystem for which IMA=
=0A=
> measurement is enabled:=0A=
> =0A=
>     ------------[ cut here ]------------=0A=
>     WARNING: CPU: 0 PID: 1 at kernel/dma/debug.c:1181 check_for_stack.par=
t.9+0xd0/0x120=0A=
>     caam_jr 2101000.jr0: DMA-API: device driver maps memory from stack [a=
ddr=3Db668049e]=0A=
>     Modules linked in:=0A=
>     CPU: 0 PID: 1 Comm: switch_root Not tainted 4.19.0-20181214-1 #2=0A=
>     Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)=0A=
>     Backtrace:=0A=
>     [<c010efb8>] (dump_backtrace) from [<c010f2d0>] (show_stack+0x20/0x24=
)=0A=
>     [<c010f2b0>] (show_stack) from [<c08b04f4>] (dump_stack+0xa0/0xcc)=0A=
>     [<c08b0454>] (dump_stack) from [<c012b610>] (__warn+0xf0/0x108)=0A=
>     [<c012b520>] (__warn) from [<c012b680>] (warn_slowpath_fmt+0x58/0x74)=
=0A=
>     [<c012b62c>] (warn_slowpath_fmt) from [<c0199acc>] (check_for_stack.p=
art.9+0xd0/0x120)=0A=
>     [<c01999fc>] (check_for_stack.part.9) from [<c019a040>] (debug_dma_ma=
p_page+0x144/0x174)=0A=
>     [<c0199efc>] (debug_dma_map_page) from [<c065f7f4>] (ahash_final_ctx+=
0x5b4/0xcf0)=0A=
>     [<c065f240>] (ahash_final_ctx) from [<c065b3c4>] (ahash_final+0x1c/0x=
20)=0A=
>     [<c065b3a8>] (ahash_final) from [<c03fe278>] (crypto_ahash_op+0x38/0x=
80)=0A=
>     [<c03fe240>] (crypto_ahash_op) from [<c03fe2e0>] (crypto_ahash_final+=
0x20/0x24)=0A=
>     [<c03fe2c0>] (crypto_ahash_final) from [<c03f19a8>] (ima_calc_file_ha=
sh+0x29c/0xa40)=0A=
>     [<c03f170c>] (ima_calc_file_hash) from [<c03f2b24>] (ima_collect_meas=
urement+0x1dc/0x240)=0A=
>     [<c03f2948>] (ima_collect_measurement) from [<c03f0a60>] (process_mea=
surement+0x4c4/0x6b8)=0A=
>     [<c03f059c>] (process_measurement) from [<c03f0cdc>] (ima_file_check+=
0x88/0xa4)=0A=
>     [<c03f0c54>] (ima_file_check) from [<c02d8adc>] (path_openat+0x5d8/0x=
1364)=0A=
>     [<c02d8504>] (path_openat) from [<c02dad24>] (do_filp_open+0x84/0xf0)=
=0A=
>     [<c02daca0>] (do_filp_open) from [<c02cf50c>] (do_open_execat+0x84/0x=
1b0)=0A=
>     [<c02cf488>] (do_open_execat) from [<c02d1058>] (__do_execve_file+0x4=
3c/0x890)=0A=
>     [<c02d0c1c>] (__do_execve_file) from [<c02d1770>] (sys_execve+0x44/0x=
4c)=0A=
>     [<c02d172c>] (sys_execve) from [<c0101000>] (ret_fast_syscall+0x0/0x2=
8)=0A=
>     ---[ end trace 3455789a10e3aefd ]---=0A=
> =0A=
> The cause is that the struct ahash_request *req is created as a=0A=
> stack-local variable up in the stack (presumably somewhere in the IMA=0A=
> implementation), then passed down into the CAAM driver, which tries to=0A=
> dma_single_map the req->result (indirectly via map_seq_out_ptr_result)=0A=
> in order to make that buffer available for the CAAM to store the result=
=0A=
> of the following hash operation.=0A=
> =0A=
> The calling code doesn't know how req will be used by the CAAM driver,=0A=
> and there could be other such occurrences where stack memory is passed=0A=
> down to the CAAM driver. Therefore we should rather fix this issue in=0A=
> the CAAM driver where the requirements are known.=0A=
> =0A=
> The problem is solved by introducing a temporary buffer in the auxiliary=
=0A=
> struct ahash_edesc, which is kmalloc'ed and can be DMA-mapped safely to=
=0A=
> receive the result from hardware. Then the result is copied back into=0A=
> req->result in the respective done callbacks that are called when the=0A=
> CAAM has finished the request.=0A=
> =0A=
Roland, thanks for the accurate analysis and the fix!=0A=
=0A=
Instead of adding a new buffer, I would prefer re-using the partial hash bu=
ffer=0A=
(state->caam_ctx) for storing also the final hash.=0A=
I'll shortly send a v2 using this approach.=0A=
=0A=
> Other hardware crypto drivers which use DMA also solve it this way, see=
=0A=
> for example atmel_sha_copy_ready_hash() in drivers/crypto/atmel-sha.c.=0A=
> =0A=
Indeed.=0A=
Unfortunately the crypto API does not guarantee req->result is DMAable (we'=
ve=0A=
gone through this also for the IV).=0A=
=0A=
I've skimmed through the crypto engine drivers (drivers/crypto/*), out of=
=0A=
curiosity - to see how many are affected by this.=0A=
=0A=
At least two other drivers seem to incorrectly DMA map req->result: amcc an=
d axis.=0A=
=0A=
Many other drivers are affected performance-wise - since they are forced to=
=0A=
memcpy the data: atmel-sha.c, ccree, chelsio, img-hash.c, inside-secure,=0A=
marvell, mxs-dcp.c, qce, sahara.c, stm32, ux500.=0A=
=0A=
Herbert, is there something we could do to avoid this?=0A=
=0A=
Thanks,=0A=
Horia=0A=