linux-kernel - Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on different channel

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]

Message-ID: <Zt_8gwj6GnV_yZ1Z@pengutronix.de>
Date: Tue, 10 Sep 2024 10:00:03 +0200
From: Sascha Hauer <s.hauer@...gutronix.de>
To: David Lin <yu-hao.lin@....com>
Cc: "linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"briannorris@...omium.org" <briannorris@...omium.org>,
	"kvalo@...nel.org" <kvalo@...nel.org>,
	"francesco@...cini.it" <francesco@...cini.it>,
	Pete Hsieh <tsung-hsien.hsieh@....com>
Subject: Re: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
 different channel

On Tue, Sep 10, 2024 at 01:52:02AM +0000, David Lin wrote:
> > From: Sascha Hauer <s.hauer@...gutronix.de>
> > Sent: Tuesday, September 10, 2024 5:05 AM
> > To: David Lin <yu-hao.lin@....com>
> > Cc: linux-wireless@...r.kernel.org; linux-kernel@...r.kernel.org;
> > briannorris@...omium.org; kvalo@...nel.org; francesco@...cini.it; Pete
> > Hsieh <tsung-hsien.hsieh@....com>
> > Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: avoid AP and STA running on
> > different channel
> > 
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report
> > this email' button
> > 
> > 
> > On Mon, Sep 02, 2024 at 04:43:11PM +0800, David Lin wrote:
> > > Current firmware doesn't support AP and STA running on different
> > > channels simultaneously.
> > > FW crash would occur in such case.
> > > This patch avoids the issue by disabling AP and STA to run on
> > > different channels.
> > >
> > > Signed-off-by: David Lin <yu-hao.lin@....com>
> > > ---
> > >
> > > v2:
> > >    - clean up code.
> > >
> > > ---
> > >  .../net/wireless/marvell/mwifiex/cfg80211.c   | 17 ++++---
> > >  drivers/net/wireless/marvell/mwifiex/util.c   | 44 +++++++++++++++++++
> > >  drivers/net/wireless/marvell/mwifiex/util.h   | 13 ++++++
> > >  3 files changed, 69 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > index 722ead51e912..3dbcab463445 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> > > @@ -781,11 +781,9 @@ mwifiex_cfg80211_set_wiphy_params(struct wiphy
> > *wiphy, u32 changed)
> > >               break;
> > >
> > >       case MWIFIEX_BSS_ROLE_STA:
> > > -             if (priv->media_connected) {
> > > -                     mwifiex_dbg(adapter, ERROR,
> > > -                                 "cannot change wiphy params
> > when connected");
> > > -                     return -EINVAL;
> > > -             }
> > > +             if (priv->media_connected)
> > > +                     break;
> > 
> > This hunk seems unrelated to this patch. If this is needed then it deserves an
> > extra patch along with an explanation why this is necessary.
> > 
> > Sascha
> > 
> 
> Without this hunk, AP and STA can't run on the same channel if some
> wiphy parameters are setting.

Ok, I now see where you are aiming at. Here's the problematic function:

> static int
> mwifiex_cfg80211_set_wiphy_params(struct wiphy *wiphy, u32 changed)
> {
> 	...
> 
> 	priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
> 
> 	switch (priv->bss_role) {
> 	case MWIFIEX_BSS_ROLE_UAP:
> 		if (priv->bss_started) {
> 			mwifiex_dbg(adapter, ERROR,
> 				    "cannot change wiphy params when bss started");
> 			return -EINVAL;
> 		}
> 
> 		...
> 		mwifiex_send_cmd(priv, HostCmd_CMD_UAP_SYS_CONFIG, ...);
> 
> 		break;
> 	case MWIFIEX_BSS_ROLE_STA:
> 		if (priv->media_connected) {
> 			mwifiex_dbg(adapter, ERROR,
> 				    "cannot change wiphy params when connected");
> 			return -EINVAL;
> 		}
> 
> 		...
> 		mwifiex_send_cmd(priv, HostCmd_CMD_802_11_SNMP_MIB, ...);
> 
> 		break;
> 	}
> 
> 	return 0;
> }

This function is for setting wiphy params like rts_threshold and others.

mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY) returns the first
priv which by default is in station mode. Now if you start priv0 in
station mode, then afterwards start priv1 in AP mode *and* have
rts_threshold = xy in your config, then you run into the
"cannot change wiphy params when connected" case.

I really wonder if the settings done in this function are per priv or
per adapter. Is there one rts_threshold setting in a mwifiex chip or are
there multiple (per vif/priv)?

If it's a global setting, then why are we interested in the
media_connected state of one specific priv? Shouldn't we check all
privs?

If it's a setting per priv, then why do we choose the same priv
everytime in this function?

Either way, this function looks fishy and changing it should be done
with an explanation, just dropping the error message and returning
success is not enough.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |