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 <[email protected]>
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).
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?
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.
> 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.
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.
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;
> }
> }
>
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 <[email protected]>
>
> 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;
> > }
> > }
> >
>