[RFC][PATCH] security/selinux: Simplify proc inode to security label mapping.

[RFC][PATCH] security/selinux: Simplify proc inode to security label mapping.

[Eric W. Biederman]

Currently selinux has incestuous knowledge of the implementation details
of procfs and sysctl that it uses to get a pathname from an inode. As it
happens the point we care is in the security_d_instantiate lsm hook so
we have a valid dentry that we can use to get the entire pathname on
the proc filesystem.  With the recent change to sys_sysctl to go through
proc/sys all proc and sysctl accesses go through the vfs, which
means we no longer need a sysctl special case.

So get the path for the dentry, remove the incestuous knowledge
and simplify the code.

caveat: Because the dentry may not yet be hashed I think dentry_path will
        append (deleted) and thus is not the right function to call.

Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 security/selinux/hooks.c |  114 ++++-----------------------------------------
 1 files changed, 11 insertions(+), 103 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index bb230d5..37ed36e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -43,7 +43,6 @@
 #include <linux/fdtable.h>
 #include <linux/namei.h>
 #include <linux/mount.h>
-#include <linux/proc_fs.h>
 #include <linux/netfilter_ipv4.h>
 #include <linux/netfilter_ipv6.h>
 #include <linux/tty.h>
@@ -70,7 +69,6 @@
 #include <net/ipv6.h>
 #include <linux/hugetlb.h>
 #include <linux/personality.h>
-#include <linux/sysctl.h>
 #include <linux/audit.h>
 #include <linux/string.h>
 #include <linux/selinux.h>
@@ -1178,39 +1176,27 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
 }
 
 #ifdef CONFIG_PROC_FS
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
-	int buflen, rc;
-	char *buffer, *path, *end;
+	int rc;
+	char *buffer, *path;
 
 	buffer = (char *)__get_free_page(GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (de && de != de->parent) {
-		buflen -= de->namelen + 1;
-		if (buflen < 0)
-			break;
-		end -= de->namelen;
-		memcpy(end, de->name, de->namelen);
-		*--end = '/';
-		path = end;
-		de = de->parent;
-	}
-	rc = security_genfs_sid("proc", path, tclass, sid);
+	path = dentry_path(dentry, buffer, PAGE_SIZE);
+	if (IS_ERR(path))
+		rc = PTR_ERR(path);
+	else
+		rc = security_genfs_sid("proc", path, tclass, sid);
 	free_page((unsigned long)buffer);
 	return rc;
 }
 #else
-static int selinux_proc_get_sid(struct proc_dir_entry *de,
+static int selinux_proc_get_sid(struct dentry *dentry,
 				u16 tclass,
 				u32 *sid)
 {
@@ -1374,10 +1360,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
 		isec->sid = sbsec->sid;
 
 		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
-			struct proc_inode *proci = PROC_I(inode);
-			if (proci->pde) {
+			if (opt_dentry) {
 				isec->sclass = inode_mode_to_security_class(inode->i_mode);
-				rc = selinux_proc_get_sid(proci->pde,
+				rc = selinux_proc_get_sid(opt_dentry,
 							  isec->sclass,
 							  &sid);
 				if (rc)
@@ -1939,82 +1924,6 @@ static int selinux_capable(struct task_struct *tsk, const struct cred *cred,
 	return task_has_capability(tsk, cred, cap, audit);
 }
 
-static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
-{
-	int buflen, rc;
-	char *buffer, *path, *end;
-
-	rc = -ENOMEM;
-	buffer = (char *)__get_free_page(GFP_KERNEL);
-	if (!buffer)
-		goto out;
-
-	buflen = PAGE_SIZE;
-	end = buffer+buflen;
-	*--end = '\0';
-	buflen--;
-	path = end-1;
-	*path = '/';
-	while (table) {
-		const char *name = table->procname;
-		size_t namelen = strlen(name);
-		buflen -= namelen + 1;
-		if (buflen < 0)
-			goto out_free;
-		end -= namelen;
-		memcpy(end, name, namelen);
-		*--end = '/';
-		path = end;
-		table = table->parent;
-	}
-	buflen -= 4;
-	if (buflen < 0)
-		goto out_free;
-	end -= 4;
-	memcpy(end, "/sys", 4);
-	path = end;
-	rc = security_genfs_sid("proc", path, tclass, sid);
-out_free:
-	free_page((unsigned long)buffer);
-out:
-	return rc;
-}
-
-static int selinux_sysctl(ctl_table *table, int op)
-{
-	int error = 0;
-	u32 av;
-	u32 tsid, sid;
-	int rc;
-
-	sid = current_sid();
-
-	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
-				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
-	if (rc) {
-		/* Default to the well-defined sysctl SID. */
-		tsid = SECINITSID_SYSCTL;
-	}
-
-	/* The op values are "defined" in sysctl.c, thereby creating
-	 * a bad coupling between this module and sysctl.c */
-	if (op == 001) {
-		error = avc_has_perm(sid, tsid,
-				     SECCLASS_DIR, DIR__SEARCH, NULL);
-	} else {
-		av = 0;
-		if (op & 004)
-			av |= FILE__READ;
-		if (op & 002)
-			av |= FILE__WRITE;
-		if (av)
-			error = avc_has_perm(sid, tsid,
-					     SECCLASS_FILE, av, NULL);
-	}
-
-	return error;
-}
-
 static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
 {
 	const struct cred *cred = current_cred();
@@ -5457,7 +5366,6 @@ static struct security_operations selinux_ops = {
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
-	.sysctl =			selinux_sysctl,
 	.capable =			selinux_capable,
 	.quotactl =			selinux_quotactl,
 	.quota_on =			selinux_quota_on,
-- 
1.6.5.2.143.g8cc62

Re: [RFC][PATCH] security/selinux: Simplify proc inode to security label mapping.

[James Morris]

On Fri, 20 Nov 2009, Eric W. Biederman wrote:

> 
> Currently selinux has incestuous knowledge of the implementation details
> of procfs and sysctl that it uses to get a pathname from an inode. As it
> happens the point we care is in the security_d_instantiate lsm hook so
> we have a valid dentry that we can use to get the entire pathname on
> the proc filesystem.  With the recent change to sys_sysctl to go through
> proc/sys all proc and sysctl accesses go through the vfs, which
> means we no longer need a sysctl special case.

I need to investigate this further, but one immediate issue is that 
Tomoyo seems to have similar code.


> So get the path for the dentry, remove the incestuous knowledge
> and simplify the code.
> 
> caveat: Because the dentry may not yet be hashed I think dentry_path will
>         append (deleted) and thus is not the right function to call.
> 
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  security/selinux/hooks.c |  114 ++++-----------------------------------------
>  1 files changed, 11 insertions(+), 103 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index bb230d5..37ed36e 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -43,7 +43,6 @@
>  #include <linux/fdtable.h>
>  #include <linux/namei.h>
>  #include <linux/mount.h>
> -#include <linux/proc_fs.h>
>  #include <linux/netfilter_ipv4.h>
>  #include <linux/netfilter_ipv6.h>
>  #include <linux/tty.h>
> @@ -70,7 +69,6 @@
>  #include <net/ipv6.h>
>  #include <linux/hugetlb.h>
>  #include <linux/personality.h>
> -#include <linux/sysctl.h>
>  #include <linux/audit.h>
>  #include <linux/string.h>
>  #include <linux/selinux.h>
> @@ -1178,39 +1176,27 @@ static inline u16 socket_type_to_security_class(int family, int type, int protoc
>  }
>  
>  #ifdef CONFIG_PROC_FS
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> +	int rc;
> +	char *buffer, *path;
>  
>  	buffer = (char *)__get_free_page(GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (de && de != de->parent) {
> -		buflen -= de->namelen + 1;
> -		if (buflen < 0)
> -			break;
> -		end -= de->namelen;
> -		memcpy(end, de->name, de->namelen);
> -		*--end = '/';
> -		path = end;
> -		de = de->parent;
> -	}
> -	rc = security_genfs_sid("proc", path, tclass, sid);
> +	path = dentry_path(dentry, buffer, PAGE_SIZE);
> +	if (IS_ERR(path))
> +		rc = PTR_ERR(path);
> +	else
> +		rc = security_genfs_sid("proc", path, tclass, sid);
>  	free_page((unsigned long)buffer);
>  	return rc;
>  }
>  #else
> -static int selinux_proc_get_sid(struct proc_dir_entry *de,
> +static int selinux_proc_get_sid(struct dentry *dentry,
>  				u16 tclass,
>  				u32 *sid)
>  {
> @@ -1374,10 +1360,9 @@ static int inode_doinit_with_dentry(struct inode *inode, struct dentry *opt_dent
>  		isec->sid = sbsec->sid;
>  
>  		if ((sbsec->flags & SE_SBPROC) && !S_ISLNK(inode->i_mode)) {
> -			struct proc_inode *proci = PROC_I(inode);
> -			if (proci->pde) {
> +			if (opt_dentry) {
>  				isec->sclass = inode_mode_to_security_class(inode->i_mode);
> -				rc = selinux_proc_get_sid(proci->pde,
> +				rc = selinux_proc_get_sid(opt_dentry,
>  							  isec->sclass,
>  							  &sid);
>  				if (rc)
> @@ -1939,82 +1924,6 @@ static int selinux_capable(struct task_struct *tsk, const struct cred *cred,
>  	return task_has_capability(tsk, cred, cap, audit);
>  }
>  
> -static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
> -{
> -	int buflen, rc;
> -	char *buffer, *path, *end;
> -
> -	rc = -ENOMEM;
> -	buffer = (char *)__get_free_page(GFP_KERNEL);
> -	if (!buffer)
> -		goto out;
> -
> -	buflen = PAGE_SIZE;
> -	end = buffer+buflen;
> -	*--end = '\0';
> -	buflen--;
> -	path = end-1;
> -	*path = '/';
> -	while (table) {
> -		const char *name = table->procname;
> -		size_t namelen = strlen(name);
> -		buflen -= namelen + 1;
> -		if (buflen < 0)
> -			goto out_free;
> -		end -= namelen;
> -		memcpy(end, name, namelen);
> -		*--end = '/';
> -		path = end;
> -		table = table->parent;
> -	}
> -	buflen -= 4;
> -	if (buflen < 0)
> -		goto out_free;
> -	end -= 4;
> -	memcpy(end, "/sys", 4);
> -	path = end;
> -	rc = security_genfs_sid("proc", path, tclass, sid);
> -out_free:
> -	free_page((unsigned long)buffer);
> -out:
> -	return rc;
> -}
> -
> -static int selinux_sysctl(ctl_table *table, int op)
> -{
> -	int error = 0;
> -	u32 av;
> -	u32 tsid, sid;
> -	int rc;
> -
> -	sid = current_sid();
> -
> -	rc = selinux_sysctl_get_sid(table, (op == 0001) ?
> -				    SECCLASS_DIR : SECCLASS_FILE, &tsid);
> -	if (rc) {
> -		/* Default to the well-defined sysctl SID. */
> -		tsid = SECINITSID_SYSCTL;
> -	}
> -
> -	/* The op values are "defined" in sysctl.c, thereby creating
> -	 * a bad coupling between this module and sysctl.c */
> -	if (op == 001) {
> -		error = avc_has_perm(sid, tsid,
> -				     SECCLASS_DIR, DIR__SEARCH, NULL);
> -	} else {
> -		av = 0;
> -		if (op & 004)
> -			av |= FILE__READ;
> -		if (op & 002)
> -			av |= FILE__WRITE;
> -		if (av)
> -			error = avc_has_perm(sid, tsid,
> -					     SECCLASS_FILE, av, NULL);
> -	}
> -
> -	return error;
> -}
> -
>  static int selinux_quotactl(int cmds, int type, int id, struct super_block *sb)
>  {
>  	const struct cred *cred = current_cred();
> @@ -5457,7 +5366,6 @@ static struct security_operations selinux_ops = {
>  	.ptrace_traceme =		selinux_ptrace_traceme,
>  	.capget =			selinux_capget,
>  	.capset =			selinux_capset,
> -	.sysctl =			selinux_sysctl,
>  	.capable =			selinux_capable,
>  	.quotactl =			selinux_quotactl,
>  	.quota_on =			selinux_quota_on,
> -- 
> 1.6.5.2.143.g8cc62
> 

-- 
James Morris
<jmorris@namei.org>

Re: [RFC][PATCH] security/selinux: Simplify proc inode to securitylabel mapping.

[Tetsuo Handa]

James Morris wrote:
> I need to investigate this further, but one immediate issue is that 
> Tomoyo seems to have similar code.

tomoyo_sysctl hook and related functions will be removed soon.

> On Fri, 20 Nov 2009, Eric W. Biederman wrote:
> > caveat: Because the dentry may not yet be hashed I think dentry_path will
> >         append (deleted) and thus is not the right function to call.

SELinux, TOMOYO, AppArmor want pathname calculation functions which don't
append " (deleted)" suffix.

Regarding TOMOYO, I want to use below function.
It is impossible to convert "pid" to "self" outside __d_path() because string
returned by __d_path() does not have hints for telling whether "pid" part is
procfs or not. As a result, I have to traverse dentry/vfsmount tree manually
in order to tell whether "pid" part is procfs or not, which makes __d_path()
useless for TOMOYO.

/**
 * tomoyo_get_absolute_path - Get the path of a dentry but ignores chroot'ed root.
 *
 * @path:   Pointer to "struct path".
 * @buffer: Pointer to buffer to return value in.
 * @buflen: Sizeof @buffer.
 *
 * Returns the buffer on success, an error code otherwise.
 *
 * Caller holds the dcache_lock and vfsmount_lock.
 * Based on __d_path() in fs/dcache.c
 *
 * If dentry is a directory, trailing '/' is appended.
 * /proc/pid is represented as /proc/self if pid is current.
 */
static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
				      const int buflen)
{
	char *pos = buffer + buflen - 1;
	struct dentry *dentry = path->dentry;
	struct vfsmount *vfsmnt = path->mnt;
	bool is_dir = (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode));
	const char *name;
	int len;

	if (buflen < 256)
		goto out;

	*pos = '\0';
	for (;;) {
		struct dentry *parent;
		if (dentry == vfsmnt->mnt_root || IS_ROOT(dentry)) {
			if (vfsmnt->mnt_parent == vfsmnt)
				break;
			dentry = vfsmnt->mnt_mountpoint;
			vfsmnt = vfsmnt->mnt_parent;
			continue;
		}
		if (is_dir) {
			is_dir = false;
			*--pos = '/';
		}
		parent = dentry->d_parent;
		name = dentry->d_name.name;
		len = dentry->d_name.len;
		if (IS_ROOT(parent) && *name > '0' && *name <= '9' &&
		    parent->d_sb &&
		    parent->d_sb->s_magic == PROC_SUPER_MAGIC) {
			char *ep;
			const pid_t pid = (pid_t) simple_strtoul(name, &ep,
								 10);
			const pid_t tgid = task_tgid_nr_ns(current,
							   dentry->d_sb->
							   s_fs_info);
			if (!*ep && pid == tgid && tgid) {
				name = "self";
				len = 4;
			}
		}
		pos -= len;
		if (pos <= buffer)
			goto out;
		memmove(pos, name, len);
		*--pos = '/';
		dentry = parent;
	}
	if (*pos == '/')
		pos++;
	len = dentry->d_name.len;
	pos -= len;
	if (pos < buffer)
		goto out;
	memmove(pos, dentry->d_name.name, len);
	return pos;
 out:
	return ERR_PTR(-ENOMEM);
}

Re: [RFC][PATCH] security/selinux: Simplify proc inode to security label mapping.

[Eric W. Biederman]

James Morris <jmorris@namei.org> writes:

> On Fri, 20 Nov 2009, Eric W. Biederman wrote:
>
>> 
>> Currently selinux has incestuous knowledge of the implementation details
>> of procfs and sysctl that it uses to get a pathname from an inode. As it
>> happens the point we care is in the security_d_instantiate lsm hook so
>> we have a valid dentry that we can use to get the entire pathname on
>> the proc filesystem.  With the recent change to sys_sysctl to go through
>> proc/sys all proc and sysctl accesses go through the vfs, which
>> means we no longer need a sysctl special case.
>
> I need to investigate this further, but one immediate issue is that 
> Tomoyo seems to have similar code.

The Tomoyo code is currently gone in the sysctl tree (and thus in
linux-next), that change was part of what got me thinking about changing
selinux as well.

If we can remove the selinux special case as well then we can actually
remove the sysctl hook from the lsm.

Eric

Re: [RFC][PATCH] security/selinux: Simplify proc inode to security label mapping.

[James Morris]

On Fri, 20 Nov 2009, Eric W. Biederman wrote:

> 
> Currently selinux has incestuous knowledge of the implementation details
> of procfs and sysctl that it uses to get a pathname from an inode. As it
> happens the point we care is in the security_d_instantiate lsm hook so
> we have a valid dentry that we can use to get the entire pathname on
> the proc filesystem.  With the recent change to sys_sysctl to go through
> proc/sys all proc and sysctl accesses go through the vfs, which
> means we no longer need a sysctl special case.
> 
> So get the path for the dentry, remove the incestuous knowledge
> and simplify the code.
> 
> caveat: Because the dentry may not yet be hashed I think dentry_path will
>         append (deleted) and thus is not the right function to call.

This seems to break labeling.  Prior to this patch, I see:


# ls -lZ /proc/1/net/rpc/nfsd.fh
-rw-------. root root system_u:object_r:sysctl_rpc_t:s0 channel

with the patch:

# ls -lZ /proc/1/net/rpc/nfsd.fh
-rw-------. root root system_u:object_r:proc_t:s0      channel


-- 
James Morris
<jmorris@namei.org>