LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
@ 2021-08-03  6:31 Dongliang Mu
  2021-08-12 10:04 ` Herbert Xu
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-08-03  6:31 UTC (permalink / raw)
  To: Corentin Labbe, Herbert Xu, David S. Miller, Maxime Ripard,
	Chen-Yu Tsai, Jernej Skrabec, Eric Biggers, Xiang Chen,
	Mauro Carvalho Chehab, Dongliang Mu
  Cc: Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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(-)

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;
 }
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-03  6:31 [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run Dongliang Mu
@ 2021-08-12 10:04 ` Herbert Xu
  2021-08-12 10:24   ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2021-08-12 10:04 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-12 10:04 ` Herbert Xu
@ 2021-08-12 10:24   ` Dongliang Mu
  2021-08-12 10:29     ` Herbert Xu
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-08-12 10:24 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-12 10:24   ` Dongliang Mu
@ 2021-08-12 10:29     ` Herbert Xu
  2021-08-12 10:40       ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2021-08-12 10:29 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-12 10:29     ` Herbert Xu
@ 2021-08-12 10:40       ` Dongliang Mu
  2021-08-12 10:43         ` Herbert Xu
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-08-12 10:40 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-12 10:40       ` Dongliang Mu
@ 2021-08-12 10:43         ` Herbert Xu
  2021-08-16 13:21           ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Herbert Xu @ 2021-08-12 10:43 UTC (permalink / raw)
  To: Dongliang Mu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-12 10:43         ` Herbert Xu
@ 2021-08-16 13:21           ` Dongliang Mu
  2021-09-23  0:54             ` Dongliang Mu
  0 siblings, 1 reply; 8+ messages in thread
From: Dongliang Mu @ 2021-08-16 13:21 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run
  2021-08-16 13:21           ` Dongliang Mu
@ 2021-09-23  0:54             ` Dongliang Mu
  0 siblings, 0 replies; 8+ messages in thread
From: Dongliang Mu @ 2021-09-23  0:54 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Corentin Labbe, David S. Miller, Maxime Ripard, Chen-Yu Tsai,
	Jernej Skrabec, Eric Biggers, Xiang Chen, Mauro Carvalho Chehab,
	Corentin Labbe, Jason A. Donenfeld, Ard Biesheuvel, linux-crypto,
	linux-arm-kernel, linux-sunxi, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-09-23  0:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-03  6:31 [PATCH v2] crypto: sun8i-ce: fix multiple memory leaks in sun8i_ce_hash_run Dongliang Mu
2021-08-12 10:04 ` Herbert Xu
2021-08-12 10:24   ` Dongliang Mu
2021-08-12 10:29     ` Herbert Xu
2021-08-12 10:40       ` Dongliang Mu
2021-08-12 10:43         ` Herbert Xu
2021-08-16 13:21           ` Dongliang Mu
2021-09-23  0:54             ` Dongliang Mu

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).