Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()

From: Brian Norris <briannorris@chromium.org>
To: qize wang <wangqize888888888@gmail.com>
Cc: linux-wireless <linux-wireless@vger.kernel.org>,
	amit karwar <amitkarwar@gmail.com>,
	Nishant Sarmukadam <nishants@marvell.com>,
	Ganapathi Bhat <gbhat@marvell.com>,
	Xinming Hu <huxinming820@gmail.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	greg@kroah.com, Dan Carpenter <dan.carpenter@oracle.com>,
	Solar Designer <solar@openwall.com>
Subject: Re: [PATCH v3] mwifiex: Fix heap overflow in mmwifiex_process_tdls_action_frame()
Date: Mon, 2 Dec 2019 15:16:35 -0800	[thread overview]
Message-ID: <CA+ASDXOj0rz4S3BoMdF1pqOiVZ4yhVE_Qy+s6iTObCnhFfwFLw@mail.gmail.com> (raw)
In-Reply-To: <20191129101054.2756-1-wangqize888888888@gmail.com>

A bit late, but a few readability and maintainability thoughts:

On Fri, Nov 29, 2019 at 2:12 AM qize wang <wangqize888888888@gmail.com> wrote:
>
> mwifiex_process_tdls_action_frame() without checking
> the incoming tdls infomation element's vality before use it,
> this may cause multi heap buffer overflows.
>
> Fix them by putting vality check before use it.
>
> IE is TLV struct, but ht_cap and  ht_oper aren’t TLV struct.
> the origin marvell driver code is wrong:
>
> memcpy(&sta_ptr->tdls_cap.ht_oper, pos,....
> memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,...
>
> Fix the bug by changing pos(the address of IE) to
> pos+2 ( the address of IE value ).
>
> v3: change commit log
>

Would have been great to have a

Cc: <stable@vger.kernel.org>

tag here. I'm not sure if "just have GregKH on CC" is the right process...

> Signed-off-by: qize wang <wangqize888888888@gmail.com>
> ---
>  drivers/net/wireless/marvell/mwifiex/tdls.c | 70 ++++++++++++++++++++++++++---
>  1 file changed, 64 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
> index 09313047beed..7caf1d26124a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/tdls.c
> +++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
> @@ -953,59 +953,117 @@ void mwifiex_process_tdls_action_frame(struct mwifiex_private *priv,
>
>                 switch (*pos) {
>                 case WLAN_EID_SUPP_RATES:
> +                       if (pos[1] > 32)

Really, you needed a magic '32' here? Would be much clearer with:

  if (pos[1] > sizeof(sta_ptr->tdls_cap.rates))

Same with many of the other cases.

> +                               return;

Is 'return' really the right answer for all of these? Just because,
e.g., the rates IE is larger than our internal struct, should we
really drop the entire frame? Should we be continuing to parse the
other IEs, if possible? Or is this overflow a sign of a totally
damaged (possibly malicious) frame, because it's required to be
smaller than this? (Sorry, I didn't read the spec here yet.)

>                         sta_ptr->tdls_cap.rates_len = pos[1];
>                         for (i = 0; i < pos[1]; i++)
>                                 sta_ptr->tdls_cap.rates[i] = pos[i + 2];
>                         break;
>
[...]
>                 case WLAN_EID_HT_CAPABILITY:
> -                       memcpy((u8 *)&sta_ptr->tdls_cap.ht_capb, pos,
> +                       if (pos > end - sizeof(struct ieee80211_ht_cap) - 2)
> +                               return;

For checks like this ("past 'end'"), it does make sense to return.

Brian