Date: Wed, 13 Dec 2017 17:10:11 +0100
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Nicolas Iooss <nicolas.iooss_linux@m4x.org>,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] staging: rtl8723bs: make memcmp() calls consistent
Message-ID: <20171213161011.GA29460@kroah.com>
References: <20171210193512.6309-1-nicolas.iooss_linux@m4x.org>
 <20171213114728.GB21746@kroah.com>
 <723e10e4-8698-926d-c4ae-5ab92955d21c@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <723e10e4-8698-926d-c4ae-5ab92955d21c@redhat.com>
User-Agent: Mutt/1.9.1 (2017-09-22)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2184
Lines: 55

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