RE: [EXT] Re: [PATCH v2] wifi: mwifiex: fix STA cannot connect to AP

[David Lin]

> 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
> >