* [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
@ 2017-07-19 21:47 Christophe JAILLET
2017-07-20 7:37 ` Arnd Bergmann
0 siblings, 1 reply; 6+ messages in thread
From: Christophe JAILLET @ 2017-07-19 21:47 UTC (permalink / raw)
To: herbert, davem, arnd
Cc: linux-crypto, linux-kernel, kernel-janitors, Christophe JAILLET
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 <christophe.jaillet@wanadoo.fr>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
2017-07-19 21:47 [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()' Christophe JAILLET
@ 2017-07-20 7:37 ` Arnd Bergmann
2017-07-20 15:04 ` Arnd Bergmann
2017-08-02 8:40 ` [PATCH v3] " Herbert Xu
0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-07-20 7:37 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Herbert Xu, David Miller, linux-crypto,
Linux Kernel Mailing List, kernel-janitors
On Wed, Jul 19, 2017 at 11:47 PM, Christophe JAILLET
<christophe.jaillet@wanadoo.fr> 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 <christophe.jaillet@wanadoo.fr>
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 <arnd@arndb.de>
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,
&crypt->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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
2017-07-20 7:37 ` Arnd Bergmann
@ 2017-07-20 15:04 ` Arnd Bergmann
2017-07-20 15:16 ` Arnd Bergmann
2017-08-02 8:40 ` [PATCH v3] " Herbert Xu
1 sibling, 1 reply; 6+ messages in thread
From: Arnd Bergmann @ 2017-07-20 15:04 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Herbert Xu, David Miller, linux-crypto,
Linux Kernel Mailing List, kernel-janitors
On Thu, Jul 20, 2017 at 9:37 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Jul 19, 2017 at 11:47 PM, Christophe JAILLET
> <christophe.jaillet@wanadoo.fr> 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 <christophe.jaillet@wanadoo.fr>
>
> 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 <arnd@arndb.de>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
2017-07-20 15:04 ` Arnd Bergmann
@ 2017-07-20 15:16 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-07-20 15:16 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Herbert Xu, David Miller, linux-crypto,
Linux Kernel Mailing List, kernel-janitors
On Thu, Jul 20, 2017 at 5:04 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
2017-07-20 7:37 ` Arnd Bergmann
2017-07-20 15:04 ` Arnd Bergmann
@ 2017-08-02 8:40 ` Herbert Xu
2017-08-02 10:59 ` Arnd Bergmann
1 sibling, 1 reply; 6+ messages in thread
From: Herbert Xu @ 2017-08-02 8:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Christophe JAILLET, David Miller, linux-crypto,
Linux Kernel Mailing List, kernel-janitors
On Thu, Jul 20, 2017 at 09:37:10AM +0200, Arnd Bergmann wrote:
>
> 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 <arnd@arndb.de>
Oops, looks like I introduced the bug. Anyway, such is the danger
of fixing compiler warnings in rarely used code.
For some reason your patch is corrupted in patchwork. Also we don't
need to check crypt->dst as free_buf_chain is a no-op if we didn't do
the allocation at all. So how about this patch?
---8<---
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")
Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/drivers/crypto/ixp4xx_crypto.c b/drivers/crypto/ixp4xx_crypto.c
index 427cbe0..55dc9c2 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,
&crypt->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,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;
}
--
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 related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] crypto: ixp4xx - Fix error handling path in 'aead_perform()'
2017-08-02 8:40 ` [PATCH v3] " Herbert Xu
@ 2017-08-02 10:59 ` Arnd Bergmann
0 siblings, 0 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-08-02 10:59 UTC (permalink / raw)
To: Herbert Xu
Cc: Christophe JAILLET, David Miller, linux-crypto,
Linux Kernel Mailing List, kernel-janitors
On Wed, Aug 2, 2017 at 10:40 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Jul 20, 2017 at 09:37:10AM +0200, Arnd Bergmann wrote:
>
> Oops, looks like I introduced the bug. Anyway, such is the danger
> of fixing compiler warnings in rarely used code.
>
> For some reason your patch is corrupted in patchwork. Also we don't
> need to check crypt->dst as free_buf_chain is a no-op if we didn't do
> the allocation at all. So how about this patch?
Looks good to me.
> ---8<---
> 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")
> Reported-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-02 10:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-19 21:47 [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()' Christophe JAILLET
2017-07-20 7:37 ` Arnd Bergmann
2017-07-20 15:04 ` Arnd Bergmann
2017-07-20 15:16 ` Arnd Bergmann
2017-08-02 8:40 ` [PATCH v3] " Herbert Xu
2017-08-02 10:59 ` Arnd Bergmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).