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

[Victor Bravo <1905@spmblk.com>]

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.

> 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 fixing systemd-caused unitialized urandom reads on
kernel side. Do you really think it's a good idea to propose that in
this case?

> 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?

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