[PATCH 1/2] mwifiex: fix use-after-free in beacon_ie processing

[PATCH 1/2] mwifiex: fix use-after-free in beacon_ie processing

[Bing Zhao]

beacon_ie buffer is allocated in mwifiex_fill_new_bss_desc()
and the buffer pointer is saved in bss_desc->beacon_buf.
beacon_ie is freed before the function returns. However,
bss_desc->beacon_buf is still being accessed afterwards.

Fix it by freeing beacon_ie (bss_desc->beacon_buf) in
caller's scope.

Reviewed-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
---
 drivers/net/wireless/mwifiex/scan.c      |    8 ++++++++
 drivers/net/wireless/mwifiex/sta_ioctl.c |   14 +++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index e7f6dea..37b24e8 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -1533,10 +1533,18 @@ static int mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
 	/* Make a copy of current BSSID descriptor */
 	memcpy(&priv->curr_bss_params.bss_descriptor, bss_desc,
 	       sizeof(priv->curr_bss_params.bss_descriptor));
+
+	/* The contents of beacon_ie will be copied to its own buffer
+	 * in mwifiex_save_curr_bcn()
+	 */
 	mwifiex_save_curr_bcn(priv);
 	spin_unlock_irqrestore(&priv->curr_bcn_buf_lock, flags);
 
 done:
+	/* beacon_ie buffer was allocated in function
+	 * mwifiex_fill_new_bss_desc(). Free it now.
+	 */
+	kfree(bss_desc->beacon_buf);
 	kfree(bss_desc);
 	return 0;
 }
diff --git a/drivers/net/wireless/mwifiex/sta_ioctl.c b/drivers/net/wireless/mwifiex/sta_ioctl.c
index e6c9b2a..27729cf 100644
--- a/drivers/net/wireless/mwifiex/sta_ioctl.c
+++ b/drivers/net/wireless/mwifiex/sta_ioctl.c
@@ -140,12 +140,13 @@ int mwifiex_request_set_multicast_list(struct mwifiex_private *priv,
 /*
  * This function fills bss descriptor structure using provided
  * information.
+ * beacon_ie buffer is allocated in this function. It is caller's
+ * responsibility to free the memory.
  */
 int mwifiex_fill_new_bss_desc(struct mwifiex_private *priv,
 			      struct cfg80211_bss *bss,
 			      struct mwifiex_bssdescriptor *bss_desc)
 {
-	int ret;
 	u8 *beacon_ie;
 	size_t beacon_ie_len;
 	struct mwifiex_bss_priv *bss_priv = (void *)bss->priv;
@@ -165,6 +166,7 @@ int mwifiex_fill_new_bss_desc(struct mwifiex_private *priv,
 
 	memcpy(bss_desc->mac_address, bss->bssid, ETH_ALEN);
 	bss_desc->rssi = bss->signal;
+	/* The caller of this function will free beacon_ie */
 	bss_desc->beacon_buf = beacon_ie;
 	bss_desc->beacon_buf_size = beacon_ie_len;
 	bss_desc->beacon_period = bss->beacon_interval;
@@ -182,10 +184,7 @@ int mwifiex_fill_new_bss_desc(struct mwifiex_private *priv,
 	else
 		bss_desc->bss_mode = NL80211_IFTYPE_STATION;
 
-	ret = mwifiex_update_bss_desc_with_ie(priv->adapter, bss_desc);
-
-	kfree(beacon_ie);
-	return ret;
+	return mwifiex_update_bss_desc_with_ie(priv->adapter, bss_desc);
 }
 
 static int mwifiex_process_country_ie(struct mwifiex_private *priv,
@@ -349,6 +348,11 @@ int mwifiex_bss_start(struct mwifiex_private *priv, struct cfg80211_bss *bss,
 	}
 
 done:
+	/* beacon_ie buffer was allocated in function
+	 * mwifiex_fill_new_bss_desc(). Free it now.
+	 */
+	if (bss_desc)
+		kfree(bss_desc->beacon_buf);
 	kfree(bss_desc);
 	return ret;
 }
-- 
1.7.0.2

[PATCH 2/2] mwifiex: remove redundant initialization for bss_descriptor

[Bing Zhao]

Initialization of bss_descriptor is unnecessary as the entire
structure will be overwritten by a memcpy.

Reported-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Doug Anderson <dianders@chromium.org>
Reviewed-by: Paul Stewart <pstew@chromium.org>
Signed-off-by: Bing Zhao <bzhao@marvell.com>
Signed-off-by: Yogesh Ashok Powar <yogeshp@marvell.com>
---
 drivers/net/wireless/mwifiex/scan.c |   29 -----------------------------
 1 files changed, 0 insertions(+), 29 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
index 37b24e8..9cf5d8f 100644
--- a/drivers/net/wireless/mwifiex/scan.c
+++ b/drivers/net/wireless/mwifiex/scan.c
@@ -1500,36 +1500,7 @@ static int mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
 	if (ret)
 		goto done;
 
-	/* Update current bss descriptor parameters */
 	spin_lock_irqsave(&priv->curr_bcn_buf_lock, flags);
-	priv->curr_bss_params.bss_descriptor.bcn_wpa_ie = NULL;
-	priv->curr_bss_params.bss_descriptor.wpa_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_rsn_ie = NULL;
-	priv->curr_bss_params.bss_descriptor.rsn_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_wapi_ie = NULL;
-	priv->curr_bss_params.bss_descriptor.wapi_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_ht_cap = NULL;
-	priv->curr_bss_params.bss_descriptor.ht_cap_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_ht_oper = NULL;
-	priv->curr_bss_params.bss_descriptor.ht_info_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_bss_co_2040 = NULL;
-	priv->curr_bss_params.bss_descriptor.bss_co_2040_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_ext_cap = NULL;
-	priv->curr_bss_params.bss_descriptor.ext_cap_offset = 0;
-	priv->curr_bss_params.bss_descriptor.beacon_buf = NULL;
-	priv->curr_bss_params.bss_descriptor.beacon_buf_size = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_vht_cap = NULL;
-	priv->curr_bss_params.bss_descriptor.vht_cap_offset = 0;
-	priv->curr_bss_params.bss_descriptor.bcn_vht_oper = NULL;
-	priv->curr_bss_params.bss_descriptor.vht_info_offset = 0;
-	priv->curr_bss_params.bss_descriptor.oper_mode = NULL;
-	priv->curr_bss_params.bss_descriptor.oper_mode_offset = 0;
-
-	/* Disable 11ac by default. Enable it only where there
-	 * exist VHT_CAP IE in AP beacon
-	 */
-	priv->curr_bss_params.bss_descriptor.disable_11ac = true;
-
 	/* Make a copy of current BSSID descriptor */
 	memcpy(&priv->curr_bss_params.bss_descriptor, bss_desc,
 	       sizeof(priv->curr_bss_params.bss_descriptor));
-- 
1.7.0.2

Re: [PATCH 2/2] mwifiex: remove redundant initialization for bss_descriptor

[Troy Kisky]

On 4/11/2013 5:53 PM, Bing Zhao wrote:
> Initialization of bss_descriptor is unnecessary as the entire
> structure will be overwritten by a memcpy.
>
> Reported-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Doug Anderson <dianders@chromium.org>
> Reviewed-by: Paul Stewart <pstew@chromium.org>
> Signed-off-by: Bing Zhao <bzhao@marvell.com>
> Signed-off-by: Yogesh Ashok Powar <yogeshp@marvell.com>
> ---
>   drivers/net/wireless/mwifiex/scan.c |   29 -----------------------------
>   1 files changed, 0 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/net/wireless/mwifiex/scan.c b/drivers/net/wireless/mwifiex/scan.c
> index 37b24e8..9cf5d8f 100644
> --- a/drivers/net/wireless/mwifiex/scan.c
> +++ b/drivers/net/wireless/mwifiex/scan.c
> @@ -1500,36 +1500,7 @@ static int mwifiex_update_curr_bss_params(struct mwifiex_private *priv,
>   	if (ret)
>   		goto done;
>   
> -	/* Update current bss descriptor parameters */
>   	spin_lock_irqsave(&priv->curr_bcn_buf_lock, flags);
> -	priv->curr_bss_params.bss_descriptor.bcn_wpa_ie = NULL;
> -	priv->curr_bss_params.bss_descriptor.wpa_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_rsn_ie = NULL;
> -	priv->curr_bss_params.bss_descriptor.rsn_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_wapi_ie = NULL;
> -	priv->curr_bss_params.bss_descriptor.wapi_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_ht_cap = NULL;
> -	priv->curr_bss_params.bss_descriptor.ht_cap_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_ht_oper = NULL;
> -	priv->curr_bss_params.bss_descriptor.ht_info_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_bss_co_2040 = NULL;
> -	priv->curr_bss_params.bss_descriptor.bss_co_2040_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_ext_cap = NULL;
> -	priv->curr_bss_params.bss_descriptor.ext_cap_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.beacon_buf = NULL;
> -	priv->curr_bss_params.bss_descriptor.beacon_buf_size = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_vht_cap = NULL;
> -	priv->curr_bss_params.bss_descriptor.vht_cap_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.bcn_vht_oper = NULL;
> -	priv->curr_bss_params.bss_descriptor.vht_info_offset = 0;
> -	priv->curr_bss_params.bss_descriptor.oper_mode = NULL;
> -	priv->curr_bss_params.bss_descriptor.oper_mode_offset = 0;
> -
> -	/* Disable 11ac by default. Enable it only where there
> -	 * exist VHT_CAP IE in AP beacon
> -	 */
> -	priv->curr_bss_params.bss_descriptor.disable_11ac = true;
> -

Should the = true line be moved below the memcpy?  Your way produces
the same behavior, but was it buggy before?
>   	/* Make a copy of current BSSID descriptor */
>   	memcpy(&priv->curr_bss_params.bss_descriptor, bss_desc,
>   	       sizeof(priv->curr_bss_params.bss_descriptor));

RE: [PATCH 2/2] mwifiex: remove redundant initialization for bss_descriptor

[Bing Zhao]

Hi Troy,

Thanks for reviewing.

> > -	priv->curr_bss_params.bss_descriptor.disable_11ac = true;
> > -
> 
> Should the = true line be moved below the memcpy?  Your way produces 
> the same behavior, but was it buggy before?

"bss_descriptor.disable_11ac = true" shouldn't be set after memcpy, because bss_desc->disable_11ac reflects the associated AP's 11ac setting here.

Perhaps we should set "bss_desc->disable_11ac = true" earlier before parsing AP's beacon IE?

@@ -184,6 +184,8 @@ int mwifiex_fill_new_bss_desc(struct mwifiex_private *priv,
        else
                bss_desc->bss_mode = NL80211_IFTYPE_STATION;

+       bss_desc->disable_11ac = true;
+
        return mwifiex_update_bss_desc_with_ie(priv->adapter, bss_desc);
 }

This flag can be reset to false if AP's beacon does advertise 11ac support.

Yogesh, any thoughts?

We had offline discussion on replacing disable_11ac with existing VHT flags.
Maybe initializing it in mwifiex_fill_new_bss_desc() is way to go?

Thanks,
Bing

> >   	/* Make a copy of current BSSID descriptor */
> >   	memcpy(&priv->curr_bss_params.bss_descriptor, bss_desc,
> >   	       sizeof(priv->curr_bss_params.bss_descriptor));