* [PATCH v2 0/4] crypto: inside-secure - set of fixes
@ 2017-11-28 15:42 Antoine Tenart
2017-11-28 15:42 ` [PATCH v2 1/4] crypto: inside-secure - per request invalidation Antoine Tenart
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Antoine Tenart @ 2017-11-28 15:42 UTC (permalink / raw)
To: herbert, davem
Cc: Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal,
oferh, igall, nadavh, linux-crypto
Hi Herbert,
This series is a set of 4 fixes on the Inside Secure SafeXcel crypto
engine driver. The series will be followed by another non-fix one.
This is based on v4.15-rc1.
Thanks,
Antoine
Since v1:
- Removed the crash.txt file which was part of patch 1/4.
Antoine Tenart (3):
crypto: inside-secure - free requests even if their handling failed
crypto: inside-secure - only update the result buffer when provided
crypto: inside-secure - fix request allocations in invalidation path
Ofer Heifetz (1):
crypto: inside-secure - per request invalidation
drivers/crypto/inside-secure/safexcel.c | 1 +
drivers/crypto/inside-secure/safexcel_cipher.c | 83 ++++++++++++++++++------
drivers/crypto/inside-secure/safexcel_hash.c | 87 +++++++++++++++++++-------
3 files changed, 131 insertions(+), 40 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH v2 1/4] crypto: inside-secure - per request invalidation 2017-11-28 15:42 [PATCH v2 0/4] crypto: inside-secure - set of fixes Antoine Tenart @ 2017-11-28 15:42 ` Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 2/4] crypto: inside-secure - free requests even if their handling failed Antoine Tenart ` (2 subsequent siblings) 3 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-11-28 15:42 UTC (permalink / raw) To: herbert, davem Cc: Ofer Heifetz, thomas.petazzoni, gregory.clement, miquel.raynal, igall, nadavh, linux-crypto, Antoine Tenart From: Ofer Heifetz <oferh@marvell.com> When an invalidation request is needed we currently override the context .send and .handle_result helpers. This is wrong as under high load other requests can already be queued and overriding the context helpers will make them execute the wrong .send and .handle_result functions. This commit fixes this by adding a needs_inv flag in the request to choose the action to perform when sending requests or handling their results. This flag will be set when needed (i.e. when the context flag will be set). Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Signed-off-by: Ofer Heifetz <oferh@marvell.com> [Antoine: commit message, and removed non related changes from the original commit] Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel_cipher.c | 71 +++++++++++++++++++++----- drivers/crypto/inside-secure/safexcel_hash.c | 68 +++++++++++++++++++----- 2 files changed, 112 insertions(+), 27 deletions(-) diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c index 5438552bc6d7..9ea24868d860 100644 --- a/drivers/crypto/inside-secure/safexcel_cipher.c +++ b/drivers/crypto/inside-secure/safexcel_cipher.c @@ -14,6 +14,7 @@ #include <crypto/aes.h> #include <crypto/skcipher.h> +#include <crypto/internal/skcipher.h> #include "safexcel.h" @@ -33,6 +34,10 @@ struct safexcel_cipher_ctx { unsigned int key_len; }; +struct safexcel_cipher_req { + bool needs_inv; +}; + static void safexcel_cipher_token(struct safexcel_cipher_ctx *ctx, struct crypto_async_request *async, struct safexcel_command_desc *cdesc, @@ -126,9 +131,9 @@ static int safexcel_context_control(struct safexcel_cipher_ctx *ctx, return 0; } -static int safexcel_handle_result(struct safexcel_crypto_priv *priv, int ring, - struct crypto_async_request *async, - bool *should_complete, int *ret) +static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int ring, + struct crypto_async_request *async, + bool *should_complete, int *ret) { struct skcipher_request *req = skcipher_request_cast(async); struct safexcel_result_desc *rdesc; @@ -265,7 +270,6 @@ static int safexcel_aes_send(struct crypto_async_request *async, spin_unlock_bh(&priv->ring[ring].egress_lock); request->req = &req->base; - ctx->base.handle_result = safexcel_handle_result; *commands = n_cdesc; *results = n_rdesc; @@ -341,8 +345,6 @@ static int safexcel_handle_inv_result(struct safexcel_crypto_priv *priv, ring = safexcel_select_ring(priv); ctx->base.ring = ring; - ctx->base.needs_inv = false; - ctx->base.send = safexcel_aes_send; spin_lock_bh(&priv->ring[ring].queue_lock); enq_ret = crypto_enqueue_request(&priv->ring[ring].queue, async); @@ -359,6 +361,26 @@ static int safexcel_handle_inv_result(struct safexcel_crypto_priv *priv, return ndesc; } +static int safexcel_handle_result(struct safexcel_crypto_priv *priv, int ring, + struct crypto_async_request *async, + bool *should_complete, int *ret) +{ + struct skcipher_request *req = skcipher_request_cast(async); + struct safexcel_cipher_req *sreq = skcipher_request_ctx(req); + int err; + + if (sreq->needs_inv) { + sreq->needs_inv = false; + err = safexcel_handle_inv_result(priv, ring, async, + should_complete, ret); + } else { + err = safexcel_handle_req_result(priv, ring, async, + should_complete, ret); + } + + return err; +} + static int safexcel_cipher_send_inv(struct crypto_async_request *async, int ring, struct safexcel_request *request, int *commands, int *results) @@ -368,8 +390,6 @@ static int safexcel_cipher_send_inv(struct crypto_async_request *async, struct safexcel_crypto_priv *priv = ctx->priv; int ret; - ctx->base.handle_result = safexcel_handle_inv_result; - ret = safexcel_invalidate_cache(async, &ctx->base, priv, ctx->base.ctxr_dma, ring, request); if (unlikely(ret)) @@ -381,11 +401,29 @@ static int safexcel_cipher_send_inv(struct crypto_async_request *async, return 0; } +static int safexcel_send(struct crypto_async_request *async, + int ring, struct safexcel_request *request, + int *commands, int *results) +{ + struct skcipher_request *req = skcipher_request_cast(async); + struct safexcel_cipher_req *sreq = skcipher_request_ctx(req); + int ret; + + if (sreq->needs_inv) + ret = safexcel_cipher_send_inv(async, ring, request, + commands, results); + else + ret = safexcel_aes_send(async, ring, request, + commands, results); + return ret; +} + static int safexcel_cipher_exit_inv(struct crypto_tfm *tfm) { struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm); struct safexcel_crypto_priv *priv = ctx->priv; struct skcipher_request req; + struct safexcel_cipher_req *sreq = skcipher_request_ctx(&req); struct safexcel_inv_result result = {}; int ring = ctx->base.ring; @@ -399,7 +437,7 @@ static int safexcel_cipher_exit_inv(struct crypto_tfm *tfm) skcipher_request_set_tfm(&req, __crypto_skcipher_cast(tfm)); ctx = crypto_tfm_ctx(req.base.tfm); ctx->base.exit_inv = true; - ctx->base.send = safexcel_cipher_send_inv; + sreq->needs_inv = true; spin_lock_bh(&priv->ring[ring].queue_lock); crypto_enqueue_request(&priv->ring[ring].queue, &req.base); @@ -424,19 +462,21 @@ static int safexcel_aes(struct skcipher_request *req, enum safexcel_cipher_direction dir, u32 mode) { struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(req->base.tfm); + struct safexcel_cipher_req *sreq = skcipher_request_ctx(req); struct safexcel_crypto_priv *priv = ctx->priv; int ret, ring; + sreq->needs_inv = false; ctx->direction = dir; ctx->mode = mode; if (ctx->base.ctxr) { - if (ctx->base.needs_inv) - ctx->base.send = safexcel_cipher_send_inv; + if (ctx->base.needs_inv) { + sreq->needs_inv = true; + ctx->base.needs_inv = false; + } } else { ctx->base.ring = safexcel_select_ring(priv); - ctx->base.send = safexcel_aes_send; - ctx->base.ctxr = dma_pool_zalloc(priv->context_pool, EIP197_GFP_FLAGS(req->base), &ctx->base.ctxr_dma); @@ -476,6 +516,11 @@ static int safexcel_skcipher_cra_init(struct crypto_tfm *tfm) alg.skcipher.base); ctx->priv = tmpl->priv; + ctx->base.send = safexcel_send; + ctx->base.handle_result = safexcel_handle_result; + + crypto_skcipher_set_reqsize(__crypto_skcipher_cast(tfm), + sizeof(struct safexcel_cipher_req)); return 0; } diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 74feb6227101..6135c9f5742c 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -32,6 +32,7 @@ struct safexcel_ahash_req { bool last_req; bool finish; bool hmac; + bool needs_inv; u8 state_sz; /* expected sate size, only set once */ u32 state[SHA256_DIGEST_SIZE / sizeof(u32)]; @@ -119,9 +120,9 @@ static void safexcel_context_control(struct safexcel_ahash_ctx *ctx, } } -static int safexcel_handle_result(struct safexcel_crypto_priv *priv, int ring, - struct crypto_async_request *async, - bool *should_complete, int *ret) +static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int ring, + struct crypto_async_request *async, + bool *should_complete, int *ret) { struct safexcel_result_desc *rdesc; struct ahash_request *areq = ahash_request_cast(async); @@ -165,9 +166,9 @@ static int safexcel_handle_result(struct safexcel_crypto_priv *priv, int ring, return 1; } -static int safexcel_ahash_send(struct crypto_async_request *async, int ring, - struct safexcel_request *request, int *commands, - int *results) +static int safexcel_ahash_send_req(struct crypto_async_request *async, int ring, + struct safexcel_request *request, int *commands, + int *results) { struct ahash_request *areq = ahash_request_cast(async); struct crypto_ahash *ahash = crypto_ahash_reqtfm(areq); @@ -292,7 +293,6 @@ static int safexcel_ahash_send(struct crypto_async_request *async, int ring, req->processed += len; request->req = &areq->base; - ctx->base.handle_result = safexcel_handle_result; *commands = n_cdesc; *results = 1; @@ -374,8 +374,6 @@ static int safexcel_handle_inv_result(struct safexcel_crypto_priv *priv, ring = safexcel_select_ring(priv); ctx->base.ring = ring; - ctx->base.needs_inv = false; - ctx->base.send = safexcel_ahash_send; spin_lock_bh(&priv->ring[ring].queue_lock); enq_ret = crypto_enqueue_request(&priv->ring[ring].queue, async); @@ -392,6 +390,26 @@ static int safexcel_handle_inv_result(struct safexcel_crypto_priv *priv, return 1; } +static int safexcel_handle_result(struct safexcel_crypto_priv *priv, int ring, + struct crypto_async_request *async, + bool *should_complete, int *ret) +{ + struct ahash_request *areq = ahash_request_cast(async); + struct safexcel_ahash_req *req = ahash_request_ctx(areq); + int err; + + if (req->needs_inv) { + req->needs_inv = false; + err = safexcel_handle_inv_result(priv, ring, async, + should_complete, ret); + } else { + err = safexcel_handle_req_result(priv, ring, async, + should_complete, ret); + } + + return err; +} + static int safexcel_ahash_send_inv(struct crypto_async_request *async, int ring, struct safexcel_request *request, int *commands, int *results) @@ -400,7 +418,6 @@ static int safexcel_ahash_send_inv(struct crypto_async_request *async, struct safexcel_ahash_ctx *ctx = crypto_ahash_ctx(crypto_ahash_reqtfm(areq)); int ret; - ctx->base.handle_result = safexcel_handle_inv_result; ret = safexcel_invalidate_cache(async, &ctx->base, ctx->priv, ctx->base.ctxr_dma, ring, request); if (unlikely(ret)) @@ -412,11 +429,30 @@ static int safexcel_ahash_send_inv(struct crypto_async_request *async, return 0; } +static int safexcel_ahash_send(struct crypto_async_request *async, + int ring, struct safexcel_request *request, + int *commands, int *results) +{ + + struct ahash_request *areq = ahash_request_cast(async); + struct safexcel_ahash_req *req = ahash_request_ctx(areq); + int ret; + + if (req->needs_inv) + ret = safexcel_ahash_send_inv(async, ring, request, + commands, results); + else + ret = safexcel_ahash_send_req(async, ring, request, + commands, results); + return ret; +} + static int safexcel_ahash_exit_inv(struct crypto_tfm *tfm) { struct safexcel_ahash_ctx *ctx = crypto_tfm_ctx(tfm); struct safexcel_crypto_priv *priv = ctx->priv; struct ahash_request req; + struct safexcel_ahash_req *rctx = ahash_request_ctx(&req); struct safexcel_inv_result result = {}; int ring = ctx->base.ring; @@ -430,7 +466,7 @@ static int safexcel_ahash_exit_inv(struct crypto_tfm *tfm) ahash_request_set_tfm(&req, __crypto_ahash_cast(tfm)); ctx = crypto_tfm_ctx(req.base.tfm); ctx->base.exit_inv = true; - ctx->base.send = safexcel_ahash_send_inv; + rctx->needs_inv = true; spin_lock_bh(&priv->ring[ring].queue_lock); crypto_enqueue_request(&priv->ring[ring].queue, &req.base); @@ -481,14 +517,16 @@ static int safexcel_ahash_enqueue(struct ahash_request *areq) struct safexcel_crypto_priv *priv = ctx->priv; int ret, ring; - ctx->base.send = safexcel_ahash_send; + req->needs_inv = false; if (req->processed && ctx->digest == CONTEXT_CONTROL_DIGEST_PRECOMPUTED) ctx->base.needs_inv = safexcel_ahash_needs_inv_get(areq); if (ctx->base.ctxr) { - if (ctx->base.needs_inv) - ctx->base.send = safexcel_ahash_send_inv; + if (ctx->base.needs_inv) { + ctx->base.needs_inv = false; + req->needs_inv = true; + } } else { ctx->base.ring = safexcel_select_ring(priv); ctx->base.ctxr = dma_pool_zalloc(priv->context_pool, @@ -622,6 +660,8 @@ static int safexcel_ahash_cra_init(struct crypto_tfm *tfm) struct safexcel_alg_template, alg.ahash); ctx->priv = tmpl->priv; + ctx->base.send = safexcel_ahash_send; + ctx->base.handle_result = safexcel_handle_result; crypto_ahash_set_reqsize(__crypto_ahash_cast(tfm), sizeof(struct safexcel_ahash_req)); -- 2.14.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/4] crypto: inside-secure - free requests even if their handling failed 2017-11-28 15:42 [PATCH v2 0/4] crypto: inside-secure - set of fixes Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 1/4] crypto: inside-secure - per request invalidation Antoine Tenart @ 2017-11-28 15:42 ` Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 4/4] crypto: inside-secure - fix request allocations in invalidation path Antoine Tenart 3 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-11-28 15:42 UTC (permalink / raw) To: herbert, davem Cc: Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto This patch frees the request private data even if its handling failed, as it would never be freed otherwise. Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Suggested-by: Ofer Heifetz <oferh@marvell.com> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/crypto/inside-secure/safexcel.c b/drivers/crypto/inside-secure/safexcel.c index 89ba9e85c0f3..4bcef78a08aa 100644 --- a/drivers/crypto/inside-secure/safexcel.c +++ b/drivers/crypto/inside-secure/safexcel.c @@ -607,6 +607,7 @@ static inline void safexcel_handle_result_descriptor(struct safexcel_crypto_priv ndesc = ctx->handle_result(priv, ring, sreq->req, &should_complete, &ret); if (ndesc < 0) { + kfree(sreq); dev_err(priv->dev, "failed to handle result (%d)", ndesc); return; } -- 2.14.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-28 15:42 [PATCH v2 0/4] crypto: inside-secure - set of fixes Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 1/4] crypto: inside-secure - per request invalidation Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 2/4] crypto: inside-secure - free requests even if their handling failed Antoine Tenart @ 2017-11-28 15:42 ` Antoine Tenart 2017-11-30 9:19 ` Kamil Konieczny 2017-11-28 15:42 ` [PATCH v2 4/4] crypto: inside-secure - fix request allocations in invalidation path Antoine Tenart 3 siblings, 1 reply; 21+ messages in thread From: Antoine Tenart @ 2017-11-28 15:42 UTC (permalink / raw) To: herbert, davem Cc: Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto The patch fixes the ahash support by only updating the result buffer when provided. Otherwise the driver could crash with NULL pointer exceptions, because the ahash caller isn't required to supply a result buffer on all calls. Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 6135c9f5742c..424f4c5d4d25 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin if (sreq->finish) result_sz = crypto_ahash_digestsize(ahash); - memcpy(sreq->state, areq->result, result_sz); + + /* The called isn't required to supply a result buffer. Updated it only + * when provided. + */ + if (areq->result) + memcpy(sreq->state, areq->result, result_sz); dma_unmap_sg(priv->dev, areq->src, sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE); -- 2.14.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-28 15:42 ` [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided Antoine Tenart @ 2017-11-30 9:19 ` Kamil Konieczny 2017-11-30 9:29 ` Antoine Tenart 2017-12-01 0:31 ` Herbert Xu 0 siblings, 2 replies; 21+ messages in thread From: Kamil Konieczny @ 2017-11-30 9:19 UTC (permalink / raw) To: Antoine Tenart, herbert, davem Cc: thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Antoine, On 28.11.2017 16:42, Antoine Tenart wrote: > The patch fixes the ahash support by only updating the result buffer > when provided. Otherwise the driver could crash with NULL pointer > exceptions, because the ahash caller isn't required to supply a result > buffer on all calls. Can you point to bug crush report ? > Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > --- > drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 6135c9f5742c..424f4c5d4d25 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin > > if (sreq->finish) > result_sz = crypto_ahash_digestsize(ahash); > - memcpy(sreq->state, areq->result, result_sz); > + > + /* The called isn't required to supply a result buffer. Updated it only > + * when provided. > + */ > + if (areq->result) > + memcpy(sreq->state, areq->result, result_sz); > > dma_unmap_sg(priv->dev, areq->src, > sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE); > can the driver get request for final/finup/digest with null req->result ? If yes (?), such checks can be done before any hardware processing, saving time, for example: int hash_final(struct ahash_request *areq) { if (!areq->result) return -EINVAL; /* normal processing here */ } -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 9:19 ` Kamil Konieczny @ 2017-11-30 9:29 ` Antoine Tenart 2017-11-30 11:52 ` Kamil Konieczny 2017-12-01 0:31 ` Herbert Xu 1 sibling, 1 reply; 21+ messages in thread From: Antoine Tenart @ 2017-11-30 9:29 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, herbert, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Kamil, On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > On 28.11.2017 16:42, Antoine Tenart wrote: > > The patch fixes the ahash support by only updating the result buffer > > when provided. Otherwise the driver could crash with NULL pointer > > exceptions, because the ahash caller isn't required to supply a result > > buffer on all calls. > > Can you point to bug crush report ? Do you want the crash dump? (It'll only be a "normal" NULL pointer dereference). > > Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") > > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> > > --- > > drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > > index 6135c9f5742c..424f4c5d4d25 100644 > > --- a/drivers/crypto/inside-secure/safexcel_hash.c > > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > > @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin > > > > if (sreq->finish) > > result_sz = crypto_ahash_digestsize(ahash); > > - memcpy(sreq->state, areq->result, result_sz); > > + > > + /* The called isn't required to supply a result buffer. Updated it only > > + * when provided. > > + */ > > + if (areq->result) > > + memcpy(sreq->state, areq->result, result_sz); > > > > dma_unmap_sg(priv->dev, areq->src, > > sg_nents_for_len(areq->src, areq->nbytes), DMA_TO_DEVICE); > > > > can the driver get request for final/finup/digest with null req->result ? I don't think that can happen. But having an update called without req->result provided is a valid call (though it's not well documented). Thanks, Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 9:29 ` Antoine Tenart @ 2017-11-30 11:52 ` Kamil Konieczny 2017-11-30 12:41 ` Antoine Tenart 0 siblings, 1 reply; 21+ messages in thread From: Kamil Konieczny @ 2017-11-30 11:52 UTC (permalink / raw) To: Antoine Tenart Cc: herbert, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Antoine, On 30.11.2017 10:29, Antoine Tenart wrote: > Hi Kamil, > > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >> On 28.11.2017 16:42, Antoine Tenart wrote: >>> The patch fixes the ahash support by only updating the result buffer >>> when provided. Otherwise the driver could crash with NULL pointer >>> exceptions, because the ahash caller isn't required to supply a result >>> buffer on all calls. >> >> Can you point to bug crush report ? > > Do you want the crash dump? (It'll only be a "normal" NULL pointer > dereference). Ah I see, in this case not. >>> Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") >>> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> >>> --- >>> drivers/crypto/inside-secure/safexcel_hash.c | 7 ++++++- >>> 1 file changed, 6 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c >>> index 6135c9f5742c..424f4c5d4d25 100644 >>> --- a/drivers/crypto/inside-secure/safexcel_hash.c >>> +++ b/drivers/crypto/inside-secure/safexcel_hash.c >>> @@ -150,7 +150,12 @@ static int safexcel_handle_req_result(struct safexcel_crypto_priv *priv, int rin >>> >>> if (sreq->finish) >>> result_sz = crypto_ahash_digestsize(ahash); >>> - memcpy(sreq->state, areq->result, result_sz); >> [...] >> can the driver get request for final/finup/digest with null req->result ? > > I don't think that can happen. But having an update called without > req->result provided is a valid call (though it's not well documented). so maybe: if (sreq->finish) { result_sz = crypto_ahash_digestsize(ahash); memcpy(sreq->state, areq->result, result_sz); } -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 11:52 ` Kamil Konieczny @ 2017-11-30 12:41 ` Antoine Tenart 2017-11-30 14:10 ` Kamil Konieczny 0 siblings, 1 reply; 21+ messages in thread From: Antoine Tenart @ 2017-11-30 12:41 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, herbert, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On Thu, Nov 30, 2017 at 12:52:42PM +0100, Kamil Konieczny wrote: > On 30.11.2017 10:29, Antoine Tenart wrote: > > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > >> can the driver get request for final/finup/digest with null req->result ? > > > > I don't think that can happen. But having an update called without > > req->result provided is a valid call (though it's not well documented). > > so maybe: > > if (sreq->finish) { > result_sz = crypto_ahash_digestsize(ahash); > memcpy(sreq->state, areq->result, result_sz); > } No, if we do this we'll lose the ability to export the current state. Thanks, Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 12:41 ` Antoine Tenart @ 2017-11-30 14:10 ` Kamil Konieczny 2017-11-30 14:30 ` Antoine Tenart 0 siblings, 1 reply; 21+ messages in thread From: Kamil Konieczny @ 2017-11-30 14:10 UTC (permalink / raw) To: Antoine Tenart Cc: herbert, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On 30.11.2017 13:41, Antoine Tenart wrote: > On Thu, Nov 30, 2017 at 12:52:42PM +0100, Kamil Konieczny wrote: >> On 30.11.2017 10:29, Antoine Tenart wrote: >>> On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >>>> can the driver get request for final/finup/digest with null req->result ? >>> >>> I don't think that can happen. But having an update called without >>> req->result provided is a valid call (though it's not well documented). Yes, this field may be unset until finup/final. One more to watch out is that in final, you can have areq->nsize != 0 so you should not use areq->nsize nor areq->src >> so maybe: >> >> if (sreq->finish) { >> result_sz = crypto_ahash_digestsize(ahash); >> memcpy(sreq->state, areq->result, result_sz); >> } > > No, if we do this we'll lose the ability to export the current state. So maybe save it into request context: result_sz = crypto_ahash_digestsize(ahash); ctx = ahash_request_ctx(areq); if (sreq->finish) memcpy(sreq->state, areq->result, result_sz); else memcpy(sreq->state, ctx->state, result_sz); -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 14:10 ` Kamil Konieczny @ 2017-11-30 14:30 ` Antoine Tenart 0 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-11-30 14:30 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, herbert, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Kamil, On Thu, Nov 30, 2017 at 03:10:28PM +0100, Kamil Konieczny wrote: > On 30.11.2017 13:41, Antoine Tenart wrote: > > > > No, if we do this we'll lose the ability to export the current state. > > So maybe save it into request context: > > result_sz = crypto_ahash_digestsize(ahash); > ctx = ahash_request_ctx(areq); > > if (sreq->finish) > memcpy(sreq->state, areq->result, result_sz); > else > memcpy(sreq->state, ctx->state, result_sz); Storing the digest into a driver own buffer could improve the export ability in some *very rare* cases. If so I would suggest not to deal with the kind of test you proposed, but to have your own buffer used each time. Anyway, this has nothing to do with the fix I'm proposing here, as it would change the driver's logic, and would solve a complete different (rare) issue. The proposal here is to have a simple fix (which is similar to what can be found in some other drivers), that can easily be backported to avoid NULL pointer dereferences in older versions of the kernel. Thanks, Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-11-30 9:19 ` Kamil Konieczny 2017-11-30 9:29 ` Antoine Tenart @ 2017-12-01 0:31 ` Herbert Xu 2017-12-01 8:11 ` Antoine Tenart 1 sibling, 1 reply; 21+ messages in thread From: Herbert Xu @ 2017-12-01 0:31 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > > can the driver get request for final/finup/digest with null req->result ? > If yes (?), such checks can be done before any hardware processing, saving time, > for example: This should not be possible through any user-space facing API. If a kernel API user did this then they're just shooting themselves in the foot. So unless there is a valida call path that leads to this then I would say that there is nothing to fix. 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 0:31 ` Herbert Xu @ 2017-12-01 8:11 ` Antoine Tenart 2017-12-01 10:18 ` Kamil Konieczny 2017-12-01 10:35 ` Herbert Xu 0 siblings, 2 replies; 21+ messages in thread From: Antoine Tenart @ 2017-12-01 8:11 UTC (permalink / raw) To: Herbert Xu Cc: Kamil Konieczny, Antoine Tenart, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Herbert, On Fri, Dec 01, 2017 at 11:31:09AM +1100, Herbert Xu wrote: > On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: > > > > can the driver get request for final/finup/digest with null req->result ? > > If yes (?), such checks can be done before any hardware processing, saving time, > > for example: > > This should not be possible through any user-space facing API. > > If a kernel API user did this then they're just shooting themselves > in the foot. > > So unless there is a valida call path that leads to this then I > would say that there is nothing to fix. I agree this should not be the case. But: - Other drivers are doing this check (grep "if (!req->result)" or "if (req->result)" to see some of them). - I see at least one commit fixing the exact same issue I'm facing here, 393897c5156a415533ff85aa381458840417b032: crypto: ccp - Check for caller result area before using it For a hash operation, the caller doesn't have to supply a result area on every call so don't use it / update it if it hasn't been supplied. I'm not entirely sure what was the code path that leads to this, I'll reproduce the issue and try to understand what is going on (I clearly recall having this crash though). The crypto API does not enforce this somehow, and this should probably be fixed. That might break some users. But it was seen as a valid use for some, so we should probably fix this in previous versions of the driver anyway. Thanks! Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 8:11 ` Antoine Tenart @ 2017-12-01 10:18 ` Kamil Konieczny 2017-12-01 10:24 ` Antoine Tenart 2017-12-01 10:36 ` Herbert Xu 2017-12-01 10:35 ` Herbert Xu 1 sibling, 2 replies; 21+ messages in thread From: Kamil Konieczny @ 2017-12-01 10:18 UTC (permalink / raw) To: Antoine Tenart, Herbert Xu Cc: davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi All, On 01.12.2017 09:11, Antoine Tenart wrote: > Hi Herbert, > > On Fri, Dec 01, 2017 at 11:31:09AM +1100, Herbert Xu wrote: >> On Thu, Nov 30, 2017 at 10:19:26AM +0100, Kamil Konieczny wrote: >>> >>> can the driver get request for final/finup/digest with null req->result ? >>> If yes (?), such checks can be done before any hardware processing, saving time, >>> for example: >> >> This should not be possible through any user-space facing API. >> >> If a kernel API user did this then they're just shooting themselves >> in the foot. >> >> So unless there is a valida call path that leads to this then I >> would say that there is nothing to fix. > > I agree this should not be the case. > > But: > - Other drivers are doing this check (grep "if (!req->result)" or > "if (req->result)" to see some of them). > - I see at least one commit fixing the exact same issue I'm facing here, > 393897c5156a415533ff85aa381458840417b032: > > crypto: ccp - Check for caller result area before using it > > For a hash operation, the caller doesn't have to supply a result > area on every call so don't use it / update it if it hasn't > been supplied. Herbert, is it possible for every init/update that areq->result can be NULL, and only for final/update/digit user set it to actual memory ? testmgr.c can check if hash update writes into areq->result and if yes, then test fails ? As I understand this, when crypto api user allocates ahash_request, crypto allocates memory for itself _plus_ for driver's context. This allocated ahash_request is "handle" for all subsequent updates/export/import, and for last final/finup, so I do not need to copy hash state into areq->result, but keep it whole time in context, in your code in sreq: struct safexcel_ahash_req *sreq = ahash_request_ctx(areq); so here sreq is async hash request context. Do you set last_req true for digest/finup/final ? If yes, then you need to copy result only when it is true, if (sreq->last_req) { result_sz = crypto_ahash_digestsize(ahash); memcpy(sreq->state, areq->result, result_sz); } I do not read all your code though, so I can be wrong here. > I'm not entirely sure what was the code path that leads to this, I'll > reproduce the issue and try to understand what is going on (I clearly > recall having this crash though). > > The crypto API does not enforce this somehow, and this should probably > be fixed. That might break some users. But it was seen as a valid use > for some, so we should probably fix this in previous versions of the > driver anyway. -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:18 ` Kamil Konieczny @ 2017-12-01 10:24 ` Antoine Tenart 2017-12-01 10:43 ` Kamil Konieczny 2017-12-01 10:36 ` Herbert Xu 1 sibling, 1 reply; 21+ messages in thread From: Antoine Tenart @ 2017-12-01 10:24 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, Herbert Xu, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Kamil, On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: > On 01.12.2017 09:11, Antoine Tenart wrote: > > - Other drivers are doing this check (grep "if (!req->result)" or > > "if (req->result)" to see some of them). > > - I see at least one commit fixing the exact same issue I'm facing here, > > 393897c5156a415533ff85aa381458840417b032: > > > > crypto: ccp - Check for caller result area before using it > > > > For a hash operation, the caller doesn't have to supply a result > > area on every call so don't use it / update it if it hasn't > > been supplied. > > Do you set last_req true for digest/finup/final ? If yes, > then you need to copy result only when it is true, > > if (sreq->last_req) { > result_sz = crypto_ahash_digestsize(ahash); > memcpy(sreq->state, areq->result, result_sz); > } Yes the last_req flag is set for the last request, so when digest/finup/final are called. But no we can't copy the result into the state based on this as an user might want to perform multiple updates, then export the context, to import it again before sending more updates. Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:24 ` Antoine Tenart @ 2017-12-01 10:43 ` Kamil Konieczny 2017-12-01 10:55 ` Antoine Tenart 0 siblings, 1 reply; 21+ messages in thread From: Kamil Konieczny @ 2017-12-01 10:43 UTC (permalink / raw) To: Antoine Tenart Cc: Herbert Xu, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Antoine, On 01.12.2017 11:24, Antoine Tenart wrote: > Hi Kamil, > > On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: >> On 01.12.2017 09:11, Antoine Tenart wrote: >>> - Other drivers are doing this check (grep "if (!req->result)" or >>> "if (req->result)" to see some of them). >>> - I see at least one commit fixing the exact same issue I'm facing here, >>> 393897c5156a415533ff85aa381458840417b032: >>> >>> crypto: ccp - Check for caller result area before using it >>> >>> For a hash operation, the caller doesn't have to supply a result >>> area on every call so don't use it / update it if it hasn't >>> been supplied. >> >> Do you set last_req true for digest/finup/final ? If yes, >> then you need to copy result only when it is true, >> >> if (sreq->last_req) { >> result_sz = crypto_ahash_digestsize(ahash); >> memcpy(sreq->state, areq->result, result_sz); >> } > > Yes the last_req flag is set for the last request, so when > digest/finup/final are called. But no we can't copy the result into the > state based on this as an user might want to perform multiple updates, > then export the context, to import it again before sending more updates. IMHO set them to false in hash update and init, set finish false and last_req true for hash final, and set both true for hash finup and digest. As Herbert tells in https://www.spinics.net/lists/linux-crypto/msg28658.html you should support scenario export + update/finup, so basically export is reading state but it do not halt your hash driver. -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:43 ` Kamil Konieczny @ 2017-12-01 10:55 ` Antoine Tenart 0 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-12-01 10:55 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, Herbert Xu, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On Fri, Dec 01, 2017 at 11:43:18AM +0100, Kamil Konieczny wrote: > On 01.12.2017 11:24, Antoine Tenart wrote: > > > > Yes the last_req flag is set for the last request, so when > > digest/finup/final are called. But no we can't copy the result into the > > state based on this as an user might want to perform multiple updates, > > then export the context, to import it again before sending more updates. > > IMHO set them to false in hash update and init, set finish false and last_req true > for hash final, and set both true for hash finup and digest. > > As Herbert tells in https://www.spinics.net/lists/linux-crypto/msg28658.html > you should support scenario export + update/finup, so basically export is reading > state but it do not halt your hash driver. Except if you import another state in the meantime. Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:18 ` Kamil Konieczny 2017-12-01 10:24 ` Antoine Tenart @ 2017-12-01 10:36 ` Herbert Xu 2017-12-01 11:24 ` Kamil Konieczny 1 sibling, 1 reply; 21+ messages in thread From: Herbert Xu @ 2017-12-01 10:36 UTC (permalink / raw) To: Kamil Konieczny Cc: Antoine Tenart, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: > > Herbert, is it possible for every init/update that areq->result can be NULL, > and only for final/update/digit user set it to actual memory ? > testmgr.c can check if hash update writes into areq->result and if yes, > then test fails ? Yes we should modify testmgr to check that. 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:36 ` Herbert Xu @ 2017-12-01 11:24 ` Kamil Konieczny 0 siblings, 0 replies; 21+ messages in thread From: Kamil Konieczny @ 2017-12-01 11:24 UTC (permalink / raw) To: Herbert Xu Cc: Antoine Tenart, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Herbert, On 01.12.2017 11:36, Herbert Xu wrote: > On Fri, Dec 01, 2017 at 11:18:30AM +0100, Kamil Konieczny wrote: >> >> Herbert, is it possible for every init/update that areq->result can be NULL, >> and only for final/update/digit user set it to actual memory ? >> testmgr.c can check if hash update writes into areq->result and if yes, >> then test fails ? > > Yes we should modify testmgr to check that. I can write patch for it. Should it WARN_ON or treat it as error ? -- Best regards, Kamil Konieczny Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 8:11 ` Antoine Tenart 2017-12-01 10:18 ` Kamil Konieczny @ 2017-12-01 10:35 ` Herbert Xu 2017-12-01 10:52 ` Antoine Tenart 1 sibling, 1 reply; 21+ messages in thread From: Herbert Xu @ 2017-12-01 10:35 UTC (permalink / raw) To: Antoine Tenart Cc: Kamil Konieczny, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto On Fri, Dec 01, 2017 at 09:11:57AM +0100, Antoine Tenart wrote: > > I agree this should not be the case. > > But: > - Other drivers are doing this check (grep "if (!req->result)" or > "if (req->result)" to see some of them). > - I see at least one commit fixing the exact same issue I'm facing here, > 393897c5156a415533ff85aa381458840417b032: > > crypto: ccp - Check for caller result area before using it > > For a hash operation, the caller doesn't have to supply a result > area on every call so don't use it / update it if it hasn't > been supplied. > > I'm not entirely sure what was the code path that leads to this, I'll > reproduce the issue and try to understand what is going on (I clearly > recall having this crash though). That's different. In that case an unconditional copy is made regardless of whether the operation is final or update. That's why a check is required. If the operation is finup/final/digest then req->result must be set and you don't need to check it. 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided 2017-12-01 10:35 ` Herbert Xu @ 2017-12-01 10:52 ` Antoine Tenart 0 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-12-01 10:52 UTC (permalink / raw) To: Herbert Xu Cc: Antoine Tenart, Kamil Konieczny, davem, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto Hi Herbert, On Fri, Dec 01, 2017 at 09:35:52PM +1100, Herbert Xu wrote: > On Fri, Dec 01, 2017 at 09:11:57AM +0100, Antoine Tenart wrote: > > > > I agree this should not be the case. > > > > But: > > - Other drivers are doing this check (grep "if (!req->result)" or > > "if (req->result)" to see some of them). > > - I see at least one commit fixing the exact same issue I'm facing here, > > 393897c5156a415533ff85aa381458840417b032: > > > > crypto: ccp - Check for caller result area before using it > > > > For a hash operation, the caller doesn't have to supply a result > > area on every call so don't use it / update it if it hasn't > > been supplied. > > > > I'm not entirely sure what was the code path that leads to this, I'll > > reproduce the issue and try to understand what is going on (I clearly > > recall having this crash though). > > That's different. In that case an unconditional copy is made > regardless of whether the operation is final or update. That's > why a check is required. > > If the operation is finup/final/digest then req->result must be > set and you don't need to check it. Ah, I didn't understand your point then. Of course ->result should be allocated for finup/final/digest. The function where I fix this is called regardless of the operation that was performed, so it can be an update() as well. Thanks, Antoine -- Antoine Ténart, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/4] crypto: inside-secure - fix request allocations in invalidation path 2017-11-28 15:42 [PATCH v2 0/4] crypto: inside-secure - set of fixes Antoine Tenart ` (2 preceding siblings ...) 2017-11-28 15:42 ` [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided Antoine Tenart @ 2017-11-28 15:42 ` Antoine Tenart 3 siblings, 0 replies; 21+ messages in thread From: Antoine Tenart @ 2017-11-28 15:42 UTC (permalink / raw) To: herbert, davem Cc: Antoine Tenart, thomas.petazzoni, gregory.clement, miquel.raynal, oferh, igall, nadavh, linux-crypto This patch makes use of the SKCIPHER_REQUEST_ON_STACK and AHASH_REQUEST_ON_STACK helpers to allocate enough memory to contain both the crypto request structures and their embedded context (__ctx). Fixes: 1b44c5a60c13 ("crypto: inside-secure - add SafeXcel EIP197 crypto engine driver") Suggested-by: Ofer Heifetz <oferh@marvell.com> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com> --- drivers/crypto/inside-secure/safexcel_cipher.c | 14 +++++++------- drivers/crypto/inside-secure/safexcel_hash.c | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/crypto/inside-secure/safexcel_cipher.c b/drivers/crypto/inside-secure/safexcel_cipher.c index 9ea24868d860..ef44f5a5a90f 100644 --- a/drivers/crypto/inside-secure/safexcel_cipher.c +++ b/drivers/crypto/inside-secure/safexcel_cipher.c @@ -422,25 +422,25 @@ static int safexcel_cipher_exit_inv(struct crypto_tfm *tfm) { struct safexcel_cipher_ctx *ctx = crypto_tfm_ctx(tfm); struct safexcel_crypto_priv *priv = ctx->priv; - struct skcipher_request req; - struct safexcel_cipher_req *sreq = skcipher_request_ctx(&req); + SKCIPHER_REQUEST_ON_STACK(req, __crypto_skcipher_cast(tfm)); + struct safexcel_cipher_req *sreq = skcipher_request_ctx(req); struct safexcel_inv_result result = {}; int ring = ctx->base.ring; - memset(&req, 0, sizeof(struct skcipher_request)); + memset(req, 0, sizeof(struct skcipher_request)); /* create invalidation request */ init_completion(&result.completion); - skcipher_request_set_callback(&req, CRYPTO_TFM_REQ_MAY_BACKLOG, + skcipher_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, safexcel_inv_complete, &result); - skcipher_request_set_tfm(&req, __crypto_skcipher_cast(tfm)); - ctx = crypto_tfm_ctx(req.base.tfm); + skcipher_request_set_tfm(req, __crypto_skcipher_cast(tfm)); + ctx = crypto_tfm_ctx(req->base.tfm); ctx->base.exit_inv = true; sreq->needs_inv = true; spin_lock_bh(&priv->ring[ring].queue_lock); - crypto_enqueue_request(&priv->ring[ring].queue, &req.base); + crypto_enqueue_request(&priv->ring[ring].queue, &req->base); spin_unlock_bh(&priv->ring[ring].queue_lock); if (!priv->ring[ring].need_dequeue) diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 424f4c5d4d25..55ae5bce483c 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -456,25 +456,25 @@ static int safexcel_ahash_exit_inv(struct crypto_tfm *tfm) { struct safexcel_ahash_ctx *ctx = crypto_tfm_ctx(tfm); struct safexcel_crypto_priv *priv = ctx->priv; - struct ahash_request req; - struct safexcel_ahash_req *rctx = ahash_request_ctx(&req); + AHASH_REQUEST_ON_STACK(req, __crypto_ahash_cast(tfm)); + struct safexcel_ahash_req *rctx = ahash_request_ctx(req); struct safexcel_inv_result result = {}; int ring = ctx->base.ring; - memset(&req, 0, sizeof(struct ahash_request)); + memset(req, 0, sizeof(struct ahash_request)); /* create invalidation request */ init_completion(&result.completion); - ahash_request_set_callback(&req, CRYPTO_TFM_REQ_MAY_BACKLOG, + ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG, safexcel_inv_complete, &result); - ahash_request_set_tfm(&req, __crypto_ahash_cast(tfm)); - ctx = crypto_tfm_ctx(req.base.tfm); + ahash_request_set_tfm(req, __crypto_ahash_cast(tfm)); + ctx = crypto_tfm_ctx(req->base.tfm); ctx->base.exit_inv = true; rctx->needs_inv = true; spin_lock_bh(&priv->ring[ring].queue_lock); - crypto_enqueue_request(&priv->ring[ring].queue, &req.base); + crypto_enqueue_request(&priv->ring[ring].queue, &req->base); spin_unlock_bh(&priv->ring[ring].queue_lock); if (!priv->ring[ring].need_dequeue) -- 2.14.3 ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2017-12-01 11:24 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-11-28 15:42 [PATCH v2 0/4] crypto: inside-secure - set of fixes Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 1/4] crypto: inside-secure - per request invalidation Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 2/4] crypto: inside-secure - free requests even if their handling failed Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 3/4] crypto: inside-secure - only update the result buffer when provided Antoine Tenart 2017-11-30 9:19 ` Kamil Konieczny 2017-11-30 9:29 ` Antoine Tenart 2017-11-30 11:52 ` Kamil Konieczny 2017-11-30 12:41 ` Antoine Tenart 2017-11-30 14:10 ` Kamil Konieczny 2017-11-30 14:30 ` Antoine Tenart 2017-12-01 0:31 ` Herbert Xu 2017-12-01 8:11 ` Antoine Tenart 2017-12-01 10:18 ` Kamil Konieczny 2017-12-01 10:24 ` Antoine Tenart 2017-12-01 10:43 ` Kamil Konieczny 2017-12-01 10:55 ` Antoine Tenart 2017-12-01 10:36 ` Herbert Xu 2017-12-01 11:24 ` Kamil Konieczny 2017-12-01 10:35 ` Herbert Xu 2017-12-01 10:52 ` Antoine Tenart 2017-11-28 15:42 ` [PATCH v2 4/4] crypto: inside-secure - fix request allocations in invalidation path Antoine Tenart
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.