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

From: Hans de Goede
Date: Wed Dec 13 2017 - 09:49:21 EST


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