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: <723e10e4-8698-926d-c4ae-5ab92955d21c@redhat.com>
Date:   Wed, 13 Dec 2017 15:49:12 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     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

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


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ