linux-kernel - Re: [PATCH v10 2/2] wifi: mwifiex: add host mlme for AP mode

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: Wed, 22 May 2024 17:59:24 -0700
From: Brian Norris <briannorris@...omium.org>
To: David Lin <yu-hao.lin@....com>
Cc: linux-wireless@...r.kernel.org, linux-kernel@...r.kernel.org,
	kvalo@...nel.org, francesco@...cini.it, tsung-hsien.hsieh@....com,
	Francesco Dolcini <francesco.dolcini@...adex.com>
Subject: Re: [PATCH v10 2/2] wifi: mwifiex: add host mlme for AP mode

Hi,

Hopefully-last comment for patch 2:

On Thu, Apr 18, 2024 at 02:06:26PM +0800, David Lin wrote:
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c

> @@ -1712,7 +1735,7 @@ static const u32 mwifiex_cipher_suites[] = {
>  };
>  
>  /* Supported mgmt frame types to be advertised to cfg80211 */
> -static const struct ieee80211_txrx_stypes
> +static struct ieee80211_txrx_stypes
>  mwifiex_mgmt_stypes[NUM_NL80211_IFTYPES] = {
>  	[NL80211_IFTYPE_STATION] = {
>  		.tx = BIT(IEEE80211_STYPE_ACTION >> 4) |
..
> +	if (adapter->host_mlme_enabled) {
> +		mwifiex_mgmt_stypes[NL80211_IFTYPE_AP].tx = 0xffff;
> +		mwifiex_mgmt_stypes[NL80211_IFTYPE_AP].rx =
> +			BIT(IEEE80211_STYPE_ASSOC_REQ >> 4) |
> +			BIT(IEEE80211_STYPE_REASSOC_REQ >> 4) |
> +			BIT(IEEE80211_STYPE_PROBE_REQ >> 4) |
> +			BIT(IEEE80211_STYPE_DISASSOC >> 4) |
> +			BIT(IEEE80211_STYPE_AUTH >> 4) |
> +			BIT(IEEE80211_STYPE_DEAUTH >> 4) |
> +			BIT(IEEE80211_STYPE_ACTION >> 4);
> +	}

This doesn't look like a sound approach. I think you should keep
'mwifiex_mgmt_stypes' const, because if you're making modifications to
it, then you may break multi-adapter situations. Consider someone loads
this driver with host_mlme_enabled==true, and then later registers a
device with host_mlme_enabled==false. The second adapter will get the
wrong values.

I think 'mwifiex_mgmt_stypes' is small enough you might as well just
make a second copy with the MLME-enabled values, rather than fiddling
with modifying a single copy.

Aside from that:

Acked-by: Brian Norris <briannorris@...omium.org>

(Feel free to carry that to a v11, since my only remaining substantial
concerns are with patch 1 I think.)

Brian