Cmt: [SRU][F][PATCH 1/1] wifi: cfg80211: Lock wiphy in cfg80211_get_station

[Magali Lemes]

On 19/11/2024 08:04, Massimiliano Pellizzer wrote:
> From: Remi Pommarel <repk at triplefau.lt>
> 
> Wiphy should be locked before calling rdev_get_station() (see lockdep
> assert in ieee80211_get_station()).
> 
> This fixes the following kernel NULL dereference:
> 
>   Unable to handle kernel NULL pointer dereference at virtual address 0000000000000050
>   Mem abort info:
>     ESR = 0x0000000096000006
>     EC = 0x25: DABT (current EL), IL = 32 bits
>     SET = 0, FnV = 0
>     EA = 0, S1PTW = 0
>     FSC = 0x06: level 2 translation fault
>   Data abort info:
>     ISV = 0, ISS = 0x00000006
>     CM = 0, WnR = 0
>   user pgtable: 4k pages, 48-bit VAs, pgdp=0000000003001000
>   [0000000000000050] pgd=0800000002dca003, p4d=0800000002dca003, pud=08000000028e9003, pmd=0000000000000000
>   Internal error: Oops: 0000000096000006 [#1] SMP
>   Modules linked in: netconsole dwc3_meson_g12a dwc3_of_simple dwc3 ip_gre gre ath10k_pci ath10k_core ath9k ath9k_common ath9k_hw ath
>   CPU: 0 PID: 1091 Comm: kworker/u8:0 Not tainted 6.4.0-02144-g565f9a3a7911-dirty #705
>   Hardware name: RPT (r1) (DT)
>   Workqueue: bat_events batadv_v_elp_throughput_metric_update
>   pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>   pc : ath10k_sta_statistics+0x10/0x2dc [ath10k_core]
>   lr : sta_set_sinfo+0xcc/0xbd4
>   sp : ffff000007b43ad0
>   x29: ffff000007b43ad0 x28: ffff0000071fa900 x27: ffff00000294ca98
>   x26: ffff000006830880 x25: ffff000006830880 x24: ffff00000294c000
>   x23: 0000000000000001 x22: ffff000007b43c90 x21: ffff800008898acc
>   x20: ffff00000294c6e8 x19: ffff000007b43c90 x18: 0000000000000000
>   x17: 445946354d552d78 x16: 62661f7200000000 x15: 57464f445946354d
>   x14: 0000000000000000 x13: 00000000000000e3 x12: d5f0acbcebea978e
>   x11: 00000000000000e3 x10: 000000010048fe41 x9 : 0000000000000000
>   x8 : ffff000007b43d90 x7 : 000000007a1e2125 x6 : 0000000000000000
>   x5 : ffff0000024e0900 x4 : ffff800000a0250c x3 : ffff000007b43c90
>   x2 : ffff00000294ca98 x1 : ffff000006831920 x0 : 0000000000000000
>   Call trace:
>    ath10k_sta_statistics+0x10/0x2dc [ath10k_core]
>    sta_set_sinfo+0xcc/0xbd4
>    ieee80211_get_station+0x2c/0x44
>    cfg80211_get_station+0x80/0x154
>    batadv_v_elp_get_throughput+0x138/0x1fc
>    batadv_v_elp_throughput_metric_update+0x1c/0xa4
>    process_one_work+0x1ec/0x414
>    worker_thread+0x70/0x46c
>    kthread+0xdc/0xe0
>    ret_from_fork+0x10/0x20
>   Code: a9bb7bfd 910003fd a90153f3 f9411c40 (f9402814)
> 
> This happens because STA has time to disconnect and reconnect before
> batadv_v_elp_throughput_metric_update() delayed work gets scheduled. In
> this situation, ath10k_sta_state() can be in the middle of resetting
> arsta data when the work queue get chance to be scheduled and ends up
> accessing it. Locking wiphy prevents that.
> 
> Fixes: 7406353d43c8 ("cfg80211: implement cfg80211_get_station cfg80211 API")
> Signed-off-by: Remi Pommarel <repk at triplefau.lt>
> Reviewed-by: Nicolas Escande <nico.escande at gmail.com>
> Acked-by: Antonio Quartulli <a at unstable.cc>
> Link: https://msgid.link/983b24a6a176e0800c01aedcd74480d9b551cb13.1716046653.git.repk@triplefau.lt
> Signed-off-by: Johannes Berg <johannes.berg at intel.com>
> (backported from commit 642f89daa34567d02f312d03e41523a894906dae)
> [mpellizzer: the original patch uses the function wiphy_lock(), which is
> not implemented in Focal. In fact, wiphy_lock() was introduced by
> a05829a7222e, which is not worth backporting. In LKML, reading the thread
> related to this patch, it is possible to notice that:
> 	Lock requirement was already there before a05829a7222e,
> 	only it was on rtnl lock to be taken instead of wiphy one.
> In this backport, therefore I am using rtnl_lock() and rtln_unlock(),
> instead of wiphy_lock().]
> CVE-2024-40911
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
>   net/wireless/util.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index d3537d621096..c5eac66fd398 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -1951,6 +1951,7 @@ int cfg80211_get_station(struct net_device *dev, const u8 *mac_addr,
>   {
>   	struct cfg80211_registered_device *rdev;
>   	struct wireless_dev *wdev;
> +	int ret;
>   
>   	wdev = dev->ieee80211_ptr;
>   	if (!wdev)
> @@ -1962,7 +1963,11 @@ int cfg80211_get_station(struct net_device *dev, const u8 *mac_addr,
>   
>   	memset(sinfo, 0, sizeof(*sinfo));
>   
> -	return rdev_get_station(rdev, dev, mac_addr, sinfo);
> +	rtnl_lock();
> +	ret = rdev_get_station(rdev, dev, mac_addr, sinfo);
> +	rtnl_unlock();
> +

I was wondering if a lock is really needed here.
Checking upstream code at 642f89daa34567d02f312d03e41523a894906dae~, we 
have cfg80211_get_station() calling rdev_get_station() without holding 
any lock. rdev_get_station() calls rdev->ops->get_station, which points 
to ieee80211_get_station(). ieee80211_get_station() indeed expects a 
lock to be held, as it has `lockdep_assert_wiphy(local->hw.wiphy);`. So 
until here, I understand the fix commit locking wiphy before calling 
rdev_get_station().

However, in focal:linux code (and maybe B and X?), there are some 
different things: ieee80211_get_station() itself locks and releases sta_mtx.
But maybe it makes sense to add this lock to f:linux, as it could cover 
cases where rdev->ops->get_station is a pointer to a function which 
doesn't have any locking in place?
Let me know what you think.


Magali

> +	return ret;
>   }
>   EXPORT_SYMBOL(cfg80211_get_station);
>