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
| ||
|
Date: Wed, 13 Dec 2017 17:00:45 +0100 From: Hans de Goede <hdegoede@...hat.com> To: Rasmus Villemoes <rasmus.villemoes@...vas.dk>, 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 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
Powered by blists - more mailing lists