linux-kernel - Re: [PATCH v2] powerpc/8xx: fix possible object reference leak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87d0mfp6w6.fsf@concordia.ellerman.id.au>
Date:   Mon, 25 Mar 2019 21:47:53 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Peng Hao <peng.hao2@....com.cn>, vitb@...nel.crashing.org,
        benh@...nel.crashing.org, paulus@...ba.org, christophe.leroy@....fr
Cc:     Wen Yang <wen.yang99@....com.cn>, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] powerpc/8xx: fix possible object reference leak

Peng Hao <peng.hao2@....com.cn> writes:

> From: Wen Yang <wen.yang99@....com.cn>
>
> The call to of_find_compatible_node returns a node pointer with refcount
> incremented thus it must be explicitly decremented after the last
> usage.
> irq_domain_add_linear also calls of_node_get to increase refcount,
> so irq_domain will not be affected when it is released.
>
> Detected by coccinelle with the following warnings:
> ./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 136, but without a corresponding object release within this function.
>
> Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
> revmap-specific initializers")
> Signed-off-by: Wen Yang <wen.yang99@....com.cn>
> Suggested-by: Christophe Leroy <christophe.leroy@....fr>
> Reviewed-by: Peng Hao <peng.hao2@....com.cn>
> Cc: Vitaly Bordug <vitb@...nel.crashing.org>
> Cc: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Cc: Paul Mackerras <paulus@...ba.org>
> Cc: Michael Ellerman <mpe@...erman.id.au>
> Cc: linuxppc-dev@...ts.ozlabs.org
> Cc: linux-kernel@...r.kernel.org
> ---
>  arch/powerpc/platforms/8xx/pic.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
> index 8d5a25d..4453df6 100644
> --- a/arch/powerpc/platforms/8xx/pic.c
> +++ b/arch/powerpc/platforms/8xx/pic.c
> @@ -153,9 +153,7 @@ int mpc8xx_pic_init(void)
>  	if (mpc8xx_pic_host == NULL) {
>  		printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>  		ret = -ENOMEM;
> -		goto out;
>  	}
> -	return 0;

It's fragile to rely on ret being equal to zero here. If the code
further up is changed it's easy enough to miss that ret is expected to
be zero.

Better to set it to zero here explicitly, this is the success path after
all, eg:

	ret = 0;

>  out:
>  	of_node_put(np);


cheers