| Message ID | 1509610224-14708-2-git-send-email-gilad@benyossef.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
On 11/2/2017 10:14 AM, Gilad Ben-Yossef wrote: > We are being passed an IV buffer from unknown origin, which may be > stack allocated and thus not safe for DMA. Allocate a DMA safe > buffer for the IV and use that instead. > IIUC this fixes only the (a)blkcipher / skcipher algorithms. What about aead, authenc? The fact that only the skcipher tcrypt tests use IVs on stack doesn't mean aead, authenc implementations are safe - other crypto API users could provide IVs laying in non-DMAable memory. To reiterate, the proper approach is to fix the crypto API to guarantee IVs are DMAable. However Herbert suggests he is not willing to do this work: https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28821.html A few high-level details mentioning what this implies would be helpful, in case somebody else decides its worth pursuing this path. The compromise is to fix all crypto drivers that need DMAable IVs. IMHO this is suboptimal, both in terms of performance (memory allocation, memcpy) and increased code complexity. Horia
Hi, On Wed, Nov 8, 2017 at 12:26 PM, Horia Geantă <horia.geanta@nxp.com> wrote: > On 11/2/2017 10:14 AM, Gilad Ben-Yossef wrote: >> We are being passed an IV buffer from unknown origin, which may be >> stack allocated and thus not safe for DMA. Allocate a DMA safe >> buffer for the IV and use that instead. >> > IIUC this fixes only the (a)blkcipher / skcipher algorithms. > What about aead, authenc? AFAIK the implementation of aead/authenc in ccree already copies the IV to a internally allocated buffer because how it deals with GCM and CTR. But you did trigger me to double check that, so thanks for that :-) > > The fact that only the skcipher tcrypt tests use IVs on stack doesn't > mean aead, authenc implementations are safe - other crypto API users > could provide IVs laying in non-DMAable memory. Of course. In fact, it might be a good idea to actually make tcrypt explicitly use a stack allocated IV just to trigger proper warnings from the DMA debug code. Does that sounds sane? > > To reiterate, the proper approach is to fix the crypto API to guarantee > IVs are DMAable. > However Herbert suggests he is not willing to do this work: > https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28821.html > > A few high-level details mentioning what this implies would be helpful, > in case somebody else decides its worth pursuing this path. > > The compromise is to fix all crypto drivers that need DMAable IVs. > IMHO this is suboptimal, both in terms of performance (memory > allocation, memcpy) and increased code complexity. As a HW based crypto driver maintainer I sympathize, but let's play devil's advocate for a second here: In the current state, HW based crypto drivers need to allocate a buffer and copy the IV, because they don't know if they got a DMAable buffer or not. SW based crypto drivers don't need to do anything. If we switch to an API that guarantee IVs are DMAable all crypto API *users* will be forced to make sure IV are located in DMAable memory, possibly resulting in the need for extra buffer allocation, whether this is needed or not and SW based crypto drivers suffer added complexicty of extracting the IV from scatter/gather list (I'm assuming we'll just add it there). Despite this hurting the driver I care about, I'm not sure this is a good trade off, really. Thinking aloud, would it be sane to add an API of "alloc and copy if buffer is on stack"? Gilad
On Wed, Nov 08, 2017 at 01:54:03PM +0200, Gilad Ben-Yossef wrote: > > As a HW based crypto driver maintainer I sympathize, but let's play > devil's advocate for a second here: > > In the current state, HW based crypto drivers need to allocate a buffer > and copy the IV, because they don't know if they got a DMAable buffer > or not. SW based crypto drivers don't need to do anything. When I suggested an API change, I was thinking of an option of supplying an SG list instead of the current kernel pointer. IOW the existing users do not have to change but where we know that the pointer can be DMAed you can opt in to the new interface. The crypto API can then provide a helper to do the copying where necessary. Cheers,
diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 78706f5..0b69103 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -695,6 +695,7 @@ static int ssi_blkcipher_complete(struct device *dev, struct ablkcipher_request *req = (struct ablkcipher_request *)areq; ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst); + kfree(req_ctx->iv); /*Decrease the inflight counter*/ if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0) @@ -757,6 +758,17 @@ static int ssi_blkcipher_process( rc = 0; goto exit_process; } + + /* The IV we are handed may be allocted from the stack so + * we must copy it to a DMAable buffer before use. + */ + req_ctx->iv = kmalloc(ivsize, GFP_KERNEL); + if (!req_ctx->iv) { + rc = -ENOMEM; + goto exit_process; + } + memcpy(req_ctx->iv, info, ivsize); + /*For CTS in case of data size aligned to 16 use CBC mode*/ if (((nbytes % AES_BLOCK_SIZE) == 0) && (ctx_p->cipher_mode == DRV_CIPHER_CBC_CTS)) { ctx_p->cipher_mode = DRV_CIPHER_CBC; @@ -778,7 +790,9 @@ static int ssi_blkcipher_process( /* STAT_PHASE_1: Map buffers */ - rc = ssi_buffer_mgr_map_blkcipher_request(ctx_p->drvdata, req_ctx, ivsize, nbytes, info, src, dst); + rc = ssi_buffer_mgr_map_blkcipher_request(ctx_p->drvdata, req_ctx, + ivsize, nbytes, req_ctx->iv, + src, dst); if (unlikely(rc != 0)) { dev_err(dev, "map_request() failed\n"); goto exit_process; @@ -830,8 +844,10 @@ static int ssi_blkcipher_process( if (cts_restore_flag != 0) ctx_p->cipher_mode = DRV_CIPHER_CBC_CTS; - if (rc != -EINPROGRESS) + if (rc != -EINPROGRESS) { kfree(req_ctx->backup_info); + kfree(req_ctx->iv); + } return rc; } diff --git a/drivers/staging/ccree/ssi_cipher.h b/drivers/staging/ccree/ssi_cipher.h index f499962..25e6335 100644 --- a/drivers/staging/ccree/ssi_cipher.h +++ b/drivers/staging/ccree/ssi_cipher.h @@ -43,6 +43,7 @@ struct blkcipher_req_ctx { u32 out_nents; u32 out_mlli_nents; u8 *backup_info; /*store iv for generated IV flow*/ + u8 *iv; bool is_giv; struct mlli_params mlli_params; };
We are being passed an IV buffer from unknown origin, which may be stack allocated and thus not safe for DMA. Allocate a DMA safe buffer for the IV and use that instead. Signed-off-by: Gilad Ben-Yossef <gilad@benyossef.com> --- drivers/staging/ccree/ssi_cipher.c | 20 ++++++++++++++++++-- drivers/staging/ccree/ssi_cipher.h | 1 + 2 files changed, 19 insertions(+), 2 deletions(-)