[PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[Darren Jenkins]

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

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[David Miller]

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

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[Inaky Perez-Gonzalez]

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.

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[Darren Jenkins]

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

 

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[Inaky Perez-Gonzalez]

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;
>  }
>  
> 

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[Darren Jenkins]

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.

Re: [PATCH] drivers/net/wimax/i2400m/fw.c fix possible double free

[David Miller]

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 :-)