linux-kernel - Re: [PATCH v2 10/35] brcmfmac: firmware: Allow platform to override macaddr

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: <b4f50489-fa4b-2c40-31ad-1b74e916cdb4@marcan.st>
Date:   Wed, 5 Jan 2022 22:26:44 +0900
From:   Hector Martin <marcan@...can.st>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <lenb@...nel.org>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Chi-hsien Lin <chi-hsien.lin@...ineon.com>,
        Wright Feng <wright.feng@...ineon.com>,
        Dmitry Osipenko <digetx@...il.com>,
        Sven Peter <sven@...npeter.dev>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>,
        Mark Kettenis <kettenis@...nbsd.org>,
        Rafał Miłecki <zajec5@...il.com>,
        Pieter-Paul Giesberts <pieter-paul.giesberts@...adcom.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Hans de Goede <hdegoede@...hat.com>,
        "John W. Linville" <linville@...driver.com>,
        "brian m. carlson" <sandals@...stytoothpaste.net>,
        "open list:TI WILINK WIRELES..." <linux-wireless@...r.kernel.org>,
        netdev <netdev@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        "open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER" 
        <brcm80211-dev-list.pdl@...adcom.com>,
        SHA-cyfmac-dev-list@...ineon.com
Subject: Re: [PATCH v2 10/35] brcmfmac: firmware: Allow platform to override
 macaddr

On 04/01/2022 23.23, Andy Shevchenko wrote:
> On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@...can.st> wrote:
>>
>> On Device Tree platforms, it is customary to be able to set the MAC
>> address via the Device Tree, as it is often stored in system firmware.
>> This is particularly relevant for Apple ARM64 platforms, where this
>> information comes from system configuration and passed through by the
>> bootloader into the DT.
>>
>> Implement support for this by fetching the platform MAC address and
>> adding or replacing the macaddr= property in nvram. This becomes the
>> dongle's default MAC address.
>>
>> On platforms with an SROM MAC address, this overrides it. On platforms
>> without one, such as Apple ARM64 devices, this is required for the
>> firmware to boot (it will fail if it does not have a valid MAC at all).
> 
> ...
> 
>> +#define BRCMF_FW_MACADDR_FMT                   "macaddr=%pM"
>> +#define BRCMF_FW_MACADDR_LEN                   (7 + ETH_ALEN * 3)
> 
> ...
> 
>>                 if (strncmp(&nvp->data[nvp->entry], "boardrev", 8) == 0)
>>                         nvp->boardrev_found = true;
>> +               /* strip macaddr if platform MAC overrides */
>> +               if (nvp->strip_mac &&
>> +                   strncmp(&nvp->data[nvp->entry], "macaddr", 7) == 0)
> 
> If it has no side effects, I would rather swap the operands of && so
> you match string first (it will be in align with above code at least,
> although I haven't checked bigger context).

I usually check for trivial flags before calling more expensive
functions because it's more efficient in the common case. Obviously here
performance doesn't matter though.

> 
> ....
> 
>> +static void brcmf_fw_add_macaddr(struct nvram_parser *nvp, u8 *mac)
>> +{
>> +       snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1,
>> +                BRCMF_FW_MACADDR_FMT, mac);
> 
> Please, avoid using implict format string, it's dangerous from security p.o.v.

What do you mean by implicit format string? The format string is at the
top of the file and its length is right next to it, which makes it
harder for them to accidentally fall out of sync.

+#define BRCMF_FW_MACADDR_FMT			"macaddr=%pM"
+#define BRCMF_FW_MACADDR_LEN			(7 + ETH_ALEN * 3)

> 
>> +       nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1;
> 
> Also, with temporary variable the code can be better to read
> 
> size_t mac_len = ...;
> 

Sure.

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub