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

[Antonio Quartulli]

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@xxxxxxxxxxx>


--
Antonio Quartulli