[PATCH] mq_open() honor leading slash

[PATCH] mq_open() honor leading slash

[Chris Wright]

SUSv3 requires the following on mq_open() with name containing leading
slash:

	If name begins with the slash character, then processes
	calling mq_open() with the same value of name shall refer to
	the same message queue object, as long as that name has not
	been removed. If name does not begin with the slash character,
	the effect is implementation-defined.  The interpretation of
	slash characters other than the leading slash character in name
	is implementation-defined.

The use of lookup_one_len() to force all lookups relative to the
mqueue_mnt root guarantees absolute rather than relative lookup without
leading slash, and generates an error on a name that contains any slashes
at all.  This is inconsitent with the part of the spec that requires
leading slash to be effectively an absolute path (unless you consider
-EACCES being "the same message queue object" ;-)

Patch below simply eats all leading slashes before passing name to
lookup_one_len() in mq_open() and mq_unlink().

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

===== ipc/mqueue.c 1.8 vs edited =====
--- 1.8/ipc/mqueue.c	Wed Apr 14 18:37:51 2004
+++ edited/ipc/mqueue.c	Thu Apr 15 10:31:59 2004
@@ -609,7 +609,7 @@
 {
 	struct dentry *dentry;
 	struct file *filp;
-	char *name;
+	char *name, *qname;
 	int fd, error;
 
 	if (IS_ERR(name = getname(u_name)))
@@ -619,8 +619,11 @@
 	if (fd < 0)
 		goto out_putname;
 
+	qname = name;
+	while (*qname == '/')
+		qname++;
 	down(&mqueue_mnt->mnt_root->d_inode->i_sem);
-	dentry = lookup_one_len(name, mqueue_mnt->mnt_root, strlen(name));
+	dentry = lookup_one_len(qname, mqueue_mnt->mnt_root, strlen(qname));
 	if (IS_ERR(dentry)) {
 		error = PTR_ERR(dentry);
 		goto out_err;
@@ -665,7 +668,7 @@
 asmlinkage long sys_mq_unlink(const char __user *u_name)
 {
 	int err;
-	char *name;
+	char *name, *qname;
 	struct dentry *dentry;
 	struct inode *inode = NULL;
 
@@ -673,8 +676,11 @@
 	if (IS_ERR(name))
 		return PTR_ERR(name);
 
+	qname = name;
+	while (*qname == '/')
+		qname++;
 	down(&mqueue_mnt->mnt_root->d_inode->i_sem);
-	dentry = lookup_one_len(name, mqueue_mnt->mnt_root, strlen(name));
+	dentry = lookup_one_len(qname, mqueue_mnt->mnt_root, strlen(qname));
 	if (IS_ERR(dentry)) {
 		err = PTR_ERR(dentry);
 		goto out_unlock;

Re: [PATCH] mq_open() honor leading slash

[Jakub Jelinek]

On Thu, Apr 15, 2004 at 11:39:51AM -0700, Chris Wright wrote:
> SUSv3 requires the following on mq_open() with name containing leading
> slash:
> 
> 	If name begins with the slash character, then processes
> 	calling mq_open() with the same value of name shall refer to
> 	the same message queue object, as long as that name has not
> 	been removed. If name does not begin with the slash character,
> 	the effect is implementation-defined.  The interpretation of
> 	slash characters other than the leading slash character in name
> 	is implementation-defined.
> 
> The use of lookup_one_len() to force all lookups relative to the
> mqueue_mnt root guarantees absolute rather than relative lookup without
> leading slash, and generates an error on a name that contains any slashes
> at all.  This is inconsitent with the part of the spec that requires
> leading slash to be effectively an absolute path (unless you consider
> -EACCES being "the same message queue object" ;-)
> 
> Patch below simply eats all leading slashes before passing name to
> lookup_one_len() in mq_open() and mq_unlink().

glibc already strips the leading slash in userland.
If you want to do it in the kernel instead, it shouldn't IMHO be silent if it
doesn't see a leading slash or sees more than one. I.e.
	error = -EINVAL;
	if (name[0] != '/')
		goto out_err;
...
	dentry = lookup_one_len(name + 1, mqueue_mnt->mnt_root, strlen(name + 1));

	Jakub

Re: [PATCH] mq_open() honor leading slash

[Chris Wright]

* Jakub Jelinek (jakub@redhat.com) wrote:
> On Thu, Apr 15, 2004 at 11:39:51AM -0700, Chris Wright wrote:
> > Patch below simply eats all leading slashes before passing name to
> > lookup_one_len() in mq_open() and mq_unlink().
> 
> glibc already strips the leading slash in userland.

Ah, OK.  I'm just using the kernel interfaces directly.

> If you want to do it in the kernel instead, it shouldn't IMHO be silent if it
> doesn't see a leading slash or sees more than one. I.e.
> 	error = -EINVAL;
> 	if (name[0] != '/')
> 		goto out_err;

Given it's implementation defined what happens w/out leading slash, I
see no trouble with allowing names w/out leading slashes to be looked
up as well as names with leading slash (in kernel rather than glibc).
But I don't feel strongly either way.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Re: [PATCH] mq_open() honor leading slash

[Manfred Spraul]

Chris Wright wrote:

>Patch below simply eats all leading slashes before passing name to
>lookup_one_len() in mq_open() and mq_unlink().
>  
>
Why should we do that in kernel space?
The kernel interface is "no slash at all". User space can add a SuS 
compatible layer on top of the kernel interface (i.e. fail with -EINVAL 
if the first character is not a /, then skip the slash and pass "name+1" 
to sys_mq_open()).

--
    Manfred

Re: [PATCH] mq_open() honor leading slash

[Chris Wright]

* Manfred Spraul (manfred@colorfullife.com) wrote:
> Chris Wright wrote:
> >Patch below simply eats all leading slashes before passing name to
> >lookup_one_len() in mq_open() and mq_unlink().
> >
> Why should we do that in kernel space?
> The kernel interface is "no slash at all". User space can add a SuS 
> compatible layer on top of the kernel interface (i.e. fail with -EINVAL 
> if the first character is not a /, then skip the slash and pass "name+1" 
> to sys_mq_open()).

Jakub said similar, and I missed the fact that glibc was sanitizing
before calling kernel.  I agree, it's fine as is.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net