wifi: cfg80211: avoid some garbage values

[PATCH wireless 0/9] wifi: cfg80211: avoid some garbage values

Posted by Su Hui 1 year, 5 months ago

Clang static checker (scan-build) has some warnings as follows.

included from drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:16
drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h:123:2:
warning:Assigned value is garbage or undefined [core.uninitialized.Assign]
  123 |         __le32 data_le = cpu_to_le32(*data);
      |         ^~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~
drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:138:3:warning
Value stored to 'err' is never read [deadcode.DeadStores]

There are some functions like brcmf_fil_{cmd,iovar,basscfg}_int_get() 
which read the value of its parameter, but some callers have not 
initialized these parameters which will be read. And this patchset fixes
these problems.

BTW, maybe merge this patchset into one patch is more better because
the num of changed code is small. I split it into multiple patches
because of these different 'Fixes' tags. 

Su Hui (9):
  wifi: cfg80211: avoid garbage value of 'io_type' in 
    brcmf_cfg80211_attach()
  wifi: brcmfmac: avoid garbage value of 'status' in
    brcmf_c_download_blob()
  wifi: cfg80211: avoid garbage value of 'noise' in
    brcmf_cfg80211_dump_survey()
  wifi: cfg80211: avoid garbage value of 'chanspec' in
    brcmf_cfg80211_get_channel()
  wifi: cfg80211: avoid garbage value of 'freq' in
    brcmf_cfg80211_mgmt_tx()
  wifi: cfg80211: avoid garbage value of 'wsec' in
    brcmf_cfg80211_reconfigure_wep()
  wifi: cfg80211: avoid garbage value of 'wsec' in
    brcmf_cfg80211_add_key()
  wifi: cfg80211: avoid garbage value of 'val' in brcmf_set_key_mgmt()
  wifi: cfg80211: avoid garbage value of 'wsec' in
    brcmf_cfg80211_{get,config_default}_key()

 .../broadcom/brcm80211/brcmfmac/cfg80211.c     | 18 +++++++++---------
 .../broadcom/brcm80211/brcmfmac/common.c       |  2 +-
 2 files changed, 10 insertions(+), 10 deletions(-)

-- 
2.30.2

Re: [PATCH wireless 0/9] wifi: cfg80211: avoid some garbage values

Posted by Johannes Berg 1 year, 5 months ago

On Tue, 2024-07-02 at 20:24 +0800, Su Hui wrote:
> 
> Su Hui (9):
>   wifi: cfg80211: avoid garbage value of 'io_type' in 
>     brcmf_cfg80211_attach()
>   wifi: brcmfmac: avoid garbage value of 'status' in
>     brcmf_c_download_blob()
>   wifi: cfg80211: avoid garbage value of 'noise' in
>     brcmf_cfg80211_dump_survey()
>   wifi: cfg80211: avoid garbage value of 'chanspec' in
>     brcmf_cfg80211_get_channel()
>   wifi: cfg80211: avoid garbage value of 'freq' in
>     brcmf_cfg80211_mgmt_tx()
>   wifi: cfg80211: avoid garbage value of 'wsec' in
>     brcmf_cfg80211_reconfigure_wep()
>   wifi: cfg80211: avoid garbage value of 'wsec' in
>     brcmf_cfg80211_add_key()
>   wifi: cfg80211: avoid garbage value of 'val' in brcmf_set_key_mgmt()
>   wifi: cfg80211: avoid garbage value of 'wsec' in
>     brcmf_cfg80211_{get,config_default}_key()
> 

Uh where did all those line breaks come from?

anyway all the titles are wrong - all of this is brcmfmac, not cfg80211.

johannes

Re: [PATCH wireless 0/9] wifi: cfg80211: avoid some garbage values

Posted by Arend Van Spriel 1 year, 5 months ago

On July 2, 2024 2:50:38 PM Johannes Berg <johannes@sipsolutions.net> wrote:

> On Tue, 2024-07-02 at 20:24 +0800, Su Hui wrote:
>>
>> Su Hui (9):
>> wifi: cfg80211: avoid garbage value of 'io_type' in
>> brcmf_cfg80211_attach()
>> wifi: brcmfmac: avoid garbage value of 'status' in
>> brcmf_c_download_blob()
>> wifi: cfg80211: avoid garbage value of 'noise' in
>> brcmf_cfg80211_dump_survey()
>> wifi: cfg80211: avoid garbage value of 'chanspec' in
>> brcmf_cfg80211_get_channel()
>> wifi: cfg80211: avoid garbage value of 'freq' in
>> brcmf_cfg80211_mgmt_tx()
>> wifi: cfg80211: avoid garbage value of 'wsec' in
>> brcmf_cfg80211_reconfigure_wep()
>> wifi: cfg80211: avoid garbage value of 'wsec' in
>> brcmf_cfg80211_add_key()
>> wifi: cfg80211: avoid garbage value of 'val' in brcmf_set_key_mgmt()
>> wifi: cfg80211: avoid garbage value of 'wsec' in
>> brcmf_cfg80211_{get,config_default}_key()
>
> Uh where did all those line breaks come from?
>
> anyway all the titles are wrong - all of this is brcmfmac, not cfg80211

It made you look though ;-)

Gr. AvS

Re: [PATCH wireless 0/9] wifi: cfg80211: avoid some garbage values

Posted by Jonas Gorski 1 year, 5 months ago

Hi,

On Tue, 2 Jul 2024 at 14:50, Su Hui <suhui@nfschina.com> wrote:
>
> Clang static checker (scan-build) has some warnings as follows.
>
> included from drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:16
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h:123:2:
> warning:Assigned value is garbage or undefined [core.uninitialized.Assign]
>   123 |         __le32 data_le = cpu_to_le32(*data);
>       |         ^~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:138:3:warning
> Value stored to 'err' is never read [deadcode.DeadStores]
>
> There are some functions like brcmf_fil_{cmd,iovar,basscfg}_int_get()
> which read the value of its parameter, but some callers have not
> initialized these parameters which will be read. And this patchset fixes
> these problems.

The core issue here seems to be that
brcmf_fil_{cmd,iovar,basscfg}_int_get() function (needlessly?) read
from *data.

So instead of forcing all callers of
brcmf_fil_{cmd,iovar,basscfg}_int_get() to initialize *data first, I
suggest changing brcmf_fil_{cmd,iovar,basscfg}_int_get() to just not
read from it.

I see no reason why they should care about what the previous value
was, since they are supposed to overwrite it anyway.

Best Regards,
Jonas

Re: [PATCH wireless 0/9] wifi: cfg80211: avoid some garbage values

Posted by Arend Van Spriel 1 year, 5 months ago

On July 2, 2024 4:02:39 PM Jonas Gorski <jonas.gorski@gmail.com> wrote:

> Hi,
>
> On Tue, 2 Jul 2024 at 14:50, Su Hui <suhui@nfschina.com> wrote:
>>
>> Clang static checker (scan-build) has some warnings as follows.
>>
>> included from drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:16
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwil.h:123:2:
>> warning:Assigned value is garbage or undefined [core.uninitialized.Assign]
>> 123 |         __le32 data_le = cpu_to_le32(*data);
>> |         ^~~~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c:138:3:warning
>> Value stored to 'err' is never read [deadcode.DeadStores]
>>
>> There are some functions like brcmf_fil_{cmd,iovar,basscfg}_int_get()
>> which read the value of its parameter, but some callers have not
>> initialized these parameters which will be read. And this patchset fixes
>> these problems.
>
> The core issue here seems to be that
> brcmf_fil_{cmd,iovar,basscfg}_int_get() function (needlessly?) read
> from *data.
>
> So instead of forcing all callers of
> brcmf_fil_{cmd,iovar,basscfg}_int_get() to initialize *data first, I
> suggest changing brcmf_fil_{cmd,iovar,basscfg}_int_get() to just not
> read from it.
>
> I see no reason why they should care about what the previous value
> was, since they are supposed to overwrite it anyway.

The issue here is that these are generic functions and there is a reason. 
Some firmware API primitives allow/require the caller to pass selection 
parameters in *data. We wanted to keep the functions generic and leave out 
that knowledge. I suppose we could introduce a separate set of api 
functions for that purpose, but it seems like significant overhead to 
silence compiler warnings. Guess I underestimate the potential risk of 
leaking few bytes of stack data.


Regards,
Arend