linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Arend Van Spriel <arend.vanspriel@broadcom.com>
To: Hans de Goede <hdegoede@redhat.com>,
	Victor Bravo <1905@spmblk.com>, Kalle Valo <kvalo@codeaurora.org>
Cc: Franky Lin <franky.lin@broadcom.com>,
	Hante Meuleman <hante.meuleman@broadcom.com>,
	Chi-Hsien Lin <chi-hsien.lin@cypress.com>,
	Wright Feng <wright.feng@cypress.com>,
	"David S. Miller" <davem@davemloft.net>,
	linux-wireless@vger.kernel.org,
	brcm80211-dev-list.pdl@broadcom.com,
	brcm80211-dev-list@cypress.com, linux-kernel@vger.kernel.org,
	Luis Chamberlain <mcgrof@kernel.org>
Subject: Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
Date: Mon, 13 May 2019 11:21:49 +0200	[thread overview]
Message-ID: <0f42608f-772e-356f-3ade-7ec1b38bd45f@broadcom.com> (raw)
In-Reply-To: <02a6dc11-7def-7d72-4640-d9d42ccec47c@redhat.com>

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

  reply	other threads:[~2019-05-13  9:21 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-04 16:26 PROBLEM: brcmfmac's DMI-based fw file names break built-in fw loader Victor Bravo
2019-05-04 19:11 ` Arend Van Spriel
2019-05-04 19:44   ` Victor Bravo
2019-05-05  8:20     ` Arend Van Spriel
2019-05-05 14:36       ` Victor Bravo
2019-05-05 14:48       ` [PATCH RFC] brcmfmac: sanitize DMI strings Victor Bravo
2019-05-05 14:52         ` Victor Bravo
2019-05-05 15:03       ` [PATCH RFC] brcmfmac: sanitize DMI strings v2 Victor Bravo
2019-05-06  8:13         ` Hans de Goede
2019-05-06  8:42           ` Kalle Valo
2019-05-06  9:14             ` Victor Bravo
2019-05-06 12:29               ` Kalle Valo
2019-05-06 14:06                 ` Victor Bravo
2019-05-06  9:06           ` Victor Bravo
2019-05-06  9:33             ` Hans de Goede
2019-05-06 10:20               ` Victor Bravo
2019-05-06 10:34                 ` Hans de Goede
2019-05-06 12:26                   ` Kalle Valo
2019-05-06 15:24                     ` Victor Bravo
2019-05-06 16:05                       ` Hans de Goede
2019-05-06 19:30                         ` Arend Van Spriel
2019-05-07 15:38                           ` Hans de Goede
2019-05-13  9:21                             ` Arend Van Spriel [this message]
2019-05-06  8:44         ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0f42608f-772e-356f-3ade-7ec1b38bd45f@broadcom.com \
    --to=arend.vanspriel@broadcom.com \
    --cc=1905@spmblk.com \
    --cc=brcm80211-dev-list.pdl@broadcom.com \
    --cc=brcm80211-dev-list@cypress.com \
    --cc=chi-hsien.lin@cypress.com \
    --cc=davem@davemloft.net \
    --cc=franky.lin@broadcom.com \
    --cc=hante.meuleman@broadcom.com \
    --cc=hdegoede@redhat.com \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=wright.feng@cypress.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).