* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 10:20 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <70677dff-4336-28d5-7ab9-7ba7c3d74ebc@redhat.com>
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;
> > > > }
> > > > }
> > > >
> > >
>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Hans de Goede @ 2019-05-06 9:33 UTC (permalink / raw)
To: Victor Bravo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190506090609.msudhncj7e5vdtzw@localhost>
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;
>>> }
>>> }
>>>
>>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 9:14 UTC (permalink / raw)
To: Kalle Valo
Cc: Hans de Goede, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <87o94gug81.fsf@codeaurora.org>
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.
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 9:06 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <0f75a3d4-94af-5503-94c3-e8af2364448d@redhat.com>
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;
> > }
> > }
> >
>
^ permalink raw reply
* RE: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
From: Tony Chuang @ 2019-05-06 8:54 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <874l68vuhi.fsf@purkki.adurom.net>
> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@codeaurora.org]
> Sent: Monday, May 06, 2019 4:49 PM
> To: Tony Chuang
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
>
> <yhchuang@realtek.com> writes:
>
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > The new rssi_level should be stored in si, otherwise the rssi_level will
> > never be updated and get a wrong RA mask, which is calculated by the
> > rssi level
> >
> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Stanislaw suggested that this should go to 5.2. So what breaks from
> user's point of view if this is not applied?
>
If the rssi level remains unchanged, then we could choose wrong ra_mask.
And some *bad rates* we be chosen by firmware.
The most hurtful scene would be *noisy environment* such as office, or public.
The latency would be high and overall throughput would be only half.
(This was tested, such as 4x Mbps -> 1x Mbps)
Yan-Hsuan
^ permalink raw reply
* Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
From: Kalle Valo @ 2019-05-06 8:48 UTC (permalink / raw)
To: yhchuang; +Cc: linux-wireless
In-Reply-To: <1556884415-23474-5-git-send-email-yhchuang@realtek.com>
<yhchuang@realtek.com> writes:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> The new rssi_level should be stored in si, otherwise the rssi_level will
> never be updated and get a wrong RA mask, which is calculated by the
> rssi level
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Stanislaw suggested that this should go to 5.2. So what breaks from
user's point of view if this is not applied?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 8:44 UTC (permalink / raw)
To: Victor Bravo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel,
Hans de Goede
In-Reply-To: <20190505150355.3fbng4ny34x255vk@localhost>
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
^ permalink raw reply
* Re: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Stanislaw Gruszka @ 2019-05-06 8:42 UTC (permalink / raw)
To: linux-wireless, Kalle Valo; +Cc: Yan-Hsuan Chuang
In-Reply-To: <20190506073917.10106-1-sgruszka@redhat.com>
This is for 5.2 and v2 obviously.
Stanislaw
On Mon, May 06, 2019 at 09:39:17AM +0200, Stanislaw Gruszka wrote:
> My compiler complains about:
>
> drivers/net/wireless/realtek/rtw88/phy.c: In function ???rtw_phy_rf_power_2_rssi???:
> drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is above array bounds [-Warray-bounds]
> linear = db_invert_table[i][j];
>
> According to comment power_db should be in range 1 ~ 96 .
> To fix add check for boundaries before access the array.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> RFC -> v1
> - add check before accessing the array insted of
> rtw_phy_power_2_db() change.
> v1 -> v2:
> - return 1 for power_db < 1
>
> drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
> index 4381b360b5b5..9ca52a4d025a 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> u8 i, j;
> u64 linear;
>
> + if (power_db > 96)
> + power_db = 96;
> + else if (power_db < 1)
> + return 1;
> +
> /* 1dB ~ 96dB */
> i = (power_db - 1) >> 3;
> j = (power_db - 1) - (i << 3);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 8:42 UTC (permalink / raw)
To: Hans de Goede
Cc: Victor Bravo, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <0f75a3d4-94af-5503-94c3-e8af2364448d@redhat.com>
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
^ permalink raw reply
* Re: [PATCH v2 0/5] rtw88: minor fixes from suggestions during review
From: Stanislaw Gruszka @ 2019-05-06 8:40 UTC (permalink / raw)
To: Kalle Valo; +Cc: yhchuang, linux-wireless
In-Reply-To: <87ftpvkal1.fsf@kamboji.qca.qualcomm.com>
On Fri, May 03, 2019 at 03:04:58PM +0300, Kalle Valo wrote:
> <yhchuang@realtek.com> writes:
>
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > The series fix some small problems for rtw88, most of the suggestions
> > are from the review process.
> >
> >
> > v1 -> v2
> >
> > - modify description for LPS, ", turn off" -> ", to turn off"
> > - drop patch "rtw88: mac: remove dangerous while (1)",
> > should re-write the power sequence parsing code to make sense of avoiding
> > infinite loop
> > - unify Makefile license to Dual GPL/BSD
> >
> >
> > Yan-Hsuan Chuang (5):
> > rtw88: add license for Makefile
> > rtw88: pci: use ieee80211_ac_numbers instead of 0-3
> > rtw88: pci: check if queue mapping exceeds size of ac_to_hwq
> > rtw88: fix unassigned rssi_level in rtw_sta_info
> > rtw88: more descriptions about LPS
>
> I was just in the next few minutes about to tag the last -next pull for
> 5.2. I'll apply patch 1 now so that we have consistent licenses for 5.2
> but the rest have to wait for 5.3.
I think '[PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_inf'
should go to 5.2 .
Stanislaw
^ permalink raw reply
* Re: pull-request: wireless-drivers 2019-04-30
From: Kalle Valo @ 2019-05-06 8:29 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20190505.005130.1921658214241614481.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Kalle Valo <kvalo@codeaurora.org>
> Date: Tue, 30 Apr 2019 19:55:45 +0300
>
>> David Miller <davem@davemloft.net> writes:
>>
>>> Thanks for the conflict resolution information, it is very helpful.
>>>
>>> However, can you put it into the merge commit text next time as well?
>>> I cut and pasted it in there when I pulled this stuff in.
>>
>> A good idea, I'll do that. Just to be sure, do you mean that I should
>> add it only with conflicts between net and net-next (like in this case)?
>> Or should I add it everytime I see a conflict, for example between
>> wireless-drivers-next and net-next? I hope my question is not too
>> confusing...
>
> When there is a major conflict for me to resolve when I pull in your
> pull reqeust, please place the conflict resolution help text into the
> merge commit message.
>
> I hope this is now clear :-)
Got it now, thanks!
--
Kalle Valo
^ permalink raw reply
* RE: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Tony Chuang @ 2019-05-06 8:29 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless@vger.kernel.org
In-Reply-To: <20190506073917.10106-1-sgruszka@redhat.com>
> Subject: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
>
> My compiler complains about:
>
> drivers/net/wireless/realtek/rtw88/phy.c: In function
> ‘rtw_phy_rf_power_2_rssi’:
> drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is
> above array bounds [-Warray-bounds]
> linear = db_invert_table[i][j];
>
> According to comment power_db should be in range 1 ~ 96 .
> To fix add check for boundaries before access the array.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> RFC -> v1
> - add check before accessing the array insted of
> rtw_phy_power_2_db() change.
> v1 -> v2:
> - return 1 for power_db < 1
>
> drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> b/drivers/net/wireless/realtek/rtw88/phy.c
> index 4381b360b5b5..9ca52a4d025a 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> u8 i, j;
> u64 linear;
>
> + if (power_db > 96)
> + power_db = 96;
> + else if (power_db < 1)
> + return 1;
> +
> /* 1dB ~ 96dB */
> i = (power_db - 1) >> 3;
> j = (power_db - 1) - (i << 3);
> --
Thanks. For this patch.
Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Yan-Hsuan
^ permalink raw reply
* Re: [PATCH] net: wireless: ath9k: Return an error when ath9k_hw_reset() fails
From: Kalle Valo @ 2019-05-06 8:24 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Jia-Ju Bai, ath9k-devel, davem, linux-wireless, netdev,
linux-kernel
In-Reply-To: <e47117d6-f918-1dd0-834e-d056534bfead@gmail.com>
Heiner Kallweit <hkallweit1@gmail.com> writes:
> On 04.05.2019 12:08, Jia-Ju Bai wrote:
>> ath9k_hw_reset() in ath9k_start() can fail, and in this case,
>> ath9k_start() should return an error instead of executing the
>> subsequent code.
>>
> Such mechanical patches w/o understanding the code are always
> problematic. Do you have any proof that this error is fatal?
> I think it is not, else we wouldn't have this line:
> ah->reset_power_on = false;
> Also you should consider that a mutex and a spinlock are held.
> Maybe changing the error message to a warning would be more
> appropriate. But this I would leave to somebody being more
> familiar with this driver.
A very good point, thanks Heiner! I will drop this unless someone
familiar with ath9k says that this is ok.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] net: wireless: b43: Avoid possible double calls to b43_one_core_detach()
From: Kalle Valo @ 2019-05-06 8:21 UTC (permalink / raw)
To: Jia-Ju Bai
Cc: davem, colin.king, yuehaibing, linux-wireless, b43-dev, netdev,
linux-kernel
In-Reply-To: <20190504091000.18665-1-baijiaju1990@gmail.com>
Jia-Ju Bai <baijiaju1990@gmail.com> writes:
> In b43_request_firmware(), when ieee80211_register_hw() fails,
> b43_one_core_detach() is called. In b43_bcma_remove() and
> b43_ssb_remove(), b43_one_core_detach() is called again. In this case,
> null-pointer dereferences and double-free problems can occur when
> the driver is removed.
>
> To fix this bug, the call to b43_one_core_detach() in
> b43_request_firmware() is deleted.
>
> This bug is found by a runtime fuzzing tool named FIZZER written by us.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
> drivers/net/wireless/broadcom/b43/main.c | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
You can use just "b43:" as prefix, no need to have "net:" nor
"wireless:" in the title. I'll fix it this time, but please use correct
style in the future.
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_title_is_wrong
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Hans de Goede @ 2019-05-06 8:13 UTC (permalink / raw)
To: Victor Bravo, Arend Van Spriel
Cc: Franky Lin, Hante Meuleman, Chi-Hsien Lin, Wright Feng,
Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190505150355.3fbng4ny34x255vk@localhost>
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;
> }
> }
>
^ permalink raw reply
* Re: [PATCH v4 07/10] net: wireless: support of_get_mac_address new ERR_PTR error
From: Kalle Valo @ 2019-05-06 7:59 UTC (permalink / raw)
To: Petr Štetiar
Cc: netdev, devicetree, QCA ath9k Development, David S. Miller,
Felix Fietkau, Lorenzo Bianconi, Matthias Brugger,
Stanislaw Gruszka, Helmut Schaa, Andrew Lunn, Florian Fainelli,
Heiner Kallweit, Frank Rowand, Srinivas Kandagatla, Maxime Ripard,
linux-wireless, linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <1556893635-18549-8-git-send-email-ynezz@true.cz>
Petr Štetiar <ynezz@true.cz> writes:
> There was NVMEM support added to of_get_mac_address, so it could now return
> ERR_PTR encoded error values, so we need to adjust all current users of
> of_get_mac_address to this new fact.
>
> Signed-off-by: Petr Štetiar <ynezz@true.cz>
> ---
>
> Changes since v3:
>
> * IS_ERR_OR_NULL -> IS_ERR
>
> drivers/net/wireless/ath/ath9k/init.c | 2 +-
> drivers/net/wireless/mediatek/mt76/eeprom.c | 2 +-
> drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
Via which tree is this supposed to go? In case something else than my
wireless-drivers-next:
Acked-by: Kalle Valo <kvalo@codeaurora.org>
--
Kalle Valo
^ permalink raw reply
* Re: static analysis issue in rtl8188de driver
From: Pkshih @ 2019-05-06 7:56 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org, colin.king@canonical.com,
kvalo@codeaurora.org, Larry.Finger@lwfinger.net
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <a1842b3e-f0af-d1a1-8609-a76c25dfd37b@canonical.com>
On Sat, 2019-05-04 at 21:50 +0000, Colin Ian King wrote:
> Hi,
>
> Static analysis with Coverity has found an issue in the rtl8188de
> wireless driver in drivers/net/wireless/realtek/rtlwifi/rtl8192de/dm.c
> in function tl92d_dm_txpower_tracking_callback_thermalmeter.
>
> The issue is that u8 array ofdm_index[3] is never initialized, however
> it is decremented and incremented in two places resulting in garbage
> value from the stack being updated in the following code:
>
> if (thermalvalue > rtlpriv->dm.thermalvalue) {
> for (i = 0; i < rf; i++)
> ofdm_index[i] -= delta;
> cck_index -= delta;
> } else {
> for (i = 0; i < rf; i++)
> ofdm_index[i] += index;
> cck_index += index;
> }
>
> At my first look at the code I believe ofdm_index should be just
> zero-initialized at declaration time, but I suspect that I'm overlooking
> something maybe a bit deeper. Any ideas?
>
Hi Colin,
Thanks for your report.
After my quick look, there are at least two obvious problems.
One is array size of ofdm_index[] should be two instead. Another is the value
of ofdm_index[] should be obtained from rtlpriv->dm.ofdm_index[].
Since the logic is quite complex, I need some time to fix it.
PK
^ permalink raw reply
* [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Stanislaw Gruszka @ 2019-05-06 7:39 UTC (permalink / raw)
To: linux-wireless; +Cc: Yan-Hsuan Chuang
My compiler complains about:
drivers/net/wireless/realtek/rtw88/phy.c: In function ‘rtw_phy_rf_power_2_rssi’:
drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is above array bounds [-Warray-bounds]
linear = db_invert_table[i][j];
According to comment power_db should be in range 1 ~ 96 .
To fix add check for boundaries before access the array.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
RFC -> v1
- add check before accessing the array insted of
rtw_phy_power_2_db() change.
v1 -> v2:
- return 1 for power_db < 1
drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 4381b360b5b5..9ca52a4d025a 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
u8 i, j;
u64 linear;
+ if (power_db > 96)
+ power_db = 96;
+ else if (power_db < 1)
+ return 1;
+
/* 1dB ~ 96dB */
i = (power_db - 1) >> 3;
j = (power_db - 1) - (i << 3);
--
2.20.1
^ permalink raw reply related
* [PATCH] ath10k: acquire lock to fix lockdep's warning
From: Claire Chang @ 2019-05-06 7:38 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, ath10k, wgong, drinkcat, Claire Chang
Lockdep warns at lockdep_assert_held(&ar->data_lock) in
ath10k_htt_rx_pn_check_replay_hl(). Acquire ar->data_lock before calling
ath10k_htt_rx_pn_check_replay_hl() to fix it.
Call trace:
ath10k_htt_rx_pn_check_replay_hl+0x118/0x134 [ath10k_core]
ath10k_htt_rx_proc_rx_ind_hl+0xd8/0x250 [ath10k_core]
ath10k_htt_t2h_msg_handler+0x148/0xf30 [ath10k_core]
ath10k_htt_htc_t2h_msg_handler+0x24/0x40 [ath10k_core]
ath10k_sdio_irq_handler+0x374/0xaa4 [ath10k_sdio]
Fixes: 130c77495708 ("ath10k: add PN replay protection for high latency devices")
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
drivers/net/wireless/ath/ath10k/htt_rx.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 9eed1cb17fda..3e3be1e5bbaf 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -1952,6 +1952,7 @@ static bool ath10k_htt_rx_proc_rx_ind_hl(struct ath10k_htt *htt,
int num_mpdu_ranges;
size_t tot_hdr_len;
struct ieee80211_channel *ch;
+ bool pn_invalid;
peer_id = __le16_to_cpu(rx->hdr.peer_id);
@@ -1983,9 +1984,13 @@ static bool ath10k_htt_rx_proc_rx_ind_hl(struct ath10k_htt *htt,
goto err;
}
- if (check_pn_type == HTT_RX_PN_CHECK &&
- ath10k_htt_rx_pn_check_replay_hl(ar, peer, rx))
- goto err;
+ if (check_pn_type == HTT_RX_PN_CHECK) {
+ spin_lock_bh(&ar->data_lock);
+ pn_invalid = ath10k_htt_rx_pn_check_replay_hl(ar, peer, rx);
+ spin_unlock_bh(&ar->data_lock);
+ if (pn_invalid)
+ goto err;
+ }
/* Strip off all headers before the MAC header before delivery to
* mac80211
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* RE: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Tony Chuang @ 2019-05-06 6:51 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <20190506064357.GB5115@redhat.com>
> Subject: Re: [PATCH 5.1] rtw88: fix subscript above array bounds compiler
> warning
>
> On Mon, May 06, 2019 at 06:32:01AM +0000, Tony Chuang wrote:
> > > Subject: [PATCH 5.1] rtw88: fix subscript above array bounds compiler
> warning
> > >
> > > My compiler complains about:
> > >
> > > drivers/net/wireless/realtek/rtw88/phy.c: In function
> > > ‘rtw_phy_rf_power_2_rssi’:
> > > drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is
> > > above array bounds [-Warray-bounds]
> > > linear = db_invert_table[i][j];
> > >
> > > According to comment power_db should be in range 1 ~ 96 .
> > > To fix add check for boundaries before access the array.
> > >
> > > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > > ---
> > > RFC -> v1
> > > - add check before accessing the array insted of
> > > rtw_phy_power_2_db() change.
> > >
> > > drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> > > b/drivers/net/wireless/realtek/rtw88/phy.c
> > > index 4381b360b5b5..9ca52a4d025a 100644
> > > --- a/drivers/net/wireless/realtek/rtw88/phy.c
> > > +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> > > @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> > > u8 i, j;
> > > u64 linear;
> > >
> > > + if (power_db > 96)
> > > + power_db = 96;
> > > + else if (power_db < 1)
> > > + power_db = 1;
> >
> > I think it's "return 1" here.
>
> Ehh, I missed that in your comment. However 'return 1' change
> the output of rtw_phy_db_2_linear() quite substantially
> as the smallest value (for power_db = 1) from db_invert_table[][]
> is 10. I'll post v2 patch, but please double check it's indeed
> correct logic. Thanks.
>
I think "return 1" is correct because 0 is not in domain 1~96.
And indeed anything to the power of zero is 1.
Thanks.
Yan-Hsuan
^ permalink raw reply
* Re: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Stanislaw Gruszka @ 2019-05-06 6:43 UTC (permalink / raw)
To: Tony Chuang; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17EB2B5@RTITMBSVM04.realtek.com.tw>
On Mon, May 06, 2019 at 06:32:01AM +0000, Tony Chuang wrote:
> > Subject: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
> >
> > My compiler complains about:
> >
> > drivers/net/wireless/realtek/rtw88/phy.c: In function
> > ‘rtw_phy_rf_power_2_rssi’:
> > drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is
> > above array bounds [-Warray-bounds]
> > linear = db_invert_table[i][j];
> >
> > According to comment power_db should be in range 1 ~ 96 .
> > To fix add check for boundaries before access the array.
> >
> > Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> > ---
> > RFC -> v1
> > - add check before accessing the array insted of
> > rtw_phy_power_2_db() change.
> >
> > drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> > b/drivers/net/wireless/realtek/rtw88/phy.c
> > index 4381b360b5b5..9ca52a4d025a 100644
> > --- a/drivers/net/wireless/realtek/rtw88/phy.c
> > +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> > @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> > u8 i, j;
> > u64 linear;
> >
> > + if (power_db > 96)
> > + power_db = 96;
> > + else if (power_db < 1)
> > + power_db = 1;
>
> I think it's "return 1" here.
Ehh, I missed that in your comment. However 'return 1' change
the output of rtw_phy_db_2_linear() quite substantially
as the smallest value (for power_db = 1) from db_invert_table[][]
is 10. I'll post v2 patch, but please double check it's indeed
correct logic. Thanks.
Stanislaw
^ permalink raw reply
* RE: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Tony Chuang @ 2019-05-06 6:32 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless@vger.kernel.org
In-Reply-To: <20190506062358.8288-1-sgruszka@redhat.com>
> Subject: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
>
> My compiler complains about:
>
> drivers/net/wireless/realtek/rtw88/phy.c: In function
> ‘rtw_phy_rf_power_2_rssi’:
> drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is
> above array bounds [-Warray-bounds]
> linear = db_invert_table[i][j];
>
> According to comment power_db should be in range 1 ~ 96 .
> To fix add check for boundaries before access the array.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> RFC -> v1
> - add check before accessing the array insted of
> rtw_phy_power_2_db() change.
>
> drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> b/drivers/net/wireless/realtek/rtw88/phy.c
> index 4381b360b5b5..9ca52a4d025a 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> u8 i, j;
> u64 linear;
>
> + if (power_db > 96)
> + power_db = 96;
> + else if (power_db < 1)
> + power_db = 1;
I think it's "return 1" here.
> +
> /* 1dB ~ 96dB */
> i = (power_db - 1) >> 3;
> j = (power_db - 1) - (i << 3);
> --
Yan-Hsuan
^ permalink raw reply
* [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Stanislaw Gruszka @ 2019-05-06 6:23 UTC (permalink / raw)
To: linux-wireless; +Cc: Yan-Hsuan Chuang
My compiler complains about:
drivers/net/wireless/realtek/rtw88/phy.c: In function ‘rtw_phy_rf_power_2_rssi’:
drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is above array bounds [-Warray-bounds]
linear = db_invert_table[i][j];
According to comment power_db should be in range 1 ~ 96 .
To fix add check for boundaries before access the array.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
RFC -> v1
- add check before accessing the array insted of
rtw_phy_power_2_db() change.
drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 4381b360b5b5..9ca52a4d025a 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
u8 i, j;
u64 linear;
+ if (power_db > 96)
+ power_db = 96;
+ else if (power_db < 1)
+ power_db = 1;
+
/* 1dB ~ 96dB */
i = (power_db - 1) >> 3;
j = (power_db - 1) - (i << 3);
--
2.20.1
^ permalink raw reply related
* RE: [PATCH] rtw88: avoid circular locking between local->iflist_mtx and rtwdev->mutex
From: Tony Chuang @ 2019-05-06 5:22 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless@vger.kernel.org
In-Reply-To: <1556886547-23632-1-git-send-email-sgruszka@redhat.com>
> Subject: [PATCH] rtw88: avoid circular locking between local->iflist_mtx and
> rtwdev->mutex
>
> Remove circular lock dependency by using atomic version of interfaces
> iterate in watch_dog_work(), hence avoid taking local->iflist_mtx
> (rtw_vif_watch_dog_iter() only update some data, it can be called from
> atomic context). Fixes below LOCKDEP warning:
>
> [ 1157.219415]
> ======================================================
> [ 1157.225772] [ INFO: possible circular locking dependency detected ]
> [ 1157.232150] 3.10.0-1043.el7.sgruszka1.x86_64.debug #1 Not tainted
> [ 1157.238346] -------------------------------------------------------
> [ 1157.244635] kworker/u4:2/14490 is trying to acquire lock:
> [ 1157.250194] (&rtwdev->mutex){+.+.+.}, at: [<ffffffffc098322b>]
> rtw_ops_config+0x2b/0x90 [rtw88]
> [ 1157.259151]
> but task is already holding lock:
> [ 1157.265085] (&local->iflist_mtx){+.+...}, at: [<ffffffffc0b8ab7a>]
> ieee80211_mgd_probe_ap.part.28+0xca/0x160 [mac80211]
> [ 1157.276169]
> which lock already depends on the new lock.
>
> [ 1157.284488]
> the existing dependency chain (in reverse order) is:
> [ 1157.292101]
> -> #2 (&local->iflist_mtx){+.+...}:
> [ 1157.296919] [<ffffffffbc741a29>] lock_acquire+0x99/0x1e0
> [ 1157.302955] [<ffffffffbce72793>] mutex_lock_nested+0x93/0x410
> [ 1157.309416] [<ffffffffc0b6038f>]
> ieee80211_iterate_interfaces+0x2f/0x60 [mac80211]
> [ 1157.317730] [<ffffffffc09811ab>]
> rtw_watch_dog_work+0xcb/0x130 [rtw88]
> [ 1157.325003] [<ffffffffbc6d77bc>] process_one_work+0x22c/0x720
> [ 1157.331481] [<ffffffffbc6d7dd6>] worker_thread+0x126/0x3b0
> [ 1157.337589] [<ffffffffbc6e107f>] kthread+0xef/0x100
> [ 1157.343260] [<ffffffffbce848b7>]
> ret_from_fork_nospec_end+0x0/0x39
> [ 1157.350091]
> -> #1 ((&(&rtwdev->watch_dog_work)->work)){+.+...}:
> [ 1157.356314] [<ffffffffbc741a29>] lock_acquire+0x99/0x1e0
> [ 1157.362427] [<ffffffffbc6d570b>] flush_work+0x5b/0x310
> [ 1157.368287] [<ffffffffbc6d740e>]
> __cancel_work_timer+0xae/0x170
> [ 1157.374940] [<ffffffffbc6d7583>]
> cancel_delayed_work_sync+0x13/0x20
> [ 1157.381930] [<ffffffffc0982b49>] rtw_core_stop+0x29/0x50
> [rtw88]
> [ 1157.388679] [<ffffffffc098bee6>] rtw_enter_ips+0x16/0x20
> [rtw88]
> [ 1157.395428] [<ffffffffc0983242>] rtw_ops_config+0x42/0x90
> [rtw88]
> [ 1157.402173] [<ffffffffc0b13343>]
> ieee80211_hw_config+0xc3/0x680 [mac80211]
> [ 1157.409854] [<ffffffffc0b3925b>]
> ieee80211_do_open+0x69b/0x9c0 [mac80211]
> [ 1157.417418] [<ffffffffc0b395e9>] ieee80211_open+0x69/0x70
> [mac80211]
> [ 1157.424496] [<ffffffffbcd03442>] __dev_open+0xe2/0x160
> [ 1157.430356] [<ffffffffbcd03773>]
> __dev_change_flags+0xa3/0x180
> [ 1157.436922] [<ffffffffbcd03879>] dev_change_flags+0x29/0x60
> [ 1157.443224] [<ffffffffbcda14c4>] devinet_ioctl+0x794/0x890
> [ 1157.449331] [<ffffffffbcda27b5>] inet_ioctl+0x75/0xa0
> [ 1157.455087] [<ffffffffbccd54eb>] sock_do_ioctl+0x2b/0x60
> [ 1157.461178] [<ffffffffbccd5753>] sock_ioctl+0x233/0x310
> [ 1157.467109] [<ffffffffbc8bd820>] do_vfs_ioctl+0x410/0x6c0
> [ 1157.473233] [<ffffffffbc8bdb71>] SyS_ioctl+0xa1/0xc0
> [ 1157.478914] [<ffffffffbce84a5e>] system_call_fastpath+0x25/0x2a
> [ 1157.485569]
> -> #0 (&rtwdev->mutex){+.+.+.}:
> [ 1157.490022] [<ffffffffbc7409d1>] __lock_acquire+0xec1/0x1630
> [ 1157.496305] [<ffffffffbc741a29>] lock_acquire+0x99/0x1e0
> [ 1157.502413] [<ffffffffbce72793>] mutex_lock_nested+0x93/0x410
> [ 1157.508890] [<ffffffffc098322b>] rtw_ops_config+0x2b/0x90
> [rtw88]
> [ 1157.515724] [<ffffffffc0b13343>]
> ieee80211_hw_config+0xc3/0x680 [mac80211]
> [ 1157.523370] [<ffffffffc0b8a4ca>]
> ieee80211_recalc_ps.part.27+0x9a/0x180 [mac80211]
> [ 1157.531685] [<ffffffffc0b8abc5>]
> ieee80211_mgd_probe_ap.part.28+0x115/0x160 [mac80211]
> [ 1157.540353] [<ffffffffc0b8b40d>]
> ieee80211_beacon_connection_loss_work+0x4d/0x80 [mac80211]
> [ 1157.549513] [<ffffffffbc6d77bc>] process_one_work+0x22c/0x720
> [ 1157.555886] [<ffffffffbc6d7dd6>] worker_thread+0x126/0x3b0
> [ 1157.562170] [<ffffffffbc6e107f>] kthread+0xef/0x100
> [ 1157.567765] [<ffffffffbce848b7>]
> ret_from_fork_nospec_end+0x0/0x39
> [ 1157.574579]
> other info that might help us debug this:
>
> [ 1157.582788] Chain exists of:
> &rtwdev->mutex --> (&(&rtwdev->watch_dog_work)->work) -->
> &local->iflist_mtx
>
> [ 1157.593024] Possible unsafe locking scenario:
>
> [ 1157.599046] CPU0 CPU1
> [ 1157.603653] ---- ----
> [ 1157.608258] lock(&local->iflist_mtx);
> [ 1157.612180]
> lock((&(&rtwdev->watch_dog_work)->work));
> [ 1157.620074] lock(&local->iflist_mtx);
> [ 1157.626555] lock(&rtwdev->mutex);
> [ 1157.630124]
> *** DEADLOCK ***
>
> [ 1157.636148] 4 locks held by kworker/u4:2/14490:
> [ 1157.640755] #0: (%s#6){.+.+.+}, at: [<ffffffffbc6d774a>]
> process_one_work+0x1ba/0x720
> [ 1157.648965] #1: ((&ifmgd->beacon_connection_loss_work)){+.+.+.}, at:
> [<ffffffffbc6d774a>] process_one_work+0x1ba/0x720
> [ 1157.659950] #2: (&wdev->mtx){+.+.+.}, at: [<ffffffffc0b8aad5>]
> ieee80211_mgd_probe_ap.part.28+0x25/0x160 [mac80211]
> [ 1157.670901] #3: (&local->iflist_mtx){+.+...}, at: [<ffffffffc0b8ab7a>]
> ieee80211_mgd_probe_ap.part.28+0xca/0x160 [mac80211]
> [ 1157.682466]
>
> Fixes: e3037485c68e ("rtw88: new Realtek 802.11ac driver")
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> drivers/net/wireless/realtek/rtw88/main.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c
> b/drivers/net/wireless/realtek/rtw88/main.c
> index 9893e5e297e3..f63c34a2e356 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c
> @@ -162,7 +162,8 @@ static void rtw_watch_dog_work(struct work_struct
> *work)
> rtwdev->stats.tx_cnt = 0;
> rtwdev->stats.rx_cnt = 0;
>
> - rtw_iterate_vifs(rtwdev, rtw_vif_watch_dog_iter, &data);
> + /* use atomic version to avoid taking local->iflist_mtx mutex */
> + rtw_iterate_vifs_atomic(rtwdev, rtw_vif_watch_dog_iter, &data);
>
> /* fw supports only one station associated to enter lps, if there are
> * more than two stations associated to the AP, then we can not enter
> --
Looks good to me.
Thanks.
Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Yan-Hsuan
^ permalink raw reply
* Re: [PATCH][next] brcmfmac: remove redundant u32 comparison with less than zero
From: Rafał Miłecki @ 2019-05-06 4:11 UTC (permalink / raw)
To: Colin King, Wright Feng
Cc: Franky Lin, Hante Meuleman, Chi-Hsien Lin, Kalle Valo,
David S . Miller, linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER <brcm80211-dev-list.pdl@broadcom.com>,,
Network Development, kernel-janitors, Linux Kernel Mailing List
In-Reply-To: <20190505211623.3153-1-colin.king@canonical.com>
On Sun, 5 May 2019 at 23:33, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The check for the u32 variable idx being less than zero is
> always false and is redundant. Remove it.
>
> Addresses-Coverity: ("Unsigned compared against 0")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> index 9d1f9ff25bfa..e874dddc7b7f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
> @@ -375,7 +375,7 @@ brcmf_msgbuf_get_pktid(struct device *dev, struct brcmf_msgbuf_pktids *pktids,
> struct brcmf_msgbuf_pktid *pktid;
> struct sk_buff *skb;
>
> - if (idx < 0 || idx >= pktids->array_size) {
> + if (idx >= pktids->array_size) {
> brcmf_err("Invalid packet id %d (max %d)\n", idx,
> pktids->array_size);
> return NULL;
It was added in the commit 2d91c8ad068a ("brcmfmac: set txflow request
id from 1 to pktids array size") and was probably meant to handle a
following brcmf_msgbuf_process_txstatus() case:
idx = le32_to_cpu(tx_status->msg.request_id) - 1;
So this patch is wrong/incomplete.
You should change that to a signed value OR add an extra check in
brcmf_msgbuf_process_txstatus() to make sure it doesn't pass -1 as u32
argument.
--
Rafał
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox