Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 22 Nov 2019 14:13:39 +0300
From:   Dan Carpenter <dan.carpenter@oracle.com>
To:     qize wang <wangqize888888888@gmail.com>
Cc:     linux-wireless@vger.kernel.org, amitkarwar <amitkarwar@gmail.com>,
        nishants <nishants@marvell.com>, gbhat <gbhat@marvell.com>,
        huxinming820 <huxinming820@gmail.com>,
        kvalo <kvalo@codeaurora.org>, Greg KH <greg@kroah.com>,
        security <security@kernel.org>,
        linux-distros <linux-distros@vs.openwall.org>,
        Solar Designer <solar@openwall.com>
Subject: Re: [PATCH] mwifiex: Fix heap overflow in
 mmwifiex_process_tdls_action_frame()
Message-ID: <20191122111339.GH617@kadam>
References: <E40E893E-D9B4-4C63-8139-1DD5E1C2CECB@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <E40E893E-D9B4-4C63-8139-1DD5E1C2CECB@gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, Nov 22, 2019 at 05:43:49PM +0800, qize wang wrote:
> 		case WLAN_EID_HT_CAPABILITY:
> -			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
                                                                ^^^^
> +			if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
> +				return;
> +			if (pos[1] != sizeof(struct ieee80211_ht_cap))
> +				return;
> +			/* copy the ie's value into ht_capb*/
> +			memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos + 2,
                                                                 ^^^^^^^

I don't understand why we changed "pos" to "pos + 2".  Presumably there
is a reason, but it needs to explained in the commit message.


> 			       sizeof(struct ieee80211_ht_cap));
> 			sta_ptr->is_11n_enabled = 1;
> 			break;
> 		case WLAN_EID_HT_OPERATION:
> -			memcpy(&sta_ptr->tdls_cap.ht_oper, pos,
                                                           ^^^
> +			if (pos > end -
> +			    sizeof(struct ieee80211_ht_operation) - 2)
> +				return;
> +			if (pos[1] != sizeof(struct ieee80211_ht_operation))
> +				return;
> +			/* copy the ie's value into ht_oper*/
> +			memcpy(&sta_ptr->tdls_cap.ht_oper, pos + 2,
                                                           ^^^^^^^

> 			       sizeof(struct ieee80211_ht_operation));
> 			break;
> 		case WLAN_EID_BSS_COEX_2040:
> +			if (pos > end - 3)
> +				return;
> +			if (pos[1] != 1)
> +				return;
> 			sta_ptr->tdls_cap.coex_2040 = pos[2];
> 			break;
> 		case WLAN_EID_EXT_CAPABILITY:
> +			if (pos > end - sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] < sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] > 8)
> +				return;
> 			memcpy((u8 *)&sta_ptr->tdls_cap.extcap, pos,
> 			       sizeof(struct ieee_types_header) +
> 			       min_t(u8, pos[1], 8));
> 			break;
> 		case WLAN_EID_RSN:
> +			if (pos > end - sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] < sizeof(struct ieee_types_header))
> +				return;
> +			if (pos[1] > IEEE_MAX_IE_SIZE -
> +			    sizeof(struct ieee_types_header))
> +				return;
> 			memcpy((u8 *)&sta_ptr->tdls_cap.rsn_ie, pos,
> 			       sizeof(struct ieee_types_header) +
> 			       min_t(u8, pos[1], IEEE_MAX_IE_SIZE -
> 				     sizeof(struct ieee_types_header)));
> 			break;
> 		case WLAN_EID_QOS_CAPA:
> +			if (pos > end - 3)
> +				return;
> +			if (pos[1] != 1)
> +				return;
> 			sta_ptr->tdls_cap.qos_info = pos[2];
> 			break;
> 		case WLAN_EID_VHT_OPERATION:
> -			if (priv->adapter->is_hw_11ac_capable)
> -				memcpy(&sta_ptr->tdls_cap.vhtoper, pos,
                                                                   ^^^

> +			if (priv->adapter->is_hw_11ac_capable) {
> +				if (pos > end -
> +				    sizeof(struct ieee80211_vht_operation) - 2)
> +					return;
> +				if (pos[1] !=
> +				    sizeof(struct ieee80211_vht_operation))
> +					return;
> +				/* copy the ie's value into vhtoper*/
> +				memcpy(&sta_ptr->tdls_cap.vhtoper, pos + 2,
                                                                   ^^^^^^^
> 				       sizeof(struct ieee80211_vht_operation));
> +			}
> 			break;
> 		case WLAN_EID_VHT_CAPABILITY:
> 			if (priv->adapter->is_hw_11ac_capable) {
> -				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos,
                                                                        ^^^

> +				if (pos > end -
> +				    sizeof(struct ieee80211_vht_cap) - 2)
> +					return;
> +				if (pos[1] != sizeof(struct ieee80211_vht_cap))
> +					return;
> +				/* copy the ie's value into vhtcap*/
> +				memcpy((u8 *)&sta_ptr->tdls_cap.vhtcap, pos + 2,
                                                                        ^^^^^^^

> 				       sizeof(struct ieee80211_vht_cap));
> 				sta_ptr->is_11ac_enabled = 1;
> 			}
> 			break;

I was confused by the "- 2" as well in the earlier versions of this
patch but Marvell approved it so I assumed it was correct.  It would be
nice if this patch were tested.

regards,
dan carpenter