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@xxxxxxx>
> Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
> Cc: stable@xxxxxxxxxxxxxxx
>
> ---
>
> v2:
> - v1 was a not finished patch that was send to the LKML by mistake
Looks fine to me:
Acked-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> 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
>
