[PATCH v3 0/5] Fix connect/p2p issue series

[PATCH v3 0/5] Fix connect/p2p issue series

[Ian Lin]

Fix several connect and p2p issues.

Note: there will be known build warning with commit
"fix P2P device discovery failure"
 - warning: parameter ‘vif’ set but not used [-Wunused-but-set-parameter]
However the purpose is to overwrite the pointer(vif) in function parameter,
and the pointer will be used ouside the function.

Changes in v3:
  - do typecast while variable assignment in brcmf_inform_single_bss()

Changes in v2:
  - fix email address mismatch with s-o-b
  - remove unnecessary type casting

Brian Henriquez (1):
  brcmfmac: correctly remove all p2p vif

Chung-Hsien Hsu (1):
  brcmfmac: fix P2P device discovery failure

Prasanna Kerekoppa (1):
  brcmfmac: Avoiding Connection delay

Syed Rafiuddeen (1):
  brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211
    layer

Wataru Gohda (1):
  brcmfmac: Fix for when connect request is not success

 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 32 +++++++++++++++++--
 .../broadcom/brcm80211/brcmfmac/p2p.c         | 10 ++++--
 2 files changed, 37 insertions(+), 5 deletions(-)

-- 
2.25.0

[PATCH v3 1/5] brcmfmac: correctly remove all p2p vif

[Ian Lin]

From: Brian Henriquez <brian.henriquez@cypress.com>

When deleting a P2P AGO interface we should make sure that
relevant entry in bss_idx[] array is removed. We were always
removing only 'vif' at P2PAPI_BSSCFG_CONNECTION before,
regardless of the number of created P2P AGO interfaces.
brcmfmac: correctly remove all p2p vif

Signed-off-by: Brian Henriquez <brian.henriquez@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
Signed-off-by: Ian Lin <ian.lin@infineon.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index 479041f070f9..b3d706a2e68c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -2424,8 +2424,12 @@ int brcmf_p2p_del_vif(struct wiphy *wiphy, struct wireless_dev *wdev)
 	brcmf_remove_interface(vif->ifp, true);
 
 	brcmf_cfg80211_arm_vif_event(cfg, NULL);
-	if (iftype != NL80211_IFTYPE_P2P_DEVICE)
-		p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
+	if (iftype != NL80211_IFTYPE_P2P_DEVICE) {
+		if (vif == p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif)
+			p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION].vif = NULL;
+		if (vif == p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION2].vif)
+			p2p->bss_idx[P2PAPI_BSSCFG_CONNECTION2].vif = NULL;
+	}
 
 	return err;
 }
-- 
2.25.0

[PATCH v3 2/5] brcmfmac: Fix for when connect request is not success

[Ian Lin]

From: Wataru Gohda <wataru.gohda@cypress.com>

Currently brcmfmac is expecting to be set for both
BRCMF_VIF_STATUS_EAP_SUCCESS and BRCMF_VIF_STATUS_EAP status bit based
on dongle event and those bits are cleared to complete connect request
successfully.

But when connect request is finished unsuccessfully, either
BRCMF_VIF_STATUS_EAP_SUCCESS / BRCMF_VIF_STATUS_EAP bits are not
cleared depending on how the connect fail event happens. These status
bits are carried over to following new connect request and this will lead
to generate below kernel warning for some case. Worst case status
mismatch happens between dongle and wpa_supplicant.

WARNING: ../net/wireless/sme.c:756 __cfg80211_connect_result+0x42c/0x4a0 [cfg80211]

The fix is to clear the BRCMF_VIF_STATUS_EAP_SUCCESS /
BRCMF_VIF_STATUS_EAP bits during the link down process and add to call
link down process when link down event received during
BRCMF_VIF_STATUS_CONNECTING as well as BRCMF_VIF_STATUS_CONNECTED
state.

Signed-off-by: Wataru Gohda <wataru.gohda@cypress.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
Signed-off-by: Ian Lin <ian.lin@infineon.com>
---
 .../broadcom/brcm80211/brcmfmac/cfg80211.c     | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 42068145a447..182b61aa346e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -1417,6 +1417,8 @@ static void brcmf_link_down(struct brcmf_cfg80211_vif *vif, u16 reason,
 					      locally_generated, GFP_KERNEL);
 	}
 	clear_bit(BRCMF_VIF_STATUS_CONNECTING, &vif->sme_state);
+	clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &vif->sme_state);
+	clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &vif->sme_state);
 	clear_bit(BRCMF_SCAN_STATUS_SUPPRESS, &cfg->scan_status);
 	brcmf_btcoex_set_mode(vif, BRCMF_BTCOEX_ENABLED, 0);
 	if (vif->profile.use_fwsup != BRCMF_PROFILE_FWSUP_NONE) {
@@ -2269,6 +2271,8 @@ brcmf_cfg80211_disconnect(struct wiphy *wiphy, struct net_device *ndev,
 
 	clear_bit(BRCMF_VIF_STATUS_CONNECTED, &ifp->vif->sme_state);
 	clear_bit(BRCMF_VIF_STATUS_CONNECTING, &ifp->vif->sme_state);
+	clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS, &ifp->vif->sme_state);
+	clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS, &ifp->vif->sme_state);
 	cfg80211_disconnected(ndev, reason_code, NULL, 0, true, GFP_KERNEL);
 
 	memcpy(&scbval.ea, &profile->bssid, ETH_ALEN);
@@ -6057,6 +6061,10 @@ brcmf_bss_connect_done(struct brcmf_cfg80211_info *cfg,
 				&ifp->vif->sme_state);
 			conn_params.status = WLAN_STATUS_SUCCESS;
 		} else {
+			clear_bit(BRCMF_VIF_STATUS_EAP_SUCCESS,
+				  &ifp->vif->sme_state);
+			clear_bit(BRCMF_VIF_STATUS_ASSOC_SUCCESS,
+				  &ifp->vif->sme_state);
 			conn_params.status = WLAN_STATUS_AUTH_TIMEOUT;
 		}
 		conn_params.links[0].bssid = profile->bssid;
@@ -6154,9 +6162,13 @@ brcmf_notify_connect_status(struct brcmf_if *ifp,
 	} else if (brcmf_is_linkdown(ifp->vif, e)) {
 		brcmf_dbg(CONN, "Linkdown\n");
 		if (!brcmf_is_ibssmode(ifp->vif) &&
-		    test_bit(BRCMF_VIF_STATUS_CONNECTED,
-			     &ifp->vif->sme_state)) {
-			if (memcmp(profile->bssid, e->addr, ETH_ALEN))
+		    (test_bit(BRCMF_VIF_STATUS_CONNECTED,
+			      &ifp->vif->sme_state) ||
+		     test_bit(BRCMF_VIF_STATUS_CONNECTING,
+			      &ifp->vif->sme_state))) {
+			if (test_bit(BRCMF_VIF_STATUS_CONNECTED,
+				     &ifp->vif->sme_state) &&
+			    memcmp(profile->bssid, e->addr, ETH_ALEN))
 				return err;
 
 			brcmf_bss_connect_done(cfg, ndev, e, false);
-- 
2.25.0

[PATCH v3 3/5] brcmfmac: Avoiding Connection delay

[Ian Lin]

From: Prasanna Kerekoppa <prasanna.kerekoppa@cypress.com>

Channel info passed by supplicant is not given to firmware. This causes
delay (about 3seconds) due to full scan. Supplicant already provides the
channel info for the specific SSID. channel_hint carries this channel
info for the connect call back.

Patch has been verified on 43012 and 43455.

Signed-off-by: Prasanna Kerekoppa <prasanna.kerekoppa@cypress.com>
Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
Signed-off-by: Ian Lin <ian.lin@infineon.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 182b61aa346e..6c37da42e61b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -2049,6 +2049,12 @@ brcmf_cfg80211_connect(struct wiphy *wiphy, struct net_device *ndev,
 		return -EOPNOTSUPP;
 	}
 
+	if (sme->channel_hint)
+		chan = sme->channel_hint;
+
+	if (sme->bssid_hint)
+		sme->bssid = sme->bssid_hint;
+
 	if (ifp->vif == cfg->p2p.bss_idx[P2PAPI_BSSCFG_PRIMARY].vif) {
 		/* A normal (non P2P) connection request setup. */
 		ie = NULL;
-- 
2.25.0

[PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Ian Lin]

From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>

cfg80211 layer on DUT STA is disconnecting ongoing connection attempt after
receiving association response, because cfg80211 layer does not have valid
AP bss information. On association response event, brcmfmac communicates
the AP bss information to cfg80211 layer, but SSID seem to be empty in AP
bss information, and cfg80211 layer prints kernel warning and then
disconnects the ongoing connection attempt.

SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
updating the SSID for hidden AP while informing its bss information
to cfg80211 layer.

Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
Signed-off-by: Ian Lin <ian.lin@infineon.com>
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 6c37da42e61b..3560afe0ccfe 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
 	u8 *notify_ie;
 	size_t notify_ielen;
 	struct cfg80211_inform_bss bss_data = {};
+	struct brcmf_tlv *ssid;
 
 	if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
 		bphy_err(drvr, "Bss info is larger than buffer. Discarding\n");
@@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
 	notify_ielen = le32_to_cpu(bi->ie_length);
 	bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
 
+	ssid = (struct brcmf_tlv *)
+		brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);
+	if (ssid && ssid->data[0] == '\0' && ssid->len == bi->SSID_len) {
+		/* Update SSID for hidden AP */
+		memcpy(ssid->data, bi->SSID, bi->SSID_len);
+	}
+
 	brcmf_dbg(CONN, "bssid: %pM\n", bi->BSSID);
 	brcmf_dbg(CONN, "Channel: %d(%d)\n", channel, freq);
 	brcmf_dbg(CONN, "Capability: %X\n", notify_capability);
-- 
2.25.0

[PATCH v3 5/5] brcmfmac: fix P2P device discovery failure

[Ian Lin]

From: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>

Commit 2de64ca7c9fa ("brcmfmac: p2p: Deal with set but unused
variables") removed the setting of vif for p2p device discovery in
brcmf_p2p_scan_prep(), causing the discovery failure.

Add back the setting to brcmf_p2p_scan_prep() to fix this.

Fixes: 2de64ca7c9fa ("brcmfmac: p2p: Deal with set but unused variables")
Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
Signed-off-by: Ian Lin <ian.lin@infineon.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
index b3d706a2e68c..068f8fe0e0c4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
@@ -912,6 +912,8 @@ int brcmf_p2p_scan_prep(struct wiphy *wiphy,
 		if (err)
 			return err;
 
+		vif = p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
+
 		/* override .run_escan() callback. */
 		cfg->escan_info.run = brcmf_p2p_run_escan;
 	}
-- 
2.25.0

Re: [PATCH v3 0/5] Fix connect/p2p issue series

[Kalle Valo]

Ian Lin <ian.lin@infineon.com> writes:

> Fix several connect and p2p issues.
>
> Note: there will be known build warning with commit
> "fix P2P device discovery failure"
>  - warning: parameter ‘vif’ set but not used [-Wunused-but-set-parameter]
> However the purpose is to overwrite the pointer(vif) in function parameter,
> and the pointer will be used ouside the function.

We have a rule that there can be no build warnings so this needs to
solved somehow.

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

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

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Kalle Valo]

Ian Lin <ian.lin@infineon.com> writes:

> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>
> cfg80211 layer on DUT STA is disconnecting ongoing connection attempt after
> receiving association response, because cfg80211 layer does not have valid
> AP bss information. On association response event, brcmfmac communicates
> the AP bss information to cfg80211 layer, but SSID seem to be empty in AP
> bss information, and cfg80211 layer prints kernel warning and then
> disconnects the ongoing connection attempt.
>
> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
> updating the SSID for hidden AP while informing its bss information
> to cfg80211 layer.
>
> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
> Signed-off-by: Ian Lin <ian.lin@infineon.com>
> ---
>  .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> index 6c37da42e61b..3560afe0ccfe 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>  	u8 *notify_ie;
>  	size_t notify_ielen;
>  	struct cfg80211_inform_bss bss_data = {};
> +	struct brcmf_tlv *ssid;
>  
>  	if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>  		bphy_err(drvr, "Bss info is larger than buffer. Discarding\n");
> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>  	notify_ielen = le32_to_cpu(bi->ie_length);
>  	bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>  
> +	ssid = (struct brcmf_tlv *)
> +		brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);

This still casts away the const. For some reason brcmf_parse_tlvs()
takes a const buffer:

static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf, int buflen, uint key)

I'm not familiar with brcmfmac internal so I don't know why it does
that, but that means the buffer cannot be modified. If you need to
modify the ssid should you make a copy of it?

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

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

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Lin Ian (CSSITB CSS ICW SW WFS / EE)]

On 9/28/2022 2:38 PM, Kalle Valo wrote:
> Caution: This e-mail originated outside Infineon Technologies. Do not click on links or open attachments unless you validate it is safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>
>
>
> Ian Lin <ian.lin@infineon.com> writes:
>
>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>
>> cfg80211 layer on DUT STA is disconnecting ongoing connection attempt after
>> receiving association response, because cfg80211 layer does not have valid
>> AP bss information. On association response event, brcmfmac communicates
>> the AP bss information to cfg80211 layer, but SSID seem to be empty in AP
>> bss information, and cfg80211 layer prints kernel warning and then
>> disconnects the ongoing connection attempt.
>>
>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>> updating the SSID for hidden AP while informing its bss information
>> to cfg80211 layer.
>>
>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>> ---
>>   .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index 6c37da42e61b..3560afe0ccfe 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>>        u8 *notify_ie;
>>        size_t notify_ielen;
>>        struct cfg80211_inform_bss bss_data = {};
>> +     struct brcmf_tlv *ssid;
>>
>>        if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>                bphy_err(drvr, "Bss info is larger than buffer. Discarding\n");
>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct brcmf_cfg80211_info *cfg,
>>        notify_ielen = le32_to_cpu(bi->ie_length);
>>        bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>
>> +     ssid = (struct brcmf_tlv *)
>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);
> This still casts away the const. For some reason brcmf_parse_tlvs()
> takes a const buffer:
>
> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf, int buflen, uint key)
>
> I'm not familiar with brcmfmac internal so I don't know why it does
> that, but that means the buffer cannot be modified. If you need to
> modify the ssid should you make a copy of it?

In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
it will find the key in buf and return the address of this key, as the 
return pointer.
This function don't intend caller to modify content of buf in most 
cases, so it defines a const return pointer.
But in this case, it just need to do it, so I need the typecast here.

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

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Lin Ian (CSSITB CSS ICW SW WFS / EE)]

On 9/28/2022 6:07 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
>
>
> On 9/28/2022 2:38 PM, Kalle Valo wrote:
>> Caution: This e-mail originated outside Infineon Technologies. Do not 
>> click on links or open attachments unless you validate it is 
>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>>
>>
>>
>> Ian Lin <ian.lin@infineon.com> writes:
>>
>>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>
>>> cfg80211 layer on DUT STA is disconnecting ongoing connection 
>>> attempt after
>>> receiving association response, because cfg80211 layer does not have 
>>> valid
>>> AP bss information. On association response event, brcmfmac 
>>> communicates
>>> the AP bss information to cfg80211 layer, but SSID seem to be empty 
>>> in AP
>>> bss information, and cfg80211 layer prints kernel warning and then
>>> disconnects the ongoing connection attempt.
>>>
>>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>>> updating the SSID for hidden AP while informing its bss information
>>> to cfg80211 layer.
>>>
>>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>>> ---
>>>   .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8 
>>> ++++++++
>>>   1 file changed, 8 insertions(+)
>>>
>>> diff --git 
>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> index 6c37da42e61b..3560afe0ccfe 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct 
>>> brcmf_cfg80211_info *cfg,
>>>        u8 *notify_ie;
>>>        size_t notify_ielen;
>>>        struct cfg80211_inform_bss bss_data = {};
>>> +     struct brcmf_tlv *ssid;
>>>
>>>        if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>>                bphy_err(drvr, "Bss info is larger than buffer. 
>>> Discarding\n");
>>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct 
>>> brcmf_cfg80211_info *cfg,
>>>        notify_ielen = le32_to_cpu(bi->ie_length);
>>>        bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>>
>>> +     ssid = (struct brcmf_tlv *)
>>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);
>> This still casts away the const. For some reason brcmf_parse_tlvs()
>> takes a const buffer:
>>
>> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf, int 
>> buflen, uint key)
>>
>> I'm not familiar with brcmfmac internal so I don't know why it does
>> that, but that means the buffer cannot be modified. If you need to
>> modify the ssid should you make a copy of it?
>
> In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
> it will find the key in buf and return the address of this key, as the 
> return pointer.
> This function don't intend caller to modify content of buf in most 
> cases, so it defines a const return pointer.
> But in this case, it just need to do it, so I need the typecast here.
>
Do you accept the typecast here?
Thank you.

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

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Kalle Valo]

"Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com> writes:

> On 9/28/2022 6:07 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
>>
>>
>> On 9/28/2022 2:38 PM, Kalle Valo wrote:
>>> Caution: This e-mail originated outside Infineon Technologies. Do
>>> not click on links or open attachments unless you validate it is
>>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>>>
>>>
>>>
>>> Ian Lin <ian.lin@infineon.com> writes:
>>>
>>>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>
>>>> cfg80211 layer on DUT STA is disconnecting ongoing connection
>>>> attempt after
>>>> receiving association response, because cfg80211 layer does not
>>>> have valid
>>>> AP bss information. On association response event, brcmfmac
>>>> communicates
>>>> the AP bss information to cfg80211 layer, but SSID seem to be
>>>> empty in AP
>>>> bss information, and cfg80211 layer prints kernel warning and then
>>>> disconnects the ongoing connection attempt.
>>>>
>>>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>>>> updating the SSID for hidden AP while informing its bss information
>>>> to cfg80211 layer.
>>>>
>>>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>>>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>>>> ---
>>>>   .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8
>>>> ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git
>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> index 6c37da42e61b..3560afe0ccfe 100644
>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct
>>>> brcmf_cfg80211_info *cfg,
>>>>        u8 *notify_ie;
>>>>        size_t notify_ielen;
>>>>        struct cfg80211_inform_bss bss_data = {};
>>>> +     struct brcmf_tlv *ssid;
>>>>
>>>>        if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>>>                bphy_err(drvr, "Bss info is larger than buffer.
>>>> Discarding\n");
>>>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct
>>>> brcmf_cfg80211_info *cfg,
>>>>        notify_ielen = le32_to_cpu(bi->ie_length);
>>>>        bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>>>
>>>> +     ssid = (struct brcmf_tlv *)
>>>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);
>>> This still casts away the const. For some reason brcmf_parse_tlvs()
>>> takes a const buffer:
>>>
>>> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf,
>>> int buflen, uint key)
>>>
>>> I'm not familiar with brcmfmac internal so I don't know why it does
>>> that, but that means the buffer cannot be modified. If you need to
>>> modify the ssid should you make a copy of it?
>>
>> In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
>> it will find the key in buf and return the address of this key, as
>> the return pointer.
>> This function don't intend caller to modify content of buf in most
>> cases, so it defines a const return pointer.
>> But in this case, it just need to do it, so I need the typecast here.
>
> Do you accept the typecast here?

To me writing a const data is wrong. IIRC it was something like six
months ago that rtw89 was also writing a const variable and it caused a
crash because the memory was in a read-only area (or something like
that).

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

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

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Arend Van Spriel]

On 10/5/2022 9:58 AM, Kalle Valo wrote:
> "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com> writes:
> 
>> On 9/28/2022 6:07 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
>>>
>>>
>>> On 9/28/2022 2:38 PM, Kalle Valo wrote:
>>>> Caution: This e-mail originated outside Infineon Technologies. Do
>>>> not click on links or open attachments unless you validate it is
>>>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>>>>
>>>>
>>>>
>>>> Ian Lin <ian.lin@infineon.com> writes:
>>>>
>>>>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>>
>>>>> cfg80211 layer on DUT STA is disconnecting ongoing connection
>>>>> attempt after
>>>>> receiving association response, because cfg80211 layer does not
>>>>> have valid
>>>>> AP bss information. On association response event, brcmfmac
>>>>> communicates
>>>>> the AP bss information to cfg80211 layer, but SSID seem to be
>>>>> empty in AP
>>>>> bss information, and cfg80211 layer prints kernel warning and then
>>>>> disconnects the ongoing connection attempt.
>>>>>
>>>>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>>>>> updating the SSID for hidden AP while informing its bss information
>>>>> to cfg80211 layer.
>>>>>
>>>>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>>>>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>>>>> ---
>>>>>    .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c   | 8
>>>>> ++++++++
>>>>>    1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git
>>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> index 6c37da42e61b..3560afe0ccfe 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct
>>>>> brcmf_cfg80211_info *cfg,
>>>>>         u8 *notify_ie;
>>>>>         size_t notify_ielen;
>>>>>         struct cfg80211_inform_bss bss_data = {};
>>>>> +     struct brcmf_tlv *ssid;
>>>>>
>>>>>         if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>>>>                 bphy_err(drvr, "Bss info is larger than buffer.
>>>>> Discarding\n");
>>>>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct
>>>>> brcmf_cfg80211_info *cfg,
>>>>>         notify_ielen = le32_to_cpu(bi->ie_length);
>>>>>         bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>>>>
>>>>> +     ssid = (struct brcmf_tlv *)
>>>>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID);
>>>> This still casts away the const. For some reason brcmf_parse_tlvs()
>>>> takes a const buffer:
>>>>
>>>> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf,
>>>> int buflen, uint key)
>>>>
>>>> I'm not familiar with brcmfmac internal so I don't know why it does
>>>> that, but that means the buffer cannot be modified. If you need to
>>>> modify the ssid should you make a copy of it?
>>>
>>> In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
>>> it will find the key in buf and return the address of this key, as
>>> the return pointer.
>>> This function don't intend caller to modify content of buf in most
>>> cases, so it defines a const return pointer.
>>> But in this case, it just need to do it, so I need the typecast here.
>>
>> Do you accept the typecast here?
> 
> To me writing a const data is wrong. IIRC it was something like six
> months ago that rtw89 was also writing a const variable and it caused a
> crash because the memory was in a read-only area (or something like
> that).

So how should this be solved. The pointer returned by the 
brcmf_parse_tlvs() function is pointing inside the buffer passed as 
first argument, ie. notify_ie which is non-const. So it is perfectly 
safe to do the cast as suggested here. We could do a pointer-arithmetic 
dance here to avoid the cast, but that only make things more obscure.

Regards,
Arend

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Lin Ian (CSSITB CSS ICW SW WFS / EE)]

On 10/5/2022 6:22 PM, Arend Van Spriel wrote:
> Caution: This e-mail originated outside Infineon Technologies. Do not 
> click on links or open attachments unless you validate it is 
> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>.
>
>
>
> On 10/5/2022 9:58 AM, Kalle Valo wrote:
>> "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com> writes:
>>
>>> On 9/28/2022 6:07 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
>>>>
>>>>
>>>> On 9/28/2022 2:38 PM, Kalle Valo wrote:
>>>>> Caution: This e-mail originated outside Infineon Technologies. Do
>>>>> not click on links or open attachments unless you validate it is
>>>>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>. 
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Ian Lin <ian.lin@infineon.com> writes:
>>>>>
>>>>>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>>>
>>>>>> cfg80211 layer on DUT STA is disconnecting ongoing connection
>>>>>> attempt after
>>>>>> receiving association response, because cfg80211 layer does not
>>>>>> have valid
>>>>>> AP bss information. On association response event, brcmfmac
>>>>>> communicates
>>>>>> the AP bss information to cfg80211 layer, but SSID seem to be
>>>>>> empty in AP
>>>>>> bss information, and cfg80211 layer prints kernel warning and then
>>>>>> disconnects the ongoing connection attempt.
>>>>>>
>>>>>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>>>>>> updating the SSID for hidden AP while informing its bss information
>>>>>> to cfg80211 layer.
>>>>>>
>>>>>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>>>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>>>>>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>>>>>> ---
>>>>>> .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 8
>>>>>> ++++++++
>>>>>>    1 file changed, 8 insertions(+)
>>>>>>
>>>>>> diff --git
>>>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> index 6c37da42e61b..3560afe0ccfe 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct
>>>>>> brcmf_cfg80211_info *cfg,
>>>>>>         u8 *notify_ie;
>>>>>>         size_t notify_ielen;
>>>>>>         struct cfg80211_inform_bss bss_data = {};
>>>>>> +     struct brcmf_tlv *ssid;
>>>>>>
>>>>>>         if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>>>>>                 bphy_err(drvr, "Bss info is larger than buffer.
>>>>>> Discarding\n");
>>>>>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct
>>>>>> brcmf_cfg80211_info *cfg,
>>>>>>         notify_ielen = le32_to_cpu(bi->ie_length);
>>>>>>         bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>>>>>
>>>>>> +     ssid = (struct brcmf_tlv *)
>>>>>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, 
>>>>>> WLAN_EID_SSID);
>>>>> This still casts away the const. For some reason brcmf_parse_tlvs()
>>>>> takes a const buffer:
>>>>>
>>>>> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf,
>>>>> int buflen, uint key)
>>>>>
>>>>> I'm not familiar with brcmfmac internal so I don't know why it does
>>>>> that, but that means the buffer cannot be modified. If you need to
>>>>> modify the ssid should you make a copy of it?
>>>>
>>>> In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
>>>> it will find the key in buf and return the address of this key, as
>>>> the return pointer.
>>>> This function don't intend caller to modify content of buf in most
>>>> cases, so it defines a const return pointer.
>>>> But in this case, it just need to do it, so I need the typecast here.
>>>
>>> Do you accept the typecast here?
>>
>> To me writing a const data is wrong. IIRC it was something like six
>> months ago that rtw89 was also writing a const variable and it caused a
>> crash because the memory was in a read-only area (or something like
>> that).
>
> So how should this be solved. The pointer returned by the
> brcmf_parse_tlvs() function is pointing inside the buffer passed as
> first argument, ie. notify_ie which is non-const. So it is perfectly
> safe to do the cast as suggested here. We could do a pointer-arithmetic
> dance here to avoid the cast, but that only make things more obscure.
>
I may calculate the offset and cast on notify_ie.
Than the code will be like this, is that ok?

u8 *notify_ie;
int ssid_offset;
ssid_offset = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID) - 
(struct brcmf_tlv *)notify_ie;
memcpy(notify_ie + ssid_offset + offsetof(struct brcmf_tlv, data), 
bi->SSID, bi->SSID_len);


> Regards,
> Arend

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Arend Van Spriel]

On 10/6/2022 10:34 AM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
> 
> 
> On 10/5/2022 6:22 PM, Arend Van Spriel wrote:
>> Caution: This e-mail originated outside Infineon Technologies. Do not 
>> click on links or open attachments unless you validate it is 
>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>. 
>>
>>
>>
>>
>> On 10/5/2022 9:58 AM, Kalle Valo wrote:
>>> "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com> writes:
>>>
>>>> On 9/28/2022 6:07 PM, Lin Ian (CSSITB CSS ICW SW WFS / EE) wrote:
>>>>>
>>>>>
>>>>> On 9/28/2022 2:38 PM, Kalle Valo wrote:
>>>>>> Caution: This e-mail originated outside Infineon Technologies. Do
>>>>>> not click on links or open attachments unless you validate it is
>>>>>> safe<https://intranet-content.infineon.com/explore/aboutinfineon/rules/informationsecurity/ug/SocialEngineering/Pages/SocialEngineeringElements_en.aspx>. 
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Ian Lin <ian.lin@infineon.com> writes:
>>>>>>
>>>>>>> From: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>>>>
>>>>>>> cfg80211 layer on DUT STA is disconnecting ongoing connection
>>>>>>> attempt after
>>>>>>> receiving association response, because cfg80211 layer does not
>>>>>>> have valid
>>>>>>> AP bss information. On association response event, brcmfmac
>>>>>>> communicates
>>>>>>> the AP bss information to cfg80211 layer, but SSID seem to be
>>>>>>> empty in AP
>>>>>>> bss information, and cfg80211 layer prints kernel warning and then
>>>>>>> disconnects the ongoing connection attempt.
>>>>>>>
>>>>>>> SSID is empty in SSID IE, but 'bi->SSID' contains a valid SSID, so
>>>>>>> updating the SSID for hidden AP while informing its bss information
>>>>>>> to cfg80211 layer.
>>>>>>>
>>>>>>> Signed-off-by: Syed Rafiuddeen <syed.rafiuddeen@cypress.com>
>>>>>>> Signed-off-by: Chung-Hsien Hsu <chung-hsien.hsu@infineon.com>
>>>>>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@infineon.com>
>>>>>>> Signed-off-by: Ian Lin <ian.lin@infineon.com>
>>>>>>> ---
>>>>>>> .../net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 8
>>>>>>> ++++++++
>>>>>>>    1 file changed, 8 insertions(+)
>>>>>>>
>>>>>>> diff --git
>>>>>>> a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>>> index 6c37da42e61b..3560afe0ccfe 100644
>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>>>>>>> @@ -3003,6 +3003,7 @@ static s32 brcmf_inform_single_bss(struct
>>>>>>> brcmf_cfg80211_info *cfg,
>>>>>>>         u8 *notify_ie;
>>>>>>>         size_t notify_ielen;
>>>>>>>         struct cfg80211_inform_bss bss_data = {};
>>>>>>> +     struct brcmf_tlv *ssid;
>>>>>>>
>>>>>>>         if (le32_to_cpu(bi->length) > WL_BSS_INFO_MAX) {
>>>>>>>                 bphy_err(drvr, "Bss info is larger than buffer.
>>>>>>> Discarding\n");
>>>>>>> @@ -3032,6 +3033,13 @@ static s32 brcmf_inform_single_bss(struct
>>>>>>> brcmf_cfg80211_info *cfg,
>>>>>>>         notify_ielen = le32_to_cpu(bi->ie_length);
>>>>>>>         bss_data.signal = (s16)le16_to_cpu(bi->RSSI) * 100;
>>>>>>>
>>>>>>> +     ssid = (struct brcmf_tlv *)
>>>>>>> +             brcmf_parse_tlvs(notify_ie, notify_ielen, 
>>>>>>> WLAN_EID_SSID);
>>>>>> This still casts away the const. For some reason brcmf_parse_tlvs()
>>>>>> takes a const buffer:
>>>>>>
>>>>>> static const struct brcmf_tlv *brcmf_parse_tlvs(const void *buf,
>>>>>> int buflen, uint key)
>>>>>>
>>>>>> I'm not familiar with brcmfmac internal so I don't know why it does
>>>>>> that, but that means the buffer cannot be modified. If you need to
>>>>>> modify the ssid should you make a copy of it?
>>>>>
>>>>> In brcmf_parse_tlvs(const void *buf, int buflen, uint key),
>>>>> it will find the key in buf and return the address of this key, as
>>>>> the return pointer.
>>>>> This function don't intend caller to modify content of buf in most
>>>>> cases, so it defines a const return pointer.
>>>>> But in this case, it just need to do it, so I need the typecast here.
>>>>
>>>> Do you accept the typecast here?
>>>
>>> To me writing a const data is wrong. IIRC it was something like six
>>> months ago that rtw89 was also writing a const variable and it caused a
>>> crash because the memory was in a read-only area (or something like
>>> that).
>>
>> So how should this be solved. The pointer returned by the
>> brcmf_parse_tlvs() function is pointing inside the buffer passed as
>> first argument, ie. notify_ie which is non-const. So it is perfectly
>> safe to do the cast as suggested here. We could do a pointer-arithmetic
>> dance here to avoid the cast, but that only make things more obscure.
>>
> I may calculate the offset and cast on notify_ie.
> Than the code will be like this, is that ok?
> 
> u8 *notify_ie;
> int ssid_offset;
> ssid_offset = brcmf_parse_tlvs(notify_ie, notify_ielen, WLAN_EID_SSID) - 
> (struct brcmf_tlv *)notify_ie;
> memcpy(notify_ie + ssid_offset + offsetof(struct brcmf_tlv, data), 
> bi->SSID, bi->SSID_len);

Hi Ian,

I am strarting to doubt the entire patch now. The notify_ie contains 
beacon/proberesp elements in TLV format. So how can this work by simply 
copying an SSID into the notify_ie buffer. The patch says: "SSID is 
empty in SSID IE" so I would conclude the element to be two bytes, ie. T 
= WLAN_EID_SSID and L=0 (no V). If this is true, it means the SSID will 
overwrite/corrupt the TLVs located after the SSID. Also the length is 
not corrected. Maybe I am mistaken and the SSID element for the 
Hidden-SSID scenarion is different. Will check the 802.11 spec.

Regards,
Arend

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Johannes Berg]

On Mon, 2022-10-10 at 11:23 +0200, Arend Van Spriel wrote:
>  Maybe I am mistaken and the SSID element for the 
> Hidden-SSID scenarion is different. Will check the 802.11 spec.
> 

hidden SSID isn't in the spec, but these days is typically implemented
by a zero-size SSID element:

 00 00

In the past, sometimes it would also be done by some APs with a zeroed
SSID element, e.g. for the SSID "myAP" you could see

 00 04 00 00 00 00

instead.

johannes

Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing its bss to cfg80211 layer

[Arend Van Spriel]

On 10/10/2022 11:41 AM, Johannes Berg wrote:
> On Mon, 2022-10-10 at 11:23 +0200, Arend Van Spriel wrote:
>>   Maybe I am mistaken and the SSID element for the
>> Hidden-SSID scenarion is different. Will check the 802.11 spec.
>>
> 
> hidden SSID isn't in the spec, but these days is typically implemented
> by a zero-size SSID element:
> 
>   00 00
> 
> In the past, sometimes it would also be done by some APs with a zeroed
> SSID element, e.g. for the SSID "myAP" you could see
> 
>   00 04 00 00 00 00
> 
> instead.

Thanks, Johannes

If both species are out there than it still means blindly copying things 
into the TLV buffer is not the proper thing to do here.

Regards,
Arend

> johannes