From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Subject: Re: [PATCH] mnt: Fix a memory stomp in umount Date: Thu, 18 Dec 2014 20:15:17 +0000 Message-ID: <20141218201517.GP22149@ZenIV.linux.org.uk> References: <87d27gkia8.fsf@x220.int.ebiederm.org> <20141218200110.GO22149@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Linus Torvalds , linux-fsdevel@vger.kernel.org To: "Eric W. Biederman" Return-path: Received: from zeniv.linux.org.uk ([195.92.253.2]:56368 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751219AbaLRUPU (ORCPT ); Thu, 18 Dec 2014 15:15:20 -0500 Content-Disposition: inline In-Reply-To: <20141218200110.GO22149@ZenIV.linux.org.uk> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Thu, Dec 18, 2014 at 08:01:10PM +0000, Al Viro wrote: > 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. Mind you, I'm not saying that this isn't worth sanitizing. Just that analysis in your commit message is incorrect - we actually do _not_ stomp on memory there. It's brittle and it's only saved by having only very few things done to that hlist, but the actual memory corruption does not happen. Again, I'm no defending that code; it's certaily better off making a properly-spliced hlist rather than relying on precise sequence of operations in namespace_unlock().