Re: [RESEND PATCH v2 2/2] mm/kmemleak: Fix UAF bug in kmemleak_scan()

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Jan 20, 2023 at 05:54:28PM -0500, Waiman Long wrote:
> On 1/20/23 14:18, Catalin Marinas wrote:
> > >   /*
> > > @@ -633,6 +642,7 @@ static void __create_object(unsigned long ptr, size_t size,
> > >   	object->count = 0;			/* white color initially */
> > >   	object->jiffies = jiffies;
> > >   	object->checksum = 0;
> > > +	object->del_state = 0;
> > >   	/* task information */
> > >   	if (in_hardirq()) {
> > > @@ -1470,9 +1480,22 @@ static void kmemleak_cond_resched(struct kmemleak_object *object)
> > >   	if (!get_object(object))
> > >   		return;	/* Try next object */
> > > +	raw_spin_lock_irq(&kmemleak_lock);
> > > +	if (object->del_state & DELSTATE_REMOVED)
> > > +		goto unlock_put;	/* Object removed */
> > > +	object->del_state |= DELSTATE_NO_DELETE;
> > > +	raw_spin_unlock_irq(&kmemleak_lock);
> > > +
> > >   	rcu_read_unlock();
> > >   	cond_resched();
> > >   	rcu_read_lock();
> > > +
> > > +	raw_spin_lock_irq(&kmemleak_lock);
> > > +	if (object->del_state & DELSTATE_REMOVED)
> > > +		list_del_rcu(&object->object_list);
> > > +	object->del_state &= ~DELSTATE_NO_DELETE;
> > > +unlock_put:
> > > +	raw_spin_unlock_irq(&kmemleak_lock);
> > >   	put_object(object);
> > >   }
> > I'm not sure this was the only problem. We do have the problem that the
> > current object may be removed from the list, solved above, but another
> > scenario I had in mind is the next object being released during this
> > brief resched period. The RCU relies on object->next->next being valid
> > but, with a brief rcu_read_unlock(), the object->next could be freed,
> > reallocated, so object->next->next invalid.
> 
> Looking at the following scenario,
> 
> object->next => A (removed)
> A->next => B (removed)
> 
> As object->next is pointing to A, A must still be allocated and not freed
> yet. Now if B is also removed, there are 2 possible case.
> 
> 1) B is removed from the list after the removal of A. In that case, it is
> not possible that A is allocated, but B is freed.
> 
> 2) B is removed before A. A->next can't pointed to B when it is being
> removed. Due to weak memory ordering, it is possible that another cpu can
> see A->next still pointing to B. In that case, I believe that it is still
> within the grace period where neither A or B is freed.
> 
> In fact, it is no different from a regular scanning of the object list
> without ever called cond_resched().

More like thinking out loud:

The lockless RCU loop relies on object->next->next being valid within
the grace period (A not freed). Due to weak memory ordering, the looping
CPU may not observe the object->next update (removal of A) by another
CPU, so it continues to loop over it. But since we do an
rcu_read_unlock() in the middle of the loop, I don't think these
assumptions are still valid, so A may be freed.

What we need is that object->next reading for the following iteration
either sees the updated object->next (B) or it sees A but the latter
still around. I think this holds with the proposed
kmemleak_cond_resched() since we now start a new grace period with
rcu_read_lock() followed by taking and releasing kmemleak_lock. The
latter would give us the memory ordering required since removing object
A from the list does take the lock.

So yeah, you are probably right, I just find it hard to get my head
around ;). I still think it would be simpler with a single kmemleak_lock
(no object->lock) but that's more involved than a simple fix.

Assuming your (and my) reasoning above is correct:

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>