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

[Victor Bravo]

On Sun, May 05, 2019 at 10:20:33AM +0200, Arend Van Spriel wrote:
> On May 4, 2019 9:44:51 PM Victor Bravo <1905@xxxxxxxxxx> 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.

Good to know. Sounds like working nvram file should never need update
and changes related to BIOS version are not needed.

I've meanwhile prepared patch for character filtering, will post in
new thread w/ better subject.

Regards,
v.