[PATCH] Fix for binary_sysctl() memory leak

[PATCH] Fix for binary_sysctl() memory leak

[Michel Lespinasse]

binary_sysctl() calls sysctl_getname() which allocates from
names_cache slab usin __getname()

The matching function to free the name is __putname(), and not
putname() which should be used only to match getname() allocations.

Signed-off-by: Michel Lespinasse <walken@google.com>
---
 kernel/sysctl_binary.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index e8bffbe..2ce1b30 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -1354,7 +1354,7 @@ static ssize_t binary_sysctl(const int *name, int nlen,
 
 	fput(file);
 out_putname:
-	putname(pathname);
+	__putname(pathname);
 out:
 	return result;
 }
-- 
1.7.3.1

Re: [PATCH] Fix for binary_sysctl() memory leak

[Andrew Morton]

On Wed, 14 Dec 2011 18:44:12 -0800
Michel Lespinasse <walken@google.com> wrote:

> binary_sysctl() calls sysctl_getname() which allocates from
> names_cache slab usin __getname()
> 
> The matching function to free the name is __putname(), and not
> putname() which should be used only to match getname() allocations.
> 
> Signed-off-by: Michel Lespinasse <walken@google.com>
> ---
>  kernel/sysctl_binary.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> index e8bffbe..2ce1b30 100644
> --- a/kernel/sysctl_binary.c
> +++ b/kernel/sysctl_binary.c
> @@ -1354,7 +1354,7 @@ static ssize_t binary_sysctl(const int *name, int nlen,
>  
>  	fput(file);
>  out_putname:
> -	putname(pathname);
> +	__putname(pathname);
>  out:
>  	return result;
>  }

I think the patch is correct but the description is misleading?

I see no memory leak here.  Calling __putname() directly simply
bypasses some audit-related stuff.

Re: [PATCH] Fix for binary_sysctl() memory leak

[Michel Lespinasse]

On Thu, Dec 15, 2011 at 2:19 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> I think the patch is correct but the description is misleading?
>
> I see no memory leak here.  Calling __putname() directly simply
> bypasses some audit-related stuff.

Hmmm, maybe I wasn't explicit enough about it. We are definitely
seeing a memory leak without the patch.

When auditing is enabled, putname() calls audit_putname *instead* (not
in addition) to __putname(). Then, if a syscall is in progress,
audit_putname does not release the name - instead, it expects the name
to get released when the syscall completes, but that will happen only
if audit_getname() was called previously, i.e. if the name was
allocated with getname() rather than the naked __getname(). So,
__getname() followed by putname() ends up leaking memory.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH] Fix for binary_sysctl() memory leak

[Andrew Morton]

On Thu, 15 Dec 2011 14:38:58 -0800
Michel Lespinasse <walken@google.com> wrote:

> On Thu, Dec 15, 2011 at 2:19 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > I think the patch is correct but the description is misleading?
> >
> > I see no memory leak here. __Calling __putname() directly simply
> > bypasses some audit-related stuff.
> 
> Hmmm, maybe I wasn't explicit enough about it. We are definitely
> seeing a memory leak without the patch.
> 
> When auditing is enabled, putname() calls audit_putname *instead* (not
> in addition) to __putname(). Then, if a syscall is in progress,
> audit_putname does not release the name - instead, it expects the name
> to get released when the syscall completes, but that will happen only
> if audit_getname() was called previously, i.e. if the name was
> allocated with getname() rather than the naked __getname(). So,
> __getname() followed by putname() ends up leaking memory.
> 

OK.  Please resend with a new changelog?

The bug surprises me - it seems that it makes it trivial for userspace
to cause leaking of mad amounts of kernel memory, which would cause the
bug to be found and fixed quickly.

Is it a recent regression, or does the bug trigger only in weird
circumstances, or what?

Re: [PATCH] Fix for binary_sysctl() memory leak

[Michel Lespinasse]

On Thu, Dec 15, 2011 at 2:44 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> OK.  Please resend with a new changelog?

All right. Will do as a reply to this (I'll add the same explanation
as in my previous email).

> The bug surprises me - it seems that it makes it trivial for userspace
> to cause leaking of mad amounts of kernel memory, which would cause the
> bug to be found and fixed quickly.
>
> Is it a recent regression, or does the bug trigger only in weird
> circumstances, or what?

As far as I can tell, the bug was introduced in v2.6.33-rc1 by commit
26a7034b40ba80f82f64fa251a2cbf49f9971c6a

It's probably not common because people use sysctl mainly at boot
time, and/or don't use the deprecated binary variant of it anymore.
But yes, it is trivial to exploit...

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH] Fix for binary_sysctl() memory leak

[Michel Lespinasse]

binary_sysctl() calls sysctl_getname() which allocates from
names_cache slab usin __getname()

The matching function to free the name is __putname(), and not
putname() which should be used only to match getname() allocations.

This is because when auditing is enabled, putname() calls
audit_putname *instead* (not in addition) to __putname(). Then, if a
syscall is in progress, audit_putname does not release the name -
instead, it expects the name to get released when the syscall
completes, but that will happen only if audit_getname() was called
previously, i.e. if the name was allocated with getname() rather than
the naked __getname(). So, __getname() followed by putname() ends up
leaking memory.

Signed-off-by: Michel Lespinasse <walken@google.com>
---
 kernel/sysctl_binary.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index e8bffbe..2ce1b30 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -1354,7 +1354,7 @@ static ssize_t binary_sysctl(const int *name, int nlen,
 
 	fput(file);
 out_putname:
-	putname(pathname);
+	__putname(pathname);
 out:
 	return result;
 }
-- 
1.7.3.1

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH] Fix for binary_sysctl() memory leak

[Eric W. Biederman]

Andrew Morton <akpm@linux-foundation.org> writes:

> On Thu, 15 Dec 2011 14:38:58 -0800
> Michel Lespinasse <walken@google.com> wrote:
>
>> On Thu, Dec 15, 2011 at 2:19 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > I think the patch is correct but the description is misleading?
>> >
>> > I see no memory leak here. __Calling __putname() directly simply
>> > bypasses some audit-related stuff.
>> 
>> Hmmm, maybe I wasn't explicit enough about it. We are definitely
>> seeing a memory leak without the patch.
>> 
>> When auditing is enabled, putname() calls audit_putname *instead* (not
>> in addition) to __putname(). Then, if a syscall is in progress,
>> audit_putname does not release the name - instead, it expects the name
>> to get released when the syscall completes, but that will happen only
>> if audit_getname() was called previously, i.e. if the name was
>> allocated with getname() rather than the naked __getname(). So,
>> __getname() followed by putname() ends up leaking memory.
>> 
>
> OK.  Please resend with a new changelog?

This seems like a reasonable change to me.  I guess I misunderstood
the __getname/putname interaction.  I expect I was focusing on the
fact you can only call getname if you have your data in userspace.

> The bug surprises me - it seems that it makes it trivial for userspace
> to cause leaking of mad amounts of kernel memory, which would cause the
> bug to be found and fixed quickly.
>
> Is it a recent regression, or does the bug trigger only in weird
> circumstances, or what?

Calling sysctl(2) is very rare.  I don't know if it actually happens
anywhere with a modern userspace except in regression tests.
Effectively we retain sysctl(2) because it doesn't take too much to
maintain.

Michel what caused you to discover this bug?  If you are using
sysctl(2) in production code I am a bit worried.

Eric

Re: [PATCH] Fix for binary_sysctl() memory leak

[Michel Lespinasse]

On Sat, Dec 17, 2011 at 2:14 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Calling sysctl(2) is very rare.  I don't know if it actually happens
> anywhere with a modern userspace except in regression tests.
> Effectively we retain sysctl(2) because it doesn't take too much to
> maintain.

Agree, there is no good reason one would call sysctl() anymore.

> Michel what caused you to discover this bug?  If you are using
> sysctl(2) in production code I am a bit worried.

Well, somebody hit it :) To be honest, I'm not sure how they came to
do that. It wasn't in a datacenter setting.

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.