[RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Davidlohr Bueso]

x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()

There is no need to fall through the out_free label thus saving a kfree call.

Signed-off-by: Davidlohr Bueso <dave@gnu.org>
---
 arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
index 994230d..db7dc35 100644
--- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
@@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
 	if (!(supported & 0x1))
 		return -ENODEV;
 
+	return ret;
+
 out_free:
 	kfree(output.pointer);
 	return ret;
-- 
1.7.0.4



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Pekka Enberg]

[-- Attachment #1: Type: text/plain, Size: 1098 bytes --]

On Thu, Sep 30, 2010 at 10:43 PM, Davidlohr Bueso <dave@gnu.org> wrote:
> x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
>
> There is no need to fall through the out_free label thus saving a kfree call.
>
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..db7dc35 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>        if (!(supported & 0x1))
>                return -ENODEV;
>
> +       return ret;
> +
>  out_free:
>        kfree(output.pointer);
>        return ret;

Where is the double free here? I can't see it. I do see memory leaks
happening in error handling paths of pcc_cpufreq_do_osc() which makes
me think we need something like the attached patch.

[-- Attachment #2: 0001-x86-cpufreq-Fix-pcc_cpufreq_do_osc-memory-leaks.patch --]
[-- Type: application/octet-stream, Size: 1397 bytes --]

From 8b18a51077c9e5b36d54a5e45f9058eb3aae2477 Mon Sep 17 00:00:00 2001
From: Pekka Enberg <penberg@cs.helsinki.fi>
Date: Thu, 30 Sep 2010 22:57:33 +0300
Subject: [PATCH] x86, cpufreq: Fix pcc_cpufreq_do_osc() memory leaks

If acpi_evaluate_object() function call doesn't fail, we must kfree()
output.buffer before returning from pcc_cpufreq_do_osc().

Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
 arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
index 994230d..4f6f679 100644
--- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
+++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
@@ -368,16 +368,22 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
 		return -ENODEV;
 
 	out_obj = output.pointer;
-	if (out_obj->type != ACPI_TYPE_BUFFER)
-		return -ENODEV;
+	if (out_obj->type != ACPI_TYPE_BUFFER) {
+		ret = -ENODEV;
+		goto out_free;
+	}
 
 	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
-	if (errors)
-		return -ENODEV;
+	if (errors) {
+		ret = -ENODEV;
+		goto out_free;
+	}
 
 	supported = *((u32 *)(out_obj->buffer.pointer + 4));
-	if (!(supported & 0x1))
-		return -ENODEV;
+	if (!(supported & 0x1)) {
+		ret = -ENODEV;
+		goto out_free;
+	}
 
 out_free:
 	kfree(output.pointer);
-- 
1.5.6.4

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Dave Jones]

On Thu, Sep 30, 2010 at 10:59:51PM +0300, Pekka Enberg wrote:
 > > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
 > > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
 > >        if (!(supported & 0x1))
 > >                return -ENODEV;
 > >
 > > +       return ret;
 > > +
 > >  out_free:
 > >        kfree(output.pointer);
 > >        return ret;
 > 
 > Where is the double free here? I can't see it. I do see memory leaks
 > happening in error handling paths of pcc_cpufreq_do_osc() which makes
 > me think we need something like the attached patch.

I think Dave's patch is correct.  There's a kfree(output.pointer) at line 359.
If we fall all the way through without hitting any of the return -ENODEVs,
we end up doing a 2nd kfree in the out_free:

	Dave

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Pekka Enberg]

  Hi Dave,

On 30.9.2010 23.06, Dave Jones wrote:
> On Thu, Sep 30, 2010 at 10:59:51PM +0300, Pekka Enberg wrote:
>   >  >  +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
>   >  >  @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>   >  >          if (!(supported&  0x1))
>   >  >                  return -ENODEV;
>   >  >
>   >  >  +       return ret;
>   >  >  +
>   >  >    out_free:
>   >  >          kfree(output.pointer);
>   >  >          return ret;
>   >
>   >  Where is the double free here? I can't see it. I do see memory leaks
>   >  happening in error handling paths of pcc_cpufreq_do_osc() which makes
>   >  me think we need something like the attached patch.
>
> I think Dave's patch is correct.  There's a kfree(output.pointer) at line 359.
> If we fall all the way through without hitting any of the return -ENODEVs,
> we end up doing a 2nd kfree in the out_free:

There's a second call to acpi_evaluate_object() which takes "output" as 
its argument and allocates more memory.

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Dave Jones]

On Fri, Oct 01, 2010 at 07:47:23AM +0300, Pekka Enberg wrote:

 > > I think Dave's patch is correct.  There's a kfree(output.pointer) at line 359.
 > > If we fall all the way through without hitting any of the return -ENODEVs,
 > > we end up doing a 2nd kfree in the out_free:
 > 
 > There's a second call to acpi_evaluate_object() which takes "output" as 
 > its argument and allocates more memory.

yup. I merged your patch. after it's soaked in -next for a day, I'll push it to
Linus.  (Looks obviously correct, but just being extra careful given where
we are in the release cycle)

	Dave

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[David Rientjes]

On Thu, 30 Sep 2010, Pekka Enberg wrote:

> From 8b18a51077c9e5b36d54a5e45f9058eb3aae2477 Mon Sep 17 00:00:00 2001
> From: Pekka Enberg <penberg@cs.helsinki.fi>
> Date: Thu, 30 Sep 2010 22:57:33 +0300
> Subject: [PATCH] x86, cpufreq: Fix pcc_cpufreq_do_osc() memory leaks
> 
> If acpi_evaluate_object() function call doesn't fail, we must kfree()
> output.buffer before returning from pcc_cpufreq_do_osc().
> 
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>

Acked-by: David Rientjes <rientjes@google.com>

> ---
>  arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..4f6f679 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -368,16 +368,22 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>  		return -ENODEV;
>  
>  	out_obj = output.pointer;
> -	if (out_obj->type != ACPI_TYPE_BUFFER)
> -		return -ENODEV;
> +	if (out_obj->type != ACPI_TYPE_BUFFER) {
> +		ret = -ENODEV;
> +		goto out_free;
> +	}
>  
>  	errors = *((u32 *)out_obj->buffer.pointer) & ~(1 << 0);
> -	if (errors)
> -		return -ENODEV;
> +	if (errors) {
> +		ret = -ENODEV;
> +		goto out_free;
> +	}
>  
>  	supported = *((u32 *)(out_obj->buffer.pointer + 4));
> -	if (!(supported & 0x1))
> -		return -ENODEV;
> +	if (!(supported & 0x1)) {
> +		ret = -ENODEV;
> +		goto out_free;
> +	}
>  
>  out_free:
>  	kfree(output.pointer);
> -- 
> 1.5.6.4
> 

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[David Rientjes]

On Thu, 30 Sep 2010, Davidlohr Bueso wrote:

> x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
> 
> There is no need to fall through the out_free label thus saving a kfree call.
> 
> Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> ---
>  arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> index 994230d..db7dc35 100644
> --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>  	if (!(supported & 0x1))
>  		return -ENODEV;
>  
> +	return ret;
> +
>  out_free:
>  	kfree(output.pointer);
>  	return ret;

Why is the kfree() unnecessary?  acpi_evaluate_object() will allocate a 
new output.pointer if it returns 0, so at this point in the code you would 
now be leaking the buffer.

Instead, it would probably be better to fix the existing memory leaks in 
that function where we return -ENODEV without going to out_free when 
output.length is non-zero.

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Dave Jones]

On Thu, Sep 30, 2010 at 01:02:54PM -0700, David Rientjes wrote:
  > > index 994230d..db7dc35 100644
 > > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
 > > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
 > > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
 > >  	if (!(supported & 0x1))
 > >  		return -ENODEV;
 > >  
 > > +	return ret;
 > > +
 > >  out_free:
 > >  	kfree(output.pointer);
 > >  	return ret;
 > 
 > Why is the kfree() unnecessary?  acpi_evaluate_object() will allocate a 
 > new output.pointer if it returns 0, so at this point in the code you would 
 > now be leaking the buffer.
 > 
 > Instead, it would probably be better to fix the existing memory leaks in 
 > that function where we return -ENODEV without going to out_free when 
 > output.length is non-zero.

Oh, *duh*. That was subtle.
You, and Pekka are of course correct.  I'll merge up Pekka's patch.

thanks,

	Dave

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Davidlohr Bueso]

On Thu, 2010-09-30 at 16:08 -0400, Dave Jones wrote:
> On Thu, Sep 30, 2010 at 01:02:54PM -0700, David Rientjes wrote:
>   > > index 994230d..db7dc35 100644
>  > > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
>  > > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
>  > > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>  > >  	if (!(supported & 0x1))
>  > >  		return -ENODEV;
>  > >  
>  > > +	return ret;
>  > > +
>  > >  out_free:
>  > >  	kfree(output.pointer);
>  > >  	return ret;
>  > 
>  > Why is the kfree() unnecessary?  acpi_evaluate_object() will allocate a 
>  > new output.pointer if it returns 0, so at this point in the code you would 
>  > now be leaking the buffer.
>  > 
>  > Instead, it would probably be better to fix the existing memory leaks in 
>  > that function where we return -ENODEV without going to out_free when 
>  > output.length is non-zero.
> 
> Oh, *duh*. That was subtle.

Yep, anyways, at least we got the correct "fix".

> You, and Pekka are of course correct.  I'll merge up Pekka's patch.
> 
> thanks,
> 
> 	Dave
> 
> 

Re: [RESEND PATCH] cpufreq: unnecesary double free in pcc_cpufreq_do_osc

[Davidlohr Bueso]

On Thu, 2010-09-30 at 13:02 -0700, David Rientjes wrote:
> On Thu, 30 Sep 2010, Davidlohr Bueso wrote:
> 
> > x86, cpufreq: avoid an unnecessary double free when finished in pcc_cpufreq_do_osc()
> > 
> > There is no need to fall through the out_free label thus saving a kfree call.
> > 
> > Signed-off-by: Davidlohr Bueso <dave@gnu.org>
> > ---
> >  arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > index 994230d..db7dc35 100644
> > --- a/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > +++ b/arch/x86/kernel/cpu/cpufreq/pcc-cpufreq.c
> > @@ -379,6 +379,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
> >  	if (!(supported & 0x1))
> >  		return -ENODEV;
> >  
> > +	return ret;
> > +
> >  out_free:
> >  	kfree(output.pointer);
> >  	return ret;
> 
> Why is the kfree() unnecessary?  acpi_evaluate_object() will allocate a 
> new output.pointer if it returns 0, so at this point in the code you would 
> now be leaking the buffer.
> 
> Instead, it would probably be better to fix the existing memory leaks in 
> that function where we return -ENODEV without going to out_free when 
> output.length is non-zero.
> 

Silly me, I must be asleep still. Pekka's attached patch is just right.
Sorry for the noise.

Davidlohr