linux-kernel - RE: [PATCH] cfg80211: Fix "suspicious RCU usage in wiphy_apply_custom

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 19 Jan 2021 07:19:43 +0000
From:   "Peer, Ilan" <ilan.peer@...el.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        Johannes Berg <johannes@...solutions.net>,
        "David S . Miller" <davem@...emloft.net>,
        "Jakub Kicinski" <kuba@...nel.org>,
        "Rojewski, Cezary" <cezary.rojewski@...el.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        "Mark Brown" <broonie@...nel.org>
CC:     "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>
Subject: RE: [PATCH] cfg80211: Fix "suspicious RCU usage in
 wiphy_apply_custom_regulatory" warning/backtrace

> -----Original Message-----
> From: Hans de Goede <hdegoede@...hat.com>
> Sent: Monday, January 18, 2021 23:09
> To: Peer, Ilan <ilan.peer@...el.com>; Johannes Berg
> <johannes@...solutions.net>; David S . Miller <davem@...emloft.net>;
> Jakub Kicinski <kuba@...nel.org>; Rojewski, Cezary
> <cezary.rojewski@...el.com>; Pierre-Louis Bossart <pierre-
> louis.bossart@...ux.intel.com>; Liam Girdwood
> <liam.r.girdwood@...ux.intel.com>; Jie Yang <yang.jie@...ux.intel.com>;
> Mark Brown <broonie@...nel.org>
> Cc: linux-wireless@...r.kernel.org; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org; alsa-devel@...a-project.org
> Subject: Re: [PATCH] cfg80211: Fix "suspicious RCU usage in
> wiphy_apply_custom_regulatory" warning/backtrace
> 
> Hi,
> 
> On 1/5/21 10:24 AM, Peer, Ilan wrote:
> > Hi,
> >
> >> -----Original Message-----
> >> From: Hans de Goede <hdegoede@...hat.com>
> >> Sent: Monday, January 04, 2021 19:07
> >> To: Johannes Berg <johannes@...solutions.net>; David S . Miller
> >> <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>; Rojewski,
> >> Cezary <cezary.rojewski@...el.com>; Pierre-Louis Bossart <pierre-
> >> louis.bossart@...ux.intel.com>; Liam Girdwood
> >> <liam.r.girdwood@...ux.intel.com>; Jie Yang
> >> <yang.jie@...ux.intel.com>; Mark Brown <broonie@...nel.org>
> >> Cc: Hans de Goede <hdegoede@...hat.com>; linux-
> >> wireless@...r.kernel.org; netdev@...r.kernel.org; linux-
> >> kernel@...r.kernel.org; alsa-devel@...a-project.org; Peer, Ilan
> >> <ilan.peer@...el.com>
> >> Subject: [PATCH] cfg80211: Fix "suspicious RCU usage in
> >> wiphy_apply_custom_regulatory" warning/backtrace
> >>
> >> Commit beee24695157 ("cfg80211: Save the regulatory domain when
> >> setting custom regulatory") adds a get_wiphy_regdom call to
> >> wiphy_apply_custom_regulatory. But as the comment above
> >> wiphy_apply_custom_regulatory says:
> >> "/* Used by drivers prior to wiphy registration */"
> >> this function is used by driver's probe function before the wiphy is
> >> registered and at this point wiphy->regd will typically by NULL and
> >> calling rcu_dereference_rtnl on a NULL pointer causes the following
> >> warning/backtrace:
> >>
> >> =============================
> >> WARNING: suspicious RCU usage
> >> 5.11.0-rc1+ #19 Tainted: G        W
> >> -----------------------------
> >> net/wireless/reg.c:144 suspicious rcu_dereference_check() usage!
> >>
> >> other info that might help us debug this:
> >>
> >> rcu_scheduler_active = 2, debug_locks = 1
> >> 2 locks held by kworker/2:0/22:
> >>  #0: ffff9a4bc104df38 ((wq_completion)events){+.+.}-{0:0}, at:
> >> process_one_work+0x1ee/0x570
> >>  #1: ffffb6e94010be78
> >> ((work_completion)(&fw_work->work)){+.+.}-{0:0},
> >> at: process_one_work+0x1ee/0x570
> >>
> >> stack backtrace:
> >> CPU: 2 PID: 22 Comm: kworker/2:0 Tainted: G        W         5.11.0-rc1+ #19
> >> Hardware name: LENOVO 60073/INVALID, BIOS 01WT17WW 08/01/2014
> >> Workqueue: events request_firmware_work_func Call Trace:
> >>  dump_stack+0x8b/0xb0
> >>  get_wiphy_regdom+0x57/0x60 [cfg80211]
> >>  wiphy_apply_custom_regulatory+0xa0/0xf0 [cfg80211]
> >>  brcmf_cfg80211_attach+0xb02/0x1360 [brcmfmac]
> >>  brcmf_attach+0x189/0x460 [brcmfmac]
> >>  brcmf_sdio_firmware_callback+0x78a/0x8f0 [brcmfmac]
> >>  brcmf_fw_request_done+0x67/0xf0 [brcmfmac]
> >>  request_firmware_work_func+0x3d/0x70
> >>  process_one_work+0x26e/0x570
> >>  worker_thread+0x55/0x3c0
> >>  ? process_one_work+0x570/0x570
> >>  kthread+0x137/0x150
> >>  ? __kthread_bind_mask+0x60/0x60
> >>  ret_from_fork+0x22/0x30
> >>
> >> Add a check for wiphy->regd being NULL before calling
> >> get_wiphy_regdom (as is already done in other places) to fix this.
> >>
> >> wiphy->regd will likely always be NULL when
> >> wiphy->wiphy_apply_custom_regulatory
> >> gets called, so arguably the tmp = get_wiphy_regdom() and
> >> rcu_free_regdom(tmp) calls should simply be dropped, this patch keeps
> >> the
> >> 2 calls, to allow drivers to call wiphy_apply_custom_regulatory more
> >> then once if necessary.
> >>
> >> Cc: Ilan Peer <ilan.peer@...el.com>
> >> Fixes: beee24695157 ("cfg80211: Save the regulatory domain when
> >> setting custom regulator")
> >> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> >> ---
> >>  net/wireless/reg.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c index
> >> bb72447ad960..9254b9cbaa21 100644
> >> --- a/net/wireless/reg.c
> >> +++ b/net/wireless/reg.c
> >> @@ -2547,7 +2547,7 @@ static void handle_band_custom(struct wiphy
> >> *wiphy,  void wiphy_apply_custom_regulatory(struct wiphy *wiphy,
> >>  				   const struct ieee80211_regdomain *regd)  {
> >> -	const struct ieee80211_regdomain *new_regd, *tmp;
> >> +	const struct ieee80211_regdomain *new_regd, *tmp = NULL;
> >>  	enum nl80211_band band;
> >>  	unsigned int bands_set = 0;
> >>
> >> @@ -2571,7 +2571,8 @@ void wiphy_apply_custom_regulatory(struct
> wiphy
> >> *wiphy,
> >>  	if (IS_ERR(new_regd))
> >>  		return;
> >>
> >> -	tmp = get_wiphy_regdom(wiphy);
> >> +	if (wiphy->regd)
> >> +		tmp = get_wiphy_regdom(wiphy);
> >>  	rcu_assign_pointer(wiphy->regd, new_regd);
> >>  	rcu_free_regdom(tmp);
> >
> > This only fixes the first case where the pointer in NULL and does not handle
> the wrong RCU usage in other cases.
> >
> > I'll prepare a fix for this.
> 
> Any luck with this? This is a regression in 5.11, so this really should be fixed in
> a future 5.11-rc and the clock is running out.
> 

Yes. The fix is ready. We'll send it.

Regards,

Ilan.