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

From: Arend Van Spriel
Date: Sat May 04 2019 - 15:11:49 EST

+ 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

Unfortunately this doesn't really help on systems which use static
kernel with firmware blobs (and also text configuration files in case of
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.

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.

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 ;-)