linux-kernel - Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190415161657.2zwboghblj5ducux@treble>
Date:   Mon, 15 Apr 2019 11:16:57 -0500
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     Andy Lutomirski <luto@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Pekka Enberg <penberg@...nel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [patch V4 01/32] mm/slab: Fix broken stack trace storage

On Mon, Apr 15, 2019 at 06:07:44PM +0200, Thomas Gleixner wrote:
> On Mon, 15 Apr 2019, Josh Poimboeuf wrote:
> > On Mon, Apr 15, 2019 at 11:02:58AM +0200, Thomas Gleixner wrote:
> > >  	addr = (unsigned long *)&((char *)addr)[obj_offset(cachep)];
> > >  
> > > -	if (size < 5 * sizeof(unsigned long))
> > > +	if (size < 5)
> > >  		return;
> > >  
> > >  	*addr++ = 0x12345678;
> > >  	*addr++ = caller;
> > >  	*addr++ = smp_processor_id();
> > > -	size -= 3 * sizeof(unsigned long);
> > > +	size -= 3;
> > > +#ifdef CONFIG_STACKTRACE
> > >  	{
> > > -		unsigned long *sptr = &caller;
> > > -		unsigned long svalue;
> > > -
> > > -		while (!kstack_end(sptr)) {
> > > -			svalue = *sptr++;
> > > -			if (kernel_text_address(svalue)) {
> > > -				*addr++ = svalue;
> > > -				size -= sizeof(unsigned long);
> > > -				if (size <= sizeof(unsigned long))
> > > -					break;
> > > -			}
> > > -		}
> > > +		struct stack_trace trace = {
> > > +			/* Leave one for the end marker below */
> > > +			.max_entries	= size - 1,
> > > +			.entries	= addr,
> > > +			.skip		= 3,
> > > +		};
> > >  
> > > +		save_stack_trace(&trace);
> > > +		addr += trace.nr_entries;
> > >  	}
> > > -	*addr++ = 0x87654321;
> > > +#endif
> > > +	*addr = 0x87654321;
> > 
> > Looks like stack_trace.nr_entries isn't initialized?  (though this code
> > gets eventually replaced by a later patch)
> 
> struct initializer initialized the non mentioned fields to 0, if I'm not
> totally mistaken.

Hm, it seems you are correct.  And I thought I knew C.

> > Who actually reads this stack trace?  I couldn't find a consumer.
> 
> It's stored directly in the memory pointed to by @addr and that's the freed
> cache memory. If that is used later (UAF) then the stack trace can be
> printed to see where it was freed.

Right... but who reads it?

-- 
Josh