linux-kernel - Re: [PATCH 4/5] wifi: brcmfmac: Support bss

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <25e43cd3-45e6-d775-87c4-9ed7cdfe3e2d@broadcom.com>
Date:   Tue, 7 Nov 2023 12:51:17 +0100
From:   Arend van Spriel <arend.vanspriel@...adcom.com>
To:     Hector Martin <marcan@...can.st>,
        Daniel Berlin <dberlin@...rlin.org>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>
Cc:     linux-wireless@...r.kernel.org,
        brcm80211-dev-list.pdl@...adcom.com,
        SHA-cyfmac-dev-list@...ineon.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] wifi: brcmfmac: Support bss_info up to v112

On 11/7/2023 12:11 PM, Hector Martin wrote:
> On 20/10/2023 18.59, Arend van Spriel wrote:
>> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
>>> From: Hector Martin <marcan@...can.st>
>>>
>>> The structures are compatible and just add fields, so we can just treat
>>> it as always v112. If we start using new fields, that will have to be
>>> gated on the version.
>>
>> Seems EHT is creeping in here.
>>
>> Having doubts about compatibility statement (see below)...
>>
>>> Signed-off-by: Hector Martin <marcan@...can.st>
>>> ---
>>>    .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  5 ++-
>>>    .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 37 +++++++++++++++++--
>>>    2 files changed, 36 insertions(+), 6 deletions(-)
>>>

[...]

>>> @@ -323,28 +324,56 @@ struct brcmf_bss_info_le {
>>>    	__le16 capability;	/* Capability information */
>>>    	u8 SSID_len;
>>>    	u8 SSID[32];
>>> +	u8 bcnflags;		/* additional flags w.r.t. beacon */
>>
>> Ehm. Coming back to your statement "structures are compatible and just
>> add fields". How are they compatible? You now treat v109 struct as v112
>> so fields below are shifted because of bcnflags. So you read invalid
>> information. This does not fly or I am missing something here.
> 
> bcmflags was previously an implied padding byte. If you actually check
> the offsets of the subsequent fields, you'll see they haven't changed.
> In fact this was added at some point in the past and just missing here,
> and is a general case of "padding bytes were not explicitly specified"
> which is arguably an anti-pattern and should never have been the case.

Yeah. Let's not argue ;-) I did miss something here and leave it with 
that. What about the EHT stuff? I would prefer to keep it out unless 
full EHT support is added.

Regards,
Arend

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4219 bytes)