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: <CAF4BwTV=S39mUcbhoxCf6Z6+gGGFy5ctfYwbKt5yDMUYD5r3gw@mail.gmail.com>
Date:   Tue, 7 Nov 2023 07:00:20 -0500
From:   Daniel Berlin <dberlin@...rlin.org>
To:     Arend van Spriel <arend.vanspriel@...adcom.com>
Cc:     Hector Martin <marcan@...can.st>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        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 Tue, Nov 7, 2023 at 6:51 AM Arend van Spriel
<arend.vanspriel@...adcom.com> wrote:
>
> 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.

The EHT stuff you flagged (defines, etc) in the other patches can be
left out - this one can't since it is definitionally part of the v112
struct.