linux-kernel - Re: [PATCH 1/4] regulator: qcom_spmi: Fix warning Bad 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: <20180716142310.GQ10204@localhost>
Date:   Mon, 16 Jul 2018 16:23:10 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Niklas Cassel <niklas.cassel@...aro.org>
Cc:     Johan Hovold <johan@...nel.org>, broonie@...nel.org,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/4] regulator: qcom_spmi: Fix warning Bad of_node_put()

On Mon, Jul 16, 2018 at 03:29:31PM +0200, Niklas Cassel wrote:
> On Mon, Jul 16, 2018 at 02:01:34PM +0200, Johan Hovold wrote:
> > Hi Niklas,
> > 
> > On Mon, Jul 16, 2018 at 01:35:22PM +0200, Niklas Cassel wrote:
> > > For of_find_node_by_name(), you typically pass what the previous call
> > > returned. Therefore, of_find_node_by_name() increases the refcount of
> > > the returned node, and decreases the refcount of the node passed as the
> > > first argument.
> > > 
> > > However, in this case we don't pass what the previous call returned,
> > > so we have to increase the refcount of the first argument to compensate.
> > 
> > I don't think this is the right fix. of_find_node_by_name() should
> > generally not be used by drivers in the first place as it searches the
> > entire tree and can end up matching an entirely unrelated node.
> > 
> > I haven't looked at the device-tree binding in question, but you
> > probably want to use something like of_get_child_by_name() instead.
> > 
> 
> Hello Johan,
> 
> of_find_node_by_name() will only search the whole tree if the
> first argument is NULL, which isn't the case here.

It's searching the entire tree *starting* at its first argument, which
means you may end up matching a completely unrelated node (i.e. not a
child or even descendant) elsewhere in the tree.

> However, of_get_child_by_name() is indeed better suited here.
> Will send out a v2.

Unless you are doing a tree-wide search, using of_get_child_by_name() is
simply wrong.

I fixed up most of these bugs a few releases ago, but they keep on
creeping in.

> Thank you for your feedback, it is much appreciated :)

No worries.

Johan