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.