ACK: [PATCH 2/2][T/X/B/C/D] mount: Don't allow copying MNT_UNBINDABLE|MNT_LOCKED mounts

[Colin Ian King]

On 13/11/2018 07:44, Tyler Hicks wrote:
> From: "Eric W. Biederman" <ebiederm at xmission.com>
> 
> BugLink: https://launchpad.net/bugs/1789161
> 
> Jonathan Calmels from NVIDIA reported that he's able to bypass the
> mount visibility security check in place in the Linux kernel by using
> a combination of the unbindable property along with the private mount
> propagation option to allow a unprivileged user to see a path which
> was purposefully hidden by the root user.
> 
> Reproducer:
>   # Hide a path to all users using a tmpfs
>   root at castiana:~# mount -t tmpfs tmpfs /sys/devices/
>   root at castiana:~#
> 
>   # As an unprivileged user, unshare user namespace and mount namespace
>   stgraber at castiana:~$ unshare -U -m -r
> 
>   # Confirm the path is still not accessible
>   root at castiana:~# ls /sys/devices/
> 
>   # Make /sys recursively unbindable and private
>   root at castiana:~# mount --make-runbindable /sys
>   root at castiana:~# mount --make-private /sys
> 
>   # Recursively bind-mount the rest of /sys over to /mnnt
>   root at castiana:~# mount --rbind /sys/ /mnt
> 
>   # Access our hidden /sys/device as an unprivileged user
>   root at castiana:~# ls /mnt/devices/
>   breakpoint cpu cstate_core cstate_pkg i915 intel_pt isa kprobe
>   LNXSYSTM:00 msr pci0000:00 platform pnp0 power software system
>   tracepoint uncore_arb uncore_cbox_0 uncore_cbox_1 uprobe virtual
> 
> Solve this by teaching copy_tree to fail if a mount turns out to be
> both unbindable and locked.
> 
> Cc: stable at vger.kernel.org
> Fixes: 5ff9d8a65ce8 ("vfs: Lock in place mounts from more privileged users")
> Reported-by: Jonathan Calmels <jcalmels at nvidia.com>
> Signed-off-by: "Eric W. Biederman" <ebiederm at xmission.com>
> (cherry picked from commit df7342b240185d58d3d9665c0bbf0a0f5570ec29)
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> ---
>  fs/namespace.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 3fbffe0788d1..5363e01ca34a 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1827,8 +1827,14 @@ struct mount *copy_tree(struct mount *mnt, struct dentry *dentry,
>  		for (s = r; s; s = next_mnt(s, r)) {
>  			if (!(flag & CL_COPY_UNBINDABLE) &&
>  			    IS_MNT_UNBINDABLE(s)) {
> -				s = skip_mnt_tree(s);
> -				continue;
> +				if (s->mnt.mnt_flags & MNT_LOCKED) {
> +					/* Both unbindable and locked. */
> +					q = ERR_PTR(-EPERM);
> +					goto out;
> +				} else {
> +					s = skip_mnt_tree(s);
> +					continue;
> +				}
>  			}
>  			if (!(flag & CL_COPY_MNT_NS_FILE) &&
>  			    is_mnt_ns_file(s->mnt.mnt_root)) {
> 

Again, clean cherry pick. Looks sensible.

Acked-by: Colin Ian King <colin.king at canonical.com>