Message-ID: <4EDD7F46.1050907@canonical.com>
Date:	Mon, 05 Dec 2011 18:34:46 -0800
From:	John Johansen <john.johansen@...onical.com>
To:	Al Viro <viro@...IV.linux.org.uk>
CC:	linux-kernel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root
 and fix a couple of bugs in d_namespace_path)

On 12/04/2011 07:27 PM, Al Viro wrote:
> 	I think I have a saner solution; the root of the problem is that
> __d_path() API is asking for trouble.  What I'm proposing is:
> 
> 	* Give prepend_path() an extra argument for reporting which
> fatherless thing has it reached if it has missed the root - struct
> path *bastard.  struct path *root becomes const on that *and* what
> we put into *bastard is properly grabbed.  We do that only if that
> pointer is not NULL, of course.  In any case we return 1 if we go
> out through global_root:
> 
> 	* modify the callers accordingly; everything except __d_path()
> will simply pass NULL as that extra argument and instead of checking
> if root has been changed, just check if return value was positive.
> 
> 	* __d_path() gets the similar extra argument itself; if it's non-NULL
> and we miss the root, the caller can expect to get (referenced) point where
> the walk has stopped stored there.  _Maybe_ it ought to fill it with
> NULL/NULL otherwise; I've just done that in the only such caller before
> calling __d_path().  If it is NULL *and* root has not been NULL/NULL (i.e.
> we asked for subtree explicitly and have nowhere to put the stopping point),
> we return NULL instead of pointer to relative pathname.
> 
> 	* seq_path_root() does _NOT_ return -ENAMETOOLONG (it's stupid anyway -
> the normal seq_file logics will take care of growing the buffer and redoing
> the call of ->show() just fine).  However, if it gets path not reachable
> from root, it returns SEQ_SKIP.  The only caller adjusted (i.e. stopped
> ignoring the return value as it used to do).
> 
> 	* unlike seq_path_root(), the caller in tomoyo passes NULL/NULL in
> *root (it wants absolute path).  It still calls with bastard == NULL, so
> no references are grabbed.
> 
> 	* apparmor d_namespace_path() calls __d_path() passing it a pointer
> to local struct path to fill.  It's been earlier initialized to NULL/NULL
> (see above in part about __d_path()), so the check for "has it been outside
> of chroot jail" is simply bastard.mnt != NULL after the call.  Moreover,
> in this case it's safe to access vfsmount and dentry; we can do the checks
> just fine, they won't be freed under us.  One more thing - I've exported
> uninlined variant of check_mnt() (called our_mnt(), for obvious reasons)
> and used it here.
> 
> Completely untested patch follows:
> 
Thanks Al, this looks good, and I have been running this one a test box doing
regression testing successfully for hours with one small change in the apparmor
portion of the code which I have included below.

With the apparmor change you can add

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

> Signed-off-by: Al Viro <viro@...iv.linux.org.uk>
> ---

<< snip >>

> --- a/security/apparmor/path.c
> +++ b/security/apparmor/path.c
> @@ -57,23 +57,16 @@ 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;
> +	struct path root = {};
> +	struct path bastard = {};
>  	char *res;
> -	int connected, error = 0;
> +	int error = 0;
>  
> -	/* Get the root we want to resolve too, released below */
> -	if (flags & PATH_CHROOT_REL) {
> -		/* resolve paths relative to chroot */
> +	/* resolve paths relative to chroot?*/
> +	if (flags & PATH_CHROOT_REL)
>  		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);
> -	}
>  
> -	tmp = root;
> -	res = __d_path(path, &tmp, buf, buflen);
> +	res = __d_path(path, &root, &bastard, buf, buflen);
>  
>  	*name = res;
>  	/* handle error conditions - and still allow a partial path to
> @@ -97,10 +90,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
> @@ -111,9 +101,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  	 *     of chroot) and specifically directed to connect paths to
>  	 *     namespace root.
>  	 */
> -	if (!connected) {
> +	if (bastard.mnt) {
This doesn't quite match the original !connected check.  It is the same for the case of
flags & PATH_CHROOT_REL (ie. root = current->fs), but for the case where we pass root = { }
bastard.mnt is always true, while in the old !connected case, it would only be true if
!our_mnt(bastard).

This isn't entirely covered by the subsequent check
			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
			     our_mnt(bastard.mnt))) {

which means apparmor ends up treating the path as disconnected when it shouldn't.
This can be fixed by changing the check to

-	if (!connected) {
+	if (bastard.mnt && (root.mnt || !our_mnt(bastard.mnt)) {

I will send a separate patch to kill
			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
			     our_mnt(bastard.mnt))

as its an unused vestige which isn't even possible, and was missed being removed during
code cleanup.

>  		/* is the disconnect path a sysctl? */
> -		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
> +		if (bastard.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
> @@ -121,8 +111,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  			error = prepend(name, *name - buf, "/proc", 5);
>  		} else 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(bastard.mnt))) {
>  			/* disconnected path, don't return pathname starting
>  			 * with '/'
>  			 */
> @@ -133,7 +122,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  	}
>  
>  out:
> -	path_put(&root);
> +	if (bastard.mnt)
> +		path_put(&bastard);
> +	if (flags & PATH_CHROOT_REL)
> +		path_put(&root);
>  
>  	return error;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists