diff mbox series

[v2] wifi: mwifiex: fix STA cannot connect to AP

Message ID 20231208234127.2251-1-yu-hao.lin@nxp.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series [v2] wifi: mwifiex: fix STA cannot connect to AP | expand

Commit Message

David Lin Dec. 8, 2023, 11:41 p.m. UTC
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@nxp.com>
Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
Cc: stable@vger.kernel.org

---

v2:
   - v1 was a not finished patch that was send to the LKML by mistake
---
 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(+)


base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c

Comments

Francesco Dolcini Dec. 11, 2023, 7:47 a.m. UTC | #1
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@nxp.com>
> Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
> Cc: stable@vger.kernel.org

Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Francesco
Rafael Beims Dec. 11, 2023, 5:29 p.m. UTC | #2
On 08/12/2023 20:41, 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@nxp.com>
> Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
> Cc: stable@vger.kernel.org
>
> ---
>
> v2:
>     - v1 was a not finished patch that was send to the LKML by mistake
> ---
>   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(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> index 7a15ea8072e6..3604abcbcff9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> +++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
> @@ -2047,6 +2047,8 @@ static int mwifiex_cfg80211_start_ap(struct wiphy *wiphy,
>   
>   	mwifiex_set_sys_config_invalid_data(bss_cfg);
>   
> +	memcpy(bss_cfg->mac_addr, priv->curr_addr, ETH_ALEN);
> +
>   	if (params->beacon_interval)
>   		bss_cfg->beacon_period = params->beacon_interval;
>   	if (params->dtim_period)
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 8e6db904e5b2..62f3c9a52a1d 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -165,6 +165,7 @@ enum MWIFIEX_802_11_PRIVACY_FILTER {
>   #define TLV_TYPE_STA_MAC_ADDR       (PROPRIETARY_TLV_BASE_ID + 32)
>   #define TLV_TYPE_BSSID              (PROPRIETARY_TLV_BASE_ID + 35)
>   #define TLV_TYPE_CHANNELBANDLIST    (PROPRIETARY_TLV_BASE_ID + 42)
> +#define TLV_TYPE_UAP_MAC_ADDRESS    (PROPRIETARY_TLV_BASE_ID + 43)
>   #define TLV_TYPE_UAP_BEACON_PERIOD  (PROPRIETARY_TLV_BASE_ID + 44)
>   #define TLV_TYPE_UAP_DTIM_PERIOD    (PROPRIETARY_TLV_BASE_ID + 45)
>   #define TLV_TYPE_UAP_BCAST_SSID     (PROPRIETARY_TLV_BASE_ID + 48)
> diff --git a/drivers/net/wireless/marvell/mwifiex/ioctl.h b/drivers/net/wireless/marvell/mwifiex/ioctl.h
> index 091e7ca79376..e8825f302de8 100644
> --- a/drivers/net/wireless/marvell/mwifiex/ioctl.h
> +++ b/drivers/net/wireless/marvell/mwifiex/ioctl.h
> @@ -107,6 +107,7 @@ struct mwifiex_uap_bss_param {
>   	u8 qos_info;
>   	u8 power_constraint;
>   	struct mwifiex_types_wmm_info wmm_info;
> +	u8 mac_addr[ETH_ALEN];
>   };
>   
>   enum {
> diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> index e78a201cd150..491e36611909 100644
> --- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
> @@ -468,6 +468,7 @@ void mwifiex_config_uap_11d(struct mwifiex_private *priv,
>   static int
>   mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
>   {
> +	struct host_cmd_tlv_mac_addr *mac_tlv;
>   	struct host_cmd_tlv_dtim_period *dtim_period;
>   	struct host_cmd_tlv_beacon_period *beacon_period;
>   	struct host_cmd_tlv_ssid *ssid;
> @@ -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;
> +	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_tlv_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


Tested-by: Rafael Beims <rafael.beims@toradex.com> # Verdin iMX8MP / 
SD8997 SD


Rafael
Brian Norris Dec. 14, 2023, 2:08 a.m. UTC | #3
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@nxp.com>
> Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap handlers")
> Cc: stable@vger.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@chromium.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
>
David Lin Dec. 14, 2023, 2:22 a.m. UTC | #4
> From: Brian Norris <briannorris@chromium.org>
> Sent: Thursday, December 14, 2023 10:08 AM
> To: David Lin <yu-hao.lin@nxp.com>
> Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org;
> kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org
> 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@nxp.com>
> > Fixes: 12190c5d80bd ("mwifiex: add cfg80211 start_ap and stop_ap
> > handlers")
> > Cc: stable@vger.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@chromium.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
>

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
> >
Francesco Dolcini Dec. 14, 2023, 7:35 a.m. UTC | #5
Hello David,

On Thu, Dec 14, 2023 at 02:22:57AM +0000, David Lin wrote:
> > From: Brian Norris <briannorris@chromium.org>
...
> > 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?

Just create `[PATCH v3] wifi: mwifiex: fix STA cannot connect to AP`

Add the change suggested by Brian and the tags you received on this v2:

 - Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
 - Tested-by: Rafael Beims <rafael.beims@toradex.com> # Verdin iMX8MP / SD8997 SD
 - Acked-by: Brian Norris <briannorris@chromium.org>

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

I would suggest not modify this in this patch, we should fix all the code that
is subjected to this potential issue.

I would personally do a follow-up patch just to add the check to avoid
overflowing the cmd buffer everywhere it is used.

Francesco
David Lin Dec. 14, 2023, 11:38 a.m. UTC | #6
> From: Francesco Dolcini <francesco@dolcini.it>
> Sent: Thursday, December 14, 2023 3:35 PM
> To: David Lin <yu-hao.lin@nxp.com>
> Cc: Brian Norris <briannorris@chromium.org>;
> linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org;
> kvalo@kernel.org; francesco@dolcini.it; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org
> Subject: Re: [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
> 
> 
> Hello David,
> 
> On Thu, Dec 14, 2023 at 02:22:57AM +0000, David Lin wrote:
> > > From: Brian Norris <briannorris@chromium.org>
> ...
> > > 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?
> 
> Just create `[PATCH v3] wifi: mwifiex: fix STA cannot connect to AP`
> 
> Add the change suggested by Brian and the tags you received on this v2:
> 
>  - Reviewed-by: Francesco Dolcini <francesco.dolcini@toradex.com>
>  - Tested-by: Rafael Beims <rafael.beims@toradex.com> # Verdin iMX8MP /
> SD8997 SD
>  - Acked-by: Brian Norris <briannorris@chromium.org>
> 

O.K. Thanks.

> > > 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.
> 
> I would suggest not modify this in this patch, we should fix all the code that
> is subjected to this potential issue.
> 
> I would personally do a follow-up patch just to add the check to avoid
> overflowing the cmd buffer everywhere it is used.
> 
> Francesco


O.K. I will only change commit message. In fact, this TLV command is added as the first one command.
Brian Norris Dec. 14, 2023, 6:52 p.m. UTC | #7
On Thu, Dec 14, 2023 at 3:38 AM David Lin <yu-hao.lin@nxp.com> wrote:
> > From: Francesco Dolcini <francesco@dolcini.it>
> >
> > On Thu, Dec 14, 2023 at 02:22:57AM +0000, David Lin wrote:
> > > > From: Brian Norris <briannorris@chromium.org>
> > > > 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.
> >
> > I would suggest not modify this in this patch, we should fix all the code that
> > is subjected to this potential issue.
> >
> > I would personally do a follow-up patch just to add the check to avoid
> > overflowing the cmd buffer everywhere it is used.

Right, there's tons of code that could potentially be affected, and
this is definitely a separate patch. (Your feature only adds on to the
existing issue, so these are separate logical changes.)

> O.K. I will only change commit message. In fact, this TLV command is added as the first one command.

Well, it doesn't really matter than your TLV is "first" -- if there's
an overflow, there's an overflow. Maybe the 8 bytes you're adding here
are the necessary tipping point. I don't know without doing some kind
of informal mathematics/proof.

Brian
David Lin Dec. 14, 2023, 11:05 p.m. UTC | #8
> From: Brian Norris <briannorris@chromium.org>
> Sent: Friday, December 15, 2023 2:52 AM
> To: David Lin <yu-hao.lin@nxp.com>
> Cc: Francesco Dolcini <francesco@dolcini.it>; linux-wireless@vger.kernel.org;
> linux-kernel@vger.kernel.org; kvalo@kernel.org; Pete Hsieh
> <tsung-hsien.hsieh@nxp.com>; stable@vger.kernel.org
> Subject: Re: [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
> 
> 
> On Thu, Dec 14, 2023 at 3:38 AM David Lin <yu-hao.lin@nxp.com> wrote:
> > > From: Francesco Dolcini <francesco@dolcini.it>
> > >
> > > On Thu, Dec 14, 2023 at 02:22:57AM +0000, David Lin wrote:
> > > > > From: Brian Norris <briannorris@chromium.org> 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.
> > >
> > > I would suggest not modify this in this patch, we should fix all the
> > > code that is subjected to this potential issue.
> > >
> > > I would personally do a follow-up patch just to add the check to
> > > avoid overflowing the cmd buffer everywhere it is used.
> 
> Right, there's tons of code that could potentially be affected, and this is
> definitely a separate patch. (Your feature only adds on to the existing issue,
> so these are separate logical changes.)
> 
> > O.K. I will only change commit message. In fact, this TLV command is added
> as the first one command.
> 
> Well, it doesn't really matter than your TLV is "first" -- if there's an overflow,
> there's an overflow. Maybe the 8 bytes you're adding here are the necessary
> tipping point. I don't know without doing some kind of informal
> mathematics/proof.
> 
> Brian

Understood. Thanks.
diff mbox series

Patch

diff --git a/drivers/net/wireless/marvell/mwifiex/cfg80211.c b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
index 7a15ea8072e6..3604abcbcff9 100644
--- a/drivers/net/wireless/marvell/mwifiex/cfg80211.c
+++ b/drivers/net/wireless/marvell/mwifiex/cfg80211.c
@@ -2047,6 +2047,8 @@  static int mwifiex_cfg80211_start_ap(struct wiphy *wiphy,
 
 	mwifiex_set_sys_config_invalid_data(bss_cfg);
 
+	memcpy(bss_cfg->mac_addr, priv->curr_addr, ETH_ALEN);
+
 	if (params->beacon_interval)
 		bss_cfg->beacon_period = params->beacon_interval;
 	if (params->dtim_period)
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
index 8e6db904e5b2..62f3c9a52a1d 100644
--- a/drivers/net/wireless/marvell/mwifiex/fw.h
+++ b/drivers/net/wireless/marvell/mwifiex/fw.h
@@ -165,6 +165,7 @@  enum MWIFIEX_802_11_PRIVACY_FILTER {
 #define TLV_TYPE_STA_MAC_ADDR       (PROPRIETARY_TLV_BASE_ID + 32)
 #define TLV_TYPE_BSSID              (PROPRIETARY_TLV_BASE_ID + 35)
 #define TLV_TYPE_CHANNELBANDLIST    (PROPRIETARY_TLV_BASE_ID + 42)
+#define TLV_TYPE_UAP_MAC_ADDRESS    (PROPRIETARY_TLV_BASE_ID + 43)
 #define TLV_TYPE_UAP_BEACON_PERIOD  (PROPRIETARY_TLV_BASE_ID + 44)
 #define TLV_TYPE_UAP_DTIM_PERIOD    (PROPRIETARY_TLV_BASE_ID + 45)
 #define TLV_TYPE_UAP_BCAST_SSID     (PROPRIETARY_TLV_BASE_ID + 48)
diff --git a/drivers/net/wireless/marvell/mwifiex/ioctl.h b/drivers/net/wireless/marvell/mwifiex/ioctl.h
index 091e7ca79376..e8825f302de8 100644
--- a/drivers/net/wireless/marvell/mwifiex/ioctl.h
+++ b/drivers/net/wireless/marvell/mwifiex/ioctl.h
@@ -107,6 +107,7 @@  struct mwifiex_uap_bss_param {
 	u8 qos_info;
 	u8 power_constraint;
 	struct mwifiex_types_wmm_info wmm_info;
+	u8 mac_addr[ETH_ALEN];
 };
 
 enum {
diff --git a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
index e78a201cd150..491e36611909 100644
--- a/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/uap_cmd.c
@@ -468,6 +468,7 @@  void mwifiex_config_uap_11d(struct mwifiex_private *priv,
 static int
 mwifiex_uap_bss_param_prepare(u8 *tlv, void *cmd_buf, u16 *param_size)
 {
+	struct host_cmd_tlv_mac_addr *mac_tlv;
 	struct host_cmd_tlv_dtim_period *dtim_period;
 	struct host_cmd_tlv_beacon_period *beacon_period;
 	struct host_cmd_tlv_ssid *ssid;
@@ -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;
+	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_tlv_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);