[PATCH 00/11] Uml simple fixes

[PATCH 00/11] Uml simple fixes

[Paolo 'Blaisorblade' Giarrusso]

Some tested UML fixes - should be applied for 2.6.21.
-- 
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade

[PATCH 01/11] uml: code convention cleanup of a file

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Fix coding conventions violations is arch/um/os-Linux/helper.c.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/os-Linux/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/um/os-Linux/helper.c b/arch/um/os-Linux/helper.c
index c7ad630..7954357 100644
--- a/arch/um/os-Linux/helper.c
+++ b/arch/um/os-Linux/helper.c
@@ -38,7 +38,7 @@ static int helper_child(void *arg)
 	char **argv = data->argv;
 	int errval;
 
-	if (helper_pause){
+	if (helper_pause) {
 		signal(SIGHUP, helper_hup);
 		pause();
 	}

[PATCH 02/11] uml - hostfs: fix double free

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Fix double free in the error path - when name is assigned into root_inode we do
not own it any more and we must not kfree() it - see patch for details.

Thanks to William Stearns for the initial report.

CC: William Stearns <wstearns@pobox.com>
Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 fs/hostfs/hostfs_kern.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index e965eb1..6f10e43 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -966,6 +966,9 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 		goto out_put;
 
 	HOSTFS_I(root_inode)->host_filename = name;
+	/* Avoid that in the error path, iput(root_inode) frees again name through
+	 * hostfs_destroy_inode! */
+	name = NULL;
 
 	err = -ENOMEM;
 	sb->s_root = d_alloc_root(root_inode);
@@ -977,7 +980,7 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
                 /* No iput in this case because the dput does that for us */
                 dput(sb->s_root);
                 sb->s_root = NULL;
-		goto out_free;
+		goto out;
         }
 
 	return(0);

[PATCH 03/11] uml - hostfs: make hostfs= option work as a jail, as intended.

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

When a given host directory is specified to be mounted both in hostfs=path1 and
with mount option -o path2, we should give access to path1/path2, but this does
not happen. Fix that in the simpler way.

Also, root_ino can be the empty string, since we use %s/%s as format.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 fs/hostfs/hostfs_kern.c |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 6f10e43..9baf697 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -47,7 +47,7 @@ struct dentry_operations hostfs_dentry_ops = {
 };
 
 /* Changed in hostfs_args before the kernel starts running */
-static char *root_ino = "/";
+static char *root_ino = "";
 static int append = 0;
 
 #define HOSTFS_SUPER_MAGIC 0x00c0ffee
@@ -947,15 +947,17 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	sb->s_magic = HOSTFS_SUPER_MAGIC;
 	sb->s_op = &hostfs_sbops;
 
-	if((data == NULL) || (*data == '\0'))
-		data = root_ino;
+	/* NULL is printed as <NULL> by sprintf: avoid that. */
+	if (data == NULL)
+		data = "";
 
 	err = -ENOMEM;
-	name = kmalloc(strlen(data) + 1, GFP_KERNEL);
+	name = kmalloc(strlen(root_ino) + 1
+			+ strlen(data) + 1, GFP_KERNEL);
 	if(name == NULL)
 		goto out;
 
-	strcpy(name, data);
+	sprintf(name, "%s/%s", root_ino, data);
 
 	root_inode = iget(sb, 0);
 	if(root_inode == NULL)

[PATCH 04/11] uml - hostfs: avoid possible escapes from hostfs=.

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Avoid accepting things like -o .., -o dir/../../dir2, -o dir/../.. .
This may be considered useless, but YMMV. I consider that this has a limited
security value, exactly like disabling module support (in many case it is
useful).

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 fs/hostfs/hostfs_kern.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 9baf697..0bcf7ac 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -936,6 +936,28 @@ static const struct address_space_operations hostfs_link_aops = {
 	.readpage	= hostfs_link_readpage,
 };
 
+static inline int str_ends_with(const char * str, const char* suffix)
+{
+	size_t len = strlen(str), suffix_len = strlen(suffix);
+	return strcmp(str + len - suffix_len, suffix) == 0;
+}
+
+static int contains_dotdot(const char* path)
+{
+	/*
+	 * Prevent escaping from hostfs=folder, even if this is not useful to
+	 * jail the UML superuser.
+	 * Since foo..bar is a valid name, we must look for /../ in the string,
+	 * or for ../ at the beginning, /.. at the end, or check whether '..' is
+	 * the complete string.
+	 */
+
+	return  strstr(path, "/../") != NULL ||
+		strcmp(path, "..") == 0 ||
+		strncmp(path, "../", strlen("../")) == 0 ||
+		str_ends_with(path, "/..");
+}
+
 static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 {
 	struct inode *root_inode;
@@ -951,6 +973,10 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	if (data == NULL)
 		data = "";
 
+	err = -EINVAL;
+	if (unlikely(contains_dotdot(data)))
+		goto out;
+
 	err = -ENOMEM;
 	name = kmalloc(strlen(root_ino) + 1
 			+ strlen(data) + 1, GFP_KERNEL);

[PATCH 05/11] hostfs: rename some vars for clarity

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

* rename name to host_root_path
* rename data to req_root.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 fs/hostfs/hostfs_kern.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/hostfs/hostfs_kern.c b/fs/hostfs/hostfs_kern.c
index 0bcf7ac..b4e127b 100644
--- a/fs/hostfs/hostfs_kern.c
+++ b/fs/hostfs/hostfs_kern.c
@@ -961,7 +961,7 @@ static int contains_dotdot(const char* path)
 static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 {
 	struct inode *root_inode;
-	char *name, *data = d;
+	char *host_root_path, *req_root = d;
 	int err;
 
 	sb->s_blocksize = 1024;
@@ -970,20 +970,20 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	sb->s_op = &hostfs_sbops;
 
 	/* NULL is printed as <NULL> by sprintf: avoid that. */
-	if (data == NULL)
-		data = "";
+	if (req_root == NULL)
+		req_root = "";
 
 	err = -EINVAL;
-	if (unlikely(contains_dotdot(data)))
+	if (unlikely(contains_dotdot(req_root)))
 		goto out;
 
 	err = -ENOMEM;
-	name = kmalloc(strlen(root_ino) + 1
-			+ strlen(data) + 1, GFP_KERNEL);
-	if(name == NULL)
+	host_root_path = kmalloc(strlen(root_ino) + 1
+			+ strlen(req_root) + 1, GFP_KERNEL);
+	if(host_root_path == NULL)
 		goto out;
 
-	sprintf(name, "%s/%s", root_ino, data);
+	sprintf(host_root_path, "%s/%s", root_ino, req_root);
 
 	root_inode = iget(sb, 0);
 	if(root_inode == NULL)
@@ -993,10 +993,10 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
 	if(err)
 		goto out_put;
 
-	HOSTFS_I(root_inode)->host_filename = name;
-	/* Avoid that in the error path, iput(root_inode) frees again name through
-	 * hostfs_destroy_inode! */
-	name = NULL;
+	HOSTFS_I(root_inode)->host_filename = host_root_path;
+	/* Avoid that in the error path, iput(root_inode) frees again
+	 * host_root_path through hostfs_destroy_inode! */
+	host_root_path = NULL;
 
 	err = -ENOMEM;
 	sb->s_root = d_alloc_root(root_inode);
@@ -1016,7 +1016,7 @@ static int hostfs_fill_sb_common(struct super_block *sb, void *d, int silent)
  out_put:
         iput(root_inode);
  out_free:
-	kfree(name);
+	kfree(host_root_path);
  out:
 	return(err);
 }

[PATCH 06/11] uml: fix a memory leak in the multicast driver

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Memory allocated by mcast_user_init must be freed in the matching mcast_remove.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/mcast_user.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/um/drivers/mcast_user.c b/arch/um/drivers/mcast_user.c
index 8138f5e..b827e82 100644
--- a/arch/um/drivers/mcast_user.c
+++ b/arch/um/drivers/mcast_user.c
@@ -50,6 +50,14 @@ static void mcast_user_init(void *data, void *dev)
 	pri->dev = dev;
 }
 
+static void mcast_remove(void *data)
+{
+	struct mcast_data *pri = data;
+
+	kfree(pri->mcast_addr);
+	pri->mcast_addr = NULL;
+}
+
 static int mcast_open(void *data)
 {
 	struct mcast_data *pri = data;
@@ -157,7 +165,7 @@ const struct net_user_info mcast_user_info = {
 	.init		= mcast_user_init,
 	.open		= mcast_open,
 	.close	 	= mcast_close,
-	.remove	 	= NULL,
+	.remove	 	= mcast_remove,
 	.set_mtu	= mcast_set_mtu,
 	.add_address	= NULL,
 	.delete_address = NULL,

[PATCH 07/11] uml: remove dead code about os_usr1_signal() and os_usr1_process()

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

os_usr1_signal() is totally unused, os_usr1_process() is used only by TT mode.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/include/os.h       |    3 ++-
 arch/um/os-Linux/process.c |    3 +++
 arch/um/os-Linux/signal.c  |    5 -----
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/um/include/os.h b/arch/um/include/os.h
index 8629bd1..5c74da4 100644
--- a/arch/um/include/os.h
+++ b/arch/um/include/os.h
@@ -192,7 +192,9 @@ extern int os_process_parent(int pid);
 extern void os_stop_process(int pid);
 extern void os_kill_process(int pid, int reap_child);
 extern void os_kill_ptraced_process(int pid, int reap_child);
+#ifdef UML_CONFIG_MODE_TT
 extern void os_usr1_process(int pid);
+#endif
 extern long os_ptrace_ldt(long pid, long addr, long data);
 
 extern int os_getpid(void);
@@ -261,7 +263,6 @@ extern void block_signals(void);
 extern void unblock_signals(void);
 extern int get_signals(void);
 extern int set_signals(int enable);
-extern void os_usr1_signal(int on);
 
 /* trap.c */
 extern void os_fill_handlinfo(struct kern_handlers h);
diff --git a/arch/um/os-Linux/process.c b/arch/um/os-Linux/process.c
index c692a19..76bdd67 100644
--- a/arch/um/os-Linux/process.c
+++ b/arch/um/os-Linux/process.c
@@ -21,6 +21,7 @@
 #include "longjmp.h"
 #include "skas_ptrace.h"
 #include "kern_constants.h"
+#include "uml-config.h"
 
 #define ARBITRARY_ADDR -1
 #define FAILURE_PID    -1
@@ -131,10 +132,12 @@ void os_kill_ptraced_process(int pid, int reap_child)
 		CATCH_EINTR(waitpid(pid, NULL, 0));
 }
 
+#ifdef UML_CONFIG_MODE_TT
 void os_usr1_process(int pid)
 {
 	kill(pid, SIGUSR1);
 }
+#endif
 
 /* Don't use the glibc version, which caches the result in TLS. It misses some
  * syscalls, and also breaks with clone(), which does not unshare the TLS.
diff --git a/arch/um/os-Linux/signal.c b/arch/um/os-Linux/signal.c
index b897e85..2667686 100644
--- a/arch/um/os-Linux/signal.c
+++ b/arch/um/os-Linux/signal.c
@@ -243,8 +243,3 @@ int set_signals(int enable)
 
 	return ret;
 }
-
-void os_usr1_signal(int on)
-{
-	change_sig(SIGUSR1, on);
-}

[PATCH 08/11] uml: mark both consoles as CON_ANYTIME

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Since both UML consoles do not use percpu variables, they may be called when the
cpu is still offline, and they may be marked CON_ANYTIME (this is documented in
kernel/printk.c, grep for CON_ANYTIME to find mentions of this).

Works well in testing done with lock debug enabled, should be safe but is
not needed for next release.

This would probably help also stderr_console.c, but this is yet to test.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/ssl.c           |    2 +-
 arch/um/drivers/stdio_console.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/um/drivers/ssl.c b/arch/um/drivers/ssl.c
index fc22b9b..4b382a6 100644
--- a/arch/um/drivers/ssl.c
+++ b/arch/um/drivers/ssl.c
@@ -179,7 +179,7 @@ static struct console ssl_cons = {
 	.write		= ssl_console_write,
 	.device		= ssl_console_device,
 	.setup		= ssl_console_setup,
-	.flags		= CON_PRINTBUFFER,
+	.flags		= CON_PRINTBUFFER|CON_ANYTIME,
 	.index		= -1,
 };
 
diff --git a/arch/um/drivers/stdio_console.c b/arch/um/drivers/stdio_console.c
index 7ff0b0f..76d1f1c 100644
--- a/arch/um/drivers/stdio_console.c
+++ b/arch/um/drivers/stdio_console.c
@@ -153,7 +153,7 @@ static struct console stdiocons = {
 	.write		= uml_console_write,
 	.device		= uml_console_device,
 	.setup		= uml_console_setup,
-	.flags		= CON_PRINTBUFFER,
+	.flags		= CON_PRINTBUFFER|CON_ANYTIME,
 	.index		= -1,
 };
 

[PATCH 09/11] uml: fix confusion irq early reenabling

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Fix confusion about call context - comments and code are inconsistent and plain
wrong, my fault.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/line.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/um/drivers/line.c b/arch/um/drivers/line.c
index 01d4ab6..f75d7b0 100644
--- a/arch/um/drivers/line.c
+++ b/arch/um/drivers/line.c
@@ -370,10 +370,10 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
 	struct tty_struct *tty = line->tty;
 	int err;
 
-	/* Interrupts are enabled here because we registered the interrupt with
+	/* Interrupts are disabled here because we registered the interrupt with
 	 * IRQF_DISABLED (see line_setup_irq).*/
 
-	spin_lock_irq(&line->lock);
+	spin_lock(&line->lock);
 	err = flush_buffer(line);
 	if (err == 0) {
 		return IRQ_NONE;
@@ -381,7 +381,7 @@ static irqreturn_t line_write_interrupt(int irq, void *data)
 		line->head = line->buffer;
 		line->tail = line->buffer;
 	}
-	spin_unlock_irq(&line->lock);
+	spin_unlock(&line->lock);
 
 	if(tty == NULL)
 		return IRQ_NONE;

[PATCH 10/11] uml - activate_fd: return ENOMEM only when appropriate

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Avoid returning ENOMEM in case of a duplicate IRQ - ENOMEM was saved into err
earlier.

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/kernel/irq.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/um/kernel/irq.c b/arch/um/kernel/irq.c
index 50a288b..dbf2f5b 100644
--- a/arch/um/kernel/irq.c
+++ b/arch/um/kernel/irq.c
@@ -142,6 +142,7 @@ int activate_fd(int irq, int fd, int type, void *dev_id)
 				     .events 		= events,
 				     .current_events 	= 0 } );
 
+	err = -EBUSY;
 	spin_lock_irqsave(&irq_lock, flags);
 	for (irq_fd = active_fds; irq_fd != NULL; irq_fd = irq_fd->next) {
 		if ((irq_fd->fd == fd) && (irq_fd->type == type)) {

[PATCH 11/11] uml: fix errno usage

[Paolo 'Blaisorblade' Giarrusso]

From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>

Avoid reusing userspace errno twice - it can be cleared by libc code everywhere
(in particular printk() does clear it in my setup).

Signed-off-by: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
---

 arch/um/drivers/daemon_user.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/um/drivers/daemon_user.c b/arch/um/drivers/daemon_user.c
index 310af0f..021b82c 100644
--- a/arch/um/drivers/daemon_user.c
+++ b/arch/um/drivers/daemon_user.c
@@ -56,30 +56,31 @@ static int connect_to_switch(struct daemon_data *pri)
 
 	pri->control = socket(AF_UNIX, SOCK_STREAM, 0);
 	if(pri->control < 0){
+		err = -errno;
 		printk("daemon_open : control socket failed, errno = %d\n", 
-		       errno);		
-		return(-errno);
+		       -err);
+		return err;
 	}
 
 	if(connect(pri->control, (struct sockaddr *) ctl_addr, 
 		   sizeof(*ctl_addr)) < 0){
-		printk("daemon_open : control connect failed, errno = %d\n",
-		       errno);
 		err = -errno;
+		printk("daemon_open : control connect failed, errno = %d\n",
+		       -err);
 		goto out;
 	}
 
 	fd = socket(AF_UNIX, SOCK_DGRAM, 0);
 	if(fd < 0){
-		printk("daemon_open : data socket failed, errno = %d\n", 
-		       errno);
 		err = -errno;
+		printk("daemon_open : data socket failed, errno = %d\n",
+		       -err);
 		goto out;
 	}
 	if(bind(fd, (struct sockaddr *) local_addr, sizeof(*local_addr)) < 0){
-		printk("daemon_open : data bind failed, errno = %d\n", 
-		       errno);
 		err = -errno;
+		printk("daemon_open : data bind failed, errno = %d\n",
+		       -err);
 		goto out_close;
 	}
 

Re: [PATCH 01/11] uml: code convention cleanup of a file

[Jeff Dike]

On Mon, Mar 05, 2007 at 09:48:59PM +0100, Paolo 'Blaisorblade' Giarrusso wrote:
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> Fix coding conventions violations is arch/um/os-Linux/helper.c.

The code which you're changing here is gone in current -mm.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [PATCH 04/11] uml - hostfs: avoid possible escapes from hostfs=.

[Jeff Dike]

On Mon, Mar 05, 2007 at 09:49:02PM +0100, Paolo 'Blaisorblade' Giarrusso wrote:
> From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> 
> Avoid accepting things like -o .., -o dir/../../dir2, -o dir/../.. .
> This may be considered useless, but YMMV. I consider that this has a limited
> security value, exactly like disabling module support (in many case it is
> useful).

Two comments on this one:

> +	return  strstr(path, "/../") != NULL ||
> +		strcmp(path, "..") == 0 ||
> +		strncmp(path, "../", strlen("../")) == 0 ||
> +		str_ends_with(path, "/..");

Minor style point - I'd be happier with more parens:

+	return  (strstr(path, "/../") != NULL) ||
+		(strcmp(path, "..") == 0) ||
+		(strncmp(path, "../", strlen("../")) == 0) ||
+		str_ends_with(path, "/..");

C gets operator precedence wrong in one or two cases, so I just put parens
any place it might matter.

Second, there is code in externfs which does the same thing without
parsing paths which you might consider instead.  It sees whether the
requested directory is outside the jail by cd-ing to it and then
repeatedly cd .. until it either reaches / or the jail root.

A copy is below for your reading pleasure.

				Jeff

-- 
Work email - jdike at linux dot intel dot com


/* This does a careful check of whether it's being asked to mount something
 * that's outside the hostfs jail.  It does so by cd-ing to the requested
 * directory and "cd .." until it either reaches / or the jail directory.
 * If it reaches /, then we were outside the jail and we return -EPERM.
 * I do things this way rather than trying to examine the passed-in root
 * to avoid having to parse the string for instances of ".." and the like.
 * Examining the string also doesn't help with symlinks that point outside
 * the jail.  Plus, if there's no parsing, there's no parser to try to exploit.
 */
static int escaping_jail(char *root)
{
	struct uml_stat jail_inode;
	struct uml_stat last_dir, cur_dir;
	int save_pwd, err, escaping = -EPERM;
	const char *path[] = { jail_dir, root, NULL };
	char tmp[HOSTFS_BUFSIZE], *file;

	err = os_stat_file(jail_dir, &jail_inode);
	if(err)
		goto out;

	save_pwd = os_open_file(".", of_read(OPENFLAGS()), 0);
	if(save_pwd < 0)
		goto out;

	file = get_path(path, tmp, sizeof(tmp));
	if(file == NULL)
		goto out_close;

	err = os_change_dir(file);
	if(err)
		goto out_free;

	err = os_stat_file(".", &cur_dir);
	if(err)
		goto out_back;

	do {
		if(SAME_INO(cur_dir, jail_inode)){
			escaping = 0;
			break;
		}

		err = os_change_dir("..");
		if(err)
			goto out_back;

		last_dir = cur_dir;

		err = os_stat_file(".", &cur_dir);
		if(err)
			goto out_back;

	} while(!SAME_INO(last_dir, cur_dir));

	err = os_fchange_dir(save_pwd);
	if(err)
		goto out_close;

	free_path(file, tmp);
	os_close_file(save_pwd);

	return escaping;

out_back:
	os_fchange_dir(save_pwd);
out_free:
	free_path(file, tmp);
out_close:
	os_close_file(save_pwd);
out:
	return err;
}

Re: [PATCH 00/11] Uml simple fixes

[Jeff Dike]

On Mon, Mar 05, 2007 at 09:42:29PM +0100, Paolo 'Blaisorblade' Giarrusso wrote:
> Some tested UML fixes - should be applied for 2.6.21.

ACK on all of these except for 1, which won't apply to current -mm,
and 4, which might need some more work.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [uml-devel] [PATCH 04/11] uml - hostfs: avoid possible escapes from hostfs=.

[Blaisorblade]

On Monday 05 March 2007 23:03, Jeff Dike wrote:
> On Mon, Mar 05, 2007 at 09:49:02PM +0100, Paolo 'Blaisorblade' Giarrusso 
wrote:
> > From: Paolo 'Blaisorblade' Giarrusso <blaisorblade@yahoo.it>
> >
> > Avoid accepting things like -o .., -o dir/../../dir2, -o dir/../.. .
> > This may be considered useless, but YMMV. I consider that this has a
> > limited security value, exactly like disabling module support (in many
> > case it is useful).
>
> Two comments on this one:
> > +	return  strstr(path, "/../") != NULL ||
> > +		strcmp(path, "..") == 0 ||
> > +		strncmp(path, "../", strlen("../")) == 0 ||
> > +		str_ends_with(path, "/..");
>
> Minor style point - I'd be happier with more parens:
>
> +	return  (strstr(path, "/../") != NULL) ||
> +		(strcmp(path, "..") == 0) ||
> +		(strncmp(path, "../", strlen("../")) == 0) ||
> +		str_ends_with(path, "/..");

Hmm. Personally I prefer the earlier style, but I haven't the last word on 
this.

> C gets operator precedence wrong in one or two cases, so I just put parens
> any place it might matter.

Indeed. For instance this patch is wrong, I did this once in a patch, and I 
saw it another time in current Ubuntu kernels:

-	a + b / 4
+	a + b >> 2

This is instead needed:

+	a + (b >> 2)

> Second, there is code in externfs which does the same thing without
> parsing paths which you might consider instead.

I must, indeed - your comment points out the symlink issue, which I didn't 
think of, and which makes my patch moot.

> It sees whether the 
> requested directory is outside the jail by cd-ing to it and then
> repeatedly cd .. until it either reaches / or the jail root.
>
> A copy is below for your reading pleasure.

I gave a look, and it's nice. Except that maybe "escapes_jail" would be a 
clearer name (there's confusion about the subject of "escaping").

Also, what about concurrent UML threads caring about current directory? I know 
that without SMP/preemption we can't have this problem, but when I see this I 
count a future bug unless this is checked (I think I reason mathematically 
about correctness, even if not formally).

And I haven't the time to check this - I think your version could be merged 
anyway, but I'm not sure.

> 				Jeff

-- 
Inform me of my mistakes, so I can add them to my list!
Paolo Giarrusso, aka Blaisorblade
http://www.user-mode-linux.org/~blaisorblade
Chiacchiera con i tuoi amici in tempo reale! 
 http://it.yahoo.com/mail_it/foot/*http://it.messenger.yahoo.com 

Re: [uml-devel] [PATCH 04/11] uml - hostfs: avoid possible escapes from hostfs=.

[Jeff Dike]

On Mon, Mar 05, 2007 at 11:59:13PM +0100, Blaisorblade wrote:
> I gave a look, and it's nice. Except that maybe "escapes_jail" would be a 
> clearer name (there's confusion about the subject of "escaping").

Feel free to change the name.

> Also, what about concurrent UML threads caring about current directory? I
> know that without SMP/preemption we can't have this problem, but
> when I see this I count a future bug unless this is checked (I think I
> reason mathematically about correctness, even if not formally).

Good point, yes this does need fixing for SMP.

				Jeff

-- 
Work email - jdike at linux dot intel dot com