[PATCH] crypto: crypto4xx: fix a potential double free in ppc4xx_trng_probe

[PATCH] crypto: crypto4xx: fix a potential double free in ppc4xx_trng_probe

[Wen Yang]

There is a possible double free issue in ppc4xx_trng_probe():

85:	dev->trng_base = of_iomap(trng, 0);
86:	of_node_put(trng);          ---> released here
87:	if (!dev->trng_base)
88:		goto err_out;
...
110:	ierr_out:
111:		of_node_put(trng);  ---> double released here
...

This issue was detected by using the Coccinelle software.
We fix it by removing the unnecessary of_node_put().

Fixes: 5343e674f32 ("crypto4xx: integrate ppc4xx-rng into crypto4xx")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Allison Randal <allison@lohutok.net>
Cc: Armijn Hemel <armijn@tjaldur.nl>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: linux-crypto@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/crypto/amcc/crypto4xx_trng.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/crypto/amcc/crypto4xx_trng.c b/drivers/crypto/amcc/crypto4xx_trng.c
index 02a6bed3..f10a87e 100644
--- a/drivers/crypto/amcc/crypto4xx_trng.c
+++ b/drivers/crypto/amcc/crypto4xx_trng.c
@@ -108,7 +108,6 @@ void ppc4xx_trng_probe(struct crypto4xx_core_device *core_dev)
 	return;
 
 err_out:
-	of_node_put(trng);
 	iounmap(dev->trng_base);
 	kfree(rng);
 	dev->trng_base = NULL;
-- 
2.9.5

Re: [PATCH] crypto: crypto4xx: fix a potential double free in ppc4xx_trng_probe

[Julia Lawall]

On Mon, 8 Jul 2019, Wen Yang wrote:

> There is a possible double free issue in ppc4xx_trng_probe():
>
> 85:	dev->trng_base = of_iomap(trng, 0);
> 86:	of_node_put(trng);          ---> released here
> 87:	if (!dev->trng_base)
> 88:		goto err_out;
> ...
> 110:	ierr_out:
> 111:		of_node_put(trng);  ---> double released here
> ...
>
> This issue was detected by using the Coccinelle software.
> We fix it by removing the unnecessary of_node_put().
>
> Fixes: 5343e674f32 ("crypto4xx: integrate ppc4xx-rng into crypto4xx")
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Armijn Hemel <armijn@tjaldur.nl>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org

Acked-by: Julia Lawall <julia.lawall@lip6.fr>


> ---
>  drivers/crypto/amcc/crypto4xx_trng.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/crypto/amcc/crypto4xx_trng.c b/drivers/crypto/amcc/crypto4xx_trng.c
> index 02a6bed3..f10a87e 100644
> --- a/drivers/crypto/amcc/crypto4xx_trng.c
> +++ b/drivers/crypto/amcc/crypto4xx_trng.c
> @@ -108,7 +108,6 @@ void ppc4xx_trng_probe(struct crypto4xx_core_device *core_dev)
>  	return;
>
>  err_out:
> -	of_node_put(trng);
>  	iounmap(dev->trng_base);
>  	kfree(rng);
>  	dev->trng_base = NULL;
> --
> 2.9.5
>
>

Re: Coccinelle: Checking the deletion of duplicate of_node_put() calls with SmPL

[Markus Elfring]

> 110:	ierr_out:

> 111:		of_node_put(trng);  ---> double released here

> ...


>
> This issue was detected by using the Coccinelle software.

Such a detection of a questionable source code place can be nice and helpful.

I constructed another script variant for the semantic patch language.

@deletion@
expression x;
identifier target;
@@
 of_node_put(x);
 if (...)
    goto target;
 ... when any
 target:
-of_node_put(x);


I observe then that this adjustment approach can generate the desired patch
for a source code extract.

elfring@Sonne:~/Projekte/Coccinelle/Probe> spatch ../janitor/delete_duplicate_of_node_put1.cocci crypto4xx_trng-excerpt1.c

…
-	of_node_put(trng);

…


But I wonder at the moment why it does not work (as expected) for the original
complete source file.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/crypto/amcc/crypto4xx_trng.c?id=5ad18b2e60b75c7297a998dea702451d33a052ed#n71
https://elixir.bootlin.com/linux/v5.2/source/drivers/crypto/amcc/crypto4xx_trng.c#L71

I am curious on further software development ideas.

Regards,
Markus

Re: Coccinelle: Checking the deletion of duplicate of_node_put() calls with SmPL

[Markus Elfring]

> But I wonder at the moment why it does not work (as expected) for the original
> complete source file.

I discovered that a diff hunk (or usable patch?) is generated
if the return statement is deleted (or commented out) before the jump label
which refers to a potentially unwanted function call at the mentioned place.
How will the support evolve for automatic adjustment of such source code
combinations by the semantic patch language (Coccinelle software)?

Regards,
Markus

Re: [PATCH] crypto: crypto4xx: fix a potential double free in ppc4xx_trng_probe

[Herbert Xu]

On Mon, Jul 08, 2019 at 02:19:03PM +0800, Wen Yang wrote:
> There is a possible double free issue in ppc4xx_trng_probe():
> 
> 85:	dev->trng_base = of_iomap(trng, 0);
> 86:	of_node_put(trng);          ---> released here
> 87:	if (!dev->trng_base)
> 88:		goto err_out;
> ...
> 110:	ierr_out:
> 111:		of_node_put(trng);  ---> double released here
> ...
> 
> This issue was detected by using the Coccinelle software.
> We fix it by removing the unnecessary of_node_put().
> 
> Fixes: 5343e674f32 ("crypto4xx: integrate ppc4xx-rng into crypto4xx")
> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Allison Randal <allison@lohutok.net>
> Cc: Armijn Hemel <armijn@tjaldur.nl>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: linux-crypto@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/crypto/amcc/crypto4xx_trng.c | 1 -
>  1 file changed, 1 deletion(-)

Patch applied.  Thanks.
-- 
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