Messages in this thread | | | Subject | Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2 | From | Hans de Goede <> | Date | Mon, 6 May 2019 12:34:17 +0200 |
| |
Hi,
On 06-05-19 12:20, Victor Bravo wrote: > On Mon, May 06, 2019 at 11:33:20AM +0200, Hans de Goede wrote: >> Hi, >> >> On 06-05-19 11:06, Victor Bravo wrote: >>> On Mon, May 06, 2019 at 10:13:38AM +0200, Hans de Goede wrote: >>>> Hi, >>> >>> Hi, >>> >>>> On 05-05-19 17:03, Victor Bravo wrote: >>>>> Sanitize DMI strings in brcmfmac driver to make resulting filenames >>>>> contain only safe characters. This version replaces all non-printable >>>>> characters incl. delete (0-31, 127-255), spaces and slashes with >>>>> underscores. >>>>> >>>>> This change breaks backward compatibility, but adds control over strings >>>>> passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE >>>>> which doesn't support spaces in filenames. >>>>> >>>>> Changes from v1: don't revert fresh commit by someone else >>>>> >>>>> Signed-off-by: Victor Bravo <1905@spmblk.com> >>>> >>>> Thank you for the patch, but I'm sorry to say this patch cannot go in as is, >>>> because it will break existing systems. >>>> >>>> If you look here: >>>> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm >>>> >>>> You will see a file named: "brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt" there, which >>>> has a space in its name (and which works fine). >>> >>> Thanks for the updates. Spaces are actually a problem as files with spaces >>> don't work when built-in with CONFIG_EXTRA_FIRMWARE (which is used with >>> non-modular kernel containing brcmfmac driver). >>> >>> If the DMI string contains slashes, they will cause problems >>> for obvious reasons too. >> >> Right, as said I'm fine with sanitizing the names, so dropping e.g. / chars, >> but replacing space with _ will cause wifi to stop working on Onda V80 Plus devices and >> we have a clear no regressions policy in the kernel. > > Please keep in mind that dropping slashes and non-printable characters > might be a regression too, as some users may already be using > intermediate directories or filenames with tabs etc. if their DMI > strings contain these. If such users exist, they will have to rename > their nvram files anyway.
I consider it very unlikely that such users exist OTOH the Onda V80 PLus nvram file is actually in linux-firmware upstream, so that e.g. Fedora 30 has working wifi OOTB on the V80 Plus, if we go with your proposal to also replace spaces, then users with a V80 Plus will all of a sudden have their wifi stop working.
If Kalle and Arend are in favor of including space in the "bad character" list then at a minimum we must first get a patch added to linux-firmware adding an ONDA-V80_PLUS.txt symlink to the ONDA-V80 PLUS.txt file.
> On the other hand, removing just slashes and non-printable characters > (i.e. (*dst < 0x20) || (dst == '/') || (dst == 0x7f)) would allow > the sanitize function to the bit array and go with hardcoded conditions > (especially if those are final and won't need further adjustments in > set of allowed characters
If we're going to do some filtering, then I suggest we play it safe and also disallow other chars which may be used as a separator somewhere, specifically ':' and ','.
Currently upstream linux-firmware has these files which rely on the DMI matching:
brcmfmac4330-sdio.Prowise-PT301.txt brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt
The others are either part of the DMI override table for devices with unsuitable DMI strings like "Default String"; or are device-tree based.
So as long as we don't break those 3 (or break the ONDA one but get a symlink in place) we can sanitize a bit more then just non-printable and '/'.
Kalle, Arend, what is your opinion on this?
Note I do not expect the ONDA V80 Plus to have a lot of Linux users, but it definitely has some.
Regards,
Hans
> >>>> I'm fine with doing some sanitizing of the strings, but replacing spaces with _ >>>> breaks existing use-cases (will cause a regression for them) and a space is absolutely >>>> a valid character in a filename and the firmware-loader can deal with this just fine. >>>> >>>> If the code for building firmwares into the kernel cannot deal with spaces then IMHO >>>> that code should be fixed instead. Have you looked into fixing that? >>> >>> Yes, but updating CONFIG_EXTRA_FIRMWARE to support spaces because of >>> this looks much like >> >> <snip off-topic remark> >> >>> Do you really think it's a good idea to propose that in >>> this case? >> >> I think expecting spaces in filenames to just work is quite reasonable, after all >> its been a long time since we've left DOS-es 8.3 filename limitations. >> >> Have you actually looked at how hard it would be to make filenames with spaces work >> with CONFIG_EXTRA_FIRMWARE ? >> >> No matter how you spin it, the space problem is a CONFIG_EXTRA_FIRMWARE bug, not an >> issue with the brcmfmac code. > > Well CONFIG_EXTRA_FIRMWARE is defined to use space as filename > separator, so I don't really dare to call that a bug. Also brcmfmac > seems to be only driver requiring support for spaces etc. in firmware > file names, and this requirement seems to be quite fresh. > > So to me the proper way to fix this via CONFIG_EXTRA_FIRMWARE seems to > be > > 1) wait some time until brcfmac's fw filenames with spaces become > de-facto standard and distros are literally full of these. > > 2) after this happens, propose update of CONFIG_EXTRA_FIRMWARE to > support spaces in filenames, arguing with status quo which coming from 1) > > Unfortunately I feel more like a programmer and this seems more like > politics, so I can promise participation in the wait part only right > now. > >>>> As for your T100HA example from earlier in this thread, the brcmfmac driver now >>>> also supports getting the firmware from a special EFI nvram variable, which the >>>> T100HA sets, so you do not need to provide a nvram file on the T100HA and things >>>> will still work. >>> >>> I don't really get this. Can you please suggest how do I make the driver >>> use something different than "brcmfmac43340-sdio.txt" or >>> "brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt" on T100HAN? >> >> If you leave out either file, then with a recent kernel you should see this >> brcm_info trigger: >> >> brcmf_info("Using nvram EFI variable\n"); >> >> So you should see this message when you do: >> >> dmesg | grep "Using nvram EFI variable" >> >> And the wifi on the T100HAN should just work, without needing to do any >> manual config / provide an nvram file in anyway. >> >> I always strive to make hardware just work with Linux and any UEFI x86 machine >> using brcmfmac which provides the necessary nvram EFI variable in its firmware >> should now just work when booting say a Fedora 30 livecd. >> >> The EFI nvram var support has been tested successfully on the following models: >> >> Acer Iconia Tab8 w1-8 >> Acer One 10 >> Asus T100CHI >> Asus T100HA >> Asus T100TA >> Asus T200TA >> Lenovo Mixx 2 8 >> Lenovo Yoga2 tablet 10 > > Thanks! Wasn't aware of this, will try in the evening. > > Regards, > v. > >> Regards, >> >> Hans >> >> >> >>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c >>>>> index 7535cb0d4ac0..84571e09b465 100644 >>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c >>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c >>>>> @@ -23,6 +23,14 @@ >>>>> /* The DMI data never changes so we can use a static buf for this */ >>>>> static char dmi_board_type[128]; >>>>> +/* Array of 128 bits representing 7-bit characters allowed in DMI strings. */ >>>>> +static unsigned char brcmf_dmi_allowed_chars[] = { >>>>> + 0x00, 0x00, 0x00, 0x00, 0xfe, 0x7f, 0xff, 0xff, >>>>> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f >>>>> +}; >>>>> + >>>>> +#define BRCMF_DMI_SAFE_CHAR '_' >>>>> + >>>>> struct brcmf_dmi_data { >>>>> u32 chip; >>>>> u32 chiprev; >>>>> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = { >>>>> {} >>>>> }; >>>>> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe) >>>>> +{ >>>>> + while (*dst) { >>>>> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8)))) >>>> >>>> At a first look I have no clue what this code is doing and I honestly do not feel >>>> like figuring it out, this is clever, but IMHO not readable. >>> >>> Understood. The cluless part actually checks corresponding bit >>> in allowed array, which is a bit mask describing what characters >>> are allowed or not. >>> >>>> Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc, >>>> so that a human can actually see in one look what the code is doing. >>>> >>>> You may want to wait for Arend to give his opinion before changing this though, >>>> maybe he likes the code as is. >>>> >>>> Also note that that should be < 0x20 of course, since we need to preserve spaces >>>> as is to avoid a regression. >>> >>> This has been already discussed, spaces are a problem. There even was an >>> opinion that adding the code that doesn't bother with spaces and slashes >>> might be a regression as well. >>> >>> Regards, >>> >>> v. >>> >>>> Regards, >>>> >>>> Hans >>>> >>>> >>>> >>>> >>>> >>>>> + *dst = safe; >>>>> + dst++; >>>>> + } >>>>> +} >>>>> + >>>>> void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev) >>>>> { >>>>> const struct dmi_system_id *match; >>>>> @@ -126,6 +143,9 @@ void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev) >>>>> if (sys_vendor && product_name) { >>>>> snprintf(dmi_board_type, sizeof(dmi_board_type), "%s-%s", >>>>> sys_vendor, product_name); >>>>> + brcmf_dmi_sanitize(dmi_board_type, >>>>> + brcmf_dmi_allowed_chars, >>>>> + BRCMF_DMI_SAFE_CHAR); >>>>> settings->board_type = dmi_board_type; >>>>> } >>>>> } >>>>> >>>> >>
| |