linux-kernel - Re: [PATCH] of: Make of_node_name

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:   Thu, 24 Jan 2019 21:00:36 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Frank Rowand <frowand.list@...il.com>,
        linux-kernel@...r.kernel.org, Rob Herring <robh+dt@...nel.org>
Cc:     vivien.didelot@...il.com,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE" 
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH] of: Make of_node_name_eq() case insensitive



On 1/24/19 6:06 PM, Frank Rowand wrote:
> On 1/24/19 5:20 PM, Florian Fainelli wrote:
>>
>>
>> On 1/24/19 3:45 PM, Frank Rowand wrote:
>>> On 1/24/19 12:08 PM, Florian Fainelli wrote:
>>>> Since c32569e358ad ("regulator: Use of_node_name_eq for node name
>>>> comparisons") Vivien reported the mc13892-regulator complaining about
>>>> not being able to find regulators.
>>>>
>>>> This is because prior to that commit we used of_node_cmp() to compare
>>>> the regulator array passed from mc13892_regulators down to
>>>> mc13xxx_parse_regulators_dt() and they are all defined in uppercase
>>>> letters by the MC13892_*_DEFINE* macros, whereas they are defined as
>>>> lowercase in the DTS.
>>>>
>>>> Fix this by use strncasecmp() since that makes sure the comparison is
>>>> case insensitive like what of_node_cmp() did.
>>>>
>>>> Reported-by: Vivien Didelot <vivien.didelot@...il.com>
>>>> Fixes: c32569e358ad ("regulator: Use of_node_name_eq for node name comparisons")
>>>> Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
>>>> ---
>>>>  drivers/of/base.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>>>> index 5226e898476e..ff47c86277cb 100644
>>>> --- a/drivers/of/base.c
>>>> +++ b/drivers/of/base.c
>>>> @@ -66,7 +66,8 @@ bool of_node_name_eq(const struct device_node *np, const char *name)
>>>>  	node_name = kbasename(np->full_name);
>>>>  	len = strchrnul(node_name, '@') - node_name;
>>>>  
>>>> -	return (strlen(name) == len) && (strncmp(node_name, name, len) == 0);
>>>> +	return (strlen(name) == len) &&
>>>> +		(strncasecmp(node_name, name, len) == 0);
>>>>  }
>>>>  EXPORT_SYMBOL(of_node_name_eq);
>>>>  
>>>>
>>>
>>> Node names are case sensitive.  Please fix mc13xxx_parse_regulators_dt() to
>>> properly handle case instead of changing of_node_name_eq().
>>
>> Fair enough, should we issue a warning if np->full_name contains upper
>> case while name does not (and vice versa) to help troubleshoot cases
>> like the one we found with Vivien?
> 
> It seems like a lot of work to detect that specific case.  If anything,
> maybe just add some text to the existing "Unknown regulator: ..." warning
> in mc13xxx_parse_regulators_dt() to mention that case matters.

I was not thinking about something very clever, just issue a warning
like this, completely untested and likely flawed:

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5226e898476e..2505286c875b 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -58,6 +58,7 @@ DEFINE_RAW_SPINLOCK(devtree_lock);
 bool of_node_name_eq(const struct device_node *np, const char *name)
 {
        const char *node_name;
+       int ret1, ret2;
        size_t len;

        if (!np)
@@ -66,7 +67,12 @@ bool of_node_name_eq(const struct device_node *np,
const char *name)
        node_name = kbasename(np->full_name);
        len = strchrnul(node_name, '@') - node_name;

-       return (strlen(name) == len) && (strncmp(node_name, name, len)
== 0);
+       ret1 = strncmp(node_name, name, len);
+       ret2 = strncasecmp(node_name, len);
+
+       WARN(ret1 ^ ret2, "Comparing case sensitive names!");
+
+       return (strlen(name) == len && (strncmp(node_name, name, len) == 0);
 }
 EXPORT_SYMBOL(of_node_name_eq);


My concern is that we have identified one place here where the
conversion to of_node_name_eq() broke that particular driver in fact,
all other regulator drivers but qcom-rpmh-regulator.c that use
of_node_name_eq() are broken after that change, but presumably this is
not the only place in the kernel where things could break, so having a
warning could potentially help (also adding the backtrace which is neat).

What should the fix look like though? Add an of_node_casename_eq() and
use it, revert Rob's commit that changes these regulators to use
of_node_name_eq()?

I don't know the regulator framework enough to know whether forcibly
making the names lowercase is not breaking sysfs/debugfs...
-- 
Florian