Messages in this thread Patch in this message | | | Subject | Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2 | From | Arend Van Spriel <> | Date | Mon, 13 May 2019 11:21:49 +0200 |
| |
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))
| |