linux-kernel - Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

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]

Date:   Sun, 05 May 2019 10:20:33 +0200
From:   Arend Van Spriel <arend.vanspriel@...adcom.com>
To:     Victor Bravo <1905@...blk.com>
CC:     Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        "Chi-Hsien Lin" <chi-hsien.lin@...ress.com>,
        Wright Feng <wright.feng@...ress.com>,
        Kalle Valo <kvalo@...eaurora.org>,
        "David S. Miller" <davem@...emloft.net>,
        <linux-wireless@...r.kernel.org>,
        <brcm80211-dev-list.pdl@...adcom.com>,
        <brcm80211-dev-list@...ress.com>, <linux-kernel@...r.kernel.org>,
        Hans de Goede <hdegoede@...hat.com>
Subject: Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

On May 4, 2019 9:44:51 PM Victor Bravo <1905@...blk.com> wrote:

> On Sat, May 04, 2019 at 09:11:09PM +0200, Arend Van Spriel wrote:
>> + Hans, Luis
>> 
>> On 5/4/2019 6:26 PM, Victor Bravo wrote:
>> > The brcmfmac driver seems to have partially fixed problems which
>> > prevented it to be used in shared system/kernel images for multiple
>> > hardware by trying to load it's <config>.txt as
>> > <config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
>> > falling back to <config>.txt. Real-life example:
>> >
>> > brcmfmac mmc1:0001:1: Direct firmware load for 
>> brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed with
>> > error -2
>> > brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
>> > BCM43340/2
>> >
>> > Unfortunately this doesn't really help on systems which use static
>> > kernel with firmware blobs (and also text configuration files in case of
>> > brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
>> > doesn't support spaces in file names - kernel build fails with
>> >
>> > CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin 
>> brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"
>> >
>> > for obvious reasons. So the only way here is to stay with good old
>> > brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
>> > per kernel image.
>> >
>> > Please consider filtering the DMI strings and replacing spaces and
>> > possibly other invalid characters with underscores, and/or adding module
>> > parameter to allow passing the string from command line (using
>> > brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
>> > brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
>> > breaking when DMI strings change on BIOS update).
>> 
>> The intent of the DMI approach was to avoid end-users from passing module
>> parameters for this. As to fixing DMI string usage patches are welcome.
>
> Well I think I could also provide a patch to fix, this can be easily
> done by adding a string of allowed characters and then replacing
> unknown ones with underscores.
>
>> > My brief grep-based research also suggest that strings retrieved
>> > by dmi_get_system_info() are passed to firmware loader without any
>> > checks for special character, /../ etc. I'm not sure whether this is
>> > considered to be proper & safe use, but if it's not, it may also have
>> > some security implications, as it allows attacker with access to DMI
>> > strings (using root rights/other OS/BIOS/physical access) to mess
>> > with kernel space or secure boot.
>> 
>> Hmm. Attackers with that kind of access can do bad is a gazillion ways.
>
> Agreed. It will be definitely easier to make filenames contain only safe
> characters than to discuss those ways.
>
>> > I would also really appreciate not allowing future brcm (and other)
>> > drivers to leave staging area before they fully support =y.
>> 
>> Define fully support. At the time we moved into the wireless tree (almost a
>> decade ago) we did support =y. As such you could consider the DMI approach a
>> regression, but I find that a bit harsh to say. Hans made a honest attempt
>> and it is something that can be fixed. It can be you providing just that ;-)
>
> Well... I agree that the idea wasn't really complete ;).
>
> As for the patches, I also realized that the txt config file actually
> comes from EFI/BIOS, so it's quite possible that it may differ between
> BIOS versions. So I'm thinking of 3 patches here:
>
>   1) Character filtering as described above.
>
>   2) Adding bios_version next to board_type, and changing load order to
>
>     <config>.<dmi_sys_vendor>.<dmi_product_name>.<dmi_bios_version>.txt
>     <config>.<dmi_sys_vendor>.<dmi_product_name>.txt
>     <config>.txt
>
>   3) Adding command-line parameters to override these on problems.
>
> 1) breaks backward compatibility, but the DMI code seems to be quite
> new so hopefully many people don't rely on it yet.
>
> 2) & 3) are backward compatible.

Actually, the configuration file, or nvram file as we tend to call it, does 
not come from EFI/BIOS. There are a few platforms that have the nvram file 
stored in EFI and it's name is well-defined. It does assume there is only 
one brcmfmac device in the system.

Regards,
Arend