Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

[Philipp Hortmann]

On 3/18/24 09:01, Dan Carpenter wrote:
> On Sun, Mar 17, 2024 at 09:20:34PM +0100, Arnd Bergmann wrote:
> > On Sun, Mar 17, 2024, at 21:07, Philipp Hortmann wrote:
> > > On 3/11/24 08:04, Dan Carpenter wrote:
> > > > On Sat, Mar 09, 2024 at 11:09:24PM +0100, Philipp Hortmann wrote:
> > > You are right with the statement that it is this commit.
> > > commit ea82ff749587807fa48e3277c977ff3cec266f25 (HEAD)
> > > Author: Lee Jones <lee.jones@xxxxxxxxxx>
> > > Date:   Wed Apr 14 19:10:39 2021 +0100
> > >
> > >       staging: wlan-ng: cfg80211: Move large struct onto the heap
> > >
> > >       Fixes the following W=1 kernel build warning(s):
> > >
> > >        drivers/staging/wlan-ng/cfg80211.c: In function ‘prism2_scan’:
> > >        drivers/staging/wlan-ng/cfg80211.c:388:1: warning: the frame size
> > > of 1296 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > >
> > > But It is not depending on the line you pointed to.
> >
> > Right, the kzalloc() already clears the data, so the memset
> > is not needed.
>
> No, it's inside a loop so it needs to be cleared on each iteration.
>
> > > I need another week to look into this.
> >
> > I'm fairly sure this fixes the bug, the problem here was that
> > the cast to (u8 *) hides the incorrect conversion:
> >
> > diff --git a/drivers/staging/wlan-ng/cfg80211.c b/drivers/staging/wlan-ng/cfg80211.c
> > index 471bb310176f..9d6a2dd35ba9 100644
> > --- a/drivers/staging/wlan-ng/cfg80211.c
> > +++ b/drivers/staging/wlan-ng/cfg80211.c
> > @@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
> >   		msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
> >   		msg2->bssindex.data = i;
> >   
> > -		result = p80211req_dorequest(wlandev, (u8 *)&msg2);
> > +		result = p80211req_dorequest(wlandev, (u8 *)msg2);
>
> Ah, well done.
>
> It feels like this is the kind of bug which should be caught with
> static analysis.  One of the things that people want from static
> analysis is looking at what a patch does.  So if we pass &msg2 and the
> patch moved msg from the stack to be kmalloc()ed, then print a warning.
> It's not something that Smatch does.
>
> I have my rename_rev.pl script (https://github.com/error27/rename_rev)
> which I use to filter out variable renames or see if (1 << foo) is
> converted to BIT(foo) correctly.  Maybe I could extend that to check
> "move stack to heap" patches...
>
> regards,
> dan carpenter

Hi,

This change alone.

+++ b/drivers/staging/wlan-ng/cfg80211.c
@@ -350,7 +350,7 @@ static int prism2_scan(struct wiphy *wiphy,
                msg2->msgcode = DIDMSG_DOT11REQ_SCAN_RESULTS;
                msg2->bssindex.data = i;

-               result = p80211req_dorequest(wlandev, (u8 *)&msg2);
+               result = p80211req_dorequest(wlandev, (u8 *)msg2);
                if ((result != 0) ||
                    (msg2->resultcode.data != 
P80211ENUM_resultcode_success)) {
                        break;
makes the driver work again under latest kernel.

I can only use unencrypted transmission. Throughput is 200 to 800kByte/s
WEP would need further investigation.

Thanks for your support.

Bye Philipp