linux-kernel - Re: [PATCH] of: resolver: Add missing 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] [thread-next>] [day] [month] [year] [list]

Message-Id: <FE79FA58-A084-47BE-B872-7D57AC2B4D75@konsulko.com>
Date:	Wed, 27 Jan 2016 18:14:00 +0200
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Amitoj Kaur Chawla <amitoj1606@...il.com>,
	Rob Herring <robh+dt@...nel.org>,
	Frank Rowand <frowand.list@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Devicetree List <devicetree@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	julia.lawall@...6.fr
Subject: Re: [PATCH] of: resolver: Add missing of_node_put

Hi Mark,

> On Jan 27, 2016, at 18:05 , Mark Rutland <mark.rutland@....com> wrote:
> 
> On Wed, Jan 27, 2016 at 08:50:17PM +0530, Amitoj Kaur Chawla wrote:
>> for_each_child_of_node performs an of_node_get on each iteration, so
>> to break out of the loop an of_node_put is required.
>> 
>> Found using Coccinelle. The semantic patch used for this is as follows:
>> 
>> // <smpl>
>> @@
>> expression e;
>> local idexpression n;
>> @@
>> 
>> for_each_child_of_node(..., n) {
>>   ... when != of_node_put(n)
>>       when != e = n
>> (
>>   return n;
>> |
>> +  of_node_put(n);
>> ?  return ...;
>> )
>>   ...
>> }
>> // </smpl
>> 
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@...il.com>
>> ---
>> drivers/of/resolver.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> index 640eb4c..e2a0143 100644
>> --- a/drivers/of/resolver.c
>> +++ b/drivers/of/resolver.c
>> @@ -40,8 +40,10 @@ static struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> 
>> 	for_each_child_of_node(node, child) {
>> 		found = __of_find_node_by_full_name(child, full_name);
>> -		if (found != NULL)
>> +		if (found != NULL) {
>> +			of_node_put(child);
>> 			return found;
>> +		}
>> 	}
>> 
>> 	return NULL;
> 
> I don't think this is quite right. When child == found, this change will
> leave it decremented.
> 


This patch is bogus. 

__of_find_node_by_full_name() is not taking a reference on the node if found. 
This method relies on keeping the reference taken by the loop.

Taking this into account all of these conccinelle tests are bogus.

The DT internal method are not using the object model in an obvious manner
and applying these patches without vetting each and everyone is bound to
break things.

Regards

— Pantelis


> Thanks,
> Mark.