Subject: Re: [PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent
To: Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Nicolas Iooss <nicolas.iooss_linux@m4x.org>
Cc: devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
References: <20171210193512.6309-1-nicolas.iooss_linux@m4x.org>
 <20171213114728.GB21746@kroah.com>
 <723e10e4-8698-926d-c4ae-5ab92955d21c@redhat.com>
 <e20db902-140d-d483-7f74-5bd7e0cf2703@prevas.dk>
From: Hans de Goede <hdegoede@redhat.com>
Message-ID: <5039dd0c-96c4-a97a-4f71-48325968c0f9@redhat.com>
Date: Wed, 13 Dec 2017 17:00:45 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.5.0
MIME-Version: 1.0
In-Reply-To: <e20db902-140d-d483-7f74-5bd7e0cf2703@prevas.dk>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2379
Lines: 62

Hi,

On 13-12-17 16:12, Rasmus Villemoes wrote:
> 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 ".

Ah yes, you're right, it only compares the first 4 chars.

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

Either one is fine with me.

Regards,

Hans