[PATCH] fs: relax mount_setattr() permission checks

[PATCH] fs: relax mount_setattr() permission checks

[Christian Brauner]

When we added mount_setattr() I added additional checks compared to the
legacy do_reconfigure_mnt() and do_change_type() helpers used by regular
mount(2). If that mount had a parent then verify that the caller and the
mount namespace the mount is attached to match and if not make sure that
it's an anonymous mount.

The real rootfs falls into neither category. It is neither an anoymous
mount because it is obviously attached to the initial mount namespace
but it also obviously doesn't have a parent mount. So that means legacy
mount(2) allows changing mount properties on the real rootfs but
mount_setattr(2) blocks this. I never thought much about this but of
course someone on this planet of earth changes properties on the real
rootfs as can be seen in [1].

Since util-linux finally switched to the new mount api in 2.39 not so
long ago it also relies on mount_setattr() and that surfaced this issue
when Fedora 39 finally switched to it. Fix this.

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2256843
Reported-by: Karel Zak <kzak@redhat.com>
Cc: stable@vger.kernel.org # v5.12+
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/namespace.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 437f60e96d40..fb0286920bce 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -4472,10 +4472,15 @@ static int do_mount_setattr(struct path *path, struct mount_kattr *kattr)
 	/*
 	 * If this is an attached mount make sure it's located in the callers
 	 * mount namespace. If it's not don't let the caller interact with it.
-	 * If this is a detached mount make sure it has an anonymous mount
-	 * namespace attached to it, i.e. we've created it via OPEN_TREE_CLONE.
+	 *
+	 * If this mount doesn't have a parent it's most often simply a
+	 * detached mount with an anonymous mount namespace. IOW, something
+	 * that's simply not attached yet. But there are apparently also users
+	 * that do change mount properties on the rootfs itself. That obviously
+	 * neither has a parent nor is it a detached mount so we cannot
+	 * unconditionally check for detached mounts.
 	 */
-	if (!(mnt_has_parent(mnt) ? check_mnt(mnt) : is_anon_ns(mnt->mnt_ns)))
+	if (mnt_has_parent(mnt) && !check_mnt(mnt))
 		goto out;
 
 	/*

---
base-commit: 2a42e144dd0b62eaf79148394ab057145afbc3c5
change-id: 20240206-vfs-mount-rootfs-70aff2e3956d

Re: [PATCH] fs: relax mount_setattr() permission checks

[Jan Kara]

On Tue 06-02-24 11:22:09, Christian Brauner wrote:
> When we added mount_setattr() I added additional checks compared to the
> legacy do_reconfigure_mnt() and do_change_type() helpers used by regular
> mount(2). If that mount had a parent then verify that the caller and the
> mount namespace the mount is attached to match and if not make sure that
> it's an anonymous mount.
> 
> The real rootfs falls into neither category. It is neither an anoymous
> mount because it is obviously attached to the initial mount namespace
> but it also obviously doesn't have a parent mount. So that means legacy
> mount(2) allows changing mount properties on the real rootfs but
> mount_setattr(2) blocks this. I never thought much about this but of
> course someone on this planet of earth changes properties on the real
> rootfs as can be seen in [1].
> 
> Since util-linux finally switched to the new mount api in 2.39 not so
> long ago it also relies on mount_setattr() and that surfaced this issue
> when Fedora 39 finally switched to it. Fix this.
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2256843
> Reported-by: Karel Zak <kzak@redhat.com>
> Cc: stable@vger.kernel.org # v5.12+
> Signed-off-by: Christian Brauner <brauner@kernel.org>

I'm not too confident with subtleties of this code but it looks good to me.
So feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/namespace.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 437f60e96d40..fb0286920bce 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -4472,10 +4472,15 @@ static int do_mount_setattr(struct path *path, struct mount_kattr *kattr)
>  	/*
>  	 * If this is an attached mount make sure it's located in the callers
>  	 * mount namespace. If it's not don't let the caller interact with it.
> -	 * If this is a detached mount make sure it has an anonymous mount
> -	 * namespace attached to it, i.e. we've created it via OPEN_TREE_CLONE.
> +	 *
> +	 * If this mount doesn't have a parent it's most often simply a
> +	 * detached mount with an anonymous mount namespace. IOW, something
> +	 * that's simply not attached yet. But there are apparently also users
> +	 * that do change mount properties on the rootfs itself. That obviously
> +	 * neither has a parent nor is it a detached mount so we cannot
> +	 * unconditionally check for detached mounts.
>  	 */
> -	if (!(mnt_has_parent(mnt) ? check_mnt(mnt) : is_anon_ns(mnt->mnt_ns)))
> +	if (mnt_has_parent(mnt) && !check_mnt(mnt))
>  		goto out;
>  
>  	/*
> 
> ---
> base-commit: 2a42e144dd0b62eaf79148394ab057145afbc3c5
> change-id: 20240206-vfs-mount-rootfs-70aff2e3956d
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: relax mount_setattr() permission checks

[Christian Brauner]

On Tue, 06 Feb 2024 11:22:09 +0100, Christian Brauner wrote:
> When we added mount_setattr() I added additional checks compared to the
> legacy do_reconfigure_mnt() and do_change_type() helpers used by regular
> mount(2). If that mount had a parent then verify that the caller and the
> mount namespace the mount is attached to match and if not make sure that
> it's an anonymous mount.
> 
> The real rootfs falls into neither category. It is neither an anoymous
> mount because it is obviously attached to the initial mount namespace
> but it also obviously doesn't have a parent mount. So that means legacy
> mount(2) allows changing mount properties on the real rootfs but
> mount_setattr(2) blocks this. I never thought much about this but of
> course someone on this planet of earth changes properties on the real
> rootfs as can be seen in [1].
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] fs: relax mount_setattr() permission checks
      https://git.kernel.org/vfs/vfs/c/853c4204729e

Re: [PATCH] fs: relax mount_setattr() permission checks

[Christian Brauner]

On Tue, Feb 06, 2024 at 11:22:09AM +0100, Christian Brauner wrote:
> When we added mount_setattr() I added additional checks compared to the
> legacy do_reconfigure_mnt() and do_change_type() helpers used by regular
> mount(2). If that mount had a parent then verify that the caller and the
> mount namespace the mount is attached to match and if not make sure that
> it's an anonymous mount.
> 
> The real rootfs falls into neither category. It is neither an anoymous
> mount because it is obviously attached to the initial mount namespace
> but it also obviously doesn't have a parent mount. So that means legacy
> mount(2) allows changing mount properties on the real rootfs but
> mount_setattr(2) blocks this. I never thought much about this but of
> course someone on this planet of earth changes properties on the real
> rootfs as can be seen in [1].
> 
> Since util-linux finally switched to the new mount api in 2.39 not so
> long ago it also relies on mount_setattr() and that surfaced this issue
> when Fedora 39 finally switched to it. Fix this.
> 
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=2256843
> Reported-by: Karel Zak <kzak@redhat.com>
> Cc: stable@vger.kernel.org # v5.12+
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---

Fwiw, I've been going back and forth on this yesterday evening because
of an inconsistency in legacy mount(2). The gist is that for changing
generic mount properties via do_reconfigure_mnt() we check_mnt() but for
changing mount propagation settings via do_change_type() we don't. For
mount_setattr(2) we should do better. So the change I originally went
with didn't bother to do check_mnt() when that thing doesn't have a
parent to be true to mount propagation behavior in legacy mount(2). But
I think that this is wrong and this should be
if ((mnt_has_parent(mnt) || !is_anon_ns(mnt->mnt_ns)) && !check_mnt(mnt))
which means we do check_mnt() even for the real rootfs which doesn't
have a parent and for both regular and mount propagation properties.
I've changed the patch to that.