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: <9686600b-f721-8849-545e-25a3854a4c9c@gmail.com>
Date:   Mon, 21 Nov 2022 16:08:47 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.3
Subject: Re: [PATCH] wifi: rtl8xxxu: Fix use after rcu_read_unlock()
To:     Ping-Ke Shih <pkshih@realtek.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     Jes Sorensen <Jes.Sorensen@gmail.com>
References: <a4c4f176-8903-6bf7-3b31-880ff7ebf195@gmail.com>
 <391fa36560424ac893d7fc52aa8e7a63@realtek.com>
Content-Language: en-US
From:   Bitterblue Smith <rtl8821cerfe2@gmail.com>
In-Reply-To: <391fa36560424ac893d7fc52aa8e7a63@realtek.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 21/11/2022 07:39, Ping-Ke Shih wrote:
> 
> 
>> -----Original Message-----
>> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> Sent: Monday, November 21, 2022 4:47 AM
>> To: linux-wireless@vger.kernel.org
>> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
>> Subject: [PATCH] wifi: rtl8xxxu: Fix use after rcu_read_unlock()
>>
>> ... in rtl8xxxu_bss_info_changed().
> 
> Mentioned where you modify in subject might be better.
> 
Okay.
>>
>> Commit a8b5aef2cca1 ("wifi: rtl8xxxu: gen2: Enable 40 MHz channel width")
>> introduced a line where the pointer returned by ieee80211_find_sta() is
>> used after rcu_read_unlock().
>>
>> Move rcu_read_unlock() a bit lower to fix this.
>>
>> Fixes: a8b5aef2cca1 ("wifi: rtl8xxxu: gen2: Enable 40 MHz channel width")
>> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
>> ---
>> My bad. I'm not sure how serious this problem is because I've been using
>> this code all this time (2+ months) without any trouble. I only noticed
>> it when moving some of this code into a new function.
> 
> 'sta' can be freed if a station leaves, and then causes use-after-free. But,
> luckily iface_work processes rtl8xxxu_bss_info_changed() and 'sta' leaving, so
> it will not happen at the same time.
> 
That's good then.

>> ---
>>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> index e8fcd531c437..28f136064297 100644
>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>> @@ -4645,7 +4645,6 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>>  			if (sta->deflink.ht_cap.cap &
>>  			    (IEEE80211_HT_CAP_SGI_40 | IEEE80211_HT_CAP_SGI_20))
>>  				sgi = 1;
>> -			rcu_read_unlock();
>>
>>  			highest_rate = fls(ramask) - 1;
>>  			if (highest_rate < DESC_RATE_MCS0) {
>> @@ -4670,6 +4669,7 @@ rtl8xxxu_bss_info_changed(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>>  				else
>>  					rarpt->txrate.bw = RATE_INFO_BW_20;
>>  			}
>> +			rcu_read_unlock();
>>  			bit_rate = cfg80211_calculate_bitrate(&rarpt->txrate);
>>  			rarpt->bit_rate = bit_rate;
>>  			rarpt->desc_rate = highest_rate;
>> --
>> 2.38.0
>>
>> ------Please consider the environment before printing this e-mail.