Messages in this thread | | | Date | Mon, 23 Jan 2023 14:40:26 -0500 | Subject | Re: [RESEND PATCH v2 2/2] mm/kmemleak: Fix UAF bug in kmemleak_scan() | From | Waiman Long <> |
| |
On 1/23/23 14:24, Catalin Marinas wrote: > 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>
I should have mentioned the fact that taking the kmemleak_lock will post some ordering guarantee since it is done after a new rcu_read_lock(). So yes, even if both A and B are removed from the object_list, they should still be around and not freed yet.
Thanks for your review.
Cheers, Longman
| |