netdev - Re: [Bugme-new] [Bug 33502] New: Caught 64-bit read from uninitialized memory in __alloc

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, 10 May 2011 21:32:28 +0200
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Christoph Lameter <cl@...ux.com>
Cc:	Vegard Nossum <vegardno@....uio.no>,
	Pekka Enberg <penberg@...helsinki.fi>,
	casteyde.christian@...e.fr,
	Andrew Morton <akpm@...ux-foundation.org>,
	netdev@...r.kernel.org, bugzilla-daemon@...zilla.kernel.org,
	bugme-daemon@...zilla.kernel.org
Subject: Re: [Bugme-new] [Bug 33502] New: Caught 64-bit read from
 uninitialized memory in __alloc_skb

Le mardi 10 mai 2011 à 13:28 -0500, Christoph Lameter a écrit :
> On Tue, 10 May 2011, Eric Dumazet wrote:
> 
> > > +	else
> > > +		p = get_freepointer(s, object);
> > > +
> > > +	local_irq_restore(flags);
> > > +#else
> > > +	p = get_freepointer(s, object);
> > > +#endif
> > > +	return p;
> > > +}
> > > +
> > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > >  {
> > >  	*(void **)(object + s->offset) = fp;
> > > @@ -1933,7 +1954,7 @@ redo:
> > >  		if (unlikely(!irqsafe_cpu_cmpxchg_double(
> > >  				s->cpu_slab->freelist, s->cpu_slab->tid,
> > >  				object, tid,
> > > -				get_freepointer(s, object), next_tid(tid)))) {
> > > +				get_freepointer_safe(s, object), next_tid(tid)))) {
> > >
> > >  			note_cmpxchg_failure("slab_alloc", s, tid);
> > >  			goto redo;
> >
> >
> > Really this wont work Stephen
> 
> I am not Stephen.
> 

Yes, sorry Christoph.

> > You have to disable IRQ _before_ even fetching 'object'
> 
> The object pointer is being obtained from a per cpu structure and
> not from the page. What is the problem with fetching the object pointer?
> 
> > Or else, you can have an IRQ, allocate this object, pass to another cpu.
> 
> If that occurs then TID is being incremented and we will restart the loop
> getting the new object pointer from the per cpu structure. The object
> pointer that we were considering is irrelevant.
> 

Problem is not restart the loop, but avoiding accessing a non valid
memory area.

> > This other cpu can free the object and unmap page right after you did
> > the probe_kernel_address(object) (successfully), and before your cpu :
> >
> > p = get_freepointer(s, object); << BUG >>
> 
> If the other cpu frees the object and unmaps the page then
> get_freepointer_safe() can obtain an arbitrary value since the TID was
> incremented. We will restart the loop and discard the value retrieved.
> 



In current code I see :

tid = c->tid;
barrier();
object = c->freelist;

There is no guarantee c->tid is fetched before c->freelist by cpu.

You need rmb() here.


I claim all this would be far more simple disabling IRQ before fetching
c->tid and c->freelist, in DEBUG_PAGE_ALLOC case.

You would not even need to use magic probe_kernel_read()


Why do you try so _hard_ trying to optimize this, I really wonder.
Nobody is able to read this code anymore and prove its correct.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html