PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

[Victor Bravo]

The brcmfmac driver seems to have partially fixed problems which
prevented it to be used in shared system/kernel images for multiple
hardware by trying to load it's <config>.txt as
<config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
falling back to <config>.txt. Real-life example:

brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed with
error -2
brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
BCM43340/2

Unfortunately this doesn't really help on systems which use static
kernel with firmware blobs (and also text configuration files in case of
brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
doesn't support spaces in file names - kernel build fails with

CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"

for obvious reasons. So the only way here is to stay with good old
brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
per kernel image.

Please consider filtering the DMI strings and replacing spaces and
possibly other invalid characters with underscores, and/or adding module
parameter to allow passing the string from command line (using
brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
breaking when DMI strings change on BIOS update).

My brief grep-based research also suggest that strings retrieved
by dmi_get_system_info() are passed to firmware loader without any
checks for special character, /../ etc. I'm not sure whether this is
considered to be proper & safe use, but if it's not, it may also have
some security implications, as it allows attacker with access to DMI
strings (using root rights/other OS/BIOS/physical access) to mess
with kernel space or secure boot.

I would also really appreciate not allowing future brcm (and other)
drivers to leave staging area before they fully support =y.

Regards,
v.b.

Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

[Arend Van Spriel]

+ Hans, Luis

On 5/4/2019 6:26 PM, Victor Bravo wrote:
> The brcmfmac driver seems to have partially fixed problems which
> prevented it to be used in shared system/kernel images for multiple
> hardware by trying to load it's <config>.txt as
> <config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
> falling back to <config>.txt. Real-life example:
> 
> brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed with
> error -2
> brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
> BCM43340/2
> 
> Unfortunately this doesn't really help on systems which use static
> kernel with firmware blobs (and also text configuration files in case of
> brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
> doesn't support spaces in file names - kernel build fails with
> 
> CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"
> 
> for obvious reasons. So the only way here is to stay with good old
> brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
> per kernel image.
> 
> Please consider filtering the DMI strings and replacing spaces and
> possibly other invalid characters with underscores, and/or adding module
> parameter to allow passing the string from command line (using
> brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
> brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
> breaking when DMI strings change on BIOS update).

The intent of the DMI approach was to avoid end-users from passing 
module parameters for this. As to fixing DMI string usage patches are 
welcome.

> My brief grep-based research also suggest that strings retrieved
> by dmi_get_system_info() are passed to firmware loader without any
> checks for special character, /../ etc. I'm not sure whether this is
> considered to be proper & safe use, but if it's not, it may also have
> some security implications, as it allows attacker with access to DMI
> strings (using root rights/other OS/BIOS/physical access) to mess
> with kernel space or secure boot.

Hmm. Attackers with that kind of access can do bad is a gazillion ways.

> I would also really appreciate not allowing future brcm (and other)
> drivers to leave staging area before they fully support =y.

Define fully support. At the time we moved into the wireless tree 
(almost a decade ago) we did support =y. As such you could consider the 
DMI approach a regression, but I find that a bit harsh to say. Hans made 
a honest attempt and it is something that can be fixed. It can be you 
providing just that ;-)

Regards,
Arend

Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

[Victor Bravo]

On Sat, May 04, 2019 at 09:11:09PM +0200, Arend Van Spriel wrote:
> + Hans, Luis
> 
> On 5/4/2019 6:26 PM, Victor Bravo wrote:
> > The brcmfmac driver seems to have partially fixed problems which
> > prevented it to be used in shared system/kernel images for multiple
> > hardware by trying to load it's <config>.txt as
> > <config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
> > falling back to <config>.txt. Real-life example:
> > 
> > brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed with
> > error -2
> > brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
> > BCM43340/2
> > 
> > Unfortunately this doesn't really help on systems which use static
> > kernel with firmware blobs (and also text configuration files in case of
> > brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
> > doesn't support spaces in file names - kernel build fails with
> > 
> > CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"
> > 
> > for obvious reasons. So the only way here is to stay with good old
> > brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
> > per kernel image.
> > 
> > Please consider filtering the DMI strings and replacing spaces and
> > possibly other invalid characters with underscores, and/or adding module
> > parameter to allow passing the string from command line (using
> > brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
> > brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
> > breaking when DMI strings change on BIOS update).
> 
> The intent of the DMI approach was to avoid end-users from passing module
> parameters for this. As to fixing DMI string usage patches are welcome.

Well I think I could also provide a patch to fix, this can be easily
done by adding a string of allowed characters and then replacing
unknown ones with underscores.

> > My brief grep-based research also suggest that strings retrieved
> > by dmi_get_system_info() are passed to firmware loader without any
> > checks for special character, /../ etc. I'm not sure whether this is
> > considered to be proper & safe use, but if it's not, it may also have
> > some security implications, as it allows attacker with access to DMI
> > strings (using root rights/other OS/BIOS/physical access) to mess
> > with kernel space or secure boot.
> 
> Hmm. Attackers with that kind of access can do bad is a gazillion ways.

Agreed. It will be definitely easier to make filenames contain only safe
characters than to discuss those ways.

> > I would also really appreciate not allowing future brcm (and other)
> > drivers to leave staging area before they fully support =y.
> 
> Define fully support. At the time we moved into the wireless tree (almost a
> decade ago) we did support =y. As such you could consider the DMI approach a
> regression, but I find that a bit harsh to say. Hans made a honest attempt
> and it is something that can be fixed. It can be you providing just that ;-)

Well... I agree that the idea wasn't really complete ;).

As for the patches, I also realized that the txt config file actually
comes from EFI/BIOS, so it's quite possible that it may differ between
BIOS versions. So I'm thinking of 3 patches here:

  1) Character filtering as described above.

  2) Adding bios_version next to board_type, and changing load order to

    <config>.<dmi_sys_vendor>.<dmi_product_name>.<dmi_bios_version>.txt
    <config>.<dmi_sys_vendor>.<dmi_product_name>.txt
    <config>.txt

  3) Adding command-line parameters to override these on problems.

1) breaks backward compatibility, but the DMI code seems to be quite
new so hopefully many people don't rely on it yet.

2) & 3) are backward compatible.

Regards,
v.

Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

[Arend Van Spriel]

On May 4, 2019 9:44:51 PM Victor Bravo <1905@spmblk.com> wrote:

> On Sat, May 04, 2019 at 09:11:09PM +0200, Arend Van Spriel wrote:
>> + Hans, Luis
>> 
>> On 5/4/2019 6:26 PM, Victor Bravo wrote:
>> > The brcmfmac driver seems to have partially fixed problems which
>> > prevented it to be used in shared system/kernel images for multiple
>> > hardware by trying to load it's <config>.txt as
>> > <config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
>> > falling back to <config>.txt. Real-life example:
>> >
>> > brcmfmac mmc1:0001:1: Direct firmware load for 
>> brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed with
>> > error -2
>> > brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
>> > BCM43340/2
>> >
>> > Unfortunately this doesn't really help on systems which use static
>> > kernel with firmware blobs (and also text configuration files in case of
>> > brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
>> > doesn't support spaces in file names - kernel build fails with
>> >
>> > CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin 
>> brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"
>> >
>> > for obvious reasons. So the only way here is to stay with good old
>> > brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
>> > per kernel image.
>> >
>> > Please consider filtering the DMI strings and replacing spaces and
>> > possibly other invalid characters with underscores, and/or adding module
>> > parameter to allow passing the string from command line (using
>> > brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
>> > brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
>> > breaking when DMI strings change on BIOS update).
>> 
>> The intent of the DMI approach was to avoid end-users from passing module
>> parameters for this. As to fixing DMI string usage patches are welcome.
>
> Well I think I could also provide a patch to fix, this can be easily
> done by adding a string of allowed characters and then replacing
> unknown ones with underscores.
>
>> > My brief grep-based research also suggest that strings retrieved
>> > by dmi_get_system_info() are passed to firmware loader without any
>> > checks for special character, /../ etc. I'm not sure whether this is
>> > considered to be proper & safe use, but if it's not, it may also have
>> > some security implications, as it allows attacker with access to DMI
>> > strings (using root rights/other OS/BIOS/physical access) to mess
>> > with kernel space or secure boot.
>> 
>> Hmm. Attackers with that kind of access can do bad is a gazillion ways.
>
> Agreed. It will be definitely easier to make filenames contain only safe
> characters than to discuss those ways.
>
>> > I would also really appreciate not allowing future brcm (and other)
>> > drivers to leave staging area before they fully support =y.
>> 
>> Define fully support. At the time we moved into the wireless tree (almost a
>> decade ago) we did support =y. As such you could consider the DMI approach a
>> regression, but I find that a bit harsh to say. Hans made a honest attempt
>> and it is something that can be fixed. It can be you providing just that ;-)
>
> Well... I agree that the idea wasn't really complete ;).
>
> As for the patches, I also realized that the txt config file actually
> comes from EFI/BIOS, so it's quite possible that it may differ between
> BIOS versions. So I'm thinking of 3 patches here:
>
>   1) Character filtering as described above.
>
>   2) Adding bios_version next to board_type, and changing load order to
>
>     <config>.<dmi_sys_vendor>.<dmi_product_name>.<dmi_bios_version>.txt
>     <config>.<dmi_sys_vendor>.<dmi_product_name>.txt
>     <config>.txt
>
>   3) Adding command-line parameters to override these on problems.
>
> 1) breaks backward compatibility, but the DMI code seems to be quite
> new so hopefully many people don't rely on it yet.
>
> 2) & 3) are backward compatible.

Actually, the configuration file, or nvram file as we tend to call it, does 
not come from EFI/BIOS. There are a few platforms that have the nvram file 
stored in EFI and it's name is well-defined. It does assume there is only 
one brcmfmac device in the system.

Regards,
Arend

Re: PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader

[Victor Bravo]

On Sun, May 05, 2019 at 10:20:33AM +0200, Arend Van Spriel wrote:
> On May 4, 2019 9:44:51 PM Victor Bravo <1905@spmblk.com> wrote:
> 
> > On Sat, May 04, 2019 at 09:11:09PM +0200, Arend Van Spriel wrote:
> > > + Hans, Luis
> > > 
> > > On 5/4/2019 6:26 PM, Victor Bravo wrote:
> > > > The brcmfmac driver seems to have partially fixed problems which
> > > > prevented it to be used in shared system/kernel images for multiple
> > > > hardware by trying to load it's <config>.txt as
> > > > <config>.<dmi_sys_vendor>.<dmi_product_name>.txt first and then
> > > > falling back to <config>.txt. Real-life example:
> > > >
> > > > brcmfmac mmc1:0001:1: Direct firmware load for
> > > brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt failed
> > > with
> > > > error -2
> > > > brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac43340-sdio for chip
> > > > BCM43340/2
> > > >
> > > > Unfortunately this doesn't really help on systems which use static
> > > > kernel with firmware blobs (and also text configuration files in case of
> > > > brcmfmac) built-in using CONFIG_EXTRA_FIRMWARE, as CONFIG_EXTRA_FIRMWARE
> > > > doesn't support spaces in file names - kernel build fails with
> > > >
> > > > CONFIG_EXTRA_FIRMWARE="brcm/brcmfmac43340-sdio.bin
> > > brcm/brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt"
> > > >
> > > > for obvious reasons. So the only way here is to stay with good old
> > > > brcmfmac43340-sdio.txt and support at most one brcmfmac-equipped machine
> > > > per kernel image.
> > > >
> > > > Please consider filtering the DMI strings and replacing spaces and
> > > > possibly other invalid characters with underscores, and/or adding module
> > > > parameter to allow passing the string from command line (using
> > > > brcmfmac.tag=t100 or brcmfmac.board=t100 to make the module load
> > > > brcmfmac43340-sdio.t100.txt seems nicer to me, and isn't prone to
> > > > breaking when DMI strings change on BIOS update).
> > > 
> > > The intent of the DMI approach was to avoid end-users from passing module
> > > parameters for this. As to fixing DMI string usage patches are welcome.
> > 
> > Well I think I could also provide a patch to fix, this can be easily
> > done by adding a string of allowed characters and then replacing
> > unknown ones with underscores.
> > 
> > > > My brief grep-based research also suggest that strings retrieved
> > > > by dmi_get_system_info() are passed to firmware loader without any
> > > > checks for special character, /../ etc. I'm not sure whether this is
> > > > considered to be proper & safe use, but if it's not, it may also have
> > > > some security implications, as it allows attacker with access to DMI
> > > > strings (using root rights/other OS/BIOS/physical access) to mess
> > > > with kernel space or secure boot.
> > > 
> > > Hmm. Attackers with that kind of access can do bad is a gazillion ways.
> > 
> > Agreed. It will be definitely easier to make filenames contain only safe
> > characters than to discuss those ways.
> > 
> > > > I would also really appreciate not allowing future brcm (and other)
> > > > drivers to leave staging area before they fully support =y.
> > > 
> > > Define fully support. At the time we moved into the wireless tree (almost a
> > > decade ago) we did support =y. As such you could consider the DMI approach a
> > > regression, but I find that a bit harsh to say. Hans made a honest attempt
> > > and it is something that can be fixed. It can be you providing just that ;-)
> > 
> > Well... I agree that the idea wasn't really complete ;).
> > 
> > As for the patches, I also realized that the txt config file actually
> > comes from EFI/BIOS, so it's quite possible that it may differ between
> > BIOS versions. So I'm thinking of 3 patches here:
> > 
> >   1) Character filtering as described above.
> > 
> >   2) Adding bios_version next to board_type, and changing load order to
> > 
> >     <config>.<dmi_sys_vendor>.<dmi_product_name>.<dmi_bios_version>.txt
> >     <config>.<dmi_sys_vendor>.<dmi_product_name>.txt
> >     <config>.txt
> > 
> >   3) Adding command-line parameters to override these on problems.
> > 
> > 1) breaks backward compatibility, but the DMI code seems to be quite
> > new so hopefully many people don't rely on it yet.
> > 
> > 2) & 3) are backward compatible.
> 
> Actually, the configuration file, or nvram file as we tend to call it, does
> not come from EFI/BIOS. There are a few platforms that have the nvram file
> stored in EFI and it's name is well-defined. It does assume there is only
> one brcmfmac device in the system.

Good to know. Sounds like working nvram file should never need update
and changes related to BIOS version are not needed.

I've meanwhile prepared patch for character filtering, will post in
new thread w/ better subject.

Regards,
v.

[PATCH RFC] brcmfmac: sanitize DMI strings

[Victor Bravo]

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.

Signed-off-by: Victor Bravo <1905@spmblk.com>

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
index 7535cb0d4ac0..fa654ce7172b 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;
@@ -43,10 +51,6 @@ static const struct brcmf_dmi_data meegopad_t08_data = {
 	BRCM_CC_43340_CHIP_ID, 2, "meegopad-t08"
 };
 
-static const struct brcmf_dmi_data pov_tab_p1006w_data = {
-	BRCM_CC_43340_CHIP_ID, 2, "pov-tab-p1006w-data"
-};
-
 static const struct dmi_system_id dmi_platform_data[] = {
 	{
 		/* Match for the GPDwin which unfortunately uses somewhat
@@ -85,20 +89,18 @@ static const struct dmi_system_id dmi_platform_data[] = {
 		},
 		.driver_data = (void *)&meegopad_t08_data,
 	},
-	{
-		/* Point of View TAB-P1006W-232 */
-		.matches = {
-			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Insyde"),
-			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "BayTrail"),
-			/* Note 105b is Foxcon's USB/PCI vendor id */
-			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "105B"),
-			DMI_EXACT_MATCH(DMI_BOARD_NAME, "0E57"),
-		},
-		.driver_data = (void *)&pov_tab_p1006w_data,
-	},
 	{}
 };
 
+void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
+{
+	while (*dst) {
+		if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
+			*dst = safe;
+		dst++;
+	}
+}
+
 void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
 {
 	const struct dmi_system_id *match;
@@ -126,6 +128,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;
 	}
 }

Re: [PATCH RFC] brcmfmac: sanitize DMI strings

[Victor Bravo]

Didn't notice that the patch removes some fresh changes which got in
while I was tuning it. Please wait for v2. Sorry for the noise.
v.

On Sun, May 05, 2019 at 04:48:52PM +0200, 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.
> 
> Signed-off-by: Victor Bravo <1905@spmblk.com>
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> index 7535cb0d4ac0..fa654ce7172b 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;
> @@ -43,10 +51,6 @@ static const struct brcmf_dmi_data meegopad_t08_data = {
>  	BRCM_CC_43340_CHIP_ID, 2, "meegopad-t08"
>  };
>  
> -static const struct brcmf_dmi_data pov_tab_p1006w_data = {
> -	BRCM_CC_43340_CHIP_ID, 2, "pov-tab-p1006w-data"
> -};
> -
>  static const struct dmi_system_id dmi_platform_data[] = {
>  	{
>  		/* Match for the GPDwin which unfortunately uses somewhat
> @@ -85,20 +89,18 @@ static const struct dmi_system_id dmi_platform_data[] = {
>  		},
>  		.driver_data = (void *)&meegopad_t08_data,
>  	},
> -	{
> -		/* Point of View TAB-P1006W-232 */
> -		.matches = {
> -			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Insyde"),
> -			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "BayTrail"),
> -			/* Note 105b is Foxcon's USB/PCI vendor id */
> -			DMI_EXACT_MATCH(DMI_BOARD_VENDOR, "105B"),
> -			DMI_EXACT_MATCH(DMI_BOARD_NAME, "0E57"),
> -		},
> -		.driver_data = (void *)&pov_tab_p1006w_data,
> -	},
>  	{}
>  };
>  
> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
> +{
> +	while (*dst) {
> +		if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
> +			*dst = safe;
> +		dst++;
> +	}
> +}
> +
>  void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
>  {
>  	const struct dmi_system_id *match;
> @@ -126,6 +128,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;
>  	}
>  }
> 

-- 
NOTE FOR WINDOWS (TM) USERS:  IN NO EVENT UNLESS REQUIRED BY APPLICABLE
LAW WILL I BE LIABLE TO YOU FOR ANY SW OR HW DAMAGE, SYSTEM MALFUNCTION,
DATA LOSS AND/OR DATA BREACH ARISING OUT WHILE YOU ARE READING THIS NOTE.

[PATCH RFC] brcmfmac: sanitize DMI strings v2

[Victor Bravo]

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>

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))))
+			*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;
 	}
 }

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

[Hans de Goede]

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).

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

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

[Kalle Valo]

Hans de Goede <hdegoede@redhat.com> writes:

>> @@ -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.

Is there an existing function for sanitising filenames so that we don't
need to reinvent the wheel, maybe something like isalnum()?

-- 
Kalle Valo

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

[Kalle Valo]

Victor Bravo <1905@spmblk.com> writes:

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

The version should be in brackets "[PATCH RFC v2]" and the change log
after "---" line:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

-- 
Kalle Valo

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

[Victor Bravo]

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

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

[Victor Bravo]

On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> >> @@ -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.
> 
> Is there an existing function for sanitising filenames so that we don't
> need to reinvent the wheel, maybe something like isalnum()?

I would definitely prefer to use existing function, but I didn't find
any suitable one. Suggestions are welcome.

As for implementation details, the one I posted was optimized for both
speed and size, and at least in my opinion this bit array driven
parametric implementation is exactly what is needed here (using a string
of allowed characters with strchr-style lookups would bring much worse
complexity, and checking the characters using series of hardcoded if
conditions could quickly grow to more than those 16 bytes used by the
array).

Regards,
v.

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

[Hans de Goede]

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.

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

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

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

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

[Victor Bravo]

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.

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

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

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

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

[Kalle Valo]

Hans de Goede <hdegoede@redhat.com> writes:

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

To me having spaces in filenames is a bad idea, but on the other hand we
do have the "don't break existing setups" rule, so it's not so simple. I
vote for not allowing spaces, I think that's the best for the long run,
but don't know what Arend thinks.

Maybe we could do some kind of fallback mechanism, like first trying the
sanitised filename and if that's not found then we try the old filename
with spaces? And if that old filename works we print a big fat warning
that the user should update the file and that the old "filename with
spaces" support is going away soon?

-- 
Kalle Valo

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

[Kalle Valo]

Victor Bravo <1905@spmblk.com> writes:

> On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>> 
>> >> @@ -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.
>> 
>> Is there an existing function for sanitising filenames so that we don't
>> need to reinvent the wheel, maybe something like isalnum()?
>
> I would definitely prefer to use existing function, but I didn't find
> any suitable one. Suggestions are welcome.

I didn't find anything either, but hopefully someone knows.

> As for implementation details, the one I posted was optimized for both
> speed and size, and at least in my opinion this bit array driven
> parametric implementation is exactly what is needed here (using a string
> of allowed characters with strchr-style lookups would bring much worse
> complexity, and checking the characters using series of hardcoded if
> conditions could quickly grow to more than those 16 bytes used by the
> array).

But is this really something which should be optimised? This is driver
initialisation, not in some hot path, right? Can you even measure the
difference?

-- 
Kalle Valo

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

[Victor Bravo]

On Mon, May 06, 2019 at 03:29:21PM +0300, Kalle Valo wrote:
> Victor Bravo <1905@spmblk.com> writes:
> 
> > On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
> >> Hans de Goede <hdegoede@redhat.com> writes:
> >> 
> >> >> @@ -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.
> >> 
> >> Is there an existing function for sanitising filenames so that we don't
> >> need to reinvent the wheel, maybe something like isalnum()?
> >
> > I would definitely prefer to use existing function, but I didn't find
> > any suitable one. Suggestions are welcome.
> 
> I didn't find anything either, but hopefully someone knows.
> 
> > As for implementation details, the one I posted was optimized for both
> > speed and size, and at least in my opinion this bit array driven
> > parametric implementation is exactly what is needed here (using a string
> > of allowed characters with strchr-style lookups would bring much worse
> > complexity, and checking the characters using series of hardcoded if
> > conditions could quickly grow to more than those 16 bytes used by the
> > array).
> 
> But is this really something which should be optimised? This is driver
> initialisation, not in some hot path, right? Can you even measure the
> difference?

I didn't measure it, but according to my understanding of "Linux Kernel
Contributor Covenant Code of Conduct Interpretation" this actually
seems to be right according to "everyone involved wants to see the best
possible solution for the overall success of Linux" (but it also depends
on defintion of "best possible" of course).

Honestly, my exact thought behind this was that switch() or if
conditions shouldn't be used here (except simple case handling few
characters) because of above rule.

I also definitely want to avoid reinventing the wheel or even having
duplicate code in the kernel, so I tried to write the routine in a way
that allows it to be used as kernel library function usable in any
driver unless we manage to find existing wheel of similar parameters
elsewhere. That's why allowed and safe are passed as arguments even if
they are available as global varible / macro.

The bitmask approach also still seems useful as it allows to easily
handle any set of allowed characters, while it seems that the set
of unwanted characters may be extended to even more than my patch
handles.

So *in case no suitable existing sanitizing function is found*, the
approach IMHO might be also

1) fine-tuning this kind of sanitizing routine and making it a kernel
"library" function outside the driver, possibly with some useful macros
- e.g. parametric macro to check whether a value is present in a set
(which could also help to make this check more readable as suggested by
Hans), and I would also really like a macro for compile-time initializon
of the bit array from string literal containing allowed characters
(which unfortunately doesn't seem possible in C yet).

2) having only brcmf_dmi_allowed_chars and BRCMF_DMI_SAFE_CHAR in the
driver and use these with the "library" function.

Unfortunately I'm new to kernel development and have no clue whether
it's a good idea to start with such a function inside driver and
eventually moving it later, or start with getting the function in final
location first and delay driver modificatios until it's ready.

Regards,
v.

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

[Victor Bravo]

On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
> 
> > 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.
> 
> To me having spaces in filenames is a bad idea, but on the other hand we
> do have the "don't break existing setups" rule, so it's not so simple. I
> vote for not allowing spaces, I think that's the best for the long run,
> but don't know what Arend thinks.

I have found a fresh judicate on this:
https://lkml.org/lkml/2018/12/22/221

It seems clear that we have to support at least spaces for some time
(maybe wih separate config option which will be deprecated but on by
defaut until old files are considered gone).

> Maybe we could do some kind of fallback mechanism, like first trying the
> sanitised filename and if that's not found then we try the old filename
> with spaces? And if that old filename works we print a big fat warning
> that the user should update the file and that the old "filename with
> spaces" support is going away soon?

In case of parametric sanitizing function, this might be achievable
by sanitizing using "final" character set first, and falling back
to "compatible" character set on file not found. So this may actually
bring another requirement on the sanitizing function.

Regards,
v.

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

[Hans de Goede]

Hi,

On 06-05-19 17:24, Victor Bravo wrote:
> On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>> 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.
>>
>> To me having spaces in filenames is a bad idea, but on the other hand we
>> do have the "don't break existing setups" rule, so it's not so simple. I
>> vote for not allowing spaces, I think that's the best for the long run,
>> but don't know what Arend thinks.
> 
> I have found a fresh judicate on this:
> https://lkml.org/lkml/2018/12/22/221
> 
> It seems clear that we have to support at least spaces for some time
> (maybe wih separate config option which will be deprecated but on by
> defaut until old files are considered gone).

Ah that issue, well that is not really comparable in that case a lot of
peoples setups were completely broken by that commit and it was a
quite surprising behavior change in a userspace facing API.

The nvram loading path already does 2 tries, I really don't want to
unnecessary complicate it with a third try.

The Onda V80 Plus is a X86 based Windows / Android dual boot tablet,
as said before I do not expect a ton of users to be running regular
Linux on it.

Given Kalle's clear preference for getting rid of the spaces lets
just do that. But first we must get a symlink added to linux-firmware
using the name with the _, newer kernels requiring a newer linux-firmware
to match is not unheard of AFAIK, so combined with the limited amount
of users I think this is a reasonable compromise.

Kalle, do you agree with getting the symlink added to linux-firmware
ASAP as a fix for the V80 Plus issue; or do you want to see a fallback
to the un-cleaned name as you suggested before ?

Regards,

Hans

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

[Arend Van Spriel]

+ Luis (for real this time)

On 5/6/2019 6:05 PM, Hans de Goede wrote:
> Hi,
> 
> On 06-05-19 17:24, Victor Bravo wrote:
>> On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
>>> Hans de Goede <hdegoede@redhat.com> writes:
>>>
>>>> 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.
>>>
>>> To me having spaces in filenames is a bad idea, but on the other hand we
>>> do have the "don't break existing setups" rule, so it's not so simple. I
>>> vote for not allowing spaces, I think that's the best for the long run,
>>> but don't know what Arend thinks.

Hi,

Had a day off today so I did see some of the discussion, but was not 
able to chime in until now.

To be honest I always disliked spaces in filenames, but that does not 
necessarily make it a bad idea. What I would like to know is why 
built-in firmware can not deal with spaces in the firmware file names. I 
think Hans mentioned it in the thread and it crossed my mind as well 
last night. From driver perspective, being brcmfmac or any other for 
that matter, there is only one API to request firmware and in my opinion 
it should behave the same no matter where the firmware is coming from. I 
would prefer to fix that for built-in firmware, but we need to 
understand where this limitation is coming from. Hopefully Luis can 
elaborate on that.

>>
>> I have found a fresh judicate on this:
>> https://lkml.org/lkml/2018/12/22/221
>>
>> It seems clear that we have to support at least spaces for some time
>> (maybe wih separate config option which will be deprecated but on by
>> defaut until old files are considered gone).
> 
> Ah that issue, well that is not really comparable in that case a lot of
> peoples setups were completely broken by that commit and it was a
> quite surprising behavior change in a userspace facing API.
> 
> The nvram loading path already does 2 tries, I really don't want to
> unnecessary complicate it with a third try.
> 
> The Onda V80 Plus is a X86 based Windows / Android dual boot tablet,
> as said before I do not expect a ton of users to be running regular
> Linux on it.
> 
> Given Kalle's clear preference for getting rid of the spaces lets
> just do that. But first we must get a symlink added to linux-firmware
> using the name with the _, newer kernels requiring a newer linux-firmware
> to match is not unheard of AFAIK, so combined with the limited amount
> of users I think this is a reasonable compromise.

Right. In the brcm folder we have bcm4329-fullmac-4.bin for older 
kernels and brcmfmac4329-sdio.bin for later kernels when we switched to 
stricter firmware naming convention.

> Kalle, do you agree with getting the symlink added to linux-firmware
> ASAP as a fix for the V80 Plus issue; or do you want to see a fallback
> to the un-cleaned name as you suggested before ?

How many releases have an issue and how many V80 Plus users running 
regular linux are actually using built-in firmware. And is it really a 
regression for them? Not saying it does not require fixing. However, as 
stated above I would prefer to fix the built-in firmware limitation if 
possible and backport that fix if it is only a few kernel releases 
provided stable allows such a backport.

Regards,
Arend

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

[Hans de Goede]

Hi,

On 06-05-19 21:30, Arend Van Spriel wrote:
> + Luis (for real this time)
> 
> On 5/6/2019 6:05 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 06-05-19 17:24, Victor Bravo wrote:
>>> On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
>>>> Hans de Goede <hdegoede@redhat.com> writes:
>>>>
>>>>> 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.
>>>>
>>>> To me having spaces in filenames is a bad idea, but on the other hand we
>>>> do have the "don't break existing setups" rule, so it's not so simple. I
>>>> vote for not allowing spaces, I think that's the best for the long run,
>>>> but don't know what Arend thinks.
> 
> Hi,
> 
> Had a day off today so I did see some of the discussion, but was not able to chime in until now.
> 
> To be honest I always disliked spaces in filenames, but that does not necessarily make it a bad idea. What I would like to know is why built-in firmware can not deal with spaces in the firmware file names. I think Hans mentioned it in the thread and it crossed my mind as well last night. From driver perspective, being brcmfmac or any other for that matter, there is only one API to request firmware and in my opinion it should behave the same no matter where the firmware is coming from. I would prefer to fix that for built-in firmware, but we need to understand where this limitation is coming 
> from. Hopefully Luis can elaborate on that.

Ok.


>>> I have found a fresh judicate on this:
>>> https://lkml.org/lkml/2018/12/22/221
>>>
>>> It seems clear that we have to support at least spaces for some time
>>> (maybe wih separate config option which will be deprecated but on by
>>> defaut until old files are considered gone).
>>
>> Ah that issue, well that is not really comparable in that case a lot of
>> peoples setups were completely broken by that commit and it was a
>> quite surprising behavior change in a userspace facing API.
>>
>> The nvram loading path already does 2 tries, I really don't want to
>> unnecessary complicate it with a third try.
>>
>> The Onda V80 Plus is a X86 based Windows / Android dual boot tablet,
>> as said before I do not expect a ton of users to be running regular
>> Linux on it.
>>
>> Given Kalle's clear preference for getting rid of the spaces lets
>> just do that. But first we must get a symlink added to linux-firmware
>> using the name with the _, newer kernels requiring a newer linux-firmware
>> to match is not unheard of AFAIK, so combined with the limited amount
>> of users I think this is a reasonable compromise.
> 
> Right. In the brcm folder we have bcm4329-fullmac-4.bin for older kernels and brcmfmac4329-sdio.bin for later kernels when we switched to stricter firmware naming convention.
> 
>> Kalle, do you agree with getting the symlink added to linux-firmware
>> ASAP as a fix for the V80 Plus issue; or do you want to see a fallback
>> to the un-cleaned name as you suggested before ?
> 
> How many releases have an issue and how many V80 Plus users running regular linux are actually using built-in firmware. And is it really a regression for them? Not saying it does not require fixing. However, as stated above I would prefer to fix the built-in firmware limitation if possible and backport that fix if it is only a few kernel releases provided stable allows such a backport.

The issue is not V80 Plus users running regular linux with built-in firmware.
The issue is that the 5.0+ kernel + a new enough linux-firmware will just
work on the V80 Plus, since linux-firmware contains a nvram file for the V80 Plus,
with the space in the name.

So if we replace the space with an _ then things will stop working for those
users. But we can avoid this by adding a compat symlink to linux-firmware, then
users will require a new linux-firmware together with the new kernel, but that
is not unheard of.

As for how many users / releases. Users who have a fresh Fedora 30 install or
a fresh install of a rolling-release distro may rely on things working ootb.

Users with an older Linux install will have manually added the nvram using
the non board specific name to get things to work, so I expect things to
stay working for them.

So taking the group of people putting regular Linux on a V80 Plus
and then taking the cross-section of the group with users with a very recent
install, I expect the amount of affected users to be very small and both
Fedora and rolling-release updates update linux-firmware regularly.

So IMHO we should be fine with the sanitizing of the DMI strings, combined
with pushing a compat patch to linux-firmware.

Regards,

Hans




> 
> Regards,
> Arend

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

[Arend Van Spriel]

On 5/7/2019 5:38 PM, Hans de Goede wrote:
> Hi,
> 
> On 06-05-19 21:30, Arend Van Spriel wrote:
>> + Luis (for real this time)
>>
>> On 5/6/2019 6:05 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 06-05-19 17:24, Victor Bravo wrote:
>>>> On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
>>>>> Hans de Goede <hdegoede@redhat.com> writes:
>>>>>
>>>>>> 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.
>>>>>
>>>>> To me having spaces in filenames is a bad idea, but on the other 
>>>>> hand we
>>>>> do have the "don't break existing setups" rule, so it's not so 
>>>>> simple. I
>>>>> vote for not allowing spaces, I think that's the best for the long 
>>>>> run,
>>>>> but don't know what Arend thinks.
>>
>> Hi,
>>
>> Had a day off today so I did see some of the discussion, but was not 
>> able to chime in until now.
>>
>> To be honest I always disliked spaces in filenames, but that does not 
>> necessarily make it a bad idea. What I would like to know is why 
>> built-in firmware can not deal with spaces in the firmware file names. 
>> I think Hans mentioned it in the thread and it crossed my mind as well 
>> last night. From driver perspective, being brcmfmac or any other for 
>> that matter, there is only one API to request firmware and in my 
>> opinion it should behave the same no matter where the firmware is 
>> coming from. I would prefer to fix that for built-in firmware, but we 
>> need to understand where this limitation is coming from. Hopefully 
>> Luis can elaborate on that.
> 
> Ok.

The issue is probably that make does simply split the EXTRA_FIRMWARE 
setting at each space character. I tried to use single quote so
"'brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt' 
brcmfmac43340-sdio.bin". No luck. So all I could think of is patch below 
which require the user to enter a special sequence, ie. _-_ where space 
should be.

"brcmfmac43340-sdio.ASUSTeK_-_COMPUTER_-_INC.-T100HAN.txt 
brcmfmac43340-sdio.bin"

It works but I had to drop the dependency so it's all a bit hacky.

Regards,
Arend

diff --git a/firmware/Makefile b/firmware/Makefile
index 37e5ae387400..a536e5d12d5f 100644
--- a/firmware/Makefile
+++ b/firmware/Makefile
@@ -5,10 +5,11 @@
  fwdir := $(subst $(quote),,$(CONFIG_EXTRA_FIRMWARE_DIR))
  fwdir := $(addprefix $(srctree)/,$(filter-out /%,$(fwdir)))$(filter 
/%,$(fwdir))

+fw_space_escape := _-_
  obj-y  := $(addsuffix .gen.o, $(subst $(quote),,$(CONFIG_EXTRA_FIRMWARE)))

-FWNAME    = $(patsubst $(obj)/%.gen.S,%,$@)
-FWSTR     = $(subst /,_,$(subst .,_,$(subst -,_,$(FWNAME))))
+FWNAME    = $(subst $(fw_space_escape),$(space),$(patsubst 
$(obj)/%.gen.S,%,$@))
+FWSTR     = $(subst $(space),_,$(subst /,_,$(subst .,_,$(subst 
-,_,$(FWNAME)))))
  ASM_WORD  = $(if $(CONFIG_64BIT),.quad,.long)
  ASM_ALIGN = $(if $(CONFIG_64BIT),3,2)
  PROGBITS  = $(if $(CONFIG_ARM),%,@)progbits
@@ -34,7 +35,7 @@ $(obj)/%.gen.S: FORCE
  	$(call filechk,fwbin)

  # The .o files depend on the binaries directly; the .S files don't.
-$(addprefix $(obj)/, $(obj-y)): $(obj)/%.gen.o: $(fwdir)/%
+$(addprefix $(obj)/, $(obj-y)): $(obj)/%.gen.o:

  targets := $(patsubst $(obj)/%,%, \
                                  $(shell find $(obj) -name \*.gen.S 
2>/dev/null))