* [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
@ 2010-03-16 11:46 Darren Jenkins
2010-03-16 21:14 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Darren Jenkins @ 2010-03-16 11:46 UTC (permalink / raw
To: Inaky Perez-Gonzalez, linux-wimax, Kernel Janitors
Cc: Cindy H Kao, Linux Kernel Mailing List, wimax, netdev
i2400m_fw_check() can free i2400m->fw_hdrs if krealloc() fails causing a double free
Add a check so we don't free the memory a second time.
coverity CID: 13455
Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
---
drivers/net/wimax/i2400m/fw.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
index 25c24f0..a97c413 100644
--- a/drivers/net/wimax/i2400m/fw.c
+++ b/drivers/net/wimax/i2400m/fw.c
@@ -1490,7 +1490,8 @@ int i2400m_fw_bootstrap(struct i2400m *i2400m, const struct firmware *fw,
if (ret < 0)
dev_err(dev, "%s: cannot use: %d, skipping\n",
i2400m->fw_name, ret);
- kfree(i2400m->fw_hdrs);
+ if (ret != -ENOMEM)
+ kfree(i2400m->fw_hdrs);
i2400m->fw_hdrs = NULL;
d_fnend(5, dev, "(i2400m %p) = %d\n", i2400m, ret);
return ret;
--
1.6.3.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
2010-03-16 11:46 Darren Jenkins
@ 2010-03-16 21:14 ` David Miller
2010-03-17 6:05 ` Inaky Perez-Gonzalez
0 siblings, 1 reply; 7+ messages in thread
From: David Miller @ 2010-03-16 21:14 UTC (permalink / raw
To: darrenrjenkins
Cc: inaky.perez-gonzalez, linux-wimax, kernel-janitors, cindy.h.kao,
dirk.j.brandewie, wimax, netdev, linux-kernel
From: Darren Jenkins <darrenrjenkins@gmail.com>
Date: Tue, 16 Mar 2010 22:46:28 +1100
> i2400m_fw_check() can free i2400m->fw_hdrs if krealloc() fails causing a double free
> Add a check so we don't free the memory a second time.
>
> coverity CID: 13455
>
> Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
Please don't fix it like this, the check is obscure and it's
allowing other bugs to happen.
If krealloc() fails, any refrence to i2400m->fw_hdrs is
referencing freed memory.
Therefore the krealloc() failure handling in this driver should NULL
out i2400m->fw_hdrs and that will fix the double kfree problem as well
as trap any stray references.
> ---
> drivers/net/wimax/i2400m/fw.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> index 25c24f0..a97c413 100644
> --- a/drivers/net/wimax/i2400m/fw.c
> +++ b/drivers/net/wimax/i2400m/fw.c
> @@ -1490,7 +1490,8 @@ int i2400m_fw_bootstrap(struct i2400m *i2400m, const struct firmware *fw,
> if (ret < 0)
> dev_err(dev, "%s: cannot use: %d, skipping\n",
> i2400m->fw_name, ret);
> - kfree(i2400m->fw_hdrs);
> + if (ret != -ENOMEM)
> + kfree(i2400m->fw_hdrs);
> i2400m->fw_hdrs = NULL;
> d_fnend(5, dev, "(i2400m %p) = %d\n", i2400m, ret);
> return ret;
> --
> 1.6.3.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
2010-03-16 21:14 ` David Miller
@ 2010-03-17 6:05 ` Inaky Perez-Gonzalez
0 siblings, 0 replies; 7+ messages in thread
From: Inaky Perez-Gonzalez @ 2010-03-17 6:05 UTC (permalink / raw
To: David Miller
Cc: cindy.h.kao, wimax, netdev, kernel-janitors, linux-kernel,
linux-wimax, darrenrjenkins
On Tue, 2010-03-16 at 14:14 -0700, David Miller wrote:
> From: Darren Jenkins <darrenrjenkins@gmail.com>
> Date: Tue, 16 Mar 2010 22:46:28 +1100
>
> > i2400m_fw_check() can free i2400m->fw_hdrs if krealloc() fails causing a double free
> > Add a check so we don't free the memory a second time.
> >
> > coverity CID: 13455
> >
> > Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
>
> Please don't fix it like this, the check is obscure and it's
> allowing other bugs to happen.
>
> If krealloc() fails, any refrence to i2400m->fw_hdrs is
> referencing freed memory.
>
> Therefore the krealloc() failure handling in this driver should NULL
> out i2400m->fw_hdrs and that will fix the double kfree problem as well
> as trap any stray references.
I agree with David, the fix is quite obscure. The error path in
i2400m_fw_check()'s call to i2400m_kzrealloc_2x() should be rather
cleaning up in a better way.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
@ 2010-03-17 12:40 Darren Jenkins
2010-03-17 22:10 ` Inaky Perez-Gonzalez
0 siblings, 1 reply; 7+ messages in thread
From: Darren Jenkins @ 2010-03-17 12:40 UTC (permalink / raw
To: David Miller
Cc: inaky.perez-gonzalez, linux-wimax, kernel-janitors, cindy.h.kao,
dirk.j.brandewie, wimax, netdev, Linux Kernel Mailing List
On Wed, Mar 17, 2010 at 8:14 AM, David Miller <davem@davemloft.net>
wrote:
> Therefore the krealloc() failure handling in this driver should NULL
> out i2400m->fw_hdrs and that will fix the double kfree problem as well
> as trap any stray references.
Yes that is a much better Idea. Thanks for the advice.
It also fixes the i2400m_barker_db problem that I didn't notice before.
Fix double free on krealloc() failure by zeroing pointer
coverity CID: 13455
Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
---
drivers/net/wimax/i2400m/fw.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
index 25c24f0..9f3b594 100644
--- a/drivers/net/wimax/i2400m/fw.c
+++ b/drivers/net/wimax/i2400m/fw.c
@@ -232,8 +232,9 @@ int i2400m_zrealloc_2x(void **ptr, size_t *_count, size_t el_size,
*_count = new_count;
*ptr = nptr;
return 0;
- } else
- return -ENOMEM;
+ }
+ *ptr = NULL;
+ return -ENOMEM;
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
2010-03-17 12:40 Darren Jenkins
@ 2010-03-17 22:10 ` Inaky Perez-Gonzalez
0 siblings, 0 replies; 7+ messages in thread
From: Inaky Perez-Gonzalez @ 2010-03-17 22:10 UTC (permalink / raw
To: Darren Jenkins
Cc: David Miller, linux-wimax, kernel-janitors@vger.kernel.org,
Kao, Cindy H, Brandewie, Dirk J, wimax@linuxwimax.org,
netdev@vger.kernel.org, Linux Kernel Mailing List
On Wed, 2010-03-17 at 05:40 -0700, Darren Jenkins wrote:
> On Wed, Mar 17, 2010 at 8:14 AM, David Miller <davem@davemloft.net>
> wrote:
>
> > Therefore the krealloc() failure handling in this driver should NULL
> > out i2400m->fw_hdrs and that will fix the double kfree problem as well
> > as trap any stray references.
>
> Yes that is a much better Idea. Thanks for the advice.
> It also fixes the i2400m_barker_db problem that I didn't notice before.
>
>
> Fix double free on krealloc() failure by zeroing pointer
If krealloc() fails to aallocate a new pointer, the old block is
unmodified, so by doing this you are leaking a buffer allocation.
I think this should be solved at the site where i2400m_zrealloc_2x() is
called, with a
if (result < 0) {
kfree(i2400m->fw_hdrs);
i2400m->fw_hdrs = NULL;
goto error_zrealloc;
}
or any other better fix. I am hesitant of having zrealloc_2x free the
original pointer because it breaks the traditional semantics that come
along being called 'realloc' (realloc if successful, keep the original
if not).
Am I missing anything?
> coverity CID: 13455
>
> Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com>
> ---
> drivers/net/wimax/i2400m/fw.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wimax/i2400m/fw.c b/drivers/net/wimax/i2400m/fw.c
> index 25c24f0..9f3b594 100644
> --- a/drivers/net/wimax/i2400m/fw.c
> +++ b/drivers/net/wimax/i2400m/fw.c
> @@ -232,8 +232,9 @@ int i2400m_zrealloc_2x(void **ptr, size_t *_count, size_t el_size,
> *_count = new_count;
> *ptr = nptr;
> return 0;
> - } else
> - return -ENOMEM;
> + }
> + *ptr = NULL;
> + return -ENOMEM;
> }
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
@ 2010-03-18 8:46 Darren Jenkins
2010-03-20 21:24 ` David Miller
0 siblings, 1 reply; 7+ messages in thread
From: Darren Jenkins @ 2010-03-18 8:46 UTC (permalink / raw
To: Inaky Perez-Gonzalez
Cc: Kao, Cindy H, wimax@linuxwimax.org, netdev@vger.kernel.org,
kernel-janitors@vger.kernel.org, Linux Kernel Mailing List,
linux-wimax, David Miller
On Thu, Mar 18, 2010 at 9:10 AM, Inaky Perez-Gonzalez
<inaky.perez-gonzalez@intel.com> wrote:
> If krealloc() fails to aallocate a new pointer, the old block is
> unmodified, so by doing this you are leaking a buffer allocation.
It seems you are right.
So now understanding correctly how krealloc() works I can see that the
double kfree() can only actually happen if the el_size parameter to
i2400m_zrealloc_2x() is zero, and it isn't at the two call sites.
So this was a false positive and I am sorry for the noise.
Darren J.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free
2010-03-18 8:46 [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free Darren Jenkins
@ 2010-03-20 21:24 ` David Miller
0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2010-03-20 21:24 UTC (permalink / raw
To: darrenrjenkins
Cc: cindy.h.kao, wimax, netdev, kernel-janitors, linux-kernel,
linux-wimax
From: Darren Jenkins <darrenrjenkins@gmail.com>
Date: Thu, 18 Mar 2010 19:46:48 +1100
>
> On Thu, Mar 18, 2010 at 9:10 AM, Inaky Perez-Gonzalez
> <inaky.perez-gonzalez@intel.com> wrote:
>
>> If krealloc() fails to aallocate a new pointer, the old block is
>> unmodified, so by doing this you are leaking a buffer allocation.
>
> It seems you are right.
> So now understanding correctly how krealloc() works I can see that the
> double kfree() can only actually happen if the el_size parameter to
> i2400m_zrealloc_2x() is zero, and it isn't at the two call sites.
>
> So this was a false positive and I am sorry for the noise.
No problem, at least the code got audited :-)
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-03-20 21:24 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-18 8:46 [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free Darren Jenkins
2010-03-20 21:24 ` David Miller
-- strict thread matches above, loose matches on Subject: below --
2010-03-17 12:40 Darren Jenkins
2010-03-17 22:10 ` Inaky Perez-Gonzalez
2010-03-16 11:46 Darren Jenkins
2010-03-16 21:14 ` David Miller
2010-03-17 6:05 ` Inaky Perez-Gonzalez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).