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

Larry H.

author

2009-05-31 01:57 UTC #0 lore raw

PATCH 1/1 Use kzfree in tty buffer management to enforce data sanitization

[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 <[email protected]>

drivers/char/tty_audit.c +5 -3

Index: 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);
 }

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Linus Torvalds

2009-05-31 02:04 UTC #1 lore raw

Quoted 2 lines · Larry H.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Larry H.

author

2009-05-31 02:37 UTC #2 lore raw

Quoted 10 lines · Linus Torvalds

Quoted 2 lines · Larry H.
This patch doesn't affect fastpaths.
This patch is ugly as hell.
You already know the size of the data to clear.

Quoted 9 lines

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.

Quoted 4 lines

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2009-05-31 06:26 UTC #3 lore raw

Hi Larry,

Quoted 1 line · Larry H.

Quoted 1 line
Quoted 1 line
This patch doesn't affect fastpaths.

Quoted 13 lines · Linus Torvalds

Quoted 13 lines
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(...)

Quoted 2 lines · Larry H.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2009-05-31 06:24 UTC #4 lore raw

In reply to Linus Torvalds

Hi Linus,

Quoted 2 lines · Larry H.

Quoted 2 lines
This patch doesn't affect fastpaths.

Quoted 7 lines · Linus Torvalds

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

Quoted 8 lines

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Alan Cox

2009-05-31 10:24 UTC #5 lore raw

Quoted 7 lines

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

Quoted 5 lines

Quoted 1 line
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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Linus Torvalds

2009-05-31 17:05 UTC #6 lore raw

Quoted 9 lines · Alan Cox

Quoted 9 lines
Quoted 8 lines
       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.

Quoted 7 lines

Quoted 3 lines
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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Alan Cox

2009-05-31 17:09 UTC #7 lore raw

Quoted 3 lines

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.

Quoted 1 line

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

Added to the tty queue will do that tomorrow

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Pekka Enberg

2009-05-31 17:21 UTC #8 lore raw

In reply to Linus Torvalds

Hi Linus,

Quoted 14 lines · Alan Cox

Quoted 14 lines
Quoted 11 lines
Quoted 8 lines
       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.

Quoted 5 lines · Linus Torvalds

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

2009-06-02 15:05 UTC #9 lore raw

In reply to Linus Torvalds

Quoted 8 lines · Linus Torvalds

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.