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

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

[Greg KH]

Some minor problems with your patch:

On Fri, Nov 29, 2019 at 04:18:21PM +0800, qize wang 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’s value ).
> 
> Signed-off-by: wangqize <540263207@qq.com>

This has to match the name on the From: line.

> ---
> v2: change commit log
>  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)
> + return;

All of your whitespace is totally damaged here, making this patch
impossible to apply :(

Please fix up your email client to not do that (you can just use 'git
send-email' directly) and resend a v3.

thanks,

greg k-h

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

[Kalle Valo]

(dropping security lists)

Greg KH <gregkh@linuxfoundation.org> writes:

> Some minor problems with your patch:
>
> On Fri, Nov 29, 2019 at 04:18:21PM +0800, qize wang 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’s value ).
>> 
>> Signed-off-by: wangqize <540263207@qq.com>
>
> This has to match the name on the From: line.
>
>> ---
>> v2: change commit log
>>  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)
>> + return;
>
> All of your whitespace is totally damaged here, making this patch
> impossible to apply :(

And even worse, it was using HTML :)

> Please fix up your email client to not do that (you can just use 'git
> send-email' directly) and resend a v3.

Yes, please. And even better if you try sending the patch to yourself
and then applying with git-am. That way you should notice any problems
with the mail settings.

More info in the link below, read it very carefully.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches