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

[ebiederm@xmission.com (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