linux-kernel - Re: [PATCH] of: fix of_update

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]

Date:	Fri, 17 Jan 2014 18:42:47 +0200
From:	Pantelis Antoniou <panto@...oniou-consulting.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Xiubo Li <Li.Xiubo@...escale.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] of: fix of_update_property()

Hi Rob,


On Jan 17, 2014, at 4:49 PM, Rob Herring wrote:

> On Thu, Jan 16, 2014 at 10:46 PM, Xiubo Li <Li.Xiubo@...escale.com> wrote:
>> The of_update_property() is intent to update a property in a node
> 
[ snip ]

>>                return of_add_property(np, newprop);
> 
> Isn't there also a race that if you do 2 updates for a non-existent
> property and both threads try to add the property, the first one will
> succeed and the 2nd will fail. The 2nd one needs to retry as well.
> 
> Also, couldn't the node itself be removed while trying to do the update?
> 
> There seem to be multiple problems with this code, but doing multiple
> simultaneous, conflicting updates seems like an unlikely case.
> 

There are multiple problems with this function.

First of all, the firing of the notifier at the beginning with OF_RECONFIG_UPDATE_PROPERTY
even though we have no idea if the property is found.

The locking is no good; the lock should be taken before the search by switching to using
__of_find_property. Threading is just not handled well at all at the moment. 
Retrying is just bogus.

The biggest problem is the semantics; IMHO we should just remove the option to create
a property if it doesn't exist. I don't think there are many callers that use update property
expecting to be created if it doesn't exist.

> Rob
> 
>> @@ -1593,7 +1594,7 @@ int of_update_property(struct device_node *np, struct property *newprop)
>>        raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> 
>>        if (!found)
>> -               return -ENODEV;
>> +               goto retry;
>> 
>> #ifdef CONFIG_PROC_DEVICETREE
>>        /* try to add to proc as well if it was initialized */
>> --
>> 1.8.4
>> 
>> 

Regards

-- Pantelis--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/