Cmt: [SRU][F][PATCH 1/1] wifi: cfg80211: Lock wiphy in cfg80211_get_station
Koichiro Den
koichiro.den at canonical.com
Fri Nov 22 14:09:47 UTC 2024
On Thu, Nov 21, 2024 at 04:55:29PM -0300, Magali Lemes wrote:
> 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.
Really nice point. Regarding F/B/X, among other .get_station
implementations, ieee80211_get_station seems the only caller of
sta_set_sinfo(), so for the case (a) below, sta_mtx looks enough to
protect us from the potential null pointer dereference issue. And as you
pointed out, adding the lock still helps generically for F/B/X to cover
the case (b), which Antonio mentioned in his ACK comment on upstream.
(a). sta_info_insert* vs get_station via batadv_v_elp_get_throughput
(b). get_station via netlink vs get_station via batadv_v_elp_get_throughput
>
>
> Magali
>
> > + return ret;
> > }
> > EXPORT_SYMBOL(cfg80211_get_station);
>
> --
> kernel-team mailing list
> kernel-team at lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list