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

From: Hans de Goede
Date: Wed Dec 13 2017 - 11:00:54 EST


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