linux-kernel - Re: [PATCH v2] wifi: mwifiex: fix STA cannot connect to AP

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: <ZXpjjmD5Se7axJju@google.com>
Date:   Wed, 13 Dec 2023 18:08:14 -0800
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,
        stable@...r.kernel.org
Subject: Re: [PATCH v2] wifi: mwifiex: fix STA cannot connect to AP

Hi,

Nitpick: "fix STA cannot connect to AP" isn't the best commit message;
that could describe an enormous number of fixes. Maybe something more
like "Configure BSSID consistently when starting AP"?

On Sat, Dec 09, 2023 at 07:41:27AM +0800, David Lin wrote:
> AP BSSID configuration is missing at AP start.
> Without this fix, FW returns STA interface MAC address after first init.
> When hostapd restarts, it gets MAC address from netdev before driver
> sets STA MAC to netdev again. Now MAC address between hostapd and net
> interface are different causes STA cannot connect to AP.
> After that MAC address of uap0 mlan0 become the same. And issue
> disappears after following hostapd restart (another issue is AP/STA MAC
> address become the same).
> This patch fixes the issue cleanly.
> 
> Signed-off-by: David Lin <yu-hao.lin@....com>
> Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
> Cc: stable@...r.kernel.org
> 
> ---
> 
> v2:
>    - v1 was a not finished patch that was send to the LKML by mistake

Looks fine to me:

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

>  drivers/net/wireless/marvell/mwifiex/cfg80211.c | 2 ++
>  drivers/net/wireless/marvell/mwifiex/fw.h       | 1 +
>  drivers/net/wireless/marvell/mwifiex/ioctl.h    | 1 +
>  drivers/net/wireless/marvell/mwifiex/uap_cmd.c  | 8 ++++++++
>  4 files changed, 12 insertions(+)

> --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c

> @@ -487,6 +488,13 @@ mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
>  	int i;
>  	u16 cmd_size = *param_size;
>  
> +	mac_tlv = (struct host_cmd_tlv_mac_addr *)tlv;

Not directly related to this patch, but while you're expanding the size
of this command buffer: it always felt like a security-hole-in-waiting
that none of these command producers do any kinds of bounds checking.
We're just "lucky" that these function only generate contents of ~100
bytes at max, while MWIFIEX_SIZE_OF_CMD_BUFFER=2048. But, just add a few
more user-space controlled TLV params, and boom, we'll have ourselves a
nice little CVE.

It probably wouldn't hurt to significantly write much of this driver,
but at a minimum, we could probably use a few checks like this:

	cmd_size += sizeof(struct host_cmd_tlv_mac_addr);
	if (cmd_size > MWIFIEX_SIZE_OF_CMD_BUFFER)
		return -1;
	// Only touch tlv *after* the bounds check.

That doesn't need to block this patch, of course.

Brian

> +	mac_tlv->header.type = cpu_to_le16(TLV_TYPE_UAP_MAC_ADDRESS);
> +	mac_tlv->header.len = cpu_to_le16(ETH_ALEN);
> +	memcpy(mac_tlv->mac_addr, bss_cfg->mac_addr, ETH_ALEN);
> +	cmd_size += sizeof(struct host_cmd_tlyyv_mac_addr);
> +	tlv += sizeof(struct host_cmd_tlv_mac_addr);
> +
>  	if (bss_cfg->ssid.ssid_len) {
>  		ssid = (struct host_cmd_tlv_ssid *)tlv;
>  		ssid->header.type = cpu_to_le16(TLV_TYPE_UAP_SSID);
> 
> base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c
> -- 
> 2.25.1
>