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@xxxxxxxxxx> 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))