Re: Bug Patch
On 14-09-08 06:09 PM, valdis.kletni...@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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Bug Patch
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. pgpsLEZjTlRai.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Bug Patch
On 14-09-08 02:08 PM, valdis.kletni...@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 >From d5f7b8929bebcf0b12d8e402932b790f61786168 Mon Sep 17 00:00:00 2001 From: Nicholas Krause 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 --- 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 NULL; 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
Re: Bug Patch
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. pgpB6ptu0hHx4.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Bug Patch
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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Bug Patch
On Mon, Sep 8, 2014 at 4:51 PM, Tobias S. Josefowitz 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 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: Bug Patch
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 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
Re: Bug Patch
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 > 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 > --- > 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Bug Patch
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 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 --- 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; 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
