linux-kernel - Re: [PATCH] fs: allow clone_private_mount() for a path on real rootfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <46dc7517-75dd-48d7-ae01-1574b49040fd@nec.com>
Date: Thu, 15 May 2025 12:12:47 +0000
From: KONDO KAZUMA(近藤　和真)
	<kazuma-kondo@....com>
To: Al Viro <viro@...iv.linux.org.uk>
CC: "brauner@...nel.org" <brauner@...nel.org>, "jack@...e.cz" <jack@...e.cz>,
	"mike@...ynton.com" <mike@...ynton.com>, "miklos@...redi.hu"
	<miklos@...redi.hu>, "amir73il@...il.com" <amir73il@...il.com>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	"linux-unionfs@...r.kernel.org" <linux-unionfs@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] fs: allow clone_private_mount() for a path on real rootfs

On 2025/05/15 4:02, Al Viro wrote:
> On Wed, May 14, 2025 at 08:37:54AM +0000, KONDO KAZUMA(近藤 和真) wrote:
>> On 2025/05/14 11:43, Al Viro wrote:
>>> On Wed, May 14, 2025 at 12:25:58AM +0000, KONDO KAZUMA(近藤 和真) wrote:
>>>
>>>> @@ -2482,17 +2482,13 @@ struct vfsmount *clone_private_mount(const struct path *path)
>>>>  	if (IS_MNT_UNBINDABLE(old_mnt))
>>>>  		return ERR_PTR(-EINVAL);
>>>>  
>>>> -	if (mnt_has_parent(old_mnt)) {
>>>> +	if (!is_mounted(&old_mnt->mnt))
>>>> +		return ERR_PTR(-EINVAL);
>>>> +
>>>> +	if (mnt_has_parent(old_mnt) || !is_anon_ns(old_mnt->mnt_ns)) {
>>>>  		if (!check_mnt(old_mnt))
>>>>  			return ERR_PTR(-EINVAL);
>>>>  	} else {
>>>> -		if (!is_mounted(&old_mnt->mnt))
>>>> -			return ERR_PTR(-EINVAL);
>>>> -
>>>> -		/* Make sure this isn't something purely kernel internal. */
>>>> -		if (!is_anon_ns(old_mnt->mnt_ns))
>>>> -			return ERR_PTR(-EINVAL);
>>>> -
>>>>  		/* Make sure we don't create mount namespace loops. */
>>>>  		if (!check_for_nsfs_mounts(old_mnt))
>>>>  			return ERR_PTR(-EINVAL);
>>>
>>> Not the right way to do that.  What we want is
>>>
>>> 	/* ours are always fine */
>>> 	if (!check_mnt(old_mnt)) {
>>> 		/* they'd better be mounted _somewhere */
>>> 		if (!is_mounted(old_mnt))
>>> 			return -EINVAL;
>>> 		/* no other real namespaces; only anon */
>>> 		if (!is_anon_ns(old_mnt->mnt_ns))
>>> 			return -EINVAL;
>>> 		/* ... and root of that anon */
>>> 		if (mnt_has_parent(old_mnt))
>>> 			return -EINVAL;
>>> 		/* Make sure we don't create mount namespace loops. */
>>> 		if (!check_for_nsfs_mounts(old_mnt))
>>> 			return ERR_PTR(-EINVAL);
>>> 	}
>>
>> Hello Al Viro,
>>
>> Thank you for your comment.
>> That code can solve my problem, and it seems to be better!
> 
> BTW, see https://lore.kernel.org/all/20250506194849.GT2023217@ZenIV/ for
> discussion about a week ago when that got noticed:
> 
> || In case of clone_private_mount(), though, there's nothing wrong
> || with "clone me a subtree of absolute root", so it has to be
> || done other way round - check if it's ours first, then in "not
> || ours" case check that it's a root of anon namespace.
> ||
> || Failing btrfs mount has ended up with upper layer pathname
> || pointing to initramfs directory where btrfs would've been
> || mounted, which had walked into that corner case.  In your
> || case the problem has already happened by that point, but on
> || a setup a-la X Terminal it would cause trouble...
> 
> Looks like such setups are less theoretical than I thought.
> 
>> So, I will revise my patch and resend it.
> 
> Probably worth gathering the comments in one place.  Something like
> 	/*
> 	 * Check if the source is acceptable; anything mounted in
> 	 * our namespace is fine, otherwise it must be the root of
> 	 * some anon namespace and we need to make sure no namespace
> 	 * loops get created.
> 	 */
> 	if (!check_mnt(old_mnt)) {
> 		if (!is_mounted(&old_mnt->mnt) ||
> 		    !is_anon_ns(old_mnt->mnt_ns) ||
> 		    mnt_has_parent(old_mnt))
> 			return ERR_PTR(-EINVAL);
> 		if (!check_for_nsfs_mounts(old_mnt))
> 			return ERR_PTR(-EINVAL);
> 	}
> might be easier to follow.

I think it's good idea.
I will send a patch with that comment.

Thanks