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: <CAHp75VdzQhkj3ovFSAG4g1tD1scBK7H0xFFot0rfz2u6i8a3FA@mail.gmail.com>
Date:   Thu, 6 Jan 2022 16:20:05 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Hector Martin <marcan@...can.st>
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 Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@...can.st> wrote:
> On 04/01/2022 23.23, Andy Shevchenko wrote:
> > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@...can.st> wrote:

...

> >> +#define BRCMF_FW_MACADDR_FMT                   "macaddr=%pM"

> >> +       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?

When I read the above code I feel uncomfortable because no-one can see
(without additional action and more reading and checking) if it's
correct or not. This is potential to be error prone.

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

It is not an argument. Just you may do the same in the code directly
and more explicitly:

Also you don't check the return code of snprintf which means that you
don't care about the result, which seems to me wrong approach. If you
don't care about the result, so it means it's not very important,
right?

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



-- 
With Best Regards,
Andy Shevchenko