[PATCH] Use kzfree in tty buffer management to enforce data sanitization

[PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Larry H.]

[PATCH] Use kzfree in tty buffer management to enforce data sanitization

This patch replaces the kfree() calls within the tty buffer management API
with kzfree(), to enforce sanitization of the buffer contents.

It also takes care of handling buffers larger than PAGE_SIZE, which are
allocated via the page allocator directly.

This prevents such information from persisting on memory, potentially
leaking sensitive data like access credentials, or being leaked to other
kernel users after re-allocation of the memory by the LIFO allocators.

This patch doesn't affect fastpaths.

Signed-off-by: Larry Highsmith <research@subreption.com>

Index: linux-2.6/drivers/char/tty_audit.c
===================================================================
--- linux-2.6.orig/drivers/char/tty_audit.c
+++ linux-2.6/drivers/char/tty_audit.c
@@ -54,10 +54,12 @@ err:
 static void tty_audit_buf_free(struct tty_audit_buf *buf)
 {
 	WARN_ON(buf->valid != 0);
-	if (PAGE_SIZE != N_TTY_BUF_SIZE)
-		kfree(buf->data);
-	else
+	if (PAGE_SIZE != N_TTY_BUF_SIZE) {
+		kzfree(buf->data);
+	} else {
+		memset(buf->data, 0, PAGE_SIZE);
 		free_page((unsigned long)buf->data);
+	}
 	kfree(buf);
 }
 

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Linus Torvalds]

On Sat, 30 May 2009, Larry H. wrote:
>
> This patch doesn't affect fastpaths.

This patch is ugly as hell.

You already know the size of the data to clear.

If we actually wanted this (and I am in _no_way_ saying we do), the only 
sane thing to do is to just do

	memset(buf->data, 0, N_TTY_BUF_SIZE);
	if (PAGE_SIZE != N_TTY_BUF_SIZE)
		kfree(...)
	else
		free_page(...)


but quite frankly, I'm not convinced about these patches at all.

I'm also not in the least convinced about how you just dismiss everybodys 
concerns.

		Linus

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Larry H.]

On 19:04 Sat 30 May     , Linus Torvalds wrote:
> 
> 
> On Sat, 30 May 2009, Larry H. wrote:
> >
> > This patch doesn't affect fastpaths.
> 
> This patch is ugly as hell.
> 
> You already know the size of the data to clear.
> 
> If we actually wanted this (and I am in _no_way_ saying we do), the only 
> sane thing to do is to just do
> 
> 	memset(buf->data, 0, N_TTY_BUF_SIZE);
> 	if (PAGE_SIZE != N_TTY_BUF_SIZE)
> 		kfree(...)
> 	else
> 		free_page(...)
> 

It wasn't me who proposed using kzfree in these places. Ask Ingo and
Peter or refer to the entire thread about my previous patches.

In a way it's convenient that a patch written as of their
'recommendations' and 'positive feedback' is being ditched and properly
outed as an overkill. Surprisingly we might agree on this one.

> but quite frankly, I'm not convinced about these patches at all.
> 
> I'm also not in the least convinced about how you just dismiss everybodys 
> concerns.

This was proposed by Ingo, Andrew, Peter and later agreed upon by Alan.
I'm not sure whose concerns are being dismissed, but it looks like when
I make a perfectly valid technical point, and document it or provide
references, it's my concerns that get dismissed. It's also typically the
same people who do it, without providing true reasoning nor facts that
support their claims.

And every time I submit a patch which _exactly_ follows what other
people suddenly decided to agree upon, it is dismissed as well. In the
end it looks like there's no intention to close some
serious security loopholes in the kernel, but engage in endless
arguments about who's right or wrong, more often than not with people
whose area of expertise is definitely not security, making ad hominem
statements and so forth.

The next time a kernel vulnerability appears that is remotely related to
some of the venues of attack I've commented, it will be useful to be
able to refer to these responses.

	Larry

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Pekka Enberg]

Hi Linus,

On Sat, 30 May 2009, Larry H. wrote:
>>
>> This patch doesn't affect fastpaths.

On Sun, May 31, 2009 at 5:04 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> This patch is ugly as hell.
>
> You already know the size of the data to clear.
>
> If we actually wanted this (and I am in _no_way_ saying we do), the only
> sane thing to do is to just do
>
>        memset(buf->data, 0, N_TTY_BUF_SIZE);
>        if (PAGE_SIZE != N_TTY_BUF_SIZE)
>                kfree(...)
>        else
>                free_page(...)
>
>
> but quite frankly, I'm not convinced about these patches at all.

I wonder why the tty code has that N_TTY_BUF_SIZE special casing in
the first place? I think we can probably just get rid of it and thus
we can use kzfree() here if we want to.

                                 Pekka

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Pekka Enberg]

Hi Larry,

On Sat, 30 May 2009, Larry H. wrote:
>>> This patch doesn't affect fastpaths.

On 19:04 Sat 30 May, Linus Torvalds wrote:
>> This patch is ugly as hell.
>>
>> You already know the size of the data to clear.
>>
>> If we actually wanted this (and I am in _no_way_ saying we do), the only
>> sane thing to do is to just do
>>
>>       memset(buf->data, 0, N_TTY_BUF_SIZE);
>>       if (PAGE_SIZE != N_TTY_BUF_SIZE)
>>               kfree(...)
>>       else
>>               free_page(...)
>>

On Sun, May 31, 2009 at 5:35 AM, Larry H. <research@subreption.com> wrote:
> It wasn't me who proposed using kzfree in these places. Ask Ingo and
> Peter or refer to the entire thread about my previous patches.

Nobody suggested using kzfree() in this _specific_place_. It's obvious
that memset() is a better solution here given the current constraints
of the code as demonstrated by Linus' patch.

                                       Pekka

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Alan Cox]

> >        memset(buf->data, 0, N_TTY_BUF_SIZE);
> >        if (PAGE_SIZE != N_TTY_BUF_SIZE)
> >                kfree(...)
> >        else
> >                free_page(...)
> >
> >
> > but quite frankly, I'm not convinced about these patches at all.
> 
> I wonder why the tty code has that N_TTY_BUF_SIZE special casing in
> the first place? I think we can probably just get rid of it and thus
> we can use kzfree() here if we want to.

Some platforms with very large page sizes override the use of page based
allocators (eg older ARM would go around allocating 32K). The normal path
is 4K or 8K page sized buffers.

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Linus Torvalds]

On Sun, 31 May 2009, Alan Cox wrote:

> > >        memset(buf->data, 0, N_TTY_BUF_SIZE);
> > >        if (PAGE_SIZE != N_TTY_BUF_SIZE)
> > >                kfree(...)
> > >        else
> > >                free_page(...)
> > >
> > >
> > > but quite frankly, I'm not convinced about these patches at all.
> > 
> > I wonder why the tty code has that N_TTY_BUF_SIZE special casing in
> > the first place? I think we can probably just get rid of it and thus
> > we can use kzfree() here if we want to.
> 
> Some platforms with very large page sizes override the use of page based
> allocators (eg older ARM would go around allocating 32K). The normal path
> is 4K or 8K page sized buffers.

I think Pekka meant the other way around - why don't we always just use 
kmalloc(N_TTY_BUF_SIZE)/kfree(), and drop the whole conditional "use page 
allocator" entirely?

I suspect the "use page allocator" is historical - ie the tty layer 
originally always did that, and then when people wanted to suppotr smaller 
areas than one page, they added the special case. I have this dim memory 
of the _original_ kmalloc not handling page-sized allocations well (due to 
embedded size/pointer overheads), but I think all current allocators are 
perfectly happy to allocate PAGE_SIZE buffers without slop.

If I'm right, then we could just use kmalloc/kfree unconditionally. Pekka?

			Linus

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Alan Cox]

> I think Pekka meant the other way around - why don't we always just use 
> kmalloc(N_TTY_BUF_SIZE)/kfree(), and drop the whole conditional "use page 
> allocator" entirely?

We certainly can nowdays - the old allocator used to allocate 8K for 4K
and a bit of memory and its many years single we acquired slab so yes it
can go.

> If I'm right, then we could just use kmalloc/kfree unconditionally. Pekka?

Added to the tty queue will do that tomorrow

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Pekka Enberg]

Hi Linus,

On Sun, 31 May 2009, Alan Cox wrote:
>>>>        memset(buf->data, 0, N_TTY_BUF_SIZE);
>>>>        if (PAGE_SIZE != N_TTY_BUF_SIZE)
>>>>                kfree(...)
>>>>        else
>>>>                free_page(...)
>>>>
>>>>
>>>> but quite frankly, I'm not convinced about these patches at all.
>>> I wonder why the tty code has that N_TTY_BUF_SIZE special casing in
>>> the first place? I think we can probably just get rid of it and thus
>>> we can use kzfree() here if we want to.
>> Some platforms with very large page sizes override the use of page based
>> allocators (eg older ARM would go around allocating 32K). The normal path
>> is 4K or 8K page sized buffers.

Linus Torvalds wrote:
> I think Pekka meant the other way around - why don't we always just use 
> kmalloc(N_TTY_BUF_SIZE)/kfree(), and drop the whole conditional "use page 
> allocator" entirely?
> 
> I suspect the "use page allocator" is historical - ie the tty layer 
> originally always did that, and then when people wanted to suppotr smaller 
> areas than one page, they added the special case. I have this dim memory 
> of the _original_ kmalloc not handling page-sized allocations well (due to 
> embedded size/pointer overheads), but I think all current allocators are 
> perfectly happy to allocate PAGE_SIZE buffers without slop.
> 
> If I'm right, then we could just use kmalloc/kfree unconditionally. Pekka?

Yup, that's what I meant. Even SLAB moves metadata off-slab to make sure 
  we support PAGE_SIZE allocations nicely. SLUB even used to pass 
kmalloc(PAGE_SIZE) directly to the page allocator and will likely do 
that again once Mel Gorman's page allocator optimization patches hit 
mainline.

			Pekka

Re: [PATCH] Use kzfree in tty buffer management to enforce data sanitization

[Christoph Lameter]

On Sun, 31 May 2009, Linus Torvalds wrote:

> I suspect the "use page allocator" is historical - ie the tty layer
> originally always did that, and then when people wanted to suppotr smaller
> areas than one page, they added the special case. I have this dim memory
> of the _original_ kmalloc not handling page-sized allocations well (due to
> embedded size/pointer overheads), but I think all current allocators are
> perfectly happy to allocate PAGE_SIZE buffers without slop.
>
> If I'm right, then we could just use kmalloc/kfree unconditionally. Pekka?

Yes. They do that in various ways. SLOB/SLUB will fall back to the
page allocator for large allocation sizes.