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

Philipp Hortmann
2024-03-18T17:07:40Z

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@linaro.org>
>>> 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