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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 18 Mar 2024 18:07:34 +0100
From: Philipp Hortmann <philipp.g.hortmann@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>, Arnd Bergmann <arnd@...db.de>
Cc: Lee Jones <lee@...nel.org>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Larry Finger <Larry.Finger@...inger.net>,
 Johannes Berg <johannes@...solutions.net>, Kalle Valo <kvalo@...nel.org>,
 Julia Lawall <julia.lawall@...ia.fr>
Subject: Re: [RFC] staging: wlan-ng: Driver broken since kernel 5.15

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@...aro.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