Bug Patch

Bug Patch

[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 
-------------- 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 

Bug Patch

[Sudip Mukherjee]

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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Bug Patch

[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

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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>

Bug Patch

[Sudip Mukherjee]

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 at kernelnewbies.org
>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>>
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Bug Patch

[Doug Wilson]

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

Bug Patch

[Valdis.Kletnieks at vt.edu]

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.
-------------- 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 

Bug Patch

[nick]

On 14-09-08 02:08 PM, Valdis.Kletnieks at 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 
-------------- 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 

Bug Patch

[Valdis.Kletnieks at vt.edu]

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.






-------------- 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 

Bug Patch

[nick]

On 14-09-08 06:09 PM, Valdis.Kletnieks at 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