From mboxrd@z Thu Jan  1 00:00:00 1970
From: Al Viro <viro@ZenIV.linux.org.uk>
Subject: Re: [PATCH] mnt: Fix a memory stomp in umount
Date: Thu, 18 Dec 2014 20:01:10 +0000
Message-ID: <20141218200110.GO22149@ZenIV.linux.org.uk>
References: <87d27gkia8.fsf@x220.int.ebiederm.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org
To: "Eric W. Biederman" <ebiederm@xmission.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from zeniv.linux.org.uk ([195.92.253.2]:56340 "EHLO
	ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751187AbaLRUBN (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Thu, 18 Dec 2014 15:01:13 -0500
Content-Disposition: inline
In-Reply-To: <87d27gkia8.fsf@x220.int.ebiederm.org>
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

On Thu, Dec 18, 2014 at 10:57:19AM -0600, Eric W. Biederman wrote:
> 
> While reviewing the code of umount_tree I realized that when we append
> to a preexisting unmounted list we do not change pprev of the former
> first item in the list.
> 
> Which means later in namespace_unlock hlist_del_init(&mnt->mnt_hash) on
> the former first item of the list will stomp unmounted.first leaving
> it set to some random mount point which we are likely to free soon.
> 
> This isn't likely to hit, but if it does I don't know how anyone could
> track it down.

All you need for that kind of loop to work is correct -next on all elements.
Seriously.  Even correct first->pprev is not needed.  Look:
        struct hlist_head head = unmounted;

        if (likely(hlist_empty(&head))) // no dereferences of ppre *or* next
		sod off

        head.first->pprev = &head.first; // pprev of the first is valid now
        INIT_HLIST_HEAD(&unmounted);

        hlist_for_each_entry(mnt, &head, mnt_hash)
                if (mnt->mnt_ex_mountpoint.mnt)
                        mntget(mnt->mnt_ex_mountpoint.mnt);
	// only needed ->next for that loop
	...
        while (!hlist_empty(&head)) {
                mnt = hlist_entry(head.first, struct mount, mnt_hash);
                hlist_del_init(&mnt->mnt_hash);
		...
	}

Now, we certainly need pprev of the first to be correct for hlist_del_init()
to work.  What we do not need is correctness of pprev of the second as we
call hlist_del_init().  And _after_ hlist_del_init() pprev of what used to
be the second (the first, now) is healed.

So we actually are OK here.