> From: Brian Norris <briannorris@xxxxxxxxxxxx>
> Sent: Thursday, December 14, 2023 10:08 AM
> To: David Lin <yu-hao.lin@xxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> kvalo@xxxxxxxxxx; francesco@xxxxxxxxxx; Pete Hsieh
> <tsung-hsien.hsieh@xxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [EXT] Re: [PATCH v2] wifi: mwifiex: fix STA cannot connect to AP
>
> 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
>
>
> 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"?
Thanks for your suggestion. I will change commit message as you suggested. Does it mean I should create another patch from v1?
>
> 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
>
I will modify the code for next patch.
David
> > + 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
> >
