userns: automount cleanups

While reviewing some code I realized that in getting d_automount working with s_user_ns I had left behind some unnecessary relics of the blind path I started down. Here are two patches that remove those relics.

Unless someone has another preference I will drop them in my userns tree and merge them that way.

Eric W. Biederman (2):
userns: Don't fail follow_automount based on s_user_ns
autofs4: Modify autofs_wait to use current_uid() and current_gid()

 fs/autofs4/waitq.c | 4 ++--
 fs/namei.c         | 3 ---
 2 files changed, 2 insertions(+), 5 deletions(-)

When vfs_submount was added the test to limit automounts from filesystems that with s_user_ns != &init_user_ns accidentially left in follow_automount. The test was never about any security concerns and was always about how do we implement this for filesystems whose s_user_ns != &init_user_ns.

At the moment this check makes no difference as there are no filesystems that both set FS_USERNS_MOUNT and implement d_automount.

Remove this check now while I am thinking about it so there will not be odd booby traps for someone who does want to make this combination work.

vfs_submount still needs improvements to allow this combination to work, and vfs_submount contains a check that presents a warning.

The autofs4 filesystem could be modified to set FS_USERNS_MOUNT and it would need not work on this code path, as userspace performs the mounts.

---
 fs/namei.c | 3 ---
 1 file changed, 3 deletions(-)

index f0c7a7b9b6ca..f47118ed36e7 100644
@@ -1142,9 +1142,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			return -ENOENT;
 	}
 
-	if (path->dentry->d_sb->s_user_ns != &init_user_ns)
-		return -EACCES;
-
 	nd->total_link_count++;
 	if (nd->total_link_count >= 40)
 		return -ELOOP;

2.14.1

I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would trigger for automount(8) run entirely with a container (eg. docker)?

Anyway, it's gone now, so ACK to these two, thanks Eric.

When vfs_submount was added the test to limit automounts from filesystems that with s_user_ns != &init_user_ns accidentially left in follow_automount. The test was never about any security concerns and was always about how do we implement this for filesystems whose s_user_ns != &init_user_ns.

At the moment this check makes no difference as there are no filesystems that both set FS_USERNS_MOUNT and implement d_automount.

Remove this check now while I am thinking about it so there will not be odd booby traps for someone who does want to make this combination work.

vfs_submount still needs improvements to allow this combination to work, and vfs_submount contains a check that presents a warning.

The autofs4 filesystem could be modified to set FS_USERNS_MOUNT and it would need not work on this code path, as userspace performs the mounts.

---
 fs/namei.c | 3 ---
 1 file changed, 3 deletions(-)

index f0c7a7b9b6ca..f47118ed36e7 100644
@@ -1142,9 +1142,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			return -ENOENT;
 	}
 
-	if (path->dentry->d_sb->s_user_ns != &init_user_ns)
-		return -EACCES;
-
 	nd->total_link_count++;
 	if (nd->total_link_count >= 40)
 		return -ELOOP;

2.14.1

The code used to do that and then I mucked with it and never quite put the code back. Today the code references current_cred()->uid and current_cred()->gid which is equivalent but more wordy, and not idiomatic.

---
 fs/autofs4/waitq.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 8fc41705c7cd..9908ecf7fce0 100644
@@ -443,8 +443,8 @@ int autofs4_wait(struct autofs_sb_info *sbi,
 		memcpy(&wq->name, &qstr, sizeof(struct qstr));
 		wq->dev = autofs4_get_dev(sbi);
 		wq->ino = autofs4_get_ino(sbi);
-		wq->uid = current_cred()->uid;
-		wq->gid = current_cred()->gid;
+		wq->uid = current_uid();
+		wq->gid = current_gid();
 		wq->pid = pid;
 		wq->tgid = tgid;
 		wq->status = -EINTR; /* Status return if interrupted */

2.14.1

I saw the "<etc>->s_user_ns != &init_user_ns" and wondered if that would trigger for automount(8) run entirely with a container (eg. docker)?

Anyway, it's gone now, so ACK to these two, thanks Eric.

autofs still needs FS_USERNS_MOUNT before you can reach that point. But docker does have a mode ?--userns-remap? where it sets up the containers mounts that way.

I think in principle that should work and be safe. I don't know how robust autofs is against malicious users. Which is the question to ask before actually adding FS_USERNS_MOUNT in struct file_system_type.

Eric

autofs still needs FS_USERNS_MOUNT before you can reach that point. But docker does have a mode ?--userns-remap? where it sets up the containers mounts that way.

I think in principle that should work and be safe. I don't know how robust autofs is against malicious users. Which is the question to ask before actually adding FS_USERNS_MOUNT in struct file_system_type.

Eric

automount(8) thinks it is running as uid 0 (which it is in the container) so this would require a bit of thought since automount(8) doesn't take special precautions.

The restrictions, in the case of running entirely within a container, are those of the container itself. For example, if there are no granular capabilities available then the admin needs to run the container as privileged in order to be able to mount NFS file systems, which is a common usage case.

Ian