[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nicholas Krause]

I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
This entry states that we are not checking the skb allocated in fw_download_code
for NULL and after checking it ,I fixed it to check for the NULL value before
returning false and exiting fw_download_code cleanly. In additon I removed the
variable, rt_status as it's easier to read this function's return value with
just true or false and rt status is a unneeded variable for the bool return
of this function.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
index 1a95d1f..66d83f8 100644
--- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
+++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
@@ -36,7 +36,6 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
 			     u32 buffer_len)
 {
 	struct r8192_priv *priv = rtllib_priv(dev);
-	bool		    rt_status = true;
 	u16		    frag_threshold;
 	u16		    frag_length, frag_offset = 0;
 	int		    i;
@@ -61,6 +60,8 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
 		}
 
 		skb  = dev_alloc_skb(frag_length + 4);
+		if (skb == NULL)
+			return false;
 		memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
 		tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
 		tcb_desc->queue_index = TXCMD_QUEUE;
@@ -99,7 +100,7 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
 
 	write_nic_byte(dev, TPPoll, TPPoll_CQ);
 
-	return rt_status;
+	return true;
 }
 
 static bool CPUcheck_maincodeok_turnonCPU(struct net_device *dev)
-- 
1.9.1

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
> This entry states that we are not checking the skb allocated in fw_download_code
> for NULL and after checking it ,I fixed it to check for the NULL value before
> returning false and exiting fw_download_code cleanly. In additon I removed the
> variable, rt_status as it's easier to read this function's return value with
> just true or false and rt status is a unneeded variable for the bool return
> of this function.
>
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> index 1a95d1f..66d83f8 100644
> --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> @@ -36,7 +36,6 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
>                              u32 buffer_len)
>  {
>         struct r8192_priv *priv = rtllib_priv(dev);
> -       bool                rt_status = true;
>         u16                 frag_threshold;
>         u16                 frag_length, frag_offset = 0;
>         int                 i;
> @@ -61,6 +60,8 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
>                 }
>
>                 skb  = dev_alloc_skb(frag_length + 4);
> +               if (skb == NULL)
> +                       return false;
>                 memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
>                 tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
>                 tcb_desc->queue_index = TXCMD_QUEUE;
> @@ -99,7 +100,7 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
>
>         write_nic_byte(dev, TPPoll, TPPoll_CQ);
>
> -       return rt_status;
> +       return true;
>  }
>
>  static bool CPUcheck_maincodeok_turnonCPU(struct net_device *dev)
> --
> 1.9.1
>

I am trying to get this patch merged and after my issues with the
kernel community, I can't get this into the mainline.
If someone wants to send it out for me and state it's from me that
would be great.
Nick

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Jeff Haran]

> -----Original Message-----
> From: kernelnewbies-bounces at kernelnewbies.org [mailto:kernelnewbies-bounces at kernelnewbies.org] On Behalf Of Nick Krause
> Sent: Tuesday, August 12, 2014 1:20 PM
> To: kernelnewbies
> Subject: Re: [PATCHv3] staging: Check for Null allocated skb in fw_download_code
> 
> On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
> > This entry states that we are not checking the skb allocated in fw_download_code
> > for NULL and after checking it ,I fixed it to check for the NULL value before
> > returning false and exiting fw_download_code cleanly. In additon I removed the
> > variable, rt_status as it's easier to read this function's return value with
> > just true or false and rt status is a unneeded variable for the bool return
> > of this function.
> >
> > Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> > ---
> >  drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> > index 1a95d1f..66d83f8 100644
> > --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> > +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c
> > @@ -36,7 +36,6 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
> >                              u32 buffer_len)
> >  {
> >         struct r8192_priv *priv = rtllib_priv(dev);
> > -       bool                rt_status = true;
> >         u16                 frag_threshold;
> >         u16                 frag_length, frag_offset = 0;
> >         int                 i;
> > @@ -61,6 +60,8 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
> >                 }
> >
> >                 skb  = dev_alloc_skb(frag_length + 4);
> > +               if (skb == NULL)
> > +                       return false;
> >                 memcpy((unsigned char *)(skb->cb), &dev, sizeof(dev));
> >                 tcb_desc = (struct cb_desc *)(skb->cb + MAX_DEV_ADDR_SIZE);
> >                 tcb_desc->queue_index = TXCMD_QUEUE;
> > @@ -99,7 +100,7 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
> >
> >         write_nic_byte(dev, TPPoll, TPPoll_CQ);
> >
> > -       return rt_status;
> > +       return true;
> >  }
> >
> >  static bool CPUcheck_maincodeok_turnonCPU(struct net_device *dev)
> > --
> > 1.9.1
> >
> 
> I am trying to get this patch merged and after my issues with the
> kernel community, I can't get this into the mainline.
> If someone wants to send it out for me and state it's from me that
> would be great.
> Nick

While the avoidance of dereferencing NULL pointers in the kernel is a laudable goal, what will be the effect of the call to write_nic_byte() at the end of the function not happening should the call to dev_alloc_skb() return NULL?

Jeff Haran

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
> On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
> > This entry states that we are not checking the skb allocated in fw_download_code
> > for NULL and after checking it ,I fixed it to check for the NULL value before
> > returning false and exiting fw_download_code cleanly.

> I am trying to get this patch merged and after my issues with the
> kernel community, I can't get this into the mainline.

No, you're having trouble getting this into mainline because you are *STILL*
persisting in submitting patches that are buggy.

In this case, the problem is you *DON'T* exit the function cleanly.

Note your patch causes an immediate return from inside a do/while loop, which
*also* contains:

	 skb_put(skb, i);

So if there's (say) 3 fragments needed, and we fail on the allocation of the
third one, you just leaked references to the first two fragments, and never
actually clean up the allocations, so we have references to leaked memory.  And
leaking memory in a case where we're almost certainly very close to OOM isn't
exactly a good idea.  Yes, failing to check the return code is a bug - but so is
failing to unwind the allocations already made.

It took me all of a minute to spot this issue - the only clue needed was that there
was a '*_put()' call in the function, which should be a warning flag that reference
counting needs to be checked.

Greg:  Consider this a NACK of this patch.

Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.

Seriously Nick.  *PLEASE* stop posting patches until you've gotten a better handle
on what code maintenance really entails.

-------------- 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/20140812/af5ed6a7/attachment-0001.bin 

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Valdis.Kletnieks at vt.edu]

On Tue, 12 Aug 2014 20:46:56 -0000, Jeff Haran said:
> While the avoidance of dereferencing NULL pointers in the kernel is a
> laudable goal, what will be the effect of the call to write_nic_byte() at the
> end of the function not happening should the call to dev_alloc_skb() return
> NULL?

Damn. I missed that one, good catch.
-------------- 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/20140812/8bb2d393/attachment.bin 

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Jerry Snitselaar]

On Tue Aug 12 14, Valdis.Kletnieks at vt.edu wrote:
> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
> > > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
> > > This entry states that we are not checking the skb allocated in fw_download_code
> > > for NULL and after checking it ,I fixed it to check for the NULL value before
> > > returning false and exiting fw_download_code cleanly.
> 
> > I am trying to get this patch merged and after my issues with the
> > kernel community, I can't get this into the mainline.
> 
> No, you're having trouble getting this into mainline because you are *STILL*
> persisting in submitting patches that are buggy.
> 
> In this case, the problem is you *DON'T* exit the function cleanly.
> 
> Note your patch causes an immediate return from inside a do/while loop, which
> *also* contains:
> 
> 	 skb_put(skb, i);
> 
> So if there's (say) 3 fragments needed, and we fail on the allocation of the
> third one, you just leaked references to the first two fragments, and never
> actually clean up the allocations, so we have references to leaked memory.  And
> leaking memory in a case where we're almost certainly very close to OOM isn't
> exactly a good idea.  Yes, failing to check the return code is a bug - but so is
> failing to unwind the allocations already made.
> 
> It took me all of a minute to spot this issue - the only clue needed was that there
> was a '*_put()' call in the function, which should be a warning flag that reference
> counting needs to be checked.
> 
> Greg:  Consider this a NACK of this patch.
> 
> Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
> 
> Seriously Nick.  *PLEASE* stop posting patches until you've gotten a better handle
> on what code maintenance really entails.
> 

A minor point, but I don't believe skb_put() has anything to do with reference counting,
though the name would make you think so. sk_buff reference counting happens in skb_get()
and the *free_skb() routines from the looks of it.

Jerry

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
> On Tue Aug 12 14, Valdis.Kletnieks at vt.edu wrote:
>> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
>> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>> > > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
>> > > This entry states that we are not checking the skb allocated in fw_download_code
>> > > for NULL and after checking it ,I fixed it to check for the NULL value before
>> > > returning false and exiting fw_download_code cleanly.
>>
>> > I am trying to get this patch merged and after my issues with the
>> > kernel community, I can't get this into the mainline.
>>
>> No, you're having trouble getting this into mainline because you are *STILL*
>> persisting in submitting patches that are buggy.
>>
>> In this case, the problem is you *DON'T* exit the function cleanly.
>>
>> Note your patch causes an immediate return from inside a do/while loop, which
>> *also* contains:
>>
>>        skb_put(skb, i);
>>
>> So if there's (say) 3 fragments needed, and we fail on the allocation of the
>> third one, you just leaked references to the first two fragments, and never
>> actually clean up the allocations, so we have references to leaked memory.  And
>> leaking memory in a case where we're almost certainly very close to OOM isn't
>> exactly a good idea.  Yes, failing to check the return code is a bug - but so is
>> failing to unwind the allocations already made.
>>
>> It took me all of a minute to spot this issue - the only clue needed was that there
>> was a '*_put()' call in the function, which should be a warning flag that reference
>> counting needs to be checked.
>>
>> Greg:  Consider this a NACK of this patch.
>>
>> Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
>>
>> Seriously Nick.  *PLEASE* stop posting patches until you've gotten a better handle
>> on what code maintenance really entails.
>>
>
> A minor point, but I don't believe skb_put() has anything to do with reference counting,
> though the name would make you think so. sk_buff reference counting happens in skb_get()
> and the *free_skb() routines from the looks of it.
>
> Jerry
>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Sorry Guys,
I will reread the function and send out a patch that is bug free and
actually works.
Thanks Greg for at least reading it for now :).
Cheers Nick

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
>> On Tue Aug 12 14, Valdis.Kletnieks at vt.edu wrote:
>>> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
>>> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>>> > > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
>>> > > This entry states that we are not checking the skb allocated in fw_download_code
>>> > > for NULL and after checking it ,I fixed it to check for the NULL value before
>>> > > returning false and exiting fw_download_code cleanly.
>>>
>>> > I am trying to get this patch merged and after my issues with the
>>> > kernel community, I can't get this into the mainline.
>>>
>>> No, you're having trouble getting this into mainline because you are *STILL*
>>> persisting in submitting patches that are buggy.
>>>
>>> In this case, the problem is you *DON'T* exit the function cleanly.
>>>
>>> Note your patch causes an immediate return from inside a do/while loop, which
>>> *also* contains:
>>>
>>>        skb_put(skb, i);
>>>
>>> So if there's (say) 3 fragments needed, and we fail on the allocation of the
>>> third one, you just leaked references to the first two fragments, and never
>>> actually clean up the allocations, so we have references to leaked memory.  And
>>> leaking memory in a case where we're almost certainly very close to OOM isn't
>>> exactly a good idea.  Yes, failing to check the return code is a bug - but so is
>>> failing to unwind the allocations already made.
>>>
>>> It took me all of a minute to spot this issue - the only clue needed was that there
>>> was a '*_put()' call in the function, which should be a warning flag that reference
>>> counting needs to be checked.
>>>
>>> Greg:  Consider this a NACK of this patch.
>>>
>>> Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
>>>
>>> Seriously Nick.  *PLEASE* stop posting patches until you've gotten a better handle
>>> on what code maintenance really entails.
>>>
>>
>> A minor point, but I don't believe skb_put() has anything to do with reference counting,
>> though the name would make you think so. sk_buff reference counting happens in skb_get()
>> and the *free_skb() routines from the looks of it.
>>
>> Jerry
>>
>> _______________________________________________
>> Kernelnewbies mailing list
>> Kernelnewbies at kernelnewbies.org
>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
> Sorry Guys,
> I will reread the function and send out a patch that is bug free and
> actually works.
> Thanks Greg for at least reading it for now :).
> Cheers Nick

I looked into what Jerry states and he seems to be right. I will paste
the code of skb_put for your convenience to check if
Jerry and me are right. In addition about the call to
write_nic_byte(dev, TPPoll, TPPoll_CQ); is there any good place to put
it besides the end of the function seems there isn't.  I was wondering
if I rewrote this to break out of the loop and keep
everything else the same is Ok.

Nick

[PATCHv3] staging: Check for Null allocated skb in fw_download_code

[Nick Krause]

On Tue, Aug 12, 2014 at 10:41 PM, Nick Krause <xerofoify@gmail.com> wrote:
> On Tue, Aug 12, 2014 at 10:01 PM, Nick Krause <xerofoify@gmail.com> wrote:
>> On Tue, Aug 12, 2014 at 8:50 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
>>> On Tue Aug 12 14, Valdis.Kletnieks at vt.edu wrote:
>>>> On Tue, 12 Aug 2014 16:19:40 -0400, Nick Krause said:
>>>> > On Tue, Aug 12, 2014 at 4:18 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
>>>> > > I am fixing the bug entry , https://bugzilla.kernel.org/show_bug.cgi?id=60461.
>>>> > > This entry states that we are not checking the skb allocated in fw_download_code
>>>> > > for NULL and after checking it ,I fixed it to check for the NULL value before
>>>> > > returning false and exiting fw_download_code cleanly.
>>>>
>>>> > I am trying to get this patch merged and after my issues with the
>>>> > kernel community, I can't get this into the mainline.
>>>>
>>>> No, you're having trouble getting this into mainline because you are *STILL*
>>>> persisting in submitting patches that are buggy.
>>>>
>>>> In this case, the problem is you *DON'T* exit the function cleanly.
>>>>
>>>> Note your patch causes an immediate return from inside a do/while loop, which
>>>> *also* contains:
>>>>
>>>>        skb_put(skb, i);
>>>>
>>>> So if there's (say) 3 fragments needed, and we fail on the allocation of the
>>>> third one, you just leaked references to the first two fragments, and never
>>>> actually clean up the allocations, so we have references to leaked memory.  And
>>>> leaking memory in a case where we're almost certainly very close to OOM isn't
>>>> exactly a good idea.  Yes, failing to check the return code is a bug - but so is
>>>> failing to unwind the allocations already made.
>>>>
>>>> It took me all of a minute to spot this issue - the only clue needed was that there
>>>> was a '*_put()' call in the function, which should be a warning flag that reference
>>>> counting needs to be checked.
>>>>
>>>> Greg:  Consider this a NACK of this patch.
>>>>
>>>> Nick: If you're going to fix this bug, *UNDERSTAND THE CODE* and fix it *CORRECTLY*.
>>>>
>>>> Seriously Nick.  *PLEASE* stop posting patches until you've gotten a better handle
>>>> on what code maintenance really entails.
>>>>
>>>
>>> A minor point, but I don't believe skb_put() has anything to do with reference counting,
>>> though the name would make you think so. sk_buff reference counting happens in skb_get()
>>> and the *free_skb() routines from the looks of it.
>>>
>>> Jerry
>>>
>>> _______________________________________________
>>> Kernelnewbies mailing list
>>> Kernelnewbies at kernelnewbies.org
>>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>> Sorry Guys,
>> I will reread the function and send out a patch that is bug free and
>> actually works.
>> Thanks Greg for at least reading it for now :).
>> Cheers Nick
>
> I looked into what Jerry states and he seems to be right. I will paste
> the code of skb_put for your convenience to check if
> Jerry and me are right. In addition about the call to
> write_nic_byte(dev, TPPoll, TPPoll_CQ); is there any good place to put
> it besides the end of the function seems there isn't.  I was wondering
> if I rewrote this to break out of the loop and keep
> everything else the same is Ok.
>
> Nick
Sorry about the multiple emails. Here is skb_put for you all to read.
Nick

1271 unsigned char *skb_put(struct sk_buff *skb, unsigned int len)
1272 {
1273 unsigned char *tmp = skb_tail_pointer(skb);
1274 SKB_LINEAR_ASSERT(skb);
1275 skb->tail += len;
1276 skb->len  += len;
1277 if (unlikely(skb->tail > skb->end))
1278 skb_over_panic(skb, len, __builtin_return_address(0));
1279 return tmp;
1280 }
1281 EXPORT_SYMBOL(skb_put);