linux-kernel - Re: [PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent

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: <e20db902-140d-d483-7f74-5bd7e0cf2703@prevas.dk>
Date:   Wed, 13 Dec 2017 16:12:23 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Hans de Goede <hdegoede@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Nicolas Iooss <nicolas.iooss_linux@....org>
CC:     <devel@...verdev.osuosl.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent

On 2017-12-13 15:49, Hans de Goede wrote:
> Hi,
> 
> On 13-12-17 12:47, Greg Kroah-Hartman wrote:
>> On Sun, Dec 10, 2017 at 08:35:12PM +0100, Nicolas Iooss wrote:
>>> rtw_pm_set() uses memcmp() with 5-chars strings and a length of 4 when
>>> parsing extra, and then parses extra+4 as an int:
>>>
>>>      if (!memcmp(extra, "lps =", 4)) {
>>>          sscanf(extra+4, "%u", &mode);
>>>      /* ... */
>>>      } else if (!memcmp(extra, "ips =", 4)) {
>>>          sscanf(extra+4, "%u", &mode);
>>>
>>> The space between the key ("lps" and "ips") and the equal sign seems
>>> suspicious. Remove it in order to make the calls to memcmp() consistent.
>>
>> But you now just changing the parsing logic.  What broke because of
>> this?  Did you test this codepath with your patch?
>>
>> I'm not disagreeing that this code seems really odd, but it must be
>> working as-is for someone, to change this logic will break their system
>> :(
> 
> I don't think this code can work actually, for the memcmp to
> match the extra argument must start with e.g. : "lps =" 

No, the extra argument just has to start with "lps ", so something like
"lps 1234" would "work". The memcmp call could just as well use "lps ".

but then mode
> gets read as: sscanf(extra+4, "%u", &mode);, with extra + 4
> pointing at the "=" in the extra arg, so sscanf will stop right
> away and store 0 in mode.

See above, we don't know there's a "=" at extra+4. But in any case,
I don't think sscanf stores anything if there are no digits (and then it
would return 0 since no specifiers matched - the code also lacks a check
of the sscanf return value). But mode is initialized, so it's not going
to use some stack garbage.

All in all, some cleanup seems warranted. Why not just do a sscanf("lps
%u", ...) call and properly check the return value of that? With
whatever prefix string one thinks would be most appropriate.

> So this is for a private extension to the iw interface. I think that
> as part of the cleanup of this driver in staging we should just
> remove all private extensions, which will nicely fix the broken
> function by simply removing it :)

Yeah, that would also work...

Rasmus