linux-kernel - Re: [BUG] SLOB's krealloc() seems bust

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 07 Oct 2008 10:00:55 -0500
From:	Matt Mackall <mpm@...enic.com>
To:	Christoph Lameter <cl@...ux-foundation.org>
Cc:	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	linux-mm <linux-mm@...ck.org>,
	Nick Piggin <nickpiggin@...oo.com.au>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Ingo Molnar <mingo@...e.hu>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [BUG] SLOB's krealloc() seems bust


On Tue, 2008-10-07 at 09:07 -0500, Christoph Lameter wrote:
> > Which basically shows us that the content of the pcpu_size[] array got
> > corrupted after the krealloc() call in split_block().
> > 
> > Which made me look at which slab allocator I had selected, which turned
> > out to be SLOB (from testing the network swap stuff).
> 
> krealloc() is in generic core code (mm/util.c) and is the same for all allocators.
> 
> krealloc uses ksize() which is somewhat dicey for SLOB because it only works
> on kmalloc'ed memory. Is the krealloc used on memory allocated with kmalloc()?
> Slob's ksize could use a BUG_ON for the case in which ksize() is used on
> kmem_cache_alloc'd memory.
> 
> /* can't use ksize for kmem_cache_alloc memory, only kmalloc */
> size_t ksize(const void *block)
> {
>         struct slob_page *sp;
> 
>         BUG_ON(!block);
>         if (unlikely(block == ZERO_SIZE_PTR))
>                 return 0;
> 
>         sp = (struct slob_page *)virt_to_page(block);
> 
> 
> Add a BUG_ON(!kmalloc_cache(sp))?

We can't dynamically determine whether a pointer points to a kmalloced
object or not. kmem_cache_alloc objects have no header and live on the
same pages as kmalloced ones.

>         if (slob_page(sp))
>                 return ((slob_t *)block - 1)->units + SLOB_UNIT;
> 			^^^^^^^ Is this correct?

The cast? Yes. We want to look at the slob_t object header immediately
before the object pointer.

But the rest of the statement looks completely broken. If our SLOB
object is 3 units (6 bytes), with a usuable size of 4 bytes, the above
will report 3 + 2 = 5 bytes. Instead we want (3 - 1) * 2 = 4, something
more like:

	return (((slob_t *)block - 1)->units - 1) * SLOB_UNIT;
			
It's a bit amazing no one's hit this before, but I guess that's because
for allocations > 6 bytes, it will under-report buffer sizes, avoiding
overruns. And for the < 6 byte cases, we've just been getting lucky.

Give this a try, please:

diff -r 5e32b09a1b2b mm/slob.c
--- a/mm/slob.c	Fri Oct 03 14:04:43 2008 -0500
+++ b/mm/slob.c	Tue Oct 07 10:00:16 2008 -0500
@@ -515,7 +515,7 @@
 
 	sp = (struct slob_page *)virt_to_page(block);
 	if (slob_page(sp))
-		return ((slob_t *)block - 1)->units + SLOB_UNIT;
+		return (((slob_t *)block - 1)->units - 1) * SLOB_UNIT;
 	else
 		return sp->page.private;
 }

-- 
Mathematics is the supreme nostalgia of our time.

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