| Message ID | 20220927034138.20463-5-ian.lin@infineon.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | Fix connect/p2p issue series | expand |
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?
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
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 >> >
"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).
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
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
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
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
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
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);