Re: [git pull] apparmor fix for __d_path() misuse

[John Johansen]

On 12/06/2011 07:11 PM, John Johansen wrote:
> On 12/06/2011 05:37 PM, Al Viro wrote:
>> On Wed, Dec 07, 2011 at 01:10:47AM +0000, Al Viro wrote:
>>
>>> So let's add d_absolute_path(path, buf, buflen).  Having it check that
>>> we'd walked to something mounted.  And returning NULL otherwise.  _Never_
>>
>> Turns out that returning ERR_PTR(-EINVAL) is more convenient.
>> All right, here's a variant that does *NOT* return any vfsmount/dentry
>> pointers at all.  And it does best-sane-effort wrt returning the pathname;
>> i.e. if it *is* something outside of chroot, that absolute pathname is
>> what we'll get.
>>
> I haven't had a chance to build and test yet but this looks good to me.  I do
> think splitting the two uses cases makes sense.  I am building a kernel now
> and will update when I am done with it.
> 
Okay its good, certainly better than what was there, I will work on cleaning up
the apparmor bits, especially the sysctl mess.

Reviewed-by: John Johansen <john.johansen@xxxxxxxxxxxxx>

> 
> 
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 10ba92d..89509b5 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -2439,16 +2439,14 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
>>  /**
>>   * prepend_path - Prepend path string to a buffer
>>   * @path: the dentry/vfsmount to report
>> - * @root: root vfsmnt/dentry (may be modified by this function)
>> + * @root: root vfsmnt/dentry
>>   * @buffer: pointer to the end of the buffer
>>   * @buflen: pointer to buffer length
>>   *
>>   * Caller holds the rename_lock.
>> - *
>> - * If path is not reachable from the supplied root, then the value of
>> - * root is changed (without modifying refcounts).
>>   */
>> -static int prepend_path(const struct path *path, struct path *root,
>> +static int prepend_path(const struct path *path,
>> +			const struct path *root,
>>  			char **buffer, int *buflen)
>>  {
>>  	struct dentry *dentry = path->dentry;
>> @@ -2483,10 +2481,10 @@ static int prepend_path(const struct path *path, struct path *root,
>>  		dentry = parent;
>>  	}
>>  
>> -out:
>>  	if (!error && !slash)
>>  		error = prepend(buffer, buflen, "/", 1);
>>  
>> +out:
>>  	br_read_unlock(vfsmount_lock);
>>  	return error;
>>  
>> @@ -2500,15 +2498,17 @@ global_root:
>>  		WARN(1, "Root dentry has weird name <%.*s>\n",
>>  		     (int) dentry->d_name.len, dentry->d_name.name);
>>  	}
>> -	root->mnt = vfsmnt;
>> -	root->dentry = dentry;
>> +	if (!slash)
>> +		error = prepend(buffer, buflen, "/", 1);
>> +	if (!error)
>> +		error = vfsmnt->mnt_ns ? 1 : 2;
>>  	goto out;
>>  }
>>  
>>  /**
>>   * __d_path - return the path of a dentry
>>   * @path: the dentry/vfsmount to report
>> - * @root: root vfsmnt/dentry (may be modified by this function)
>> + * @root: root vfsmnt/dentry
>>   * @buf: buffer to return value in
>>   * @buflen: buffer length
>>   *
>> @@ -2519,10 +2519,10 @@ global_root:
>>   *
>>   * "buflen" should be positive.
>>   *
>> - * If path is not reachable from the supplied root, then the value of
>> - * root is changed (without modifying refcounts).
>> + * If the path is not reachable from the supplied root, return %NULL.
>>   */
>> -char *__d_path(const struct path *path, struct path *root,
>> +char *__d_path(const struct path *path,
>> +	       const struct path *root,
>>  	       char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>> @@ -2533,7 +2533,28 @@ char *__d_path(const struct path *path, struct path *root,
>>  	error = prepend_path(path, root, &res, &buflen);
>>  	write_sequnlock(&rename_lock);
>>  
>> -	if (error)
>> +	if (error < 0)
>> +		return ERR_PTR(error);
>> +	if (error > 0)
>> +		return NULL;
>> +	return res;
>> +}
>> +
>> +char *d_absolute_path(const struct path *path,
>> +	       char *buf, int buflen)
>> +{
>> +	struct path root = {};
>> +	char *res = buf + buflen;
>> +	int error;
>> +
>> +	prepend(&res, &buflen, "\0", 1);
>> +	write_seqlock(&rename_lock);
>> +	error = prepend_path(path, &root, &res, &buflen);
>> +	write_sequnlock(&rename_lock);
>> +
>> +	if (error > 1)
>> +		error = -EINVAL;
>> +	if (error < 0)
>>  		return ERR_PTR(error);
>>  	return res;
>>  }
>> @@ -2541,8 +2562,9 @@ char *__d_path(const struct path *path, struct path *root,
>>  /*
>>   * same as __d_path but appends "(deleted)" for unlinked files.
>>   */
>> -static int path_with_deleted(const struct path *path, struct path *root,
>> -				 char **buf, int *buflen)
>> +static int path_with_deleted(const struct path *path,
>> +			     const struct path *root,
>> +			     char **buf, int *buflen)
>>  {
>>  	prepend(buf, buflen, "\0", 1);
>>  	if (d_unlinked(path->dentry)) {
>> @@ -2579,7 +2601,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>>  	struct path root;
>> -	struct path tmp;
>>  	int error;
>>  
>>  	/*
>> @@ -2594,9 +2615,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
>>  
>>  	get_fs_root(current->fs, &root);
>>  	write_seqlock(&rename_lock);
>> -	tmp = root;
>> -	error = path_with_deleted(path, &tmp, &res, &buflen);
>> -	if (error)
>> +	error = path_with_deleted(path, &root, &res, &buflen);
>> +	if (error < 0)
>>  		res = ERR_PTR(error);
>>  	write_sequnlock(&rename_lock);
>>  	path_put(&root);
>> @@ -2617,7 +2637,6 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>>  	struct path root;
>> -	struct path tmp;
>>  	int error;
>>  
>>  	if (path->dentry->d_op && path->dentry->d_op->d_dname)
>> @@ -2625,9 +2644,8 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>>  
>>  	get_fs_root(current->fs, &root);
>>  	write_seqlock(&rename_lock);
>> -	tmp = root;
>> -	error = path_with_deleted(path, &tmp, &res, &buflen);
>> -	if (!error && !path_equal(&tmp, &root))
>> +	error = path_with_deleted(path, &root, &res, &buflen);
>> +	if (error > 0)
>>  		error = prepend_unreachable(&res, &buflen);
>>  	write_sequnlock(&rename_lock);
>>  	path_put(&root);
>> @@ -2758,19 +2776,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
>>  	write_seqlock(&rename_lock);
>>  	if (!d_unlinked(pwd.dentry)) {
>>  		unsigned long len;
>> -		struct path tmp = root;
>>  		char *cwd = page + PAGE_SIZE;
>>  		int buflen = PAGE_SIZE;
>>  
>>  		prepend(&cwd, &buflen, "\0", 1);
>> -		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
>> +		error = prepend_path(&pwd, &root, &cwd, &buflen);
>>  		write_sequnlock(&rename_lock);
>>  
>> -		if (error)
>> +		if (error < 0)
>>  			goto out;
>>  
>>  		/* Unreachable from current root */
>> -		if (!path_equal(&tmp, &root)) {
>> +		if (error > 0) {
>>  			error = prepend_unreachable(&cwd, &buflen);
>>  			if (error)
>>  				goto out;
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 6d3a196..cfc6d44 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -1048,15 +1048,12 @@ static int show_mountinfo(struct seq_file *m, void *v)
>>  	if (err)
>>  		goto out;
>>  	seq_putc(m, ' ');
>> -	seq_path_root(m, &mnt_path, &root, " \t\n\\");
>> -	if (root.mnt != p->root.mnt || root.dentry != p->root.dentry) {
>> -		/*
>> -		 * Mountpoint is outside root, discard that one.  Ugly,
>> -		 * but less so than trying to do that in iterator in a
>> -		 * race-free way (due to renames).
>> -		 */
>> -		return SEQ_SKIP;
>> -	}
>> +
>> +	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
>> +	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
>> +	if (err)
>> +		goto out;
>> +
>>  	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
>>  	show_mnt_opts(m, mnt);
>>  
>> @@ -2776,3 +2773,8 @@ void kern_unmount(struct vfsmount *mnt)
>>  	}
>>  }
>>  EXPORT_SYMBOL(kern_unmount);
>> +
>> +bool our_mnt(struct vfsmount *mnt)
>> +{
>> +	return check_mnt(mnt);
>> +}
>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>> index 05d6b0e..dba43c3 100644
>> --- a/fs/seq_file.c
>> +++ b/fs/seq_file.c
>> @@ -449,8 +449,6 @@ EXPORT_SYMBOL(seq_path);
>>  
>>  /*
>>   * Same as seq_path, but relative to supplied root.
>> - *
>> - * root may be changed, see __d_path().
>>   */
>>  int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  		  char *esc)
>> @@ -463,6 +461,8 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  		char *p;
>>  
>>  		p = __d_path(path, root, buf, size);
>> +		if (!p)
>> +			return SEQ_SKIP;
>>  		res = PTR_ERR(p);
>>  		if (!IS_ERR(p)) {
>>  			char *end = mangle_path(buf, p, esc);
>> @@ -474,7 +474,7 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  	}
>>  	seq_commit(m, res);
>>  
>> -	return res < 0 ? res : 0;
>> +	return res < 0 && res != -ENAMETOOLONG ? res : 0;
>>  }
>>  
>>  /*
>> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
>> index 4df9261..ed9f74f 100644
>> --- a/include/linux/dcache.h
>> +++ b/include/linux/dcache.h
>> @@ -339,7 +339,8 @@ extern int d_validate(struct dentry *, struct dentry *);
>>   */
>>  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
>>  
>> -extern char *__d_path(const struct path *path, struct path *root, char *, int);
>> +extern char *__d_path(const struct path *, const struct path *, char *, int);
>> +extern char *d_absolute_path(const struct path *, char *, int);
>>  extern char *d_path(const struct path *, char *, int);
>>  extern char *d_path_with_unreachable(const struct path *, char *, int);
>>  extern char *dentry_path_raw(struct dentry *, char *, int);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e313022..019dc55 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1942,6 +1942,7 @@ extern int fd_statfs(int, struct kstatfs *);
>>  extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
>>  extern int freeze_super(struct super_block *super);
>>  extern int thaw_super(struct super_block *super);
>> +extern bool our_mnt(struct vfsmount *mnt);
>>  
>>  extern int current_umask(void);
>>  
>> diff --git a/security/apparmor/path.c b/security/apparmor/path.c
>> index 36cc0cc..b566eba 100644
>> --- a/security/apparmor/path.c
>> +++ b/security/apparmor/path.c
>> @@ -57,23 +57,44 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen)
>>  static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  			    char **name, int flags)
>>  {
>> -	struct path root, tmp;
>>  	char *res;
>> -	int connected, error = 0;
>> +	int error = 0;
>> +	int connected = 1;
>> +
>> +	if (path->mnt->mnt_flags & MNT_INTERNAL) {
>> +		/* it's not mounted anywhere */
>> +		res = dentry_path(path->dentry, buf, buflen);
>> +		*name = res;
>> +		if (IS_ERR(res)) {
>> +			*name = buf;
>> +			return PTR_ERR(res);
>> +		}
>> +		if (path->dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
>> +		    strncmp(*name, "/sys/", 5) == 0) {
>> +			/* TODO: convert over to using a per namespace
>> +			 * control instead of hard coded /proc
>> +			 */
>> +			return prepend(name, *name - buf, "/proc", 5);
>> +		}
>> +		return 0;
>> +	}
>>  
>> -	/* Get the root we want to resolve too, released below */
>> +	/* resolve paths relative to chroot?*/
>>  	if (flags & PATH_CHROOT_REL) {
>> -		/* resolve paths relative to chroot */
>> +		struct path root;
>>  		get_fs_root(current->fs, &root);
>> -	} else {
>> -		/* resolve paths relative to namespace */
>> -		root.mnt = current->nsproxy->mnt_ns->root;
>> -		root.dentry = root.mnt->mnt_root;
>> -		path_get(&root);
>> +		res = __d_path(path, &root, buf, buflen);
>> +		if (res && !IS_ERR(res)) {
>> +			/* everything's fine */
>> +			*name = res;
>> +			path_put(&root);
>> +			goto ok;
>> +		}
>> +		path_put(&root);
>> +		connected = 0;
>>  	}
>>  
>> -	tmp = root;
>> -	res = __d_path(path, &tmp, buf, buflen);
>> +	res = d_absolute_path(path, buf, buflen);
>>  
>>  	*name = res;
>>  	/* handle error conditions - and still allow a partial path to
>> @@ -84,7 +105,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  		*name = buf;
>>  		goto out;
>>  	}
>> +	if (!our_mnt(path->mnt))
>> +		connected = 0;
>>  
>> +ok:
>>  	/* Handle two cases:
>>  	 * 1. A deleted dentry && profile is not allowing mediation of deleted
>>  	 * 2. On some filesystems, newly allocated dentries appear to the
>> @@ -97,10 +121,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  			goto out;
>>  	}
>>  
>> -	/* Determine if the path is connected to the expected root */
>> -	connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
>> -
>> -	/* If the path is not connected,
>> +	/* If the path is not connected to the expected root,
>>  	 * check if it is a sysctl and handle specially else remove any
>>  	 * leading / that __d_path may have returned.
>>  	 * Unless
>> @@ -112,17 +133,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  	 *     namespace root.
>>  	 */
>>  	if (!connected) {
>> -		/* is the disconnect path a sysctl? */
>> -		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
>> -		    strncmp(*name, "/sys/", 5) == 0) {
>> -			/* TODO: convert over to using a per namespace
>> -			 * control instead of hard coded /proc
>> -			 */
>> -			error = prepend(name, *name - buf, "/proc", 5);
>> -		} else if (!(flags & PATH_CONNECT_PATH) &&
>> +		if (!(flags & PATH_CONNECT_PATH) &&
>>  			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
>> -			     (tmp.mnt == current->nsproxy->mnt_ns->root &&
>> -			      tmp.dentry == tmp.mnt->mnt_root))) {
>> +			     our_mnt(path->mnt))) {
>>  			/* disconnected path, don't return pathname starting
>>  			 * with '/'
>>  			 */
>> @@ -133,8 +146,6 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  	}
>>  
>>  out:
>> -	path_put(&root);
>> -
>>  	return error;
>>  }
>>  
>> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
>> index 738bbdf..36fa7c9 100644
>> --- a/security/tomoyo/realpath.c
>> +++ b/security/tomoyo/realpath.c
>> @@ -101,9 +101,8 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
>>  {
>>  	char *pos = ERR_PTR(-ENOMEM);
>>  	if (buflen >= 256) {
>> -		struct path ns_root = { };
>>  		/* go to whatever namespace root we are under */
>> -		pos = __d_path(path, &ns_root, buffer, buflen - 1);
>> +		pos = d_absolute_path(path, buffer, buflen - 1);
>>  		if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
>>  			struct inode *inode = path->dentry->d_inode;
>>  			if (inode && S_ISDIR(inode->i_mode)) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/