Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <51236a0c-5210-533b-7755-94145e6c5a1d@infineon.com>
Date:   Thu, 6 Oct 2022 16:34:49 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.1
Subject: Re: [PATCH v3 4/5] brcmfmac: Update SSID of hidden AP while informing
 its bss to cfg80211 layer
Content-Language: en-US
To:     Arend Van Spriel <aspriel@gmail.com>, Kalle Valo <kvalo@kernel.org>
CC:     <linux-wireless@vger.kernel.org>,
        <brcm80211-dev-list@broadcom.com>, <franky.lin@broadcom.com>,
        <hante.meuleman@broadcom.com>, <Double.Lo@infineon.com>
References: <20220927034138.20463-1-ian.lin@infineon.com>
 <20220927034138.20463-5-ian.lin@infineon.com> <874jwsrojr.fsf@kernel.org>
 <4e602611-aed5-dfe7-6ce7-42d1fc7ca53e@infineon.com>
 <b43d934d-f3e8-9459-0096-11908df27c15@infineon.com>
 <87y1tupuqy.fsf@kernel.org> <9e166687-1d35-4216-5c2c-a0e783ead406@gmail.com>
From:   "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com>
In-Reply-To: <9e166687-1d35-4216-5c2c-a0e783ead406@gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


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