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
On Mon, Sep 08, 2014 at 06:36:34AM -0400, nick wrote:
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
From d5f7b8929bebcf0b12d8e402932b790f61786168 Mon Sep 17 00:00:00 2001 From: Nicholas Krause <xerofoify@gmail.com> 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 <xerofoify@gmail.com> --- drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c index 73410cc..dc8520d 100644 --- a/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c +++ b/drivers/staging/rtl8192u/ieee80211/ieee80211_rx.c @@ -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
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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
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 On Mon, Sep 8, 2014 at 12:36 PM, nick <xerofoify@gmail.com> wrote:
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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Mon, Sep 8, 2014 at 4:51 PM, Tobias S. Josefowitz <t.josefowitz@gmail.com> wrote:
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
On Mon, Sep 8, 2014 at 12:36 PM, nick <xerofoify@gmail.com> wrote:
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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Tobi, Nick,
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
On Mon, 08 Sep 2014 06:36:34 -0400, nick said:
Found a bug and attempted to fix it 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.
On 14-09-08 02:08 PM, Valdis.Kletnieks@vt.edu wrote:
On Mon, 08 Sep 2014 06:36:34 -0400, nick said:
Found a bug and attempted to fix it 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.
Fixed the patch, and Valdis thanks for the note about reading the code properly first. Nick
On Mon, 08 Sep 2014 17:05:39 -0400, nick said:
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.
+ 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.
On 14-09-08 06:09 PM, Valdis.Kletnieks@vt.edu wrote:
On Mon, 08 Sep 2014 17:05:39 -0400, nick said:
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.
+ 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.
Thanks for at least giving me credit. Nick
participants (5)
-
Doug Wilson -
nick -
Sudip Mukherjee -
Tobias S. Josefowitz -
Valdis.Kletnieks＠vt.edu