Bug Patch

DISCUSSION nick 5 participants 9 messages Sep 8, 2014 – Sep 9, 2014

Share & tools

nick

author

2014-09-08 10:36 UTC #0 lore raw

Hey Guys,
Found a bug and attempted to fix it. I am attaching the patch, no build or checkpatch errors and also checked to see if I need to clean up any memory when returning, and this seems to be true. Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-staging-Fix-ieee_80211_rx.c-to-check-for-Null-alloca.patch
Type: text/x-patch
Size: 1148 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140908/1d9cf850/attachment.bin

Sudip Mukherjee

2014-09-08 11:10 UTC #1 lore raw

Quoted 23 lines · nick

Hey Guys,
Found a bug and attempted to fix it. I am attaching the patch, no build or checkpatch errors and also checked to see if I need to clean up any memory when returning, and this seems to be true. Nick
Quoted 1 line
From d5f7b8929bebcf0b12d8e402932b790f61786168 Mon Sep 17 00:00:00 2001
From: Nicholas Krause <[email protected]>
Date: Mon, 8 Sep 2014 05:57:09 -0400
Subject: [PATCH] staging: Fix ieee_80211_rx.c to check for Null allocated skb
In ieee_80211_rx.c we may have a Null allocated sub in parse_subframe and need to check if the allocated skb is NUll. If it is return -ENOMEM.
Signed-off-by Nicholas Krause <[email protected]>
---
 drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c +0 -0
index 73410cc..dc8520d 100644

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c static u8 parse_subframe(struct sk_buff *skb, · line 851

Quoted 5 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c +2 -0

@@ -847,6 +847,8 @@ static u8 parse_subframe(struct sk_buff *skb,
 #else
 			/* Allocate new skb for releasing to upper layer */
 			sub_skb = dev_alloc_skb(nSubframe_Length + 12);
+			if (!sub_skb)
+				return -ENOMEM;

parse_subframe is returning 0 as error.
if you see the functions that are calling parse_subframe , you will see they are comparing the return value with zero for failure. So you are returning -ENOMEM that is a success and it will try to process that frame , and i hope you can imagine what happens next.

thanks
sudip

Quoted 11 lines

drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c +0 -1
 			skb_reserve(sub_skb, 12);
 			data_ptr = (u8 *)skb_put(sub_skb, nSubframe_Length);
 			memcpy(data_ptr,skb->data,nSubframe_Length);
--
1.9.1
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Tobias S. Josefowitz

2014-09-08 11:21 UTC #2 lore raw

In reply to nick

Hi Nick,

parse_subframe() is a static function and the only caller assumes that skb is != NULL and would be in trouble way before parse_subframe() if skb was indeed NULL.

Tobi

Quoted 10 lines · nick

Hey Guys,
Found a bug and attempted to fix it. I am attaching the patch, no build or checkpatch errors and also checked to see if I need to clean up any memory when returning, and this seems to be true. Nick
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Sudip Mukherjee

2014-09-08 11:35 UTC #3 lore raw

Quoted 8 lines · Tobias S. Josefowitz

Hi Nick,
parse_subframe() is a static function and the only caller assumes that skb is != NULL and would be in trouble way before parse_subframe() if skb was indeed NULL.
Tobi

Hi Tobi,
I think Nick's patch is regarding dev_alloc_skb(nSubframe_Length + 12) ; There is no error check for the return value of dev_alloc_skb , and it can return NULL if it fails and the memory is not allocated. I admit return -ENOMEM is wrong , but still I think Nick has found something this time.

sudip

Quoted 16 lines · nick

Quoted 10 lines
Hey Guys,
Found a bug and attempted to fix it. I am attaching the patch, no build or checkpatch errors and also checked to see if I need to clean up any memory when returning, and this seems to be true. Nick
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Doug Wilson

2014-09-08 12:45 UTC #4 lore raw

Tobi, Nick,

Quoted 6 lines

I think Nick's patch is regarding dev_alloc_skb(nSubframe_Length + 12) ; There is no error check for the return value of dev_alloc_skb , and it can return NULL if it fails and the memory is not allocated. I admit return -ENOMEM is wrong , but still I think Nick has found something this time.

Nick, the patch you sent is doing the right thing, but like Tobi
mentioned -ENOMEM is wrong.
dev_alloc_skb internally calls __netdev_alloc_skb and the comment on top of the function says
*%NULL is returned if there is no free memory*. So could you change the patch accordingly.

for eg: if (sub_skb == NULL)

                       return NULL;

- Doug

Valdis.Kletnieks at vt.edu

2014-09-08 18:08 UTC #5 lore raw

In reply to nick

Review

Quoted 4 lines · nick

Found a bug and attempted to fix it
(quoted diff) +2 -0
 			sub_skb = dev_alloc_skb(nSubframe_Length + 12);
+			if (!sub_skb)
+				return -ENOMEM;

Nick - we've told you before to research this stuff more fully before posting patches. As others have pointed out, there's exactly one caller, who wants a different return on error.

For bonus points - explain why you're returning a -ENOMEM from a function that's declared as 'static u8 parse_subframe(...'.

This is *NOT* how you convince us that you should be allowed anywhere near
kernel code.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140908/c4a2a560/attachment.bin

nick

author

2014-09-08 21:05 UTC #6 lore raw

Quoted 11 lines · Valdis.Kletnieks at vt.edu

Review
Quoted 4 lines · nick
Found a bug and attempted to fix it
(quoted diff) +2 -0
 			sub_skb = dev_alloc_skb(nSubframe_Length + 12);
+			if (!sub_skb)
+				return -ENOMEM;
Nick - we've told you before to research this stuff more fully before posting patches. As others have pointed out, there's exactly one caller, who wants a different return on error.

Quoted 6 lines

For bonus points - explain why you're returning a -ENOMEM from a function that's declared as 'static u8 parse_subframe(...'.
This is *NOT* how you convince us that you should be allowed anywhere near kernel code.

Fixed the patch, and Valdis thanks for the note about reading the code properly first. Nick
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-staging-Fix-ieee_80211_rx.c-to-check-for-Null-alloca.patch
Type: text/x-patch
Size: 1145 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140908/4028da71/attachment.bin

Valdis.Kletnieks at vt.edu

2014-09-08 22:09 UTC #7 lore raw

Review

Quoted 5 lines · nick

In ieee_80211_rx.c we may have a Null allocated sub in parse_subframe and need to check if the allocated skb is NUll. If it is return -ENOMEM.
(quoted diff) +2 -0
+			if (!sub_skb)
+				return NULL;

1) null, Null, and NULL are all OK in various contexts. NUll isn't.

2) the rest of the file uses 'return 0;' not 'return NULL;'

Oh, and (3) What's wrong with this picture?

Nick, this is *exactly* the reason why *NOBODY* wants to accept code from you. It's faster and more efficient for me to code the patch myself and stick a Reported-By: crediting you for spotting the bug than it takes for multiple iterations to get your patch right.

And the *reason* I'm submitting the patch myself is because every single problem that's been pointed out to you with this patch is something that has been pointed out to you before on other patches.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140908/0b882694/attachment-0001.bin

nick

author

2014-09-09 00:51 UTC #8 lore raw

Quoted 14 lines · Valdis.Kletnieks at vt.edu

Review
Quoted 5 lines · nick
In ieee_80211_rx.c we may have a Null allocated sub in parse_subframe and need to check if the allocated skb is NUll. If it is return -ENOMEM.
(quoted diff) +2 -0
+			if (!sub_skb)
+				return NULL;
1) null, Null, and NULL are all OK in various contexts. NUll isn't.
2) the rest of the file uses 'return 0;' not 'return NULL;'
Oh, and (3) What's wrong with this picture?

Quoted 14 lines

Nick, this is *exactly* the reason why *NOBODY* wants to accept code from you. It's faster and more efficient for me to code the patch myself and stick a Reported-By: crediting you for spotting the bug than it takes for multiple iterations to get your patch right.
And the *reason* I'm submitting the patch myself is because every single problem that's been pointed out to you with this patch is something that has been pointed out to you before on other patches.

Thanks for at least giving me credit.
Nick