From mboxrd@z Thu Jan 1 00:00:00 1970 From: ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) Subject: Re: ucount: use-after-free read in inc_ucount & dec_ucount Date: Sun, 05 Mar 2017 12:41:16 -0600 Message-ID: <87y3wjlg6r.fsf@xmission.com> References: <180fb7dc-790e-8e82-0cc1-c6e15ddcd20b@gmail.com> <87pohy1fx6.fsf@xmission.com> <1aafd5e9-d9de-e5d9-a77d-cf245c5a5c6a@gmail.com> <87shmsoar7.fsf@xmission.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: (Dmitry Vyukov's message of "Sun, 5 Mar 2017 11:53:14 +0100") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Dmitry Vyukov Cc: syzkaller , containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Jan Kara , JongHwan Kim List-Id: containers.vger.kernel.org Dmitry Vyukov writes: > > Nice. I think it should work. > > I would also do: > > diff --git a/kernel/ucount.c b/kernel/ucount.c > index 8a11fc0cb459..233c8e46acd5 100644 > --- a/kernel/ucount.c > +++ b/kernel/ucount.c > @@ -143,19 +143,18 @@ static struct ucounts *get_ucounts(struct > user_namespace *ns, kuid_t uid) > > new->ns = ns; > new->uid = uid; > - atomic_set(&new->count, 0); > + atomic_set(&new->count, 1); > > spin_lock_irq(&ucounts_lock); > ucounts = find_ucounts(ns, uid, hashent); > if (ucounts) { > + atomic_inc(&ucounts->count); > kfree(new); > } else { > hlist_add_head(&new->node, hashent); > ucounts = new; > } > } > - if (!atomic_add_unless(&ucounts->count, 1, INT_MAX)) > - ucounts = NULL; > spin_unlock_irq(&ucounts_lock); > return ucounts; > } No. As that allows ucounts->count to be incremented to the point it goes negative. Counter wrap-around is just as bad as imperfect locking if you can trigger it, and has been a cause of use-after-free errors etc. So it is a feature that if the count is maxed out for a given kuid that get_ucounts will fail. Eric