[PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent

[PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent

[Nicolas Iooss]

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.

Signed-off-by: Nicolas Iooss <nicolas.iooss_linux@m4x.org>
---
 drivers/staging/rtl8723bs/os_dep/ioctl_linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
index 3fca0c2d4c8d..ffcfefefc898 100644
--- a/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
+++ b/drivers/staging/rtl8723bs/os_dep/ioctl_linux.c
@@ -4561,10 +4561,10 @@ static int rtw_pm_set(struct net_device *dev,
 
 	DBG_871X("[%s] extra = %s\n", __func__, extra);
 
-	if (!memcmp(extra, "lps =", 4)) {
+	if (!memcmp(extra, "lps=", 4)) {
 		sscanf(extra+4, "%u", &mode);
 		ret = rtw_pm_set_lps(padapter, mode);
-	} else if (!memcmp(extra, "ips =", 4)) {
+	} else if (!memcmp(extra, "ips=", 4)) {
 		sscanf(extra+4, "%u", &mode);
 		ret = rtw_pm_set_ips(padapter, mode);
 	} else {
-- 
2.15.0

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

[Greg Kroah-Hartman]

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
:(

thanks,

greg k-h

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

[Hans de Goede]

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

The rtw_pm_set function is only used in the rtw_private_handler[]
function pointer array, which itself is used here:

struct iw_handler_def rtw_handlers_def = {
         .standard = rtw_handlers,
         .num_standard = ARRAY_SIZE(rtw_handlers),
#if defined(CONFIG_WEXT_PRIV)
         .private = rtw_private_handler,
         .private_args = (struct iw_priv_args *)rtw_private_args,
         .num_private = ARRAY_SIZE(rtw_private_handler),
         .num_private_args = ARRAY_SIZE(rtw_private_args),
#endif
         .get_wireless_stats = rtw_get_wireless_stats,
};

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 :)

Regards,

Hans

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

[Rasmus Villemoes]

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

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

[Hans de Goede]

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

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

[Greg Kroah-Hartman]

On Wed, Dec 13, 2017 at 03:49:12PM +0100, 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 =" 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.
> 
> The rtw_pm_set function is only used in the rtw_private_handler[]
> function pointer array, which itself is used here:
> 
> struct iw_handler_def rtw_handlers_def = {
>         .standard = rtw_handlers,
>         .num_standard = ARRAY_SIZE(rtw_handlers),
> #if defined(CONFIG_WEXT_PRIV)
>         .private = rtw_private_handler,
>         .private_args = (struct iw_priv_args *)rtw_private_args,
>         .num_private = ARRAY_SIZE(rtw_private_handler),
>         .num_private_args = ARRAY_SIZE(rtw_private_args),
> #endif
>         .get_wireless_stats = rtw_get_wireless_stats,
> };
> 
> 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 :)

Yes, any private extensions should just be deleted.

thanks,

greg k-h