Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2

[Hans de Goede]

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@xxxxxxxxxx>
> > > >
> > > > 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;
> > > > >     	}
> > > > >     }