Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
On Thu, Jul 20, 2017 at 5:04 PM, Arnd Bergmannwrote: > Coincidentally, I just came across an older patch of mine that actually > fixes the warning properly, but that for some reason ended up not > getting merged: > > https://patchwork.kernel.org/patch/8236811/ > > How about we just revert my broken 0f987e25cb8a patch, and I apply > the mach-ixp4xx patch instead? Nevermind that, I just tried it and the warning is back after reverting my 0f987e25cb8a patch, regardless of the indirect-io patch. Arnd
Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
On Thu, Jul 20, 2017 at 9:37 AM, Arnd Bergmannwrote: > On Wed, Jul 19, 2017 at 11:47 PM, Christophe JAILLET > wrote: >> In commit 0f987e25cb8a, the source processing has been moved in front of >> the destination processing, but the error handling path has not been >> modified accordingly. >> Free resources in the correct order to avoid some leaks. >> >> Fixes: 0f987e25cb8a ("crypto: ixp4xx - Fix false lastlen uninitialised >> warning") >> Signed-off-by: Christophe JAILLET > > Thanks for spotting my mistake! > > I've looked at it again and think it's unfortunately still wrong with > your patch, > as there is a 'goto free_buf_src' after dma_pool_alloc(), and that now needs > to jump to free_buf_dst instead. We may also need an extra check to make > sure we don't free an uninitialized pointer again. > > Can you have a look at this version below and send whatever you find > to be correct in the end? > > Signed-off-by: Arnd Bergmann Coincidentally, I just came across an older patch of mine that actually fixes the warning properly, but that for some reason ended up not getting merged: https://patchwork.kernel.org/patch/8236811/ How about we just revert my broken 0f987e25cb8a patch, and I apply the mach-ixp4xx patch instead? Arnd
Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
On Wed, Jul 19, 2017 at 11:47 PM, Christophe JAILLETwrote: > In commit 0f987e25cb8a, the source processing has been moved in front of > the destination processing, but the error handling path has not been > modified accordingly. > Free resources in the correct order to avoid some leaks. > > Fixes: 0f987e25cb8a ("crypto: ixp4xx - Fix false lastlen uninitialised > warning") > Signed-off-by: Christophe JAILLET Thanks for spotting my mistake! I've looked at it again and think it's unfortunately still wrong with your patch, as there is a 'goto free_buf_src' after dma_pool_alloc(), and that now needs to jump to free_buf_dst instead. We may also need an extra check to make sure we don't free an uninitialized pointer again. Can you have a look at this version below and send whatever you find to be correct in the end? Signed-off-by: Arnd Bergmann diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c index 427cbe012729..08b740dddb20 100644 --- a/drivers/crypto/ixp4xx_crypto.c +++ b/drivers/crypto/ixp4xx_crypto.c @@ -1073,7 +1073,7 @@ static int aead_perform(struct aead_request *req, int encrypt, req_ctx->hmac_virt = dma_pool_alloc(buffer_pool, flags, >icv_rev_aes); if (unlikely(!req_ctx->hmac_virt)) - goto free_buf_src; + goto free_buf_dst; if (!encrypt) { scatterwalk_map_and_copy(req_ctx->hmac_virt, req->src, cryptlen, authsize, 0); @@ -1088,10 +1088,11 @@ static int aead_perform(struct aead_request *req, int encrypt, BUG_ON(qmgr_stat_overflow(SEND_QID)); return -EINPROGRESS; +free_buf_dst: + if (crypt->dst) + free_buf_chain(dev, req_ctx->dst, crypt->dst_buf); free_buf_src: free_buf_chain(dev, req_ctx->src, crypt->src_buf); -free_buf_dst: - free_buf_chain(dev, req_ctx->dst, crypt->dst_buf); crypt->ctl_flags = CTL_FLAG_UNUSED; return -ENOMEM; }
[PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
In commit 0f987e25cb8a, the source processing has been moved in front of the destination processing, but the error handling path has not been modified accordingly. Free resources in the correct order to avoid some leaks. Fixes: 0f987e25cb8a ("crypto: ixp4xx - Fix false lastlen uninitialised warning") Signed-off-by: Christophe JAILLET--- drivers/crypto/ixp4xx_crypto.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c index 427cbe012729..1ccbe15b5f16 100644 --- a/drivers/crypto/ixp4xx_crypto.c +++ b/drivers/crypto/ixp4xx_crypto.c @@ -1088,10 +1088,10 @@ static int aead_perform(struct aead_request *req, int encrypt, BUG_ON(qmgr_stat_overflow(SEND_QID)); return -EINPROGRESS; -free_buf_src: - free_buf_chain(dev, req_ctx->src, crypt->src_buf); free_buf_dst: free_buf_chain(dev, req_ctx->dst, crypt->dst_buf); +free_buf_src: + free_buf_chain(dev, req_ctx->src, crypt->src_buf); crypt->ctl_flags = CTL_FLAG_UNUSED; return -ENOMEM; } -- 2.11.0