lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <20190506090609.msudhncj7e5vdtzw@localhost> Date: Mon, 6 May 2019 11:06:09 +0200 From: Victor Bravo <1905@...blk.com> To: Hans de Goede <hdegoede@...hat.com> Cc: Arend Van Spriel <arend.vanspriel@...adcom.com>, Franky Lin <franky.lin@...adcom.com>, Hante Meuleman <hante.meuleman@...adcom.com>, Chi-Hsien Lin <chi-hsien.lin@...ress.com>, Wright Feng <wright.feng@...ress.com>, Kalle Valo <kvalo@...eaurora.org>, "David S. Miller" <davem@...emloft.net>, linux-wireless@...r.kernel.org, brcm80211-dev-list.pdl@...adcom.com, brcm80211-dev-list@...ress.com, linux-kernel@...r.kernel.org Subject: Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2 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@...blk.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; > > } > > } > > >
Powered by blists - more mailing lists