From: Dongliang Mu <mudongliangabcd@gmail.com> To: Corentin Labbe <clabbe.montjoie@gmail.com> Cc: Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Ard Biesheuvel <ardb@kernel.org>, Jonathan Corbet <corbet@lwn.net>, Eric Biggers <ebiggers@google.com>, Xiang Chen <chenxiang66@hisilicon.com>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Corentin Labbe <clabbe@baylibre.com>, "Jason A. Donenfeld" <Jason@zx2c4.com>, linux-crypto@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run Date: Tue, 3 Aug 2021 14:06:43 +0800 [thread overview] Message-ID: <CAD-N9QVfreHCV7673DVq6aS3+3RYQ4-x8eZuGF0pvUbRhTL-Cg@mail.gmail.com> (raw) In-Reply-To: <CAD-N9QWvR-7caWCnk1CMo8sWPEC=CfKU2_v=YkTVr0o5L7wehA@mail.gmail.com> On Tue, Aug 3, 2021 at 1:57 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Tue, Aug 3, 2021 at 2:25 AM Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > > > > Le Mon, Jul 26, 2021 at 11:27:12PM +0800, Dongliang Mu a écrit : > > > In sun8i_ce_hash_run, all the dma_mmap_sg/single will cause memory leak > > > due to no corresponding unmap operation if errors happen. > > > > > > Fix this by adding error handling part for all the dma_mmap_sg/single. > > > > > > > I think it could be better worded, error handling is already there (but bad). > > Sure. How about "Fix this by freeing the objects allocated by > dma_mmap_sg/single when errors occur."? > > > > > > Fixes: 56f6d5aee88d ("crypto: sun8i-ce - support hash algorithms") > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > .../crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 28 +++++++++---------- > > > 1 file changed, 13 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > index 88194718a806..d454ad99deee 100644 > > > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > @@ -286,16 +286,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > > > > /* the padding could be up to two block. */ > > > buf = kzalloc(bs * 2, GFP_KERNEL | GFP_DMA); > > > - if (!buf) { > > > - err = -ENOMEM; > > > - goto theend; > > > > Please keep all goto error for being consistent. > > OK, no problem. > > BTW, usually for the 1st malloc failure, I prefer returning with errno directly. For this case, as crypto_finalize_hash_request must be executed, so directly return is incorrect. I will add a goto label before crypto_finalize_hash_request. > > > > > > - } > > > + if (!buf) > > > + return -ENOMEM; > > > bf = (__le32 *)buf; > > > > > > result = kzalloc(digestsize, GFP_KERNEL | GFP_DMA); > > > if (!result) { > > > - err = -ENOMEM; > > > - goto theend; > > > + kfree(buf); > > > + return -ENOMEM; > > > } > > > > > > flow = rctx->flow; > > > @@ -321,7 +319,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (nr_sgs <= 0 || nr_sgs > MAX_SG) { > > > dev_err(ce->dev, "Invalid sg number %d\n", nr_sgs); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_result; > > > } > > > > > > len = areq->nbytes; > > > @@ -334,7 +332,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (len > 0) { > > > dev_err(ce->dev, "remaining len %d\n", len); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_unmap_sg; > > > } > > > addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE); > > > cet->t_dst[0].addr = cpu_to_le32(addr_res); > > > @@ -342,7 +340,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (dma_mapping_error(ce->dev, addr_res)) { > > > dev_err(ce->dev, "DMA map dest\n"); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_unmap_sg; > > > } > > > > > > byte_count = areq->nbytes; > > > @@ -392,7 +390,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (dma_mapping_error(ce->dev, addr_pad)) { > > > dev_err(ce->dev, "DMA error on padding SG\n"); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_addr_res; > > > } > > > > > > if (ce->variant->hash_t_dlen_in_bits) > > > @@ -405,15 +403,15 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(areq->base.tfm)); > > > > > > dma_unmap_single(ce->dev, addr_pad, j * 4, DMA_TO_DEVICE); > > > +err_addr_res: > > > + dma_unmap_single(ce->dev, addr_res, digestsize, DMA_FROM_DEVICE); > > > +err_unmap_sg: > > > dma_unmap_sg(ce->dev, areq->src, sg_nents(areq->src), > > > DMA_TO_DEVICE); > > > - dma_unmap_single(ce->dev, addr_res, digestsize, DMA_FROM_DEVICE); > > > - > > > - > > > memcpy(areq->result, result, algt->alg.hash.halg.digestsize); > > > > The result should be copied only when everything is ok. Please add a "if (!err)" > > Sure. > > > > > Thanks for your work > > Regards
WARNING: multiple messages have this Message-ID (diff)
From: Dongliang Mu <mudongliangabcd@gmail.com> To: Corentin Labbe <clabbe.montjoie@gmail.com> Cc: Herbert Xu <herbert@gondor.apana.org.au>, "David S. Miller" <davem@davemloft.net>, Maxime Ripard <mripard@kernel.org>, Chen-Yu Tsai <wens@csie.org>, Jernej Skrabec <jernej.skrabec@gmail.com>, Ard Biesheuvel <ardb@kernel.org>, Jonathan Corbet <corbet@lwn.net>, Eric Biggers <ebiggers@google.com>, Xiang Chen <chenxiang66@hisilicon.com>, Mauro Carvalho Chehab <mchehab+huawei@kernel.org>, Corentin Labbe <clabbe@baylibre.com>, "Jason A. Donenfeld" <Jason@zx2c4.com>, linux-crypto@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run Date: Tue, 3 Aug 2021 14:06:43 +0800 [thread overview] Message-ID: <CAD-N9QVfreHCV7673DVq6aS3+3RYQ4-x8eZuGF0pvUbRhTL-Cg@mail.gmail.com> (raw) In-Reply-To: <CAD-N9QWvR-7caWCnk1CMo8sWPEC=CfKU2_v=YkTVr0o5L7wehA@mail.gmail.com> On Tue, Aug 3, 2021 at 1:57 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Tue, Aug 3, 2021 at 2:25 AM Corentin Labbe <clabbe.montjoie@gmail.com> wrote: > > > > Le Mon, Jul 26, 2021 at 11:27:12PM +0800, Dongliang Mu a écrit : > > > In sun8i_ce_hash_run, all the dma_mmap_sg/single will cause memory leak > > > due to no corresponding unmap operation if errors happen. > > > > > > Fix this by adding error handling part for all the dma_mmap_sg/single. > > > > > > > I think it could be better worded, error handling is already there (but bad). > > Sure. How about "Fix this by freeing the objects allocated by > dma_mmap_sg/single when errors occur."? > > > > > > Fixes: 56f6d5aee88d ("crypto: sun8i-ce - support hash algorithms") > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > .../crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 28 +++++++++---------- > > > 1 file changed, 13 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > index 88194718a806..d454ad99deee 100644 > > > --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c > > > @@ -286,16 +286,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > > > > /* the padding could be up to two block. */ > > > buf = kzalloc(bs * 2, GFP_KERNEL | GFP_DMA); > > > - if (!buf) { > > > - err = -ENOMEM; > > > - goto theend; > > > > Please keep all goto error for being consistent. > > OK, no problem. > > BTW, usually for the 1st malloc failure, I prefer returning with errno directly. For this case, as crypto_finalize_hash_request must be executed, so directly return is incorrect. I will add a goto label before crypto_finalize_hash_request. > > > > > > - } > > > + if (!buf) > > > + return -ENOMEM; > > > bf = (__le32 *)buf; > > > > > > result = kzalloc(digestsize, GFP_KERNEL | GFP_DMA); > > > if (!result) { > > > - err = -ENOMEM; > > > - goto theend; > > > + kfree(buf); > > > + return -ENOMEM; > > > } > > > > > > flow = rctx->flow; > > > @@ -321,7 +319,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (nr_sgs <= 0 || nr_sgs > MAX_SG) { > > > dev_err(ce->dev, "Invalid sg number %d\n", nr_sgs); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_result; > > > } > > > > > > len = areq->nbytes; > > > @@ -334,7 +332,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (len > 0) { > > > dev_err(ce->dev, "remaining len %d\n", len); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_unmap_sg; > > > } > > > addr_res = dma_map_single(ce->dev, result, digestsize, DMA_FROM_DEVICE); > > > cet->t_dst[0].addr = cpu_to_le32(addr_res); > > > @@ -342,7 +340,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (dma_mapping_error(ce->dev, addr_res)) { > > > dev_err(ce->dev, "DMA map dest\n"); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_unmap_sg; > > > } > > > > > > byte_count = areq->nbytes; > > > @@ -392,7 +390,7 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > if (dma_mapping_error(ce->dev, addr_pad)) { > > > dev_err(ce->dev, "DMA error on padding SG\n"); > > > err = -EINVAL; > > > - goto theend; > > > + goto err_addr_res; > > > } > > > > > > if (ce->variant->hash_t_dlen_in_bits) > > > @@ -405,15 +403,15 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) > > > err = sun8i_ce_run_task(ce, flow, crypto_tfm_alg_name(areq->base.tfm)); > > > > > > dma_unmap_single(ce->dev, addr_pad, j * 4, DMA_TO_DEVICE); > > > +err_addr_res: > > > + dma_unmap_single(ce->dev, addr_res, digestsize, DMA_FROM_DEVICE); > > > +err_unmap_sg: > > > dma_unmap_sg(ce->dev, areq->src, sg_nents(areq->src), > > > DMA_TO_DEVICE); > > > - dma_unmap_single(ce->dev, addr_res, digestsize, DMA_FROM_DEVICE); > > > - > > > - > > > memcpy(areq->result, result, algt->alg.hash.halg.digestsize); > > > > The result should be copied only when everything is ok. Please add a "if (!err)" > > Sure. > > > > > Thanks for your work > > Regards _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-08-03 6:07 UTC|newest] Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-07-26 15:27 [PATCH] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run Dongliang Mu 2021-07-26 15:27 ` Dongliang Mu 2021-08-02 18:25 ` Corentin Labbe 2021-08-02 18:25 ` Corentin Labbe 2021-08-03 5:57 ` Dongliang Mu 2021-08-03 5:57 ` Dongliang Mu 2021-08-03 5:57 ` Dongliang Mu 2021-08-03 6:06 ` Dongliang Mu [this message] 2021-08-03 6:06 ` Dongliang Mu 2021-08-03 6:06 ` Dongliang Mu
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=CAD-N9QVfreHCV7673DVq6aS3+3RYQ4-x8eZuGF0pvUbRhTL-Cg@mail.gmail.com \ --to=mudongliangabcd@gmail.com \ --cc=Jason@zx2c4.com \ --cc=ardb@kernel.org \ --cc=chenxiang66@hisilicon.com \ --cc=clabbe.montjoie@gmail.com \ --cc=clabbe@baylibre.com \ --cc=corbet@lwn.net \ --cc=davem@davemloft.net \ --cc=ebiggers@google.com \ --cc=herbert@gondor.apana.org.au \ --cc=jernej.skrabec@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-crypto@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-sunxi@lists.linux.dev \ --cc=mchehab+huawei@kernel.org \ --cc=mripard@kernel.org \ --cc=wens@csie.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.