Re: [PATCH] wifi: cfg80211: Lock wiphy in cfg80211_get_station

[Antonio Quartulli <a@unstable.cc>]

Hi,

On 21/05/2024 14:15, Remi Pommarel wrote:
> On Tue, May 21, 2024 at 09:43:56AM +0200, Antonio Quartulli wrote:
>> Hi,
>>
>> On 18/05/2024 17:50, Remi Pommarel wrote:
>>> Wiphy should be locked before calling rdev_get_station() (see lockdep
>>> assert in ieee80211_get_station()).
>>
>> Adding the lock is fine as nowadays it is taken in pre_doit and released in
>> post_doit (with some exceptions). Therefore when invoking .get_station from
>> a side path the lock should be taken too.
>>
>> It was actually a05829a7222e9d10c416dd2dbbf3929fe6646b89 that introduced
>> this requirement AFAICS.
> 
> IIUC lock requirement was already there before this commit, only it was on
> rtnl lock to be taken instead of wiphy one.

You're right.

> 
>>
>>>
>>> This fixes the following kernel NULL dereference:
>>
>> As already said by Johannes, I am not sure it truly fixes this NULL
>> dereference though.
>>
>> Have you checked where in ath10k_sta_statistics this is exactly happening?
>> Do you think some sta was partly released and thus fields were NULLified?
> 
> ath10k_sta_statistics+0x10 is associated with the arsta->arvif->ar
> statement. It crashes because arsta->arvif is NULL.
> 
> Here is a scenario that explains the crash where the same sta
> disconnects then reconnects quickly (e.g. OPEN network):
> 
> 
> CPU0:                                       CPU1:
> 
> batadv_v_elp_periodic_work()
>   queue_work(batadv_v_elp_get_throughput)
> 
>                                              ieee80211_del_station()
>                                              ieee80211_add_station()
>                                               sta_info_insert()
>                                                list_add_tail_rcu()
>                                                ath10k_sta_state()
>                                                 memset(arsta, 0, sizeof(arsta))
> batadv_v_elp_get_throughput()
>   cfg80211_get_station()
>    ieee80211_get_station() <-- Find sta with its MAC in list
>     ath10k_sta_statistics()
>      arsta->arvif->ar <-- arsta is still zeroed
> 
> 
> In other words if the same sta has enough time to disconnect then
> reconnect before batadv_v_elp_get_throughput get scheduled, sta could be
> partially initialized when ath10k_sta_statistics() is called. Locking
> the wiphy ensure sta is fully initialized if found.
> 

We were just wondering how you could get the arvif being NULL and your 
explanation makes sense.

This said, holding the lock is required when invoking get_station via 
netlink, therefore it's meaningful to hold it even when side invoking it 
from another module.

Acked-by: Antonio Quartulli <a@unstable.cc>


-- 
Antonio Quartulli