[PATCH] 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 <xerofoify@gmail.com> --- drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c index 1a95d1f..829af66 100644 --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c @@ -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
On Tue, Aug 12, 2014 at 11:24 PM, Nicholas Krause <xerofoify@gmail.com> wrote:
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 <xerofoify@gmail.com> --- drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c index 1a95d1f..829af66 100644 --- a/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c +++ b/drivers/staging/rtl8192e/rtl8192e/r8192E_firmware.c @@ -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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies Sorry about the two patches please read only the second one, I send twice. Nick
On Tue, 12 Aug 2014 23:24:32 -0400, Nicholas Krause said:
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); + 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.
On Tue, Aug 12, 2014 at 10:35 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Tue, 12 Aug 2014 23:24:32 -0400, Nicholas Krause said:
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); + 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.
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 someone else to send the patch on behalf of you, anyone who volunteers would like to verify that the patch is indeed tested properly before forwarding. Thanks - Manish
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Tue, 12 Aug 2014 22:53:37 -0700, Manish Katiyar said:
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. :)
On Wed, Aug 13, 2014 at 5:56 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Tue, 12 Aug 2014 22:53:37 -0700, Manish Katiyar said:
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. :)
No need to trigger it, faking it would be enough, e.g.: - skb = dev_alloc_skb(frag_length + 4); + { + static int i; + if (++i < 3) + skb = dev_alloc_skb(frag_length + 4); + else + skb = NULL; + } + if (skb == NULL) { + rt_status = false; + break; + } -- Thanks. -- Max
On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
No need to trigger it, faking it would be enough, e.g.:
+ 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 :)
On Wed, Aug 13, 2014 at 12:02 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
No need to trigger it, faking it would be enough, e.g.:
+ 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 :) Seems to be called a lot. In addition I can only build test this as I don't have the hardware. Cheers Nick
On Wed, Aug 13, 2014 at 12:55 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Wed, Aug 13, 2014 at 12:02 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
No need to trigger it, faking it would be enough, e.g.:
+ 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 :) 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
Am 2014-08-13 20:03, schrieb Nick Krause:
On Wed, Aug 13, 2014 at 12:55 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Wed, Aug 13, 2014 at 12:02 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
No need to trigger it, faking it would be enough, e.g.: + 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 :) 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@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies This error indicates that you are using a function that is not yet known to the compiler. Check if there's a prototype or function definition BEFORE it's actual use.
br, phil
On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
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. -- Thanks. -- Max
On Wed Aug 13 14, Max Filippov wrote:
On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
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
On Wed, Aug 13, 2014 at 2:25 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
On Wed Aug 13 14, Max Filippov wrote:
On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
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@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
On Wed, Aug 13, 2014 at 2:27 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Wed, Aug 13, 2014 at 2:25 PM, Jerry Snitselaar <dev@snitselaar.org> wrote:
On Wed Aug 13 14, Max Filippov wrote:
On Wed, Aug 13, 2014 at 10:03 PM, Nick Krause <xerofoify@gmail.com> wrote:
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@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
Also just checked and the patch is fixed and builds at least from my testing. Nick
-----Original Message----- From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies- bounces@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
Seems to be called a lot. In addition I can only build test this as I don't have the hardware. Cheers Nick
This code is a device driver. I may be an old fuddy-duddy in this regard, but I don't inflict my code on others until I've tested it. If you don't have the hardware, you can't have tested it. I think your patch is thus premature. If you are unable to obtain the hardware for testing, you might want to consider working on parts of the kernel that are hardware independent. Jeff Haran
On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:
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.
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.... On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
This is the fixed patch, I do get an error about uninitialized variables
Are you *trying* to get put in *everybody's* killfile?
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??!?
On Wed, Aug 13, 2014 at 3:17 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:
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.
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....
On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
This is the fixed patch, I do get an error about uninitialized variables
Are you *trying* to get put in *everybody's* killfile?
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??!?
I checked the patched and it worked , uninitialized variables are common and I hit 12 of them. Nick
On Wed, Aug 13, 2014 at 3:24 PM, Nick Krause <xerofoify@gmail.com> wrote:
On Wed, Aug 13, 2014 at 3:17 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 14:03:08 -0400, Nick Krause said:
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.
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....
On Wed, 13 Aug 2014 15:01:45 -0400, Nick Krause said:
This is the fixed patch, I do get an error about uninitialized variables
Are you *trying* to get put in *everybody's* killfile?
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??!?
I checked the patched and it worked , uninitialized variables are common and I hit 12 of them. Nick
Valdis, Sorry about the stupid question , I assumed it was something else I was fixing. In addition I fixed up the patch and there are not more errors based on using gcc's optimization features. Sorry Nick :(
On Wed, 13 Aug 2014 12:55:00 -0400, Nick Krause said:
On Wed, Aug 13, 2014 at 12:02 PM, <Valdis.Kletnieks@vt.edu> wrote:
On Wed, 13 Aug 2014 18:50:43 +0400, Max Filippov said:
No need to trigger it, faking it would be enough, e.g.:
+ 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 :)
Seems to be called a lot.
Oh? Explain to us what this function is actually doing, in general terms, and then give us a rough estimate of how many times per boot it's called.
participants (8)
-
Jeff Haran -
Jerry Snitselaar -
Manish Katiyar -
Max Filippov -
Nicholas Krause -
Nick Krause
-
Philipp Muhoray -
Valdis.Kletnieks＠vt.edu