linux-kernel - Re: [PATCH] soc: qcom: smp2p: Add of_node

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: <20190805225810.GC6470@builder>
Date:   Mon, 5 Aug 2019 15:58:10 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Nishka Dasgupta <nishkadg.linux@...il.com>
Cc:     agross@...nel.org, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] soc: qcom: smp2p: Add of_node_put() at goto

On Sun 04 Aug 09:25 PDT 2019, Nishka Dasgupta wrote:

> Each iteration of for_each_available_child_of_node puts the previous
> node, but in the case of a goto from the middle of the loop, there is no
> put, thus causing a memory leak. Hence make the gotos within the loop
> first go to a new label where an of_node_put() puts the last used node,
> before falling through to the original label.
> Issue found with Coccinelle.
> 

Good catch, thanks for the patch!

> Signed-off-by: Nishka Dasgupta <nishkadg.linux@...il.com>
> ---
>  drivers/soc/qcom/smp2p.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
> index c7300d54e444..d223e914487d 100644
> --- a/drivers/soc/qcom/smp2p.c
> +++ b/drivers/soc/qcom/smp2p.c
> @@ -501,7 +501,7 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  		entry = devm_kzalloc(&pdev->dev, sizeof(*entry), GFP_KERNEL);
>  		if (!entry) {
>  			ret = -ENOMEM;
> -			goto unwind_interfaces;
> +			goto release_child;
>  		}
>  
>  		entry->smp2p = smp2p;
> @@ -509,18 +509,18 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  
>  		ret = of_property_read_string(node, "qcom,entry-name", &entry->name);
>  		if (ret < 0)
> -			goto unwind_interfaces;
> +			goto release_child;
>  
>  		if (of_property_read_bool(node, "interrupt-controller")) {
>  			ret = qcom_smp2p_inbound_entry(smp2p, entry, node);
>  			if (ret < 0)
> -				goto unwind_interfaces;
> +				goto release_child;
>  
>  			list_add(&entry->node, &smp2p->inbound);
>  		} else  {
>  			ret = qcom_smp2p_outbound_entry(smp2p, entry, node);
>  			if (ret < 0)
> -				goto unwind_interfaces;
> +				goto release_child;
>  
>  			list_add(&entry->node, &smp2p->outbound);
>  		}
> @@ -541,6 +541,8 @@ static int qcom_smp2p_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +release_child:
> +	of_node_put(node);

Following the loop over the children we request the irq and if that
falls we'll jump to unwind_interfaces. So while it would work fine to
jump to release_child within the loop and then unwind_interfaces after
the loop, it doesn't follow the idiomatic way of using the error path to
"unroll" things that has been setup up until a particular point in the
function.

So I would rather see that you of_node_put() in the loop and then jump
to unwind_interfaces as is done today.

Regards,
Bjorn

>  unwind_interfaces:
>  	list_for_each_entry(entry, &smp2p->inbound, node)
>  		irq_domain_remove(entry->domain);
> -- 
> 2.19.1
>