staging: Check for Null return of allocated skb in fw_download_code

This patch checks if we are getting a Null allocated skb in the while/do loop of this function. Further more no reference checking is needing as skb_put is only working with the packet defined by skb and not allocating a new one. Further more I am breaking out of the loop in order to make sure we call nic_write_byte and cause no errors with writing the nic byte(s) of private date and then call rt_status which I changed to false in the loop with my additon of checking the allocated skb.

Signed-off-by Nicholas Krause <[email protected]>

---
 drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +4 -1

index 1a95d1f..829af66 100644
@@ -61,6 +61,10 @@ static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address,
                }
 
                skb  = dev_alloc_skb(frag_length + 4);
+               if (skb == NULL) {
+                       rt_status =  false;
+                       break;
+               }
                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;
--

1.9.1

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c static bool fw_download_code(struct net_device *dev, u8 *code_virtual_address, · line 67

Quoted 7 lines · Nicholas Krause

This patch checks if we are getting a Null allocated skb in the while/do loop of this function.

              skb  = dev_alloc_skb(frag_length + 4);

drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +4 -0

+             if (skb == NULL) {
+                     rt_status =  false;
+                     break;
+             }

Nick, it's *STILL* wrong. And although I admit I blew it on the refcount issue, that doesn't change the fact that you're still leaking memory here.

Work this through. Hand simulate it. Pretend that 3 fragments are needed, and that the first two succeed but the third one fails.

Who frees the first two fragments before you return?

For bonus points - explain under what conditions this bug can *possibly* be triggered on an actual system. Consider in your reply both (a) when this
code is called and (b) what the system state *should* be at that point, and what it has to be for this bug to trigger. Given your answer to (a) and (b),
extrapolate to what the next few hundredths of a second likely hold for this system even after we've fixed the bug you're mis-fixing here.

Once you've done that, you'll hopefully understand why you're trying to fix a bug from 2009.

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

And it may also be a good idea to post the logs regarding how you tested your patch and verified that the fix works as expected. As you are asking

Quoted 2 lines · Manish Katiyar

And it may also be a good idea to post the logs regarding how you tested your patch and verified that the fix works as expected. As you are asking

I want to see how he sets up an environment where he can *trigger* the issue reliably. :)

Quoted 1 line · Max Filippov

No need to trigger it, faking it would be enough, e.g.:

Review

Quoted 4 lines

(quoted diff) +4 -0

+                if (++i < 3)
+                        skb  = dev_alloc_skb(frag_length + 4);
+                else
+                        skb = NULL;

Don't bet on this triggering on a real system without some additional scaffolding - take a look at what the function is doing, and ask yourself how many times it will be called on the average system :)

Quoted 9 lines · On Wed, Aug 13, 2014 at 12:02 PM, <[email protected]> wrote:

Quoted 6 lines · Max Filippov

No need to trigger it, faking it would be enough, e.g.:

(quoted diff) +4 -0

+                if (++i < 3)
+                        skb  = dev_alloc_skb(frag_length + 4);
+                else
+                        skb = NULL;

Quoted 3 lines

Don't bet on this triggering on a real system without some additional scaffolding - take a look at what the function is doing, and ask yourself how many times it will be called on the average system :)

Seems to be called a lot. In addition I can only build test this as I don't have the hardware.
Cheers Nick

Quoted 8 lines · On Wed, Aug 13, 2014 at 12:02 PM, <[email protected]> wrote:

Quoted 7 lines · Max Filippov

Quoted 6 lines

No need to trigger it, faking it would be enough, e.g.:

(quoted diff) +4 -0

+                if (++i < 3)
+                        skb  = dev_alloc_skb(frag_length + 4);
+                else
+                        skb = NULL;

Quoted 6 lines

Quoted 3 lines

Don't bet on this triggering on a real system without some additional scaffolding - take a look at what the function is doing, and ask yourself how many times it will be called on the average system :)

Seems to be called a lot. In addition I can only build test this as I don't have the hardware.
Cheers Nick

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function 'skb_quene_purge' [-Werror=implicit-function-declaration]

     skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this, I will attach my patch so I can fix this out and send a proper patch :).
By the way thanks for the help guys :).
Nick

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this, I will attach my patch so I can fix this out and send a proper patch :).

Quoted 7 lines · Nick Krause

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this, I will attach my patch so I can fix this out and send a proper patch :).

Typo, skb_queue_purge.

Quoted 11 lines · Max Filippov

Quoted 7 lines · Nick Krause

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this, I will attach my patch so I can fix this out and send a proper patch :).

Typo, skb_queue_purge.

I imagine that array index isn't going to go over very well.

Jerry

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Quoted 13 lines · Jerry Snitselaar

Quoted 11 lines · Max Filippov

Quoted 7 lines · Nick Krause

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this, I will attach my patch so I can fix this out and send a proper patch :).

Typo, skb_queue_purge.

Quoted 8 lines

I imagine that array index isn't going to go over very well.

Jerry

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies at kernelnewbies.org
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

I known the patch is premature but a NULL pointer and in a device driver needs to be patched no matter what.
If some one reading this has the hardware , it would be great if they can test this :).
Cheers Nick

-----Original Message-----
From: kernelnewbies-bounces at kernelnewbies.org [mailto:kernelnewbies- bounces at kernelnewbies.org] On Behalf Of Nick Krause
Sent: Wednesday, August 13, 2014 11:03 AM
To: Valdis Kletnieks
Cc: Max Filippov; kernelnewbies; Manish Katiyar
Subject: Re: [PATCH] staging: Check for Null return of allocated skb in fw_download_code

Quoted 3 lines

Seems to be called a lot. In addition I can only build test this as I don't have the hardware.
Cheers Nick

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ???skb_quene_purge??? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

I am wondering how do I fix this,

This is the fixed patch, I do get an error about uninitialized variables

If someone wants to send this out, please do so as this is has been built and applied tested.

Quoted 5 lines · Nick Krause

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

Well, this is a C 101 problem.

Quoted 1 line

I am wondering how do I fix this,

You fix this by not doing any further kernel hacking until you've gotten a handle on *VERY BASIC* C development concepts.

Also, you're going to have to justify why you're being a total dumbass and coding skb_waitQ[tcb_desc->queue_index] when the *obvious* code is skb_waitQ[TXCMD_QUEUE] - yes, an optimizing compiler will do that substitution, but code clarity is important.

Argh. I may have to break out my +5 Trout of Smacking....

Quoted 2 lines · Nick Krause

This is the fixed patch, I do get an error about uninitialized variables

Are you *trying* to get put in *everybody's* killfile?

Quoted 2 lines

If someone wants to send this out, please do so as this is has been built and applied tested.

And you think that *anybody* wants to upstream a patch from you that even *you* admit still has trouble??!?

Quoted 31 lines · On Wed, Aug 13, 2014 at 3:17 PM, <[email protected]> wrote:

Quoted 5 lines · Nick Krause

I did test my patch by doing a kernel build and I get this error, drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c:66:4: error:
implicit declaration of function ?skb_quene_purge? [-Werror=implicit-function-declaration]

    skb_quene_purge(&priv->rtllib->skb_waitQ[tcb_desc->queue_index]);

Well, this is a C 101 problem.

Quoted 1 line

I am wondering how do I fix this,

You fix this by not doing any further kernel hacking until you've gotten a handle on *VERY BASIC* C development concepts.

Also, you're going to have to justify why you're being a total dumbass and coding skb_waitQ[tcb_desc->queue_index] when the *obvious* code is skb_waitQ[TXCMD_QUEUE] - yes, an optimizing compiler will do that substitution, but code clarity is important.

Argh. I may have to break out my +5 Trout of Smacking....

Quoted 2 lines · Nick Krause

This is the fixed patch, I do get an error about uninitialized variables

Are you *trying* to get put in *everybody's* killfile?

Quoted 2 lines

If someone wants to send this out, please do so as this is has been built and applied tested.

Quoted 2 lines

And you think that *anybody* wants to upstream a patch from you that even *you* admit still has trouble??!?

I checked the patched and it worked , uninitialized variables are common and I hit 12 of them.
Nick

Quoted 9 lines · On Wed, Aug 13, 2014 at 12:02 PM, <[email protected]> wrote:

Quoted 6 lines · Max Filippov

No need to trigger it, faking it would be enough, e.g.:

(quoted diff) +4 -0

+                if (++i < 3)
+                        skb  = dev_alloc_skb(frag_length + 4);
+                else
+                        skb = NULL;

Quoted 3 lines

Don't bet on this triggering on a real system without some additional scaffolding - take a look at what the function is doing, and ask yourself how many times it will be called on the average system :)

Seems to be called a lot.