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: <9e166687-1d35-4216-5c2c-a0e783ead406@gmail.com>
Date:   Wed, 5 Oct 2022 12:22:43 +0200
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:     Kalle Valo <kvalo@kernel.org>,
        "Lin Ian (CSSITB CSS ICW SW WFS / EE)" <ian.lin@infineon.com>
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>
From:   Arend Van Spriel <aspriel@gmail.com>
In-Reply-To: <87y1tupuqy.fsf@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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