3.16.81-rc1 review patch. If anyone has any objections, please let me know.
------------------
From: Ganapathi Bhat <[email protected]>
commit 3d94a4a8373bf5f45cf5f939e88b8354dbf2311b upstream.
mwifiex_process_country_ie() function parse elements of bss
descriptor in beacon packet. When processing WLAN_EID_COUNTRY
element, there is no upper limit check for country_ie_len before
calling memcpy. The destination buffer domain_info->triplet is an
array of length MWIFIEX_MAX_TRIPLET_802_11D(83). The remote
attacker can build a fake AP with the same ssid as real AP, and
send malicous beacon packet with long WLAN_EID_COUNTRY elemen
(country_ie_len > 83). Attacker can force STA connect to fake AP
on a different channel. When the victim STA connects to fake AP,
will trigger the heap buffer overflow. Fix this by checking for
length and if found invalid, don not connect to the AP.
This fix addresses CVE-2019-14895.
Reported-by: huangwen <[email protected]>
Signed-off-by: Ganapathi Bhat <[email protected]>
Signed-off-by: Kalle Valo <[email protected]>
[bwh: Backported to 3.16:
- Use wiphy_dbg() instead of mwifiex_dbg()
- Adjust filename]
Signed-off-by: Ben Hutchings <[email protected]>
---
drivers/net/wireless/mwifiex/sta_ioctl.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
--- a/drivers/net/wireless/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
@@ -223,6 +223,14 @@ static int mwifiex_process_country_ie(st
"11D: skip setting domain info in FW\n");
return 0;
}
+
+ if (country_ie_len >
+ (IEEE80211_COUNTRY_STRING_LEN + MWIFIEX_MAX_TRIPLET_802_11D)) {
+ wiphy_dbg(priv->wdev->wiphy,
+ "11D: country_ie_len overflow!, deauth AP\n");
+ return -EINVAL;
+ }
+
memcpy(priv->adapter->country_code, &country_ie[2], 2);
domain_info->country_code[0] = country_ie[2];
@@ -266,8 +274,9 @@ int mwifiex_bss_start(struct mwifiex_pri
priv->scan_block = false;
if (bss) {
- if (adapter->region_code == 0x00)
- mwifiex_process_country_ie(priv, bss);
+ if (adapter->region_code == 0x00 &&
+ mwifiex_process_country_ie(priv, bss))
+ return -EINVAL;
/* Allocate and fill new bss descriptor */
bss_desc = kzalloc(sizeof(struct mwifiex_bssdescriptor),
Hi Ben,
On Wed, Jan 08, 2020 at 07:44:00PM +0000, Ben Hutchings wrote:
> 3.16.81-rc1 review patch. If anyone has any objections, please let me know.
>
> ------------------
>
> From: Ganapathi Bhat <[email protected]>
>
> commit 3d94a4a8373bf5f45cf5f939e88b8354dbf2311b upstream.
>
> mwifiex_process_country_ie() function parse elements of bss
> descriptor in beacon packet. When processing WLAN_EID_COUNTRY
> element, there is no upper limit check for country_ie_len before
> calling memcpy. The destination buffer domain_info->triplet is an
> array of length MWIFIEX_MAX_TRIPLET_802_11D(83). The remote
> attacker can build a fake AP with the same ssid as real AP, and
> send malicous beacon packet with long WLAN_EID_COUNTRY elemen
> (country_ie_len > 83). Attacker can force STA connect to fake AP
> on a different channel. When the victim STA connects to fake AP,
> will trigger the heap buffer overflow. Fix this by checking for
> length and if found invalid, don not connect to the AP.
>
> This fix addresses CVE-2019-14895.
>
> Reported-by: huangwen <[email protected]>
> Signed-off-by: Ganapathi Bhat <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
> [bwh: Backported to 3.16:
> - Use wiphy_dbg() instead of mwifiex_dbg()
> - Adjust filename]
> Signed-off-by: Ben Hutchings <[email protected]>
> ---
> drivers/net/wireless/mwifiex/sta_ioctl.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> --- a/drivers/net/wireless/mwifiex/sta_ioctl.c
> +++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
> @@ -223,6 +223,14 @@ static int mwifiex_process_country_ie(st
> "11D: skip setting domain info in FW\n");
> return 0;
> }
> +
> + if (country_ie_len >
> + (IEEE80211_COUNTRY_STRING_LEN + MWIFIEX_MAX_TRIPLET_802_11D)) {
> + wiphy_dbg(priv->wdev->wiphy,
> + "11D: country_ie_len overflow!, deauth AP\n");
> + return -EINVAL;
> + }
> +
> memcpy(priv->adapter->country_code, &country_ie[2], 2);
>
> domain_info->country_code[0] = country_ie[2];
> @@ -266,8 +274,9 @@ int mwifiex_bss_start(struct mwifiex_pri
> priv->scan_block = false;
>
> if (bss) {
> - if (adapter->region_code == 0x00)
> - mwifiex_process_country_ie(priv, bss);
> + if (adapter->region_code == 0x00 &&
> + mwifiex_process_country_ie(priv, bss))
> + return -EINVAL;
>
> /* Allocate and fill new bss descriptor */
> bss_desc = kzalloc(sizeof(struct mwifiex_bssdescriptor),
>
Brian Norris noted that this commit has unbalanced locking and
submitted a followup as per:
https://lkml.kernel.org/linux-wireless/[email protected]/T/#u
https://patchwork.kernel.org/patch/11320227/
Regards,
Salvatore
On Thu, 2020-01-09 at 13:12 +0100, Salvatore Bonaccorso wrote:
> Hi Ben,
>
> On Wed, Jan 08, 2020 at 07:44:00PM +0000, Ben Hutchings wrote:
> > 3.16.81-rc1 review patch. If anyone has any objections, please let me know.
> >
> > ------------------
> >
> > From: Ganapathi Bhat <[email protected]>
> >
> > commit 3d94a4a8373bf5f45cf5f939e88b8354dbf2311b upstream.
[...]
> Brian Norris noted that this commit has unbalanced locking and
> submitted a followup as per:
>
> https://lkml.kernel.org/linux-wireless/[email protected]/T/#u
> https://patchwork.kernel.org/patch/11320227/
Right, but is the new behaviour (possible wrong preemption count)
actually worse than the old behaviour (possible heap buffer overflow)?
I think we are better off applying this now and adding that fix once
it's upstream.
Ben.
--
Ben Hutchings
Every program is either trivial or else contains at least one bug