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

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

[Su Hui]

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

[PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Su Hui]

 brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
 brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.

Fixes: 83cf17aa8082 ("brcmfmac: adopt new d11 interface")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 5fe0e671ecb3..6be7e7bd8ce7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -8249,7 +8249,7 @@ struct brcmf_cfg80211_info *brcmf_cfg80211_attach(struct brcmf_pub *drvr,
 	struct brcmf_cfg80211_vif *vif;
 	struct brcmf_if *ifp;
 	s32 err = 0;
-	s32 io_type;
+	s32 io_type = 0;
 	u16 *cap = NULL;
 
 	if (!ndev) {
-- 
2.30.2

[PATCH wireless 2/9] wifi: brcmfmac: avoid garbage value of 'status' in brcmf_c_download_blob()

[Su Hui]

brcmf_fil_iovar_int_get() reads the value of 'status' and passes it
to brcmf_fil_iovar_data_get(). The garbage value of 'status' will
finally be copied to 'ifp->drvr->proto_buf'. Initialize 'status' to
avoid garbage value.

Fixes: fdd0bd88ceae ("brcmfmac: add CLM download support")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index b24faae35873..03277131ef2b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -127,7 +127,7 @@ static int brcmf_c_download_blob(struct brcmf_if *ifp,
 	u32 datalen;
 	u32 cumulative_len;
 	u16 dl_flag = DL_BEGIN;
-	u32 status;
+	u32 status = 0;
 	s32 err;
 
 	brcmf_dbg(TRACE, "Enter\n");
-- 
2.30.2

[PATCH wireless 3/9] wifi: cfg80211: avoid garbage value of 'noise' in brcmf_cfg80211_dump_survey()

[Su Hui]

brcmf_fil_cmd_int_get() reads the value of 'noise' and passes it to
brcmf_fil_cmd_data_get(). Initialize 'noise' to avoid garbage value.

Fixes: 6c04deae1438 ("brcmfmac: Add dump_survey cfg80211 ops for HostApd AutoChannelSelection")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 6be7e7bd8ce7..2377b88d7ee0 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -8084,7 +8084,7 @@ brcmf_cfg80211_dump_survey(struct wiphy *wiphy, struct net_device *ndev,
 	struct ieee80211_supported_band *band;
 	enum nl80211_band band_id;
 	struct cca_msrmnt_query req;
-	u32 noise;
+	u32 noise = 0;
 	int err;
 
 	brcmf_dbg(TRACE, "Enter: channel idx=%d\n", idx);
-- 
2.30.2

[PATCH wireless 4/9] wifi: cfg80211: avoid garbage value of 'chanspec' in brcmf_cfg80211_get_channel()

[Su Hui]

brcmf_fil_iovar_int_get() reads the value of 'chanspec'.
Initialize 'chanspec' to avoid garbage value.

Fixes: ee6e7aa38394 ("brcmfmac: support get_channel cfg80211 callback")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 2377b88d7ee0..e672565de437 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5678,7 +5678,7 @@ static int brcmf_cfg80211_get_channel(struct wiphy *wiphy,
 	struct brcmu_chan ch;
 	enum nl80211_band band = 0;
 	enum nl80211_chan_width width = 0;
-	u32 chanspec;
+	u32 chanspec = 0;
 	int freq, err;
 
 	if (!ndev || drvr->bus_if->state != BRCMF_BUS_UP)
-- 
2.30.2

[PATCH wireless 5/9] wifi: cfg80211: avoid garbage value of 'freq' in brcmf_cfg80211_mgmt_tx()

[Su Hui]

brcmf_fil_cmd_int_get() reads the value of 'freq'.
Initialize 'freq' to avoid garbage value.

Fixes: c2ff8cad6423 ("brcm80211: make mgmt_tx in brcmfmac accept a NULL channel")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index e672565de437..9a5b1f77c890 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -5511,7 +5511,7 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
 	struct brcmf_fil_af_params_le *af_params;
 	bool ack;
 	s32 chan_nr;
-	u32 freq;
+	u32 freq = 0;
 
 	brcmf_dbg(TRACE, "Enter\n");
 
-- 
2.30.2

[PATCH wireless 6/9] wifi: cfg80211: avoid garbage value of 'wsec' in brcmf_cfg80211_reconfigure_wep()

[Su Hui]

brcmf_fil_bsscfg_int_get() reads the value of 'wsec'.
Initialize 'wsec' to avoid garbage value.

Fixes: 118eb304d055 ("brcmfmac: Fix WEP configuration for AP mode.")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 9a5b1f77c890..eb1196db8407 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2975,7 +2975,7 @@ brcmf_cfg80211_reconfigure_wep(struct brcmf_if *ifp)
 	s32 err;
 	u8 key_idx;
 	struct brcmf_wsec_key *key;
-	s32 wsec;
+	s32 wsec = 0;
 
 	for (key_idx = 0; key_idx < BRCMF_MAX_DEFAULT_KEYS; key_idx++) {
 		key = &ifp->vif->profile.key[key_idx];
-- 
2.30.2

[PATCH wireless 7/9] wifi: cfg80211: avoid garbage value of 'wsec' in brcmf_cfg80211_add_key()

[Su Hui]

brcmf_fil_bsscfg_int_get() reads the value of 'wsec'.
Initialize 'wsec' to avoid garbage value.

Fixes: f09d0c02b63d ("brcmfmac: use different fw api for encryption,auth. config")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index eb1196db8407..2a97d4d7f684 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2782,7 +2782,7 @@ brcmf_cfg80211_add_key(struct wiphy *wiphy, struct net_device *ndev,
 	struct brcmf_pub *drvr = cfg->pub;
 	struct brcmf_wsec_key *key;
 	s32 val;
-	s32 wsec;
+	s32 wsec = 0;
 	s32 err;
 	u8 keybuf[8];
 	bool ext_key;
-- 
2.30.2

[PATCH wireless 8/9] wifi: cfg80211: avoid garbage value of 'val' in brcmf_set_key_mgmt()

[Su Hui]

brcmf_fil_bsscfg_int_get() reads the value of 'val'.
Initialize 'val' to avoid garbage vlaue.

Fixes: 240d61a9ddeb ("brcmfmac: add 802.11w management frame protection support")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 2a97d4d7f684..95193e09504f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2085,7 +2085,7 @@ brcmf_set_key_mgmt(struct net_device *ndev, struct cfg80211_connect_params *sme)
 	struct brcmf_if *ifp = netdev_priv(ndev);
 	struct brcmf_cfg80211_profile *profile = &ifp->vif->profile;
 	struct brcmf_pub *drvr = ifp->drvr;
-	s32 val;
+	s32 val = 0;
 	s32 err;
 	const struct brcmf_tlv *rsn_ie;
 	const u8 *ie;
-- 
2.30.2

[PATCH wireless 9/9] wifi: cfg80211: avoid garbage value of 'wsec' in brcmf_cfg80211_{get,config_default}_key()

[Su Hui]

brcmf_fil_bsscfg_int_get() reads the value of 'wsec'.
Initialize 'wsec' to avoid garbage value.

Fixes: 5b435de0d786 ("net: wireless: add brcm80211 drivers")
Signed-off-by: Su Hui <suhui@nfschina.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 95193e09504f..eb8d7455d0ea 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2707,7 +2707,7 @@ brcmf_cfg80211_config_default_key(struct wiphy *wiphy, struct net_device *ndev,
 	struct brcmf_if *ifp = netdev_priv(ndev);
 	struct brcmf_pub *drvr = ifp->drvr;
 	u32 index;
-	u32 wsec;
+	u32 wsec = 0;
 	s32 err = 0;
 
 	brcmf_dbg(TRACE, "Enter\n");
@@ -2907,7 +2907,7 @@ brcmf_cfg80211_get_key(struct wiphy *wiphy, struct net_device *ndev,
 	struct brcmf_cfg80211_profile *profile = &ifp->vif->profile;
 	struct brcmf_pub *drvr = cfg->pub;
 	struct brcmf_cfg80211_security *sec;
-	s32 wsec;
+	s32 wsec = 0;
 	s32 err = 0;
 
 	brcmf_dbg(TRACE, "Enter\n");
-- 
2.30.2

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

[Johannes Berg]

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 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Dan Carpenter]

On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>  brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>  brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.

Since you're going to be resending anyway, please delete the space char
from the start of the line.

It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
It looks like it just goes to great lengths to preserve the original
data in io_type...  So it likely is harmless enough but still a strange
and complicated way write a no-op.

regards,
dan carpenter

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

[Jonas Gorski]

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

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




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

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




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

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

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]

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





[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 759 bytes --]

On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
>
> Since you're going to be resending anyway, please delete the space char
> from the start of the line.
>
> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
> It looks like it just goes to great lengths to preserve the original
> data in io_type...  So it likely is harmless enough but still a strange
> and complicated way write a no-op.

Not sure if it helps, but I tried to explain the reason in response to 
patch 0 (cover letter).

Regards,
Arend




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> writes:

> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
>>
>> Since you're going to be resending anyway, please delete the space char
>> from the start of the line.
>>
>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
>> It looks like it just goes to great lengths to preserve the original
>> data in io_type...  So it likely is harmless enough but still a strange
>> and complicated way write a no-op.
>
> Not sure if it helps, but I tried to explain the reason in response to
> patch 0 (cover letter).

Would it make more sense to have just one patch? It's the same issue
anyway.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Dan Carpenter]

On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> 
> > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> >> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
> >>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
> >>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
> >>
> >> Since you're going to be resending anyway, please delete the space char
> >> from the start of the line.
> >>
> >> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
> >> It looks like it just goes to great lengths to preserve the original
> >> data in io_type...  So it likely is harmless enough but still a strange
> >> and complicated way write a no-op.
> >
> > Not sure if it helps, but I tried to explain the reason in response to
> > patch 0 (cover letter).
> 
> Would it make more sense to have just one patch? It's the same issue
> anyway.

The Fixes tags are different though.  I'd probably leave them as
separate patches just because of that.

regards,
dan carpenter

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]

On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote:

> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>
>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>
>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
>>>
>>> Since you're going to be resending anyway, please delete the space char
>>> from the start of the line.
>>>
>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
>>> It looks like it just goes to great lengths to preserve the original
>>> data in io_type...  So it likely is harmless enough but still a strange
>>> and complicated way write a no-op.
>>
>> Not sure if it helps, but I tried to explain the reason in response to
>> patch 0 (cover letter).
>
> Would it make more sense to have just one patch? It's the same issue
> anyway.

Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]).

Regards,
Arend

> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches




[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1265 bytes --]

On July 2, 2024 5:37:10 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote:
>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>>
>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
>>>>
>>>> Since you're going to be resending anyway, please delete the space char
>>>> from the start of the line.
>>>>
>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
>>>> It looks like it just goes to great lengths to preserve the original
>>>> data in io_type...  So it likely is harmless enough but still a strange
>>>> and complicated way write a no-op.
>>>
>>> Not sure if it helps, but I tried to explain the reason in response to
>>> patch 0 (cover letter).
>>
>> Would it make more sense to have just one patch? It's the same issue
>> anyway.
>
> The Fixes tags are different though.  I'd probably leave them as
> separate patches just because of that.

Depending how you look at the problem those tags are wrong.

Regards,
Arend



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Dan Carpenter]

On Tue, Jul 02, 2024 at 06:26:49PM +0200, Arend Van Spriel wrote:
> On July 2, 2024 5:37:10 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> > On Tue, Jul 02, 2024 at 06:29:20PM +0300, Kalle Valo wrote:
> > > Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> > > 
> > > > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > > 
> > > > > On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
> > > > > > brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
> > > > > > brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage value.
> > > > > 
> > > > > Since you're going to be resending anyway, please delete the space char
> > > > > from the start of the line.
> > > > > 
> > > > > It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
> > > > > It looks like it just goes to great lengths to preserve the original
> > > > > data in io_type...  So it likely is harmless enough but still a strange
> > > > > and complicated way write a no-op.
> > > > 
> > > > Not sure if it helps, but I tried to explain the reason in response to
> > > > patch 0 (cover letter).
> > > 
> > > Would it make more sense to have just one patch? It's the same issue
> > > anyway.
> > 
> > The Fixes tags are different though.  I'd probably leave them as
> > separate patches just because of that.
> 
> Depending how you look at the problem those tags are wrong.

Tags are often unfair in that way where you could blame different
commits and you have to pick one.  We end up picking the practical
commit instead of the most guilty commit.

Like if you do a partial or incorrect fix normally you'll be blamed
instead of the original patch which has no fix at all.  It works because
if the backporter hasn't tried to backport the partial fix, they don't
care about the complete fix either.

regards,
dan carpenter

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Su Hui]

On 2024/7/2 23:39, Arend Van Spriel wrote:
> On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote:
>
>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org> 
>>> wrote:
>>>
>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage 
>>>>> value.
>>>>
>>>> Since you're going to be resending anyway, please delete the space 
>>>> char
>>>> from the start of the line.
>>>>
>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
>>>> It looks like it just goes to great lengths to preserve the original
>>>> data in io_type...  So it likely is harmless enough but still a 
>>>> strange
>>>> and complicated way write a no-op.
>>>
>>> Not sure if it helps, but I tried to explain the reason in response to
>>> patch 0 (cover letter).
>>
>> Would it make more sense to have just one patch? It's the same issue
>> anyway.
>
> Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]).
It seems you will send a new patch to solve this issue.
And I guess there is no need for me to resend a v2 patchset or just one 
patch.

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Arend Van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1532 bytes --]

On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote:

> On 2024/7/2 23:39, Arend Van Spriel wrote:
>> On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote:
>>
>>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>>
>>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org>
>>>> wrote:
>>>>
>>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
>>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage
>>>>>> value.
>>>>>
>>>>> Since you're going to be resending anyway, please delete the space
>>>>> char
>>>>> from the start of the line.
>>>>>
>>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
>>>>> It looks like it just goes to great lengths to preserve the original
>>>>> data in io_type...  So it likely is harmless enough but still a
>>>>> strange
>>>>> and complicated way write a no-op.
>>>>
>>>> Not sure if it helps, but I tried to explain the reason in response to
>>>> patch 0 (cover letter).
>>>
>>> Would it make more sense to have just one patch? It's the same issue
>>> anyway.
>>
>> Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]).
> It seems you will send a new patch to solve this issue.
> And I guess there is no need for me to resend a v2 patchset or just one
> patch.

I am not entirely sure. If both gcc and clang would warn about using 
uninitialized data I would be fine with these patches rolled into one.

Regards,
Arend



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Su Hui]

On 2024/7/3 12:42, Arend Van Spriel wrote:
> On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote:
>
>> On 2024/7/2 23:39, Arend Van Spriel wrote:
>>> On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote:
>>>
>>>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>>>
>>>>> On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org>
>>>>> wrote:
>>>>>
>>>>>> On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
>>>>>>> brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes 
>>>>>>> it to
>>>>>>> brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage
>>>>>>> value.
>>>>>>
>>>>>> Since you're going to be resending anyway, please delete the space
>>>>>> char
>>>>>> from the start of the line.
>>>>>>
>>>>>> It's weird that brcmf_fil_cmd_data_get() uses the uninitialized 
>>>>>> data.
>>>>>> It looks like it just goes to great lengths to preserve the original
>>>>>> data in io_type...  So it likely is harmless enough but still a
>>>>>> strange
>>>>>> and complicated way write a no-op.
>>>>>
>>>>> Not sure if it helps, but I tried to explain the reason in 
>>>>> response to
>>>>> patch 0 (cover letter).
>>>>
>>>> Would it make more sense to have just one patch? It's the same issue
>>>> anyway.
>>>
>>> Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]).
>> It seems you will send a new patch to solve this issue.
>> And I guess there is no need for me to resend a v2 patchset or just one
>> patch.
>
> I am not entirely sure. If both gcc and clang would warn about using 
> uninitialized data I would be fine with these patches rolled into one.
It's sad that gcc wouldn't warn about this uninitialized data. And my 
gcc version
is  10.2.1 20210110 (Debian 10.2.1-6) .
By the way, I found a funny thing about this uninitialized warning.
Just with the patch as follows , gcc will give a uninitialized warning.

--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -130,6 +130,7 @@ static int brcmf_c_download_blob(struct brcmf_if *ifp,
         u32 status;
         s32 err;

+       err = brcmf_fil_iovar_int_get(ifp, statvar, &status);
         brcmf_dbg(TRACE, "Enter\n");

         chunk_buf = kzalloc(struct_size(chunk_buf, data, MAX_CHUNK_LEN),

It seems that gcc only issue this uninitialized warning in some sitution.
I think it's worth a patch to fix this uninitialized problem.  :)

Regards,
Su Hui

Re: [PATCH wireless 1/9] wifi: cfg80211: avoid garbage value of 'io_type' in brcmf_cfg80211_attach()

[Dan Carpenter]

On Wed, Jul 03, 2024 at 06:42:32AM +0200, Arend Van Spriel wrote:
> On July 3, 2024 3:42:18 AM Su Hui <suhui@nfschina.com> wrote:
> 
> > On 2024/7/2 23:39, Arend Van Spriel wrote:
> > > On July 2, 2024 5:29:27 PM Kalle Valo <kvalo@kernel.org> wrote:
> > > 
> > > > Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> > > > 
> > > > > On July 2, 2024 3:57:27 PM Dan Carpenter <dan.carpenter@linaro.org>
> > > > > wrote:
> > > > > 
> > > > > > On Tue, Jul 02, 2024 at 08:24:44PM +0800, Su Hui wrote:
> > > > > > > brcmf_fil_cmd_int_get() reads the value of 'io_type' and passes it to
> > > > > > > brcmf_fil_cmd_data_get(). Initialize 'io_type' to avoid garbage
> > > > > > > value.
> > > > > > 
> > > > > > Since you're going to be resending anyway, please delete the space
> > > > > > char
> > > > > > from the start of the line.
> > > > > > 
> > > > > > It's weird that brcmf_fil_cmd_data_get() uses the uninitialized data.
> > > > > > It looks like it just goes to great lengths to preserve the original
> > > > > > data in io_type...  So it likely is harmless enough but still a
> > > > > > strange
> > > > > > and complicated way write a no-op.
> > > > > 
> > > > > Not sure if it helps, but I tried to explain the reason in response to
> > > > > patch 0 (cover letter).
> > > > 
> > > > Would it make more sense to have just one patch? It's the same issue
> > > > anyway.
> > > 
> > > Yes, but I would solve it in brcmf_fil_* functions (fwil.[ch]).
> > It seems you will send a new patch to solve this issue.
> > And I guess there is no need for me to resend a v2 patchset or just one
> > patch.
> 
> I am not entirely sure. If both gcc and clang would warn about using
> uninitialized data I would be fine with these patches rolled into one.

We should definitely fix this, it's just a matter of how.  UBSan will
also detect these at run time.  And honestly, it's not clear to me where
these eventually do get copied to?  Is it to the firmware?  In that case
it might be that we'd treat these as a CVE.

regards,
dan carpenter