| Message ID | 20210803063149.2821093-1-mudongliangabcd@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run | expand |
On Tue, Aug 03, 2021 at 02:31:38PM +0800, Dongliang Mu wrote: > > -theend: > - kfree(buf); > +err_result: > kfree(result); > - crypto_finalize_hash_request(engine, breq, err); > +err_buf: > + kfree(buf); > +out: > + if (!err) > + crypto_finalize_hash_request(engine, breq, err); > return 0; This does not look right. You're returning zero in case of an error 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
On Thu, Aug 12, 2021 at 6:05 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Tue, Aug 03, 2021 at 02:31:38PM +0800, Dongliang Mu wrote: > > > > -theend: > > - kfree(buf); > > +err_result: > > kfree(result); > > - crypto_finalize_hash_request(engine, breq, err); > > +err_buf: > > + kfree(buf); > > +out: > > + if (!err) > > + crypto_finalize_hash_request(engine, breq, err); > > return 0; > > This does not look right. You're returning zero in case of an error Hi Herbert, Corentin Labbe said, For the error code, I am not sure it is needed, error code is already given to user via crypto_finalize_hash_request(). The "return 0" is for crypto/crypto_engine API, returning an error will not change anything since we dont have retry_support. So I propose you to focus on dma_map_xxx() fix patch. See details in [1]. P.S., my previous patch returns err variable. [1] https://lkml.org/lkml/2021/7/26/164 > > 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
On Thu, Aug 12, 2021 at 06:24:25PM +0800, Dongliang Mu wrote: > On Thu, Aug 12, 2021 at 6:05 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > On Tue, Aug 03, 2021 at 02:31:38PM +0800, Dongliang Mu wrote: > > > > > > -theend: > > > - kfree(buf); > > > +err_result: > > > kfree(result); > > > - crypto_finalize_hash_request(engine, breq, err); > > > +err_buf: > > > + kfree(buf); > > > +out: > > > + if (!err) > > > + crypto_finalize_hash_request(engine, breq, err); > > > return 0; > > > > This does not look right. You're returning zero in case of an error > > Hi Herbert, > > Corentin Labbe said, > > For the error code, I am not sure it is needed, error code is already > given to user via crypto_finalize_hash_request(). Yes but your patch changes this. You're now skipping the finalize call and thus throwing away err if it's not zero. If it's supposed to do this you need to explain it in your patch submission. 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
On Thu, Aug 12, 2021 at 6:30 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Thu, Aug 12, 2021 at 06:24:25PM +0800, Dongliang Mu wrote: > > On Thu, Aug 12, 2021 at 6:05 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > > > On Tue, Aug 03, 2021 at 02:31:38PM +0800, Dongliang Mu wrote: > > > > > > > > -theend: > > > > - kfree(buf); > > > > +err_result: > > > > kfree(result); > > > > - crypto_finalize_hash_request(engine, breq, err); > > > > +err_buf: > > > > + kfree(buf); > > > > +out: > > > > + if (!err) > > > > + crypto_finalize_hash_request(engine, breq, err); > > > > return 0; > > > > > > This does not look right. You're returning zero in case of an error > > > > Hi Herbert, > > > > Corentin Labbe said, > > > > For the error code, I am not sure it is needed, error code is already > > given to user via crypto_finalize_hash_request(). > > Yes but your patch changes this. You're now skipping the finalize > call and thus throwing away err if it's not zero. > I see. Thanks for your comment. I agree with you and would like to modify "return 0" to "return err" in the next version. Even if the parent function does not care about the return value, it is still necessary to keep the right return value. Corentin, how do you think? > If it's supposed to do this you need to explain it in your patch > submission. > > 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
On Thu, Aug 12, 2021 at 06:40:20PM +0800, Dongliang Mu wrote: > > I agree with you and would like to modify "return 0" to "return err" > in the next version. Even if the parent function does not care about > the return value, it is still necessary to keep the right return > value. Well if the parent is ignoring the error then returning the error there obviously isn't going to help. 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
On Thu, Aug 12, 2021 at 6:43 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Thu, Aug 12, 2021 at 06:40:20PM +0800, Dongliang Mu wrote: > > > > I agree with you and would like to modify "return 0" to "return err" > > in the next version. Even if the parent function does not care about > > the return value, it is still necessary to keep the right return > > value. > > Well if the parent is ignoring the error then returning the error > there obviously isn't going to help. If so, do I still need to change my current patch? Dongliang Mu > > 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
On Mon, Aug 16, 2021 at 9:21 PM Dongliang Mu <mudongliangabcd@gmail.com> wrote: > > On Thu, Aug 12, 2021 at 6:43 PM Herbert Xu <herbert@gondor.apana.org.au> wrote: > > > > On Thu, Aug 12, 2021 at 06:40:20PM +0800, Dongliang Mu wrote: > > > > > > I agree with you and would like to modify "return 0" to "return err" > > > in the next version. Even if the parent function does not care about > > > the return value, it is still necessary to keep the right return > > > value. > > > > Well if the parent is ignoring the error then returning the error > > there obviously isn't going to help. > > If so, do I still need to change my current patch? Any update on this thread? > > Dongliang Mu > > > > > 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
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c index 88194718a806..05bb781da701 100644 --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-hash.c @@ -288,14 +288,14 @@ int sun8i_ce_hash_run(struct crypto_engine *engine, void *breq) buf = kzalloc(bs * 2, GFP_KERNEL | GFP_DMA); if (!buf) { err = -ENOMEM; - goto theend; + goto out; } bf = (__le32 *)buf; result = kzalloc(digestsize, GFP_KERNEL | GFP_DMA); if (!result) { err = -ENOMEM; - goto theend; + goto err_buf; } flow = rctx->flow; @@ -321,7 +321,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 +334,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 +342,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 +392,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 +405,18 @@ 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); -theend: - kfree(buf); +err_result: kfree(result); - crypto_finalize_hash_request(engine, breq, err); +err_buf: + kfree(buf); +out: + if (!err) + crypto_finalize_hash_request(engine, breq, err); return 0; }
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 freeing the objects allocated bydma_mmap_sg/single when errors occur. Fixes: 56f6d5aee88d ("crypto: sun8i-ce - support hash algorithms") Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- v1->v2: fix some wording and keep error handling code consistent .../crypto/allwinner/sun8i-ce/sun8i-ce-hash.c | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-)