linux-kernel - Re: [PATCH] of: of_mdio: Fix some endianness problems.

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]

Date:	Mon, 1 Nov 2010 10:20:48 -0400
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Sergei Shtylyov <sshtylyov@...sta.com>
Cc:	David Daney <ddaney@...iumnetworks.com>, linux-mips@...ux-mips.org,
	ralf@...ux-mips.org, devicetree-discuss@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org,
	Jeremy Kerr <jeremy.kerr@...onical.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Dan Carpenter <error27@...il.com>,
	Greg Kroah-Hartman <gregkh@...e.de>
Subject: Re: [PATCH] of: of_mdio: Fix some endianness problems.

On Mon, Nov 1, 2010 at 7:44 AM, Sergei Shtylyov <sshtylyov@...sta.com> wrote:
> Hello.
>
> On 31-10-2010 5:54, Grant Likely wrote:
>
>>>> In of_mdiobus_register(), the __be32 *addr variable is dereferenced.
>>>> This will not work on little-endian targets.  Also since it is
>>>> unsigned, checking for less than zero is redundant.
>
>>>> Fix these two issues.
>
>>>> Signed-off-by: David Daney<ddaney@...iumnetworks.com>
>>>> Cc: Grant Likely<grant.likely@...retlab.ca>
>>>> Cc: Jeremy Kerr<jeremy.kerr@...onical.com>
>>>> Cc: Benjamin Herrenschmidt<benh@...nel.crashing.org>
>>>> Cc: Dan Carpenter<error27@...il.com>
>>>> Cc: Greg Kroah-Hartman<gregkh@...e.de>
>>>> ---
>>>>  drivers/of/of_mdio.c |   23 ++++++++++++++---------
>>>>  1 files changed, 14 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
>>>> index 1fce00e..b370306 100644
>>>> --- a/drivers/of/of_mdio.c
>>>> +++ b/drivers/of/of_mdio.c
>>>> @@ -52,27 +52,32 @@ int of_mdiobus_register(struct mii_bus *mdio, struct
>>>> device_node *np)
>>>>
>>>>       /* Loop over the child nodes and register a phy_device for each
>>>> one */
>>>>       for_each_child_of_node(np, child) {
>>>> -             const __be32 *addr;
>>>> +             const __be32 *paddr;
>>>> +             u32 addr;
>>>>               int len;
>>>>
>>>>               /* A PHY must have a reg property in the range [0-31] */
>>>> -             addr = of_get_property(child, "reg",&len);
>>>> -             if (!addr || len<  sizeof(*addr) || *addr>= 32 || *addr<
>>>>  0) {
>>>> +             paddr = of_get_property(child, "reg",&len);
>>>> +             if (!paddr || len<  sizeof(*paddr)) {
>>>> +addr_err:
>>>>                       dev_err(&mdio->dev, "%s has invalid PHY
>>>> address\n",
>>>>                               child->full_name);
>>>>                       continue;
>>>>               }
>>>> +             addr = be32_to_cpup(paddr);
>>>> +             if (addr>= 32)
>>>> +                     goto addr_err;
>
>>> goto's are fine for jumping to the end of a function to unwind
>>> allocations, but please don't use it in this manner.  The original
>>> structure will actually work just fine if you do it thusly:
>
>>>                if (!paddr || len<  sizeof(*paddr) ||
>>>                    *(addr = be32_to_cpup(paddr))>= 32) {
>>>                        dev_err(&mdio->dev, "%s has invalid PHY
>>> address\n",
>>>                                child->full_name);
>>>                        continue;
>>>                }
>
>>> Otherwise this patch looks good. After you've reworked and retested
>>> I'll pick it up for 2.6.37 (or dave will).
>
>> Actually, I mistyped this.  I think it should be:
>>
>>                if (!paddr || len<  sizeof(*paddr) ||
>>                    (addr = be32_to_cpup(paddr))>= 32) {
>
>  This assignment would probably cause checkpatch.pl to complain...

checkpatch isn't always right.  Alternately, I'd also be okay with
David's approach of splitting the tests into two if() blocks if
without the goto...

Actually, don't worry about it.  I just merged David's patch and
removed the goto myself in the process.

g.
--
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/