Prev: x86: Fix placement of FIX_OHCI1394_BASE
Next: gigaset: correct range checking off by one error
From: David Miller on 16 Mar 2010 17:20
From: Darren Jenkins <darrenrjenkins(a)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(a)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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Inaky Perez-Gonzalez on 17 Mar 2010 02:10
On Tue, 2010-03-16 at 14:14 -0700, David Miller wrote:
> From: Darren Jenkins <darrenrjenkins(a)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(a)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.






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Darren Jenkins on 17 Mar 2010 08:50

On Wed, Mar 17, 2010 at 8:14 AM, David Miller <davem(a)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(a)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






--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Inaky Perez-Gonzalez on 17 Mar 2010 18:20
On Wed, 2010-03-17 at 05:40 -0700, Darren Jenkins wrote:
> On Wed, Mar 17, 2010 at 8:14 AM, David Miller <davem(a)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(a)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;
> }
>
>



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Darren Jenkins on 18 Mar 2010 04:50

On Thu, Mar 18, 2010 at 9:10 AM, Inaky Perez-Gonzalez
<inaky.perez-gonzalez(a)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.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: x86: Fix placement of FIX_OHCI1394_BASE
Next: gigaset: correct range checking off by one error