Re: [PATCH] revert- sys_setaltroot

Re: [PATCH] revert- sys_setaltroot

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Tue, 2004-10-26 at 15:22 +0000, Linux Kernel Mailing List wrote:
> ChangeSet 1.2187, 2004/10/26 08:22:01-07:00, akpm@osdl.org
> 
> 	[PATCH] revert- sys_setaltroot	
> 	We decided to do this a different way.

Can you elaborate? We don't have open_exec() in userspace, so I'm
not sure it's possible to do this properly for setuid binaries and
execute-only binaries in userspace without being seriously exploitable.

I assume the problem with it was that you can set the altroot to a
directory containing a bogus /etc/shadow and then 'su' with a password
of your own choosing, or something like that? 

There's no reason to make the user-specified altroot survive across 
exec() though -- and as long it _doesn't_ persist, it should be
perfectly safe to let users set it. I'm working on a patch which puts
sys_setaltroot back but discards it on exec. This allows me to ditch all
the horrid path walking stuff from qemu, and as soon as I make qemu
understand AT_EXECFD it should let me run execute-only i386 binaries on
my ppc box too.

T'would be useful to confirm the original objections to it and that I
haven't missed a viable 'different way' to do it, though.

-- 
dwmw2

[-- Attachment #2: altroot-3.patch --]
[-- Type: text/x-patch, Size: 12759 bytes --]

===== arch/i386/kernel/entry.S 1.88 vs edited =====
--- 1.88/arch/i386/kernel/entry.S	2004-12-12 18:37:55 +00:00
+++ edited/arch/i386/kernel/entry.S	2004-12-20 13:08:02 +00:00
@@ -861,7 +861,7 @@
 	.long sys_mq_getsetattr
 	.long sys_ni_syscall		/* reserved for kexec */
 	.long sys_waitid
-	.long sys_ni_syscall		/* 285 */ /* available */
+	.long sys_setaltroot		/* 285 */
 	.long sys_add_key
 	.long sys_request_key
 	.long sys_keyctl
===== arch/ia64/kernel/entry.S 1.69 vs edited =====
--- 1.69/arch/ia64/kernel/entry.S	2004-11-23 20:18:30 +00:00
+++ edited/arch/ia64/kernel/entry.S	2004-12-20 13:08:02 +00:00
@@ -1531,7 +1531,7 @@
 	data8 sys_add_key
 	data8 sys_request_key
 	data8 sys_keyctl
-	data8 sys_ni_syscall
+	data8 sys_setaltroot
 	data8 sys_ni_syscall			// 1275
 	data8 sys_ni_syscall
 	data8 sys_ni_syscall
===== arch/ppc/kernel/misc.S 1.63 vs edited =====
--- 1.63/arch/ppc/kernel/misc.S	2004-10-22 10:27:40 +01:00
+++ edited/arch/ppc/kernel/misc.S	2004-12-20 13:08:02 +00:00
@@ -1450,3 +1450,4 @@
 	.long sys_add_key
 	.long sys_request_key		/* 270 */
 	.long sys_keyctl
+	.long sys_setaltroot
===== arch/ppc64/kernel/misc.S 1.93 vs edited =====
--- 1.93/arch/ppc64/kernel/misc.S	2004-11-11 08:23:03 +00:00
+++ edited/arch/ppc64/kernel/misc.S	2004-12-20 13:08:02 +00:00
@@ -966,7 +966,8 @@
 	.llong .sys32_add_key
 	.llong .sys32_request_key
 	.llong .compat_sys_keyctl
-
+	.llong .sys_setaltroot
+	
 	.balign 8
 _GLOBAL(sys_call_table)
 	.llong .sys_restart_syscall	/* 0 */
@@ -1241,3 +1242,4 @@
 	.llong .sys_add_key
 	.llong .sys_request_key		/* 270 */
 	.llong .sys_keyctl
+	.llong .sys_setaltroot
===== arch/sparc/kernel/systbls.S 1.29 vs edited =====
--- 1.29/arch/sparc/kernel/systbls.S	2004-11-08 02:08:03 +00:00
+++ edited/arch/sparc/kernel/systbls.S	2004-12-20 13:08:02 +00:00
@@ -75,7 +75,7 @@
 /*265*/	.long sys_timer_delete, sys_timer_create, sys_nis_syscall, sys_io_setup, sys_io_destroy
 /*270*/	.long sys_io_submit, sys_io_cancel, sys_io_getevents, sys_mq_open, sys_mq_unlink
 /*275*/	.long sys_mq_timedsend, sys_mq_timedreceive, sys_mq_notify, sys_mq_getsetattr, sys_waitid
-/*280*/	.long sys_ni_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/	.long sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl
 
 #ifdef CONFIG_SUNOS_EMUL
 	/* Now the SunOS syscall table. */
===== arch/sparc64/kernel/systbls.S 1.66 vs edited =====
--- 1.66/arch/sparc64/kernel/systbls.S	2004-11-11 08:23:03 +00:00
+++ edited/arch/sparc64/kernel/systbls.S	2004-12-20 13:08:02 +00:00
@@ -76,7 +76,7 @@
 	.word sys_timer_delete, sys32_timer_create, sys_ni_syscall, compat_sys_io_setup, sys_io_destroy
 /*270*/	.word sys32_io_submit, sys_io_cancel, compat_sys_io_getevents, sys32_mq_open, sys_mq_unlink
 	.word sys_mq_timedsend, sys_mq_timedreceive, compat_sys_mq_notify, compat_sys_mq_getsetattr, compat_sys_waitid
-/*280*/	.word sys_ni_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/	.word sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl
 
 #endif /* CONFIG_COMPAT */
 
@@ -142,7 +142,7 @@
 	.word sys_timer_delete, sys_timer_create, sys_ni_syscall, sys_io_setup, sys_io_destroy
 /*270*/	.word sys_io_submit, sys_io_cancel, sys_io_getevents, sys_mq_open, sys_mq_unlink
 	.word sys_mq_timedsend, sys_mq_timedreceive, sys_mq_notify, sys_mq_getsetattr, sys_waitid
-/*280*/	.word sys_nis_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/	.word sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl
 
 #if defined(CONFIG_SUNOS_EMUL) || defined(CONFIG_SOLARIS_EMUL) || \
     defined(CONFIG_SOLARIS_EMUL_MODULE)
===== fs/binfmt_elf.c 1.92 vs edited =====
--- 1.92/fs/binfmt_elf.c	2004-11-16 19:01:21 +00:00
+++ edited/fs/binfmt_elf.c	2004-12-20 13:08:02 +00:00
@@ -37,6 +37,7 @@
 #include <linux/pagemap.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/namei.h>
 
 #include <asm/uaccess.h>
 #include <asm/param.h>
@@ -626,7 +627,7 @@
 			 */
 			SET_PERSONALITY(loc->elf_ex, ibcs2_interpreter);
 
-			interpreter = open_exec(elf_interpreter);
+			interpreter = open_exec(elf_interpreter, LOOKUP_NOALT);
 			retval = PTR_ERR(interpreter);
 			if (IS_ERR(interpreter))
 				goto out_free_interp;
===== fs/binfmt_em86.c 1.8 vs edited =====
--- 1.8/fs/binfmt_em86.c	2004-05-10 12:25:55 +01:00
+++ edited/fs/binfmt_em86.c	2004-12-20 13:08:02 +00:00
@@ -19,6 +19,7 @@
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/errno.h>
+#include <linux/namei.h>
 
 
 #define EM86_INTERP	"/usr/bin/em86"
@@ -82,7 +83,7 @@
 	 * Note that we use open_exec() as the name is now in kernel
 	 * space, and we don't need to copy it.
 	 */
-	file = open_exec(interp);
+	file = open_exec(interp, LOOKUP_NOALT);
 	if (IS_ERR(file))
 		return PTR_ERR(file);
 
===== fs/binfmt_flat.c 1.11 vs edited =====
--- 1.11/fs/binfmt_flat.c	2004-10-19 06:26:36 +01:00
+++ edited/fs/binfmt_flat.c	2004-12-20 13:08:02 +00:00
@@ -36,6 +36,7 @@
 #include <linux/personality.h>
 #include <linux/init.h>
 #include <linux/flat.h>
+#include <linux/namei.h>
 
 #include <asm/byteorder.h>
 #include <asm/system.h>
@@ -774,7 +775,7 @@
 
 	/* Open the file up */
 	bprm.filename = buf;
-	bprm.file = open_exec(bprm.filename);
+	bprm.file = open_exec(bprm.filename, LOOKUP_NOALT);
 	res = PTR_ERR(bprm.file);
 	if (IS_ERR(bprm.file))
 		return res;
===== fs/binfmt_misc.c 1.30 vs edited =====
--- 1.30/fs/binfmt_misc.c	2004-06-29 15:43:10 +01:00
+++ edited/fs/binfmt_misc.c	2004-12-20 13:08:02 +00:00
@@ -179,7 +179,7 @@
 
 	bprm->interp = iname;	/* for binfmt_script */
 
-	interp_file = open_exec (iname);
+	interp_file = open_exec (iname, LOOKUP_NOALT);
 	retval = PTR_ERR (interp_file);
 	if (IS_ERR (interp_file))
 		goto _error;
===== fs/binfmt_script.c 1.9 vs edited =====
--- 1.9/fs/binfmt_script.c	2004-06-29 22:35:34 +01:00
+++ edited/fs/binfmt_script.c	2004-12-20 13:08:03 +00:00
@@ -85,9 +85,14 @@
 	/*
 	 * OK, now restart the process with the interpreter's dentry.
 	 */
-	file = open_exec(interp);
+	file = open_exec(interp, 0);
 	if (IS_ERR(file))
 		return PTR_ERR(file);
+
+	/* If we had an altroot (which may be untrusted) we can reset it
+	   to the default for the personality now that we've got the 
+	   script's interpreter itself open. */
+	set_fs_altroot();
 
 	bprm->file = file;
 	retval = prepare_binprm(bprm);
===== fs/compat.c 1.47 vs edited =====
--- 1.47/fs/compat.c	2004-12-10 17:57:46 +00:00
+++ edited/fs/compat.c	2004-12-20 13:08:03 +00:00
@@ -1393,7 +1393,7 @@
 		goto out_ret;
 	memset(bprm, 0, sizeof(*bprm));
 
-	file = open_exec(filename);
+	file = open_exec(filename, 0);
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
 		goto out_kfree;
@@ -1447,6 +1447,7 @@
 		free_arg_pages(bprm);
 
 		/* execve success */
+		set_fs_altroot();
 		security_bprm_free(bprm);
 		kfree(bprm);
 		return retval;
===== fs/exec.c 1.146 vs edited =====
--- 1.146/fs/exec.c	2004-12-10 17:57:46 +00:00
+++ edited/fs/exec.c	2004-12-20 13:08:03 +00:00
@@ -473,14 +473,14 @@
 
 #endif /* CONFIG_MMU */
 
-struct file *open_exec(const char *name)
+struct file *open_exec(const char *name, int flags)
 {
 	struct nameidata nd;
 	int err;
 	struct file *file;
 
 	nd.intent.open.flags = FMODE_READ;
-	err = path_lookup(name, LOOKUP_FOLLOW|LOOKUP_OPEN, &nd);
+	err = path_lookup(name, LOOKUP_FOLLOW|LOOKUP_OPEN|flags, &nd);
 	file = ERR_PTR(err);
 
 	if (!err) {
@@ -1007,7 +1007,7 @@
 
 	        loader = PAGE_SIZE*MAX_ARG_PAGES-sizeof(void *);
 
-		file = open_exec("/sbin/loader");
+		file = open_exec("/sbin/loader", LOOKUP_NOALT);
 		retval = PTR_ERR(file);
 		if (IS_ERR(file))
 			return retval;
@@ -1100,7 +1100,7 @@
 		goto out_ret;
 	memset(bprm, 0, sizeof(*bprm));
 
-	file = open_exec(filename);
+	file = open_exec(filename, 0);
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
 		goto out_kfree;
@@ -1155,6 +1155,7 @@
 		free_arg_pages(bprm);
 
 		/* execve success */
+		set_fs_altroot();
 		security_bprm_free(bprm);
 		kfree(bprm);
 		return retval;
===== fs/namei.c 1.115 vs edited =====
--- 1.115/fs/namei.c	2004-10-28 08:39:57 +01:00
+++ edited/fs/namei.c	2004-12-20 13:08:03 +00:00
@@ -913,20 +913,20 @@
 	return 1;
 }
 
-void set_fs_altroot(void)
+int __set_fs_altroot(const char *altroot)
 {
-	char *emul = __emul_prefix();
 	struct nameidata nd;
 	struct vfsmount *mnt = NULL, *oldmnt;
 	struct dentry *dentry = NULL, *olddentry;
 	int err;
-
-	if (!emul)
+	if (!altroot)
 		goto set_it;
-	err = path_lookup(emul, LOOKUP_FOLLOW|LOOKUP_DIRECTORY|LOOKUP_NOALT, &nd);
+	err = path_lookup(altroot, LOOKUP_FOLLOW|LOOKUP_DIRECTORY|LOOKUP_NOALT, &nd);
 	if (!err) {
 		mnt = nd.mnt;
 		dentry = nd.dentry;
+	} else {
+		return err;
 	}
 set_it:
 	write_lock(&current->fs->lock);
@@ -939,6 +939,58 @@
 		dput(olddentry);
 		mntput(oldmnt);
 	}
+	return 0;
+}
+
+void set_fs_altroot(void)
+{
+	char *emul = __emul_prefix();
+
+	__set_fs_altroot(emul);
+}
+
+asmlinkage long sys_setaltroot(const char __user * altroot)
+{
+	char *emul = NULL;
+	int ret;
+
+	if (altroot) {
+		emul = getname(altroot);
+		if (IS_ERR(emul)) {
+			ret = PTR_ERR(emul);
+			goto out;
+		}
+	}
+
+	if (atomic_read(&current->fs->count) != 1) {
+		struct fs_struct *fsp, *ofsp;
+
+		fsp = copy_fs_struct(current->fs);
+		if (fsp == NULL) {
+			ret = -ENOMEM;
+			goto out_putname;
+		}
+
+		task_lock(current);
+		ofsp = current->fs;
+		current->fs = fsp;
+		task_unlock(current);
+
+		put_fs_struct(ofsp);
+	}
+
+	/*
+	 * At that point we are guaranteed to be the sole owner of
+	 * current->fs.
+	 */
+
+	ret = __set_fs_altroot(emul);
+
+out_putname:
+	if (emul)
+		putname(emul);
+out:
+	return ret;
 }
 
 int fastcall path_lookup(const char *name, unsigned int flags, struct nameidata *nd)
===== include/asm-i386/unistd.h 1.43 vs edited =====
--- 1.43/include/asm-i386/unistd.h	2004-10-24 11:32:46 +01:00
+++ edited/include/asm-i386/unistd.h	2004-12-20 13:08:41 +00:00
@@ -290,7 +290,7 @@
 #define __NR_mq_getsetattr	(__NR_mq_open+5)
 #define __NR_sys_kexec_load	283
 #define __NR_waitid		284
-/* #define __NR_sys_setaltroot	285 */
+#define __NR_setaltroot		285
 #define __NR_add_key		286
 #define __NR_request_key	287
 #define __NR_keyctl		288
===== include/asm-ia64/unistd.h 1.53 vs edited =====
--- 1.53/include/asm-ia64/unistd.h	2004-11-23 20:18:59 +00:00
+++ edited/include/asm-ia64/unistd.h	2004-12-20 13:08:03 +00:00
@@ -263,6 +263,7 @@
 #define __NR_add_key			1271
 #define __NR_request_key		1272
 #define __NR_keyctl			1273
+#define __NR_setaltroot			1274
 
 #ifdef __KERNEL__
 
===== include/asm-ppc/unistd.h 1.34 vs edited =====
--- 1.34/include/asm-ppc/unistd.h	2004-10-22 10:27:40 +01:00
+++ edited/include/asm-ppc/unistd.h	2004-12-20 13:08:03 +00:00
@@ -276,8 +276,9 @@
 #define __NR_add_key		269
 #define __NR_request_key	270
 #define __NR_keyctl		271
+#define __NR_setaltroot		272
 
-#define __NR_syscalls		272
+#define __NR_syscalls		273
 
 #define __NR(n)	#n
 
===== include/asm-ppc64/unistd.h 1.36 vs edited =====
--- 1.36/include/asm-ppc64/unistd.h	2004-11-16 03:29:08 +00:00
+++ edited/include/asm-ppc64/unistd.h	2004-12-20 13:08:03 +00:00
@@ -282,8 +282,9 @@
 #define __NR_add_key		269
 #define __NR_request_key	270
 #define __NR_keyctl		271
+#define __NR_setaltroot		272
 
-#define __NR_syscalls		272
+#define __NR_syscalls		273
 #ifdef __KERNEL__
 #define NR_syscalls	__NR_syscalls
 #endif
===== include/asm-sparc64/unistd.h 1.36 vs edited =====
--- 1.36/include/asm-sparc64/unistd.h	2004-10-24 10:47:46 +01:00
+++ edited/include/asm-sparc64/unistd.h	2004-12-20 13:09:06 +00:00
@@ -298,7 +298,7 @@
 #define __NR_mq_notify		277
 #define __NR_mq_getsetattr	278
 #define __NR_waitid		279
-/*#define __NR_sys_setaltroot	280 available (was setaltroot) */
+#define __NR_setaltroot		280
 #define __NR_add_key		281
 #define __NR_request_key	282
 #define __NR_keyctl		283
===== include/linux/fs.h 1.362 vs edited =====
--- 1.362/include/linux/fs.h	2004-10-29 09:14:03 +01:00
+++ edited/include/linux/fs.h	2004-12-20 13:08:03 +00:00
@@ -1376,7 +1376,7 @@
 extern int may_open(struct nameidata *, int, int);
 
 extern int kernel_read(struct file *, unsigned long, char *, unsigned long);
-extern struct file * open_exec(const char *);
+extern struct file * open_exec(const char *, int);
  
 /* fs/dcache.c -- generic fs support functions */
 extern int is_subdir(struct dentry *, struct dentry *);
===== include/linux/syscalls.h 1.17 vs edited =====
--- 1.17/include/linux/syscalls.h	2004-10-24 10:47:46 +01:00
+++ edited/include/linux/syscalls.h	2004-12-20 13:08:03 +00:00
@@ -490,6 +490,7 @@
 				void __user *res);
 asmlinkage long sys_syslog(int type, char __user *buf, int len);
 asmlinkage long sys_uselib(const char __user *library);
+asmlinkage long sys_setaltroot(const char __user *altroot);
 asmlinkage long sys_ni_syscall(void);
 
 asmlinkage long sys_add_key(const char __user *_type,

Re: [PATCH] revert- sys_setaltroot

[Andrew Morton]

David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Tue, 2004-10-26 at 15:22 +0000, Linux Kernel Mailing List wrote:
>  > ChangeSet 1.2187, 2004/10/26 08:22:01-07:00, akpm@osdl.org
>  > 
>  > 	[PATCH] revert- sys_setaltroot	
>  > 	We decided to do this a different way.
> 
>  Can you elaborate?

There were security problems and suggestions that a namespace-based
approach would be better.   umm, have a random sprinkle of emails:



Begin forwarded message:

Date: Tue, 19 Oct 2004 13:14:55 -0700
From: "Seth, Rohit" <rohit.seth@intel.com>
To: "Andrew Morton" <akpm@osdl.org>
Cc: <torvalds@osdl.org>
Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT


Seth, Rohit <> wrote on Tuesday, October 19, 2004 10:14 AM:

> Andrew Morton <mailto:akpm@osdl.org> wrote on Tuesday, October 19,
> 2004 12:35 AM: 
> 
>> "Seth, Rohit" <rohit.seth@intel.com> wrote:
>>> 
>>> [sys-altroot4.patch  application/octet-stream (7320 bytes)]
>> 
>> hm.  Should sys_setaltroot() really not require any privileges?
>> 
> 
> As the extent of change is limited to current execution thread (just
> like set_personality) so it is fine to not require any privileges. 
> 

I need to take this back.  There is indeed a security hole that is
introduced by this new system call.  And that is mainly coming because
of the fact that (unlike set_personality) alternate root is maintained
across execs (and also the fact that there is a random string that can
become the alternate root  for a user).  So any program can exec a suid
program and then use the alternate root to exploit system security.

I will be sending you the patch to fix this issue soon.

Thanks, rohit

>> Not that I can see any problems with it, but it needs
>> consideration... 




Begin forwarded message:

Date: Mon, 25 Oct 2004 19:14:05 -0700
From: "Seth, Rohit" <rohit.seth@intel.com>
To: "Andrew Morton" <akpm@osdl.org>, <torvalds@osdl.org>, <viro@parcelfarce.linux.theplanet.co.uk>
Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT


Andrew Morton <mailto:akpm@osdl.org> wrote on Monday, October 25, 2004
12:31 PM:

> "Seth, Rohit" <rohit.seth@intel.com> wrote:
>> 
>> altroot()
> 
> Do we have a decision/direction on this?  I have a
> revert-sys_setaltroot() 
> patch lined up and my trigger finger is itchy!

I know there is already enough said about altroot and the need to
restrict its usage.  Because of the critical need having another stab at
it.  Would it make sense to have different semantics of the new system
call (call it sys_setexecdomain).  Here super-user makes a new system
call to register a new exec_domain.  And as part of this system call, a
new data structure, something that specify a mapping from personality to
alternate root.  Whenever any app makes a new setpersonality system
call, if that correspond to one of these specified then alternate root
takes into affect....much on the same lines that is happening in kernel
today.

Sys_setaltroot, I agree, in its current form should be reverted.

Please let me know what you think about this.

Thanks, rohit




Begin forwarded message:

Date: Tue, 19 Oct 2004 15:32:57 -0700 (PDT)
From: Linus Torvalds <torvalds@osdl.org>
To: "Seth, Rohit" <rohit.seth@intel.com>, Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
Cc: Andrew Morton <akpm@osdl.org>, "David S. Miller" <davem@davemloft.net>
Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT



Hmm. Do we actually want to clear the alternate root? It looks like that 
would make altroot useless.

Who uses it? I thought it was only really used for the emulation 
environments on sparc64 etc. And clearly now ia64. But not saving it 
across an exec would make it useless, no?

		Linus


On Tue, 19 Oct 2004, Seth, Rohit wrote:

> Seth, Rohit <> wrote on Tuesday, October 19, 2004 1:15 PM:
> > I need to take this back.  There is indeed a security hole that is
> > introduced by this new system call.  And that is mainly coming
> > because of the fact that (unlike set_personality) alternate root is
> > maintained across execs (and also the fact that there is a random
> > string that can become the alternate root  for a user).  So any
> > program can exec a suid program and then use the alternate root to
> > exploit system security.      
> > 
> > I will be sending you the patch to fix this issue soon.
> 
> 
> Andrew,
> 
> Please find attached a small patch that fixes the above security issue
> with set_altroot system call.
> 
> Thanks, rohit
> 
> 

----

Previously, altroot was cleared only on crossing exec-domain
boundaries. Specifically, it was not cleared on exec. This has security
implications. This patch resolves the issue by clearing the altroot
on exec.

Signed-off-by: Arun Sharma <arun.sharma@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Rohit Seth <rohit.seth@intel.com>

Index: linux-2.6.8/fs/exec.c
===================================================================
--- linux-2.6.8.orig/fs/exec.c	2004-08-13 22:36:56.000000000 -0700
+++ linux-2.6.8/fs/exec.c	2004-10-19 14:34:25.805323416 -0700
@@ -786,6 +786,14 @@
 	spin_unlock(&files->file_lock);
 }
 
+static inline void clear_altroot(void)
+{
+	dput(current->fs->altroot);
+	mntput(current->fs->altrootmnt);
+	current->fs->altrootmnt = NULL;
+	current->fs->altroot = NULL;
+}
+
 int flush_old_exec(struct linux_binprm * bprm)
 {
 	char * name;
@@ -821,6 +829,8 @@
 	/* This is the point of no return */
 	steal_locks(files);
 	put_files_struct(files);
+	if (current->fs->altroot)
+		clear_altroot();
 
 	current->sas_ss_sp = current->sas_ss_size = 0;
 



Begin forwarded message:

Date: Mon, 25 Oct 2004 19:46:30 -0700 (PDT)
From: Linus Torvalds <torvalds@osdl.org>
To: Andrew Morton <akpm@osdl.org>
Cc: "Seth, Rohit" <rohit.seth@intel.com>, viro@parcelfarce.linux.theplanet.co.uk
Subject: Re: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT




On Mon, 25 Oct 2004, Andrew Morton wrote:
> 
> This is an area in which I have never worked and I don't feel that I'm in a
> position to offer advice - we need to keep poking Linus & Al.

I think it's ok to just revert sys_altroot() for now. The VFS-internal 
altroot usage doesn't seem to be any different from sparc64, so..

> But from a high level one does ask "if the namespace stuff cannot satisfy
> your requirement, then is it broken?".

namespaces are about private mounts, not directly about emulation. altroot
definitely _is_ directly about emulation, and I suspect we could do away
with altroot if we had some kind of "per-personality namespace", which
would switch together with the emulated environment.

The reason I'd like to get rid of altroot (and would still prefer 
something namespace-based) is simply that altroot has these really nasty 
conceptual problems with dual lookups etc. Dammit, but it's an ugly wart.

		Linus




Begin forwarded message:

Date: Fri, 22 Oct 2004 17:28:35 -0700 (PDT)
From: Linus Torvalds <torvalds@osdl.org>
To: "Seth, Rohit" <rohit.seth@intel.com>
Cc: Andrew Morton <akpm@osdl.org>, Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT




On Fri, 22 Oct 2004, Seth, Rohit wrote:
> 
> Could you please let me know how we should proceed with this issue so
> that emulators can seemlessly support different execution domains.

Well, my _personal_ preference by far is to use the equivalent of "chroot" 
for the emulated environment. So whenever you hit a x86 binary, you just 
chroot to /emul, and then you run it there.

NOTE! I say "equivalent", because you could do it by creating a separate
namespace instead, if you wanted to - and just overmounting /lib and
/usr/lib with the emulation environment versions in that particular
namespace. 

Also, note the bind mounts: if you do end up having "/emul", you want to
have the local /lib and /usr/lib be there, but the rest would be just bind
mounts back to the original root, making things pretty similar (ie people
would still see all their normal files in their home directories etc, it
would be just /lib that effectively gets swapped out).

I agree that the altroot() thing is very _convenient_, but it's always 
been an ugly wart on the VFS layer, and so I'd rather get rid of it if we 
can, rather than add users.

Al may have more intelligent suggestions.

		Linus





Begin forwarded message:

Date: Wed, 20 Oct 2004 23:07:57 -0700 (PDT)
From: Linus Torvalds <torvalds@osdl.org>
To: "Seth, Rohit" <rohit.seth@intel.com>
Cc: viro@parcelfarce.linux.theplanet.co.uk, Andrew Morton <akpm@osdl.org>, "David S. Miller" <davem@davemloft.net>
Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT




On Wed, 20 Oct 2004, Seth, Rohit wrote:
> 
> Without the sys_altroot system call, the kernel still has the support of
> providing alternate root.  Though that support is embeded in
> exec_domains.  The new system call support is extending that mechanism
> to normal applications.  In its current form, emulators will be the most
> immediate customers for this system call.

No.

You're not thinking this through.

You not only expose the dang thing with a system call, you also _clear_ it 
at every execve.

Which, as far as I can tell, makes it totally useless for what it used to 
do.

I might be wrong, but the point is that you seem to have appropriated a 
(bad) interface, removing the usefullness from the original use, and 
taking it over for your own uses.

So no, the kernel does _not_ support alternate root any more, afaik. The 
code is there, but the support is gone.

And quite frankly, if we're going to break the only previous user of
altroot, I'd rather remove the feature entirely than replace one horrid
user with another. I bet you can do your emulation thing more cleanly with 
namespaces anyway.

		Linus

Re: [PATCH] revert- sys_setaltroot

[David Woodhouse]

On Wed, 2004-12-22 at 03:03 -0800, Andrew Morton wrote:
> There were security problems and suggestions that a namespace-based
> approach would be better.   umm, have a random sprinkle of emails:

Thanks.

> Begin forwarded message:
> 
> Date: Tue, 19 Oct 2004 15:32:57 -0700 (PDT)
> From: Linus Torvalds <torvalds@osdl.org>
> To: "Seth, Rohit" <rohit.seth@intel.com>, Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
> Cc: Andrew Morton <akpm@osdl.org>, "David S. Miller" <davem@davemloft.net>
> Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT
> 
> 
> 
> Hmm. Do we actually want to clear the alternate root? It looks like that 
> would make altroot useless.
> 
> Who uses it? I thought it was only really used for the emulation 
> environments on sparc64 etc. And clearly now ia64. But not saving it 
> across an exec would make it useless, no?

No, clearing the altroot doesn't make it useless. If we're executing
another non-native binary, the emulator will set the altroot again. This
isn't _supposed_ to be a chroot. It's mostly to keep the dynamic linker
happy, and resetting it on exec is fine. We were doing that anyway when
it was a function of the personality, because executing new binaries
also resets the personality.

I'm using it on ppc to run i386 binaries -- it's certainly not limited
to sparc64 and ia64. 

> Begin forwarded message:
> 
> Date: Fri, 22 Oct 2004 17:28:35 -0700 (PDT)
> From: Linus Torvalds <torvalds@osdl.org>
> To: "Seth, Rohit" <rohit.seth@intel.com>
> Cc: Andrew Morton <akpm@osdl.org>, Al Viro <viro@parcelfarce.linux.theplanet.co.uk>
> Subject: RE: FW: [PATCH] Support ia32 exec domains without CONFIG_IA32_SUPPORT
>
> On Fri, 22 Oct 2004, Seth, Rohit wrote:
> > 
> > Could you please let me know how we should proceed with this issue so
> > that emulators can seemlessly support different execution domains.
> 
> Well, my _personal_ preference by far is to use the equivalent of "chroot" 
> for the emulated environment. So whenever you hit a x86 binary, you just 
> chroot to /emul, and then you run it there.
> 
> NOTE! I say "equivalent", because you could do it by creating a separate
> namespace instead, if you wanted to - and just overmounting /lib and
> /usr/lib with the emulation environment versions in that particular
> namespace. 
> 
> Also, note the bind mounts: if you do end up having "/emul", you want to
> have the local /lib and /usr/lib be there, but the rest would be just bind
> mounts back to the original root, making things pretty similar (ie people
> would still see all their normal files in their home directories etc, it
> would be just /lib that effectively gets swapped out).

I see the point but I'm not sure I understand in detail how this can
work with namespace. It's not just /lib and /usr/lib which exist in the
altroot -- we also need certain files from /etc too, while leaving the
rest of /etc as it was. Do we have to bind-mount just those specific
files? And can we arrange for the alternate namespace to be
automatically dropped on exec() too? And can we do this without needing
the interpreter to be setuid in all cases?

> Al may have more intelligent suggestions.

Hopefully :)

-- 
dwmw2

Re: [PATCH] revert- sys_setaltroot

[David Woodhouse]

On Wed, 2004-12-22 at 03:03 -0800, Andrew Morton wrote:
> There were security problems and suggestions that a namespace-based
> approach would be better.   umm, have a random sprinkle of emails:

The security problems have been addressed, and the namespace-based
approach was never coherently explained. I can't see how such would
work. 

-- 
dwmw2

Re: [PATCH] revert- sys_setaltroot

[Arjan van de Ven]

On Mon, 2005-01-17 at 13:35 +0000, David Woodhouse wrote:
> On Wed, 2004-12-22 at 03:03 -0800, Andrew Morton wrote:
> > There were security problems and suggestions that a namespace-based
> > approach would be better.   umm, have a random sprinkle of emails:
> 
> The security problems have been addressed, and the namespace-based
> approach was never coherently explained. I can't see how such would
> work.

see viro's recent proposal for bindmounts and the like.

Re: [PATCH] revert- sys_setaltroot

[David Woodhouse]

On Mon, 2005-01-17 at 14:42 +0100, Arjan van de Ven wrote:
> > The security problems have been addressed, and the namespace-based
> > approach was never coherently explained. I can't see how such would
> > work.
> 
> see viro's recent proposal for bindmounts and the like.

Is that likely to be usable any time soon?

-- 
dwmw2