From: Herbert Xu <herbert@gondor.apana.org.au> To: Arnd Bergmann <arnd@arndb.de> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>, David Miller <davem@davemloft.net>, linux-crypto@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org Subject: [PATCH v3] crypto: ixp4xx - Fix error handling path in 'aead_perform()' Date: Wed, 2 Aug 2017 16:40:47 +0800 [thread overview] Message-ID: <20170802084046.GA6740@gondor.apana.org.au> (raw) In-Reply-To: <CAK8P3a3RuQmudGRGvpE1i=RwjJ_KVU1S+bDqX6Bp6VLB7G+2cg@mail.gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Herbert Xu <herbert@gondor.apana.org.au> To: Arnd Bergmann <arnd@arndb.de> Cc: Christophe JAILLET <christophe.jaillet@wanadoo.fr>, David Miller <davem@davemloft.net>, linux-crypto@vger.kernel.org, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, kernel-janitors@vger.kernel.org Subject: [PATCH v3] crypto: ixp4xx - Fix error handling path in 'aead_perform()' Date: Wed, 02 Aug 2017 08:40:47 +0000 [thread overview] Message-ID: <20170802084046.GA6740@gondor.apana.org.au> (raw) In-Reply-To: <CAK8P3a3RuQmudGRGvpE1i=RwjJ_KVU1S+bDqX6Bp6VLB7G+2cg@mail.gmail.com> 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
next prev parent reply other threads:[~2017-08-02 8:40 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-07-19 21:47 [PATCH] crypto: ixp4xx - Fix error handling path in 'aead_perform()' Christophe JAILLET 2017-07-19 21:47 ` Christophe JAILLET 2017-07-20 7:37 ` Arnd Bergmann 2017-07-20 7:37 ` Arnd Bergmann 2017-07-20 15:04 ` Arnd Bergmann 2017-07-20 15:04 ` Arnd Bergmann 2017-07-20 15:16 ` Arnd Bergmann 2017-07-20 15:16 ` Arnd Bergmann 2017-08-02 8:40 ` Herbert Xu [this message] 2017-08-02 8:40 ` [PATCH v3] " Herbert Xu 2017-08-02 10:59 ` Arnd Bergmann 2017-08-02 10:59 ` Arnd Bergmann
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170802084046.GA6740@gondor.apana.org.au \ --to=herbert@gondor.apana.org.au \ --cc=arnd@arndb.de \ --cc=christophe.jaillet@wanadoo.fr \ --cc=davem@davemloft.net \ --cc=kernel-janitors@vger.kernel.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.