Cmt: [SRU][F][PATCH 1/1] wifi: cfg80211: Lock wiphy in cfg80211_get_station
Massimiliano Pellizzer
massimiliano.pellizzer at canonical.com
Fri Nov 22 13:19:03 UTC 2024
On Fri, 22 Nov 2024 at 14:17, Massimiliano Pellizzer
<massimiliano.pellizzer at canonical.com> wrote:
>
> On Thu, 21 Nov 2024 at 20:55, Magali Lemes <magali.lemes at canonical.com> 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.
> >
> >
> > Magali
> >
>
> Thanks for reviewing. This is a really good point.
>
> From the following LKML thread [1] it seems like at some point they
> started using
> whipy locks instead of a bunch of other mutex (sta_mtx, key_mtx,
> chanctx_mtx, ampdu_mlme.mtx, etc.).
> In particular, the commit 4d3acf4311a04 replaces the usage of sta_mtx with
> the lockdep_assertion inside ieee80211_get_station(). My
> understanding, therefore,
> is that before the patchset [1] the lock was acquired inside each
> get_station() function,
> while after [1] the lock needs to be acquired sooner in the call stack.
>
> So I think you might be right, probably a lock is not needed in F/B/X.
> This means that the
> break commit should be changed to 4d3acf4311a04 in this case.
>
> What do you think?
[1] https://lore.kernel.org/all/20230828135928.917909b5954f.I81a21aafae702b20349aef8ecb73538f394c3b72@changeid/T/#u
>
> --
> Massimiliano Pellizzer
>
> > > + return ret;
> > > }
> > > EXPORT_SYMBOL(cfg80211_get_station);
> > >
More information about the kernel-team
mailing list