[PATCH] staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[PATCH] staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Zhang Shurong]

The "exc->key_len" is a u16 that comes from the user.  If it's over
IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.

Fixes: b121d84882b9 ("staging: ks7010: simplify calls to memcpy()")

Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
---
 drivers/staging/ks7010/ks_wlan_net.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ks7010/ks_wlan_net.c b/drivers/staging/ks7010/ks_wlan_net.c
index e03c87f0bfe7..0fb97a79ad0b 100644
--- a/drivers/staging/ks7010/ks_wlan_net.c
+++ b/drivers/staging/ks7010/ks_wlan_net.c
@@ -1583,8 +1583,10 @@ static int ks_wlan_set_encode_ext(struct net_device *dev,
 			commit |= SME_WEP_FLAG;
 		}
 		if (enc->key_len) {
-			memcpy(&key->key_val[0], &enc->key[0], enc->key_len);
-			key->key_len = enc->key_len;
+			int key_len = clamp_val(enc->key_len, 0, IW_ENCODING_TOKEN_MAX);
+
+			memcpy(&key->key_val[0], &enc->key[0], key_len);
+			key->key_len = key_len;
 			commit |= (SME_WEP_VAL1 << index);
 		}
 		break;
-- 
2.30.2

Re: [PATCH] staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Markus Elfring]

> The "exc->key_len" is a u16 that comes from the user.  If it's over
> IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.

Please choose an imperative change suggestion.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94

Regards,
Markus

Re: [PATCH] staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Greg Kroah-Hartman]

On Sun, Jul 09, 2023 at 09:21:45PM +0200, Markus Elfring wrote:
> > The "exc->key_len" is a u16 that comes from the user.  If it's over
> > IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.
> 
> Please choose an imperative change suggestion.

Please stop reviewing staging patches, it is not helpful for anyone.

greg k-h

Re: staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Markus Elfring]

>>> The "exc->key_len" is a u16 that comes from the user.  If it's over
>>> IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.
>>
>> Please choose an imperative change suggestion.
>
> Please stop reviewing staging patches, it is not helpful for anyone.

It seems that further contributors need to become more aware about
(and finally adhere to) requirements from the Linux development documentation.

Regards,
Markus

Re: staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Zhang Shurong]

Hi Markus,

> 2023年7月10日 03:56，Markus Elfring <Markus.Elfring@web.de> 写道：
> 
>>>> The "exc->key_len" is a u16 that comes from the user.  If it's over
>>>> IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.
>>> 
>>> Please choose an imperative change suggestion.
>> 
>> Please stop reviewing staging patches, it is not helpful for anyone.
> 
> It seems that further contributors need to become more aware about
> (and finally adhere to) requirements from the Linux development documentation.
> 
> Regards,
> Markus

I'm guilty of my incorrect patch format. And do I need to write another
patch to fix this format issue?

Best regards
Shurong

Re: staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Greg Kroah-Hartman]

On Mon, Jul 10, 2023 at 01:41:07PM +0800, Zhang Shurong wrote:
> Hi Markus,
> 
> > 2023年7月10日 03:56，Markus Elfring <Markus.Elfring@web.de> 写道：
> > 
> >>>> The "exc->key_len" is a u16 that comes from the user.  If it's over
> >>>> IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.
> >>> 
> >>> Please choose an imperative change suggestion.
> >> 
> >> Please stop reviewing staging patches, it is not helpful for anyone.
> > 
> > It seems that further contributors need to become more aware about
> > (and finally adhere to) requirements from the Linux development documentation.
> > 
> > Regards,
> > Markus
> 
> I'm guilty of my incorrect patch format. And do I need to write another
> patch to fix this format issue?

One of the hardest things in kernel development is learning what
reviews, and what reviewers, should be ignored.  This is one of them :)

No need to change the patch yet, let me get to this change this week and
I'll let you know.

thanks,

greg k-h

Re: staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Markus Elfring]

…
>>> It seems that further contributors need to become more aware about
>>> (and finally adhere to) requirements from the Linux development documentation.
…
>> I'm guilty of my incorrect patch format. And do I need to write another
>> patch to fix this format issue?
>
> One of the hardest things in kernel development is learning what
> reviews, and what reviewers, should be ignored.  This is one of them :)

Recurring communication difficulties triggered such a response.
Please reconsider it more!


> No need to change the patch yet, let me get to this change this week

The desired patch review may take more time.


> and I'll let you know.

Some contributors will be curious how corresponding messages will evolve.

Regards,
Markus

Re: staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Markus Elfring]

…
>> It seems that further contributors need to become more aware about
>> (and finally adhere to) requirements from the Linux development documentation.
…
> I'm guilty of my incorrect patch format. And do I need to write another
> patch to fix this format issue?

This can be an usual consequence for the current change review
if you would really like to take provided information and advices
better into account.
Some communication parameters influence if further development ideas
will actually be picked up for desirable improvements
(also for your change suggestions).

Regards,
Markus

Re: [PATCH] staging: ks7010: potential buffer overflow in ks_wlan_set_encode_ext()

[Dan Carpenter]

On Sun, Jul 09, 2023 at 01:50:07PM +0800, Zhang Shurong wrote:
> The "exc->key_len" is a u16 that comes from the user.  If it's over
> IW_ENCODING_TOKEN_MAX (64) that could lead to memory corruption.
> 
> Fixes: b121d84882b9 ("staging: ks7010: simplify calls to memcpy()")
> 
> Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>

Reviewed-by: Dan Carpenter <dan.carpenter@linaro.org>

regards,
dan carpenter