[RFC]Introduce generalized hooks for getting and setting inode secctx

[RFC]Introduce generalized hooks for getting and setting inode secctx

[David P. Quigley]

This patch set does two things. First it factors the section of vfs_setxattr
that does the real work into a helper function. This allows LSMs the ability to
set the xattrs they need without hitting the permission check inside
vfs_setxattr each time. Second it introduces two new hooks
inode_{get,set}secctx. The first hook retreives all security information the
LSM feels is relavent in the form of a security context. The second hook given
this context can set the in-core and on-disk store for the particular inode.
This differentiation is necessary since there are times when it is necessary
only to set the in-core representation.

[PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[David P. Quigley]

This factors out the part of the vfs_setxattr function that performs the
setting of the xattr and its notification. This is needed so the SELinux
implementation of inode_setsecctx can handle the setting of it's xattr while
maintaining the proper separation of layers.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/xattr.c            |   40 ++++++++++++++++++++++++++--------------
 include/linux/xattr.h |    1 +
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 3acab16..3811a41 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -66,23 +66,12 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 
 	return permission(inode, mask, NULL);
 }
-
-int
-vfs_setxattr(struct dentry *dentry, char *name, void *value,
+int do_setxattr(struct dentry *dentry, char *name, void *value,
 		size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
-
-	error = xattr_permission(inode, name, MAY_WRITE);
-	if (error)
-		return error;
-
-	mutex_lock(&inode->i_mutex);
-	error = security_inode_setxattr(dentry, name, value, size, flags);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
+	int error = -EOPNOTSUPP;
+	
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
@@ -98,6 +87,29 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
 		if (!error)
 			fsnotify_xattr(dentry);
 	}
+
+	return error;
+}
+
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+		size_t size, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
+	mutex_lock(&inode->i_mutex);
+	error = security_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+
+	error = do_setxattr(dentry, name, value, size, flags);
+
 out:
 	mutex_unlock(&inode->i_mutex);
 	return error;
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index df6b95d..c7c1dc5 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -50,6 +50,7 @@ ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
 int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
+int do_setxattr(struct dentry *, char *, void *, size_t, int);
 int vfs_removexattr(struct dentry *, char *);
 
 ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
-- 
1.5.4.1

[PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[David P. Quigley]

This patch introduces two new hooks. One to get all relevant information from
an LSM about an inode an the second given that context to set it on the
inode. The setcontext call takes a flag to indicate if it should set the incore
representation, the ondisk representation or both. This hook is for use in the
labeled NFS code and addresses concerns of how to set security on an inode in a
multi-xattr LSM.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 include/linux/security.h |   18 ++++++++++++++++++
 security/dummy.c         |   12 ++++++++++++
 security/security.c      |   12 ++++++++++++
 security/selinux/hooks.c |   31 ++++++++++++++++++++++++++++++-
 4 files changed, 72 insertions(+), 1 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index fe52cde..bb71ac9 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -112,6 +112,10 @@ struct request_sock;
 #define LSM_UNSAFE_PTRACE	2
 #define LSM_UNSAFE_PTRACE_CAP	4
 
+/* Flags for setsecctx */
+#define LSM_SETCORE	1
+#define LSM_SETDISK	2
+
 #ifdef CONFIG_SECURITY
 
 /**
@@ -1395,6 +1399,9 @@ struct security_operations {
 	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
 	void (*release_secctx)(char *secdata, u32 seclen);
 
+	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen, int flags);
+	int (*inode_getsecctx)(struct dentry *dentry, void **ctx, u32 *ctxlen);
+
 #ifdef CONFIG_SECURITY_NETWORK
 	int (*unix_stream_connect) (struct socket * sock,
 				    struct socket * other, struct sock * newsk);
@@ -1634,6 +1641,8 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
 int security_secctx_to_secid(char *secdata, u32 seclen, u32 *secid);
 void security_release_secctx(char *secdata, u32 seclen);
 
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, int flags);
+int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen);
 #else /* CONFIG_SECURITY */
 
 /*
@@ -2316,6 +2325,15 @@ static inline int security_secctx_to_secid(char *secdata,
 static inline void security_release_secctx(char *secdata, u32 seclen)
 {
 }
+
+static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, int flags)
+{
+	return -EOPNOTSUPP;
+}
+static inline int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return -EOPNOTSUPP;
+}
 #endif	/* CONFIG_SECURITY */
 
 #ifdef CONFIG_SECURITY_NETWORK
diff --git a/security/dummy.c b/security/dummy.c
index 649326b..774e243 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -960,6 +960,16 @@ static void dummy_release_secctx(char *secdata, u32 seclen)
 {
 }
 
+static int dummy_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, int flags)
+{
+	return -EOPNOTSUPP;
+}
+
+static int dummy_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+
 #ifdef CONFIG_KEYS
 static inline int dummy_key_alloc(struct key *key, struct task_struct *ctx,
 				  unsigned long flags)
@@ -1118,6 +1128,8 @@ void security_fixup_ops (struct security_operations *ops)
  	set_to_dummy_if_null(ops, secid_to_secctx);
 	set_to_dummy_if_null(ops, secctx_to_secid);
  	set_to_dummy_if_null(ops, release_secctx);
+	set_to_dummy_if_null(ops, inode_setsecctx);
+	set_to_dummy_if_null(ops, inode_getsecctx);
 #ifdef CONFIG_SECURITY_NETWORK
 	set_to_dummy_if_null(ops, unix_stream_connect);
 	set_to_dummy_if_null(ops, unix_may_send);
diff --git a/security/security.c b/security/security.c
index d15e56c..84db95a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -845,6 +845,18 @@ void security_release_secctx(char *secdata, u32 seclen)
 }
 EXPORT_SYMBOL(security_release_secctx);
 
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, int flags)
+{
+	return security_ops->inode_setsecctx(dentry, ctx, ctxlen, flags);
+}
+EXPORT_SYMBOL(security_inode_setsecctx);
+
+int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	return security_ops->inode_getsecctx(dentry, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_inode_getsecctx);
+
 #ifdef CONFIG_SECURITY_NETWORK
 
 int security_unix_stream_connect(struct socket *sock, struct socket *other,
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 75c2e99..47e8fb0 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -75,6 +75,7 @@
 #include <linux/string.h>
 #include <linux/selinux.h>
 #include <linux/mutex.h>
+#include <linux/fsnotify.h>
 
 #include "avc.h"
 #include "objsec.h"
@@ -5163,6 +5164,33 @@ static void selinux_release_secctx(char *secdata, u32 seclen)
 	kfree(secdata);
 }
 
+/*
+ *	This hook requires that the inode i_mutex be locked
+ */
+static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int rc = 0;
+
+	if (flags & LSM_SETCORE) {
+		rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX,
+						ctx, ctxlen, 0);
+		if(rc)
+			return rc;
+	}
+	if (flags & LSM_SETDISK)
+		rc = do_setxattr(dentry, XATTR_NAME_SELINUX, ctx, ctxlen, 0); 
+	
+	return rc;
+}
+static int selinux_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
+{
+	struct inode *inode = dentry->d_inode;
+	
+	*ctxlen = selinux_inode_getsecurity(inode, XATTR_SELINUX_SUFFIX,
+						ctx, true);
+	return *ctxlen;
+}
 #ifdef CONFIG_KEYS
 
 static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
@@ -5351,7 +5379,8 @@ static struct security_operations selinux_ops = {
 	.secid_to_secctx =		selinux_secid_to_secctx,
 	.secctx_to_secid =		selinux_secctx_to_secid,
 	.release_secctx =		selinux_release_secctx,
-
+	.inode_setsecctx =		selinux_inode_setsecctx,
+	.inode_getsecctx =		selinux_inode_getsecctx,
         .unix_stream_connect =		selinux_socket_unix_stream_connect,
 	.unix_may_send =		selinux_socket_unix_may_send,
 
-- 
1.5.4.1

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Paul Moore]

On Wednesday 05 March 2008 1:54:48 pm David P. Quigley wrote:
> This patch introduces two new hooks. One to get all relevant
> information from an LSM about an inode an the second given that
> context to set it on the inode. The setcontext call takes a flag to
> indicate if it should set the incore representation, the ondisk
> representation or both. This hook is for use in the labeled NFS code
> and addresses concerns of how to set security on an inode in a
> multi-xattr LSM.
>
> Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> ---
>  include/linux/security.h |   18 ++++++++++++++++++
>  security/dummy.c         |   12 ++++++++++++
>  security/security.c      |   12 ++++++++++++
>  security/selinux/hooks.c |   31 ++++++++++++++++++++++++++++++-
>  4 files changed, 72 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index fe52cde..bb71ac9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -112,6 +112,10 @@ struct request_sock;
>  #define LSM_UNSAFE_PTRACE	2
>  #define LSM_UNSAFE_PTRACE_CAP	4
>
> +/* Flags for setsecctx */
> +#define LSM_SETCORE	1
> +#define LSM_SETDISK	2
> +
>  #ifdef CONFIG_SECURITY
>
>  /**
> @@ -1395,6 +1399,9 @@ struct security_operations {
>  	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
>  	void (*release_secctx)(char *secdata, u32 seclen);
>
> +	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32
> ctxlen, int flags);
> +	int (*inode_getsecctx)(struct dentry *dentry, 
> void **ctx, u32 *ctxlen);

Not a terribly big deal, but I liked James' suggestion of 'file_<blah>' 
instead of 'inode_<blah>'.

-- 
paul moore
linux security @ hp

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Stephen Smalley]

On Wed, 2008-03-05 at 15:45 -0500, Paul Moore wrote:
> On Wednesday 05 March 2008 1:54:48 pm David P. Quigley wrote:
> > This patch introduces two new hooks. One to get all relevant
> > information from an LSM about an inode an the second given that
> > context to set it on the inode. The setcontext call takes a flag to
> > indicate if it should set the incore representation, the ondisk
> > representation or both. This hook is for use in the labeled NFS code
> > and addresses concerns of how to set security on an inode in a
> > multi-xattr LSM.
> >
> > Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> > ---
> >  include/linux/security.h |   18 ++++++++++++++++++
> >  security/dummy.c         |   12 ++++++++++++
> >  security/security.c      |   12 ++++++++++++
> >  security/selinux/hooks.c |   31 ++++++++++++++++++++++++++++++-
> >  4 files changed, 72 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index fe52cde..bb71ac9 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -112,6 +112,10 @@ struct request_sock;
> >  #define LSM_UNSAFE_PTRACE	2
> >  #define LSM_UNSAFE_PTRACE_CAP	4
> >
> > +/* Flags for setsecctx */
> > +#define LSM_SETCORE	1
> > +#define LSM_SETDISK	2
> > +
> >  #ifdef CONFIG_SECURITY
> >
> >  /**
> > @@ -1395,6 +1399,9 @@ struct security_operations {
> >  	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
> >  	void (*release_secctx)(char *secdata, u32 seclen);
> >
> > +	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32
> > ctxlen, int flags);
> > +	int (*inode_getsecctx)(struct dentry *dentry, 
> > void **ctx, u32 *ctxlen);
> 
> Not a terribly big deal, but I liked James' suggestion of 'file_<blah>' 
> instead of 'inode_<blah>'.

I wasn't as keen on it - at present, we use file_ for hooks that operate
on an open file (struct file).

And it is already the case that e.g. inode_getsecurity and
inode_setsecurity can and are used on socket inodes via f[gs]etxattr to
get the socket inode's security label.  

For actually getting the sk security label (which ideally would always
be kept in sync, but that isn't addressed today), we might have a
sk_[gs]etsecctx.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Casey Schaufler]

--- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:

> This patch introduces two new hooks. One to get all relevant information from
> an LSM about an inode an the second given that context to set it on the
> inode. The setcontext call takes a flag to indicate if it should set the
> incore
> representation, the ondisk representation or both.

Is this advisory or manditory? What should the behavior be for
virtual file systems like selinuxfs and smackfs when the "on disk"
bit is set? I understand that the intended target is NFS, but that
is not going to stop someone from using it to other purposes. I
would suggest that the flag be advisory and that the behavior of
where the value gets set be left to the LSM and file system to
work out.


> This hook is for use in
> the
> labeled NFS code and addresses concerns of how to set security on an inode in
> a
> multi-xattr LSM.

I think this looks right. Let me make sure that everything is the
way I think it is, just to be sure.

If I call inode_getsecid() and pass that to secid_to_secctx(),
I'm guaranteed to get the same thing I would have gotten if I
called inode_getsecctx, assuming rational implementations of
the hooks of course. Similarly, calling inode_getsecctx() and
passing the result to secctx_to_secid() is the same as
inode_getsecid(). If I have stacked LSMs (someday) the secid
will represent the combination of all the modules and the secctx
will describe all the LSM attributes regardless of how they are
stored.

> Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> ---
>  include/linux/security.h |   18 ++++++++++++++++++
>  security/dummy.c         |   12 ++++++++++++
>  security/security.c      |   12 ++++++++++++
>  security/selinux/hooks.c |   31 ++++++++++++++++++++++++++++++-
>  4 files changed, 72 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index fe52cde..bb71ac9 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -112,6 +112,10 @@ struct request_sock;
>  #define LSM_UNSAFE_PTRACE	2
>  #define LSM_UNSAFE_PTRACE_CAP	4
>  
> +/* Flags for setsecctx */
> +#define LSM_SETCORE	1
> +#define LSM_SETDISK	2
> +
>  #ifdef CONFIG_SECURITY
>  
>  /**
> @@ -1395,6 +1399,9 @@ struct security_operations {
>  	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
>  	void (*release_secctx)(char *secdata, u32 seclen);
>  
> +	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen, int
> flags);
> +	int (*inode_getsecctx)(struct dentry *dentry, void **ctx, u32 *ctxlen);
> +
>  #ifdef CONFIG_SECURITY_NETWORK
>  	int (*unix_stream_connect) (struct socket * sock,
>  				    struct socket * other, struct sock * newsk);
> @@ -1634,6 +1641,8 @@ int security_secid_to_secctx(u32 secid, char **secdata,
> u32 *seclen);
>  int security_secctx_to_secid(char *secdata, u32 seclen, u32 *secid);
>  void security_release_secctx(char *secdata, u32 seclen);
>  
> +int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen,
> int flags);
> +int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32
> *ctxlen);
>  #else /* CONFIG_SECURITY */
>  
>  /*
> @@ -2316,6 +2325,15 @@ static inline int security_secctx_to_secid(char
> *secdata,
>  static inline void security_release_secctx(char *secdata, u32 seclen)
>  {
>  }
> +
> +static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx,
> u32 ctxlen, int flags)
> +{
> +	return -EOPNOTSUPP;
> +}
> +static inline int security_inode_getsecctx(struct dentry *dentry, void
> **ctx, u32 *ctxlen)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif	/* CONFIG_SECURITY */
>  
>  #ifdef CONFIG_SECURITY_NETWORK
> diff --git a/security/dummy.c b/security/dummy.c
> index 649326b..774e243 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -960,6 +960,16 @@ static void dummy_release_secctx(char *secdata, u32
> seclen)
>  {
>  }
>  
> +static int dummy_inode_setsecctx(struct dentry *dentry, void *ctx, u32
> ctxlen, int flags)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int dummy_inode_getsecctx(struct dentry *dentry, void **ctx, u32
> *ctxlen)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  #ifdef CONFIG_KEYS
>  static inline int dummy_key_alloc(struct key *key, struct task_struct *ctx,
>  				  unsigned long flags)
> @@ -1118,6 +1128,8 @@ void security_fixup_ops (struct security_operations
> *ops)
>   	set_to_dummy_if_null(ops, secid_to_secctx);
>  	set_to_dummy_if_null(ops, secctx_to_secid);
>   	set_to_dummy_if_null(ops, release_secctx);
> +	set_to_dummy_if_null(ops, inode_setsecctx);
> +	set_to_dummy_if_null(ops, inode_getsecctx);
>  #ifdef CONFIG_SECURITY_NETWORK
>  	set_to_dummy_if_null(ops, unix_stream_connect);
>  	set_to_dummy_if_null(ops, unix_may_send);
> diff --git a/security/security.c b/security/security.c
> index d15e56c..84db95a 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -845,6 +845,18 @@ void security_release_secctx(char *secdata, u32 seclen)
>  }
>  EXPORT_SYMBOL(security_release_secctx);
>  
> +int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen,
> int flags)
> +{
> +	return security_ops->inode_setsecctx(dentry, ctx, ctxlen, flags);
> +}
> +EXPORT_SYMBOL(security_inode_setsecctx);
> +
> +int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen)
> +{
> +	return security_ops->inode_getsecctx(dentry, ctx, ctxlen);
> +}
> +EXPORT_SYMBOL(security_inode_getsecctx);
> +
>  #ifdef CONFIG_SECURITY_NETWORK
>  
>  int security_unix_stream_connect(struct socket *sock, struct socket *other,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 75c2e99..47e8fb0 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -75,6 +75,7 @@
>  #include <linux/string.h>
>  #include <linux/selinux.h>
>  #include <linux/mutex.h>
> +#include <linux/fsnotify.h>
>  
>  #include "avc.h"
>  #include "objsec.h"
> @@ -5163,6 +5164,33 @@ static void selinux_release_secctx(char *secdata, u32
> seclen)
>  	kfree(secdata);
>  }
>  
> +/*
> + *	This hook requires that the inode i_mutex be locked
> + */
> +static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32
> ctxlen, int flags)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	int rc = 0;
> +
> +	if (flags & LSM_SETCORE) {
> +		rc = selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX,
> +						ctx, ctxlen, 0);
> +		if(rc)
> +			return rc;
> +	}
> +	if (flags & LSM_SETDISK)
> +		rc = do_setxattr(dentry, XATTR_NAME_SELINUX, ctx, ctxlen, 0); 
> +	
> +	return rc;
> +}
> +static int selinux_inode_getsecctx(struct dentry *dentry, void **ctx, u32
> *ctxlen)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	
> +	*ctxlen = selinux_inode_getsecurity(inode, XATTR_SELINUX_SUFFIX,
> +						ctx, true);
> +	return *ctxlen;
> +}
>  #ifdef CONFIG_KEYS
>  
>  static int selinux_key_alloc(struct key *k, struct task_struct *tsk,
> @@ -5351,7 +5379,8 @@ static struct security_operations selinux_ops = {
>  	.secid_to_secctx =		selinux_secid_to_secctx,
>  	.secctx_to_secid =		selinux_secctx_to_secid,
>  	.release_secctx =		selinux_release_secctx,
> -
> +	.inode_setsecctx =		selinux_inode_setsecctx,
> +	.inode_getsecctx =		selinux_inode_getsecctx,
>          .unix_stream_connect =		selinux_socket_unix_stream_connect,
>  	.unix_may_send =		selinux_socket_unix_may_send,
>  
> -- 
> 1.5.4.1
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Christoph Hellwig]

On Wed, Mar 05, 2008 at 01:54:47PM -0500, David P. Quigley wrote:
> This factors out the part of the vfs_setxattr function that performs the
> setting of the xattr and its notification. This is needed so the SELinux
> implementation of inode_setsecctx can handle the setting of it's xattr while
> maintaining the proper separation of layers.

Looks good, but I'm not entirely sure do_setxattr is a good name for
this exported functionality.  __vfs_setxattr_noperm might be better.

Please also add a kerneldoc comment for each new global function.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Christoph Hellwig]

On Wed, Mar 05, 2008 at 01:54:48PM -0500, David P. Quigley wrote:
> This patch introduces two new hooks. One to get all relevant information from
> an LSM about an inode an the second given that context to set it on the
> inode. The setcontext call takes a flag to indicate if it should set the incore
> representation, the ondisk representation or both. This hook is for use in the
> labeled NFS code and addresses concerns of how to set security on an inode in a
> multi-xattr LSM.

I don't quite understand the rationale of the incore vs ondisk flag.
Why are these separate?  

> +	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen, int flags);
> +	int (*inode_getsecctx)(struct dentry *dentry, void **ctx, u32 *ctxlen);

The dentry in here seems odd given the inode name.   Given that we're
talking about per-inode security labels here an struct inode * would
make more sense to me.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Stephen Smalley]

On Thu, 2008-03-06 at 13:30 +0100, Christoph Hellwig wrote:
> On Wed, Mar 05, 2008 at 01:54:48PM -0500, David P. Quigley wrote:
> > This patch introduces two new hooks. One to get all relevant information from
> > an LSM about an inode an the second given that context to set it on the
> > inode. The setcontext call takes a flag to indicate if it should set the incore
> > representation, the ondisk representation or both. This hook is for use in the
> > labeled NFS code and addresses concerns of how to set security on an inode in a
> > multi-xattr LSM.
> 
> I don't quite understand the rationale of the incore vs ondisk flag.
> Why are these separate?

In-core only:  NFS client gets the file security context for an inode
from the server and needs to set the in-core security context for its
inode accordingly.  But it does not want to call back to i_op->setxattr
and try to _set_ the context on the server when it does this.  So it
only calls with the incore flag.

On-disk:  NFS server receives a file security context to set on a file
from the client, and wants to update both the in-core security context
for the inode and the on-disk xattr.  So it calls with the ondisk flag.

It actually only requires a boolean flag.  

> > +	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen, int flags);
> > +	int (*inode_getsecctx)(struct dentry *dentry, void **ctx, u32 *ctxlen);
> 
> The dentry in here seems odd given the inode name.   Given that we're
> talking about per-inode security labels here an struct inode * would
> make more sense to me.

We'd agree, but unfortunately the i_op->setxattr and ->getxattr methods
take a dentry as input even though the fs code immediately turns around
and extracts the inode from it.  Not sure why that is or whether it can
be changed.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Christoph Hellwig]

On Thu, Mar 06, 2008 at 08:50:22AM -0500, Stephen Smalley wrote:
> In-core only:  NFS client gets the file security context for an inode
> from the server and needs to set the in-core security context for its
> inode accordingly.  But it does not want to call back to i_op->setxattr
> and try to _set_ the context on the server when it does this.  So it
> only calls with the incore flag.
> 
> On-disk:  NFS server receives a file security context to set on a file
> from the client, and wants to update both the in-core security context
> for the inode and the on-disk xattr.  So it calls with the ondisk flag.
> 
> It actually only requires a boolean flag.  

Yes, the boolean might be better.

I still don't quite understand why we would only set the security
context in-core only as this looks like a potential loss of metadata
updates for me.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Stephen Smalley]

On Thu, 2008-03-06 at 08:54 -0500, Christoph Hellwig wrote:
> On Thu, Mar 06, 2008 at 08:50:22AM -0500, Stephen Smalley wrote:
> > In-core only:  NFS client gets the file security context for an inode
> > from the server and needs to set the in-core security context for its
> > inode accordingly.  But it does not want to call back to i_op->setxattr
> > and try to _set_ the context on the server when it does this.  So it
> > only calls with the incore flag.
> > 
> > On-disk:  NFS server receives a file security context to set on a file
> > from the client, and wants to update both the in-core security context
> > for the inode and the on-disk xattr.  So it calls with the ondisk flag.
> > 
> > It actually only requires a boolean flag.  
> 
> Yes, the boolean might be better.
> 
> I still don't quite understand why we would only set the security
> context in-core only as this looks like a potential loss of metadata
> updates for me.

It isn't truly changing the security context - it is notifying the
security module on the client side of the security context provided by
the server for a given inode.  In the case of uids, the nfs client code
can directly set the inode->i_uid to the server-provided value from the
fattr, but for the inode->i_security, the nfs client code has to call
into the security module to set it in-core.

Maybe they should be different hooks altogether - just not sure what to
call the incore case.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Christoph Hellwig]

On Thu, Mar 06, 2008 at 09:05:04AM -0500, Stephen Smalley wrote:
> It isn't truly changing the security context - it is notifying the
> security module on the client side of the security context provided by
> the server for a given inode.  In the case of uids, the nfs client code
> can directly set the inode->i_uid to the server-provided value from the
> fattr, but for the inode->i_security, the nfs client code has to call
> into the security module to set it in-core.
> 
> Maybe they should be different hooks altogether - just not sure what to
> call the incore case.

Ok, this makes a lot more sense.  These defintively should be different
hooks in that case, and no matter what name they have (no good ideas
from me either currently) they should be documented properly in the
kerneldoc to state something like your above message.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[James Morris]

On Thu, 6 Mar 2008, Christoph Hellwig wrote:

> Ok, this makes a lot more sense.  These defintively should be different
> hooks in that case, and no matter what name they have (no good ideas
> from me either currently)

Perhaps setsecctx and storesecctx ? 



-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Stephen Smalley]

On Fri, 2008-03-07 at 01:25 +1100, James Morris wrote:
> On Thu, 6 Mar 2008, Christoph Hellwig wrote:
> 
> > Ok, this makes a lot more sense.  These defintively should be different
> > hooks in that case, and no matter what name they have (no good ideas
> > from me either currently)
> 
> Perhaps setsecctx and storesecctx ? 

Or possibly notifysecctx (notify security module of a secctx value for
the inode) vs. setsecctx (set this sectx on this inode, including both
in-core update and invoking the __vfs_setxattr_noperm helper).

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Dave Quigley]

On Thu, 2008-03-06 at 13:27 +0100, Christoph Hellwig wrote:
> On Wed, Mar 05, 2008 at 01:54:47PM -0500, David P. Quigley wrote:
> > This factors out the part of the vfs_setxattr function that performs the
> > setting of the xattr and its notification. This is needed so the SELinux
> > implementation of inode_setsecctx can handle the setting of it's xattr while
> > maintaining the proper separation of layers.
> 
> Looks good, but I'm not entirely sure do_setxattr is a good name for
> this exported functionality.  __vfs_setxattr_noperm might be better.
> 
> Please also add a kerneldoc comment for each new global function.

Will do. I have to release a new patch set with the hook changed to take
a bool instead of a flag so you should see this updated sometime later
today.

Dave

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Dave Quigley]

On Thu, 2008-03-06 at 09:48 -0500, Stephen Smalley wrote:
> On Fri, 2008-03-07 at 01:25 +1100, James Morris wrote:
> > On Thu, 6 Mar 2008, Christoph Hellwig wrote:
> > 
> > > Ok, this makes a lot more sense.  These defintively should be different
> > > hooks in that case, and no matter what name they have (no good ideas
> > > from me either currently)
> > 
> > Perhaps setsecctx and storesecctx ? 
> 
> Or possibly notifysecctx (notify security module of a secctx value for
> the inode) vs. setsecctx (set this sectx on this inode, including both
> in-core update and invoking the __vfs_setxattr_noperm helper).
> 

So are we keeping the dentry parameter for these calls, or am I changing
them over to an inode. If it is going to use an inode this means I need
to change the parameters for the xattr code. Is there a reason why the
xattr code takes dentries instead of an inode? 

Dave

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Christoph Hellwig]

On Thu, Mar 06, 2008 at 12:13:58PM -0500, Dave Quigley wrote:
> So are we keeping the dentry parameter for these calls, or am I changing
> them over to an inode. If it is going to use an inode this means I need
> to change the parameters for the xattr code. Is there a reason why the
> xattr code takes dentries instead of an inode? 

Ah, that's the reason why you use dentries.    Either keep the dentry
in the call that does the xattr modification for now and document that
why you're doing it, or if you feel eager fix up the xattr interface.

In fact the new fine-grained xattr interface already only passed inodes
which is what the inode operations should have been doing aswell -
xattrs are a concept tied to the inode and not in any way to a
hiearchical pathname component.

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Christoph Hellwig]

On Thu, Mar 06, 2008 at 11:47:06AM -0500, Dave Quigley wrote:
> Will do. I have to release a new patch set with the hook changed to take
> a bool instead of a flag so you should see this updated sometime later
> today.

I think it really should be a separate hook for the two different
use-cases as suggested by Stephen.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Dave Quigley]

So I have converted all the xattr internals over to an inode from a
dentry but there is one issue with that. To set EAs on CIFS they need a
full path for the file. I don't think we can reconcile using inodes in
the vfs operation with CIFS needing a path. If you have a suggestion on
how to handle this I'm more than willing to listen. Everything else
however seems to be a trivial change.

Dave

On Fri, 2008-03-07 at 11:03 +0100, Christoph Hellwig wrote:
> On Thu, Mar 06, 2008 at 12:13:58PM -0500, Dave Quigley wrote:
> > So are we keeping the dentry parameter for these calls, or am I changing
> > them over to an inode. If it is going to use an inode this means I need
> > to change the parameters for the xattr code. Is there a reason why the
> > xattr code takes dentries instead of an inode? 
> 
> Ah, that's the reason why you use dentries.    Either keep the dentry
> in the call that does the xattr modification for now and document that
> why you're doing it, or if you feel eager fix up the xattr interface.
> 
> In fact the new fine-grained xattr interface already only passed inodes
> which is what the inode operations should have been doing aswell -
> xattrs are a concept tied to the inode and not in any way to a
> hiearchical pathname component.

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Dave Quigley]

Yea, I didn't get to read the rest of the emails before I responded to
this. In the lastest version it is two hooks. inode_notifysecctx and
inode_setsecctx which set incore and both respectivly.

Dave


On Fri, 2008-03-07 at 11:05 +0100, Christoph Hellwig wrote:
> On Thu, Mar 06, 2008 at 11:47:06AM -0500, Dave Quigley wrote:
> > Will do. I have to release a new patch set with the hook changed to take
> > a bool instead of a flag so you should see this updated sometime later
> > today.
> 
> I think it really should be a separate hook for the two different
> use-cases as suggested by Stephen.

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Miklos Szeredi]

> So I have converted all the xattr internals over to an inode from a
> dentry but there is one issue with that. To set EAs on CIFS they need a
> full path for the file. I don't think we can reconcile using inodes in
> the vfs operation with CIFS needing a path. If you have a suggestion on
> how to handle this I'm more than willing to listen. Everything else
> however seems to be a trivial change.

Since there are no hardlinks in CIFS (or are there?) it coukld get the
dentry back with d_find_alias().

But what's the point?  Why is it better to pass the inode, rather than
dentry down to the filesystem?

Hiding info from lower layers is not generally a good idea if there
are valid uses for it.  I don't buy Chritoph's argument, that
filesystems working with paths instead of inodes are inherently
broken.

Miklos

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Casey Schaufler]

--- Dave Quigley <dpquigl@tycho.nsa.gov> wrote:

> Yea, I didn't get to read the rest of the emails before I responded to
> this. In the lastest version it is two hooks. inode_notifysecctx and
> inode_setsecctx which set incore and both respectivly.

So what is the desired behavior of these two calls in the case
where on-disk and inode secctx are in lockstep? Does "notify"
imply "set" in this case?

What about the case where there is no "disk", as is the case
for virtual filesystems? Would "set" imply "notify" in this case?

I think that if the answer to these questions is "it will never
come up because it's only for NFS" what you have is an NFS
specific interface in the LSM. That may be OK, but it ain't pretty.
On the other hand, NFS is sufficiently important that a little
ugly may be a price worth paying.
 
> Dave
> 
> 
> On Fri, 2008-03-07 at 11:05 +0100, Christoph Hellwig wrote:
> > On Thu, Mar 06, 2008 at 11:47:06AM -0500, Dave Quigley wrote:
> > > Will do. I have to release a new patch set with the hook changed to take
> > > a bool instead of a flag so you should see this updated sometime later
> > > today.
> > 
> > I think it really should be a separate hook for the two different
> > use-cases as suggested by Stephen.
> 
> 
> 


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Dave Quigley]

On Fri, 2008-03-07 at 17:54 +0100, Miklos Szeredi wrote:
> > So I have converted all the xattr internals over to an inode from a
> > dentry but there is one issue with that. To set EAs on CIFS they need a
> > full path for the file. I don't think we can reconcile using inodes in
> > the vfs operation with CIFS needing a path. If you have a suggestion on
> > how to handle this I'm more than willing to listen. Everything else
> > however seems to be a trivial change.
> 
> Since there are no hardlinks in CIFS (or are there?) it coukld get the
> dentry back with d_find_alias().
> 
> But what's the point?  Why is it better to pass the inode, rather than
> dentry down to the filesystem?
> 
> Hiding info from lower layers is not generally a good idea if there
> are valid uses for it.  I don't buy Chritoph's argument, that
> filesystems working with paths instead of inodes are inherently
> broken.
> 
> Miklos

This isn't hiding information from the lower layers. The only use of the
dentry is much higher up in the call chain. If you take a look at
sys_chmod (another inode attr modifying call) the dentry is really only
used in

sys_chmod->chown_common->notify_change->fsnotify_change

The operations that actually change the inode metadata on disk do not
touch the dentry at all except to get the inode(rightly so since it is
an INODE operation). 

Dave

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Dave Quigley]

For some odd reason I can't quite parse the first two parts of your
email but to answer your question about it being an NFS only hook. As of
right now the only user is going to be NFS however any remote filesystem
(labeled CIFS anyone?) will potentially have this problem. Also even
though we don't have one today if there ever were an LSM that used
multiple xattrs for their security attributes this is a useful interface
to them. So there are many uses for this hook but currently the only one
is NFS.

Dave

On Fri, 2008-03-07 at 09:11 -0800, Casey Schaufler wrote:
> --- Dave Quigley <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org> wrote:
> 
> > Yea, I didn't get to read the rest of the emails before I responded to
> > this. In the lastest version it is two hooks. inode_notifysecctx and
> > inode_setsecctx which set incore and both respectivly.
> 
> So what is the desired behavior of these two calls in the case
> where on-disk and inode secctx are in lockstep? Does "notify"
> imply "set" in this case?
> 
> What about the case where there is no "disk", as is the case
> for virtual filesystems? Would "set" imply "notify" in this case?
> 
> I think that if the answer to these questions is "it will never
> come up because it's only for NFS" what you have is an NFS
> specific interface in the LSM. That may be OK, but it ain't pretty.
> On the other hand, NFS is sufficiently important that a little
> ugly may be a price worth paying.
>  
> > Dave
> > 
> > 
> > On Fri, 2008-03-07 at 11:05 +0100, Christoph Hellwig wrote:
> > > On Thu, Mar 06, 2008 at 11:47:06AM -0500, Dave Quigley wrote:
> > > > Will do. I have to release a new patch set with the hook changed to take
> > > > a bool instead of a flag so you should see this updated sometime later
> > > > today.
> > > 
> > > I think it really should be a separate hook for the two different
> > > use-cases as suggested by Stephen.
> > 
> > 
> > 
> 
> 
> Casey Schaufler
> casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Casey Schaufler]

--- Dave Quigley <dpquigl@tycho.nsa.gov> wrote:

> For some odd reason I can't quite parse the first two parts

Let me try a different angle on the question. Maybe it just
doesn't come up as a real issue, and I'm concerned about nothing.

Just for grins lets say I wanted to set the secctx on a directory
in a derivative of ramfs in some unspecified way that is not
related to mkdir. There are no on-disk inodes. Should the code call
inode_setsecctx, inode_notifysecctx, or both? It seems rational to
me to call inode_setsecctx, but since the differentiation between
the interfaces is the "on disk" factor and ramfs only exists as
in core, it would seem that inode_notifysecctx would be correct.

Like I say, maybe it never comes up, but having these two very
similar interfaces (or the old flag) begs the question of when
to use each for things other than their original purpose. I think
we'll live in a better LSM if it's clear.

> of your
> email but to answer your question about it being an NFS only hook. As of
> right now the only user is going to be NFS however any remote filesystem
> (labeled CIFS anyone?) will potentially have this problem. Also even
> though we don't have one today if there ever were an LSM that used
> multiple xattrs for their security attributes this is a useful interface
> to them. So there are many uses for this hook but currently the only one
> is NFS.

Ok then, no worries.

Thank you


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Stephen Smalley]

On Fri, 2008-03-07 at 10:14 -0800, Casey Schaufler wrote:
> --- Dave Quigley <dpquigl@tycho.nsa.gov> wrote:
> 
> > For some odd reason I can't quite parse the first two parts
> 
> Let me try a different angle on the question. Maybe it just
> doesn't come up as a real issue, and I'm concerned about nothing.
> 
> Just for grins lets say I wanted to set the secctx on a directory
> in a derivative of ramfs in some unspecified way that is not
> related to mkdir. There are no on-disk inodes. Should the code call
> inode_setsecctx, inode_notifysecctx, or both? It seems rational to
> me to call inode_setsecctx, but since the differentiation between
> the interfaces is the "on disk" factor and ramfs only exists as
> in core, it would seem that inode_notifysecctx would be correct.

If you are setting (changing) the value, then use inode_setsecctx (or
the existing inode_setsecurity, which is used by the xattr code, but
that is limited to setting a single xattr value by name).

If you are just notifying the security module of a value that should be
associated with the inode, use inode_notifysecctx.

Reasonable?

> 
> Like I say, maybe it never comes up, but having these two very
> similar interfaces (or the old flag) begs the question of when
> to use each for things other than their original purpose. I think
> we'll live in a better LSM if it's clear.
> 
> > of your
> > email but to answer your question about it being an NFS only hook. As of
> > right now the only user is going to be NFS however any remote filesystem
> > (labeled CIFS anyone?) will potentially have this problem. Also even
> > though we don't have one today if there ever were an LSM that used
> > multiple xattrs for their security attributes this is a useful interface
> > to them. So there are many uses for this hook but currently the only one
> > is NFS.
> 
> Ok then, no worries.
> 
> Thank you
> 
> 
> Casey Schaufler
> casey@schaufler-ca.com
-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.nsa.gov> wrote:

> 
> On Fri, 2008-03-07 at 10:14 -0800, Casey Schaufler wrote:
> > --- Dave Quigley <dpquigl@tycho.nsa.gov> wrote:
> > 
> > > For some odd reason I can't quite parse the first two parts
> > 
> > Let me try a different angle on the question. Maybe it just
> > doesn't come up as a real issue, and I'm concerned about nothing.
> > 
> > Just for grins lets say I wanted to set the secctx on a directory
> > in a derivative of ramfs in some unspecified way that is not
> > related to mkdir. There are no on-disk inodes. Should the code call
> > inode_setsecctx, inode_notifysecctx, or both? It seems rational to
> > me to call inode_setsecctx, but since the differentiation between
> > the interfaces is the "on disk" factor and ramfs only exists as
> > in core, it would seem that inode_notifysecctx would be correct.
> 
> If you are setting (changing) the value, then use inode_setsecctx (or
> the existing inode_setsecurity, which is used by the xattr code, but
> that is limited to setting a single xattr value by name).

So any code that wants to explicitly set the secctx (as opposed to a
specific attribute value) calls inode_setsecctx. This makes perfect
sense.

> If you are just notifying the security module of a value that should be
> associated with the inode, use inode_notifysecctx.

So this is a way for filesystem code to pass information to an LSM
without specifying semantics. Is there an expectation that
inode_getsecctx return the value sent by inode_notifysecctx, or
would you expect the "notify" secctx to be stored elsewhere? 

> Reasonable?

I think that the theory is fine, but I forsee implementation
complications if the details of what is expected of an LSM aren't
clear. At this point I'm not objecting to the interface so much as
hoping to be able to use it properly, even with my limited and
deteriorating mental facilties.

Thank you.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Stephen Smalley]

On Fri, 2008-03-07 at 10:49 -0800, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > 
> > On Fri, 2008-03-07 at 10:14 -0800, Casey Schaufler wrote:
> > > --- Dave Quigley <dpquigl@tycho.nsa.gov> wrote:
> > > 
> > > > For some odd reason I can't quite parse the first two parts
> > > 
> > > Let me try a different angle on the question. Maybe it just
> > > doesn't come up as a real issue, and I'm concerned about nothing.
> > > 
> > > Just for grins lets say I wanted to set the secctx on a directory
> > > in a derivative of ramfs in some unspecified way that is not
> > > related to mkdir. There are no on-disk inodes. Should the code call
> > > inode_setsecctx, inode_notifysecctx, or both? It seems rational to
> > > me to call inode_setsecctx, but since the differentiation between
> > > the interfaces is the "on disk" factor and ramfs only exists as
> > > in core, it would seem that inode_notifysecctx would be correct.
> > 
> > If you are setting (changing) the value, then use inode_setsecctx (or
> > the existing inode_setsecurity, which is used by the xattr code, but
> > that is limited to setting a single xattr value by name).
> 
> So any code that wants to explicitly set the secctx (as opposed to a
> specific attribute value) calls inode_setsecctx. This makes perfect
> sense.
> 
> > If you are just notifying the security module of a value that should be
> > associated with the inode, use inode_notifysecctx.
> 
> So this is a way for filesystem code to pass information to an LSM
> without specifying semantics. Is there an expectation that
> inode_getsecctx return the value sent by inode_notifysecctx, or
> would you expect the "notify" secctx to be stored elsewhere?

The former (getsecctx should return the value sent by notifysecctx).
Not a separate value.

The other model I suppose would be something more along the lines of
David Howell's interfaces for creating a task security struct with a
particular value and then letting the caller set ->security directly.
In this case, it would be creating an inode security struct with a
particular value and then letting the fs code set inode->i_security
directly.  That seems non-optimal though for this situation (in David's
case, the setup of the task security struct happens once early on, and
then the swapping of the task security pointer happens later when
performing actions that shouldn't be treated as happening under the
current task's credentials). 

> > Reasonable?
> 
> I think that the theory is fine, but I forsee implementation
> complications if the details of what is expected of an LSM aren't
> clear. At this point I'm not objecting to the interface so much as
> hoping to be able to use it properly, even with my limited and
> deteriorating mental facilties.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.nsa.gov> wrote:

> 
> ...
> > So this is a way for filesystem code to pass information to an LSM
> > without specifying semantics. Is there an expectation that
> > inode_getsecctx return the value sent by inode_notifysecctx, or
> > would you expect the "notify" secctx to be stored elsewhere?
> 
> The former (getsecctx should return the value sent by notifysecctx).
> Not a separate value.

Now that took me by surprise.

I spent a good deal of time working with POSIX, so my perspective
may be a bit twisted, but I looks to me that from an interface
standpoint, inode_setsecctx and inode_notifysecctx are
indistinguishable. How would the man pages for the two differ?
Would you ever use both interfaces on the same inode?

Don't take this as me being contrary, I really want to understand
how this makes for a better LSM, not just a bigger one.
 
> The other model I suppose would be something more along the lines of
> David Howell's interfaces for creating a task security struct with a
> particular value and then letting the caller set ->security directly.
> In this case, it would be creating an inode security struct with a
> particular value and then letting the fs code set inode->i_security
> directly.  That seems non-optimal though for this situation (in David's
> case, the setup of the task security struct happens once early on, and
> then the swapping of the task security pointer happens later when
> performing actions that shouldn't be treated as happening under the
> current task's credentials). 

David has said, unless I'm remembering incorrectly again, that he
would expect NFS to use his scheme. I would be happier with a single
scheme than this pair. Which of the real/effective secctx values
would be affected by each of these interfaces? Maybe the right
thing is to have setsecctx hit the real and notifysecctx the
effective. Maybe that's a dumb idea. I hope that the interactions
between those schemes can be worked out before either gets adopted.
If not, there's likely to be tears.

Thank you.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Stephen Smalley]

On Fri, 2008-03-07 at 11:48 -0800, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > 
> > ...
> > > So this is a way for filesystem code to pass information to an LSM
> > > without specifying semantics. Is there an expectation that
> > > inode_getsecctx return the value sent by inode_notifysecctx, or
> > > would you expect the "notify" secctx to be stored elsewhere?
> > 
> > The former (getsecctx should return the value sent by notifysecctx).
> > Not a separate value.
> 
> Now that took me by surprise.
> 
> I spent a good deal of time working with POSIX, so my perspective
> may be a bit twisted, but I looks to me that from an interface
> standpoint, inode_setsecctx and inode_notifysecctx are
> indistinguishable. How would the man pages for the two differ?
> Would you ever use both interfaces on the same inode?
> 
> Don't take this as me being contrary, I really want to understand
> how this makes for a better LSM, not just a bigger one.

I'll try again to explain, but everything below has been said previously
in this discussion.

inode_setsecctx:  Change the security context of an inode.  Updates the
incore security context managed by the security module and invokes the
fs code as needed (via __vfs_setxattr_noperm) to update any backing
xattrs that represent the context.  Example usage:  NFS server invokes
this hook to change the security context in its incore inode and on the
backing filesystem to a value provided by the client on a SETATTR
operation.

inode_notifysecctx:  Notify the security module of what the security
context of an inode should be.  Initializes the incore security context
managed by the security module for this inode.  Example usage:  NFS
client invokes this hook to initialize the security context in its
incore inode to the value provided by the server for the file when the
server returned the file's attributes to the client.
 
> > The other model I suppose would be something more along the lines of
> > David Howell's interfaces for creating a task security struct with a
> > particular value and then letting the caller set ->security directly.
> > In this case, it would be creating an inode security struct with a
> > particular value and then letting the fs code set inode->i_security
> > directly.  That seems non-optimal though for this situation (in David's
> > case, the setup of the task security struct happens once early on, and
> > then the swapping of the task security pointer happens later when
> > performing actions that shouldn't be treated as happening under the
> > current task's credentials). 
> 
> David has said, unless I'm remembering incorrectly again, that he
> would expect NFS to use his scheme. I would be happier with a single
> scheme than this pair. Which of the real/effective secctx values
> would be affected by each of these interfaces? Maybe the right
> thing is to have setsecctx hit the real and notifysecctx the
> effective. Maybe that's a dumb idea. I hope that the interactions
> between those schemes can be worked out before either gets adopted.
> If not, there's likely to be tears.

You're confusing the task security credentials with the inode security
context again.  David's work is only dealing with assuming different
task credentials than the current process.  Not managing the inode
security contexts.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Miklos Szeredi]

> This isn't hiding information from the lower layers. The only use of the
> dentry is much higher up in the call chain. If you take a look at
> sys_chmod (another inode attr modifying call) the dentry is really only
> used in
> 
> sys_chmod->chown_common->notify_change->fsnotify_change

And i_op->setattr().

> 
> The operations that actually change the inode metadata on disk do not
> touch the dentry at all except to get the inode(rightly so since it is
> an INODE operation). 

"Disk" and "inode" are concepts specific to a certain class of
filesystems, but make no sense for a different set.  What makes sense
for all filesystems is the hierarchy of path components, which is what
dentries represent.

Miklos

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Chris Wright]

* Casey Schaufler (casey@schaufler-ca.com) wrote:
> > > So this is a way for filesystem code to pass information to an LSM
> > > without specifying semantics. Is there an expectation that
> > > inode_getsecctx return the value sent by inode_notifysecctx, or
> > > would you expect the "notify" secctx to be stored elsewhere?
> > 
> > The former (getsecctx should return the value sent by notifysecctx).
> > Not a separate value.
> 
> Now that took me by surprise.
> 
> I spent a good deal of time working with POSIX, so my perspective
> may be a bit twisted, but I looks to me that from an interface
> standpoint, inode_setsecctx and inode_notifysecctx are
> indistinguishable. How would the man pages for the two differ?
> Would you ever use both interfaces on the same inode?

Assuming a simple model (no time of day magic or composite labels) they
differ by calling ->setxattr so that the fs actually puts bit on persitent
storage (may not be disk for ram backed fs).  The callers are from the
kernel (you don't have to worry about that) and the implementation is
simply update your blob or update your blob and tell fs to update as well.

thanks,
-chris

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Dave Quigley]

On Fri, 2008-03-07 at 21:24 +0100, Miklos Szeredi wrote:
> > This isn't hiding information from the lower layers. The only use of the
> > dentry is much higher up in the call chain. If you take a look at
> > sys_chmod (another inode attr modifying call) the dentry is really only
> > used in
> > 
> > sys_chmod->chown_common->notify_change->fsnotify_change
> 
> And i_op->setattr().

Which is in the same boat as setxattr since most filesystems just grab
the inode from the dentry that is passed in (although I didn't look as
extensively as I did with setxattr). This is another example of
needlessly passing a dentry where an inode is sufficient and correct. So
once again the only real purpose for the dentry to be there is
fsnotify_change. 

I also don't see a reason for getattr to take a vfsmount and a dentry.
Even fuse_getattr does nothing with the vfsmount and only pulls the
inode from the dentry to pass into fuse_do_getattr(which takes an
inode). The libfs code for simple_getattr does nothing with them as
well. Can anyone cite a real use for all of this? It seems that the
pervasiveness of dentries in all the file system code isn't justified.
Note I haven't looked through every file system (yet) but from what I've
seen there are no real users of these dentries except for CIFS.

> 
> > 
> > The operations that actually change the inode metadata on disk do not
> > touch the dentry at all except to get the inode(rightly so since it is
> > an INODE operation). 
> 
> "Disk" and "inode" are concepts specific to a certain class of
> filesystems, but make no sense for a different set.  What makes sense
> for all filesystems is the hierarchy of path components, which is what
> dentries represent.


Would you care to elaborate? Perhaps an example in tree (or out). Path
names are nothing but a user friendly way of telling the file system
which inode you want. I've even had someone ask me once if they could
just open a file by inode.

> 
> Miklos

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Casey Schaufler]

--- Stephen Smalley <sds@tycho.nsa.gov> wrote:

> 
> On Fri, 2008-03-07 at 11:48 -0800, Casey Schaufler wrote:
> > --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > 
> > > 
> > > ...
> > > > So this is a way for filesystem code to pass information to an LSM
> > > > without specifying semantics. Is there an expectation that
> > > > inode_getsecctx return the value sent by inode_notifysecctx, or
> > > > would you expect the "notify" secctx to be stored elsewhere?
> > > 
> > > The former (getsecctx should return the value sent by notifysecctx).
> > > Not a separate value.
> > 
> > Now that took me by surprise.
> > 
> > I spent a good deal of time working with POSIX, so my perspective
> > may be a bit twisted, but I looks to me that from an interface
> > standpoint, inode_setsecctx and inode_notifysecctx are
> > indistinguishable. How would the man pages for the two differ?
> > Would you ever use both interfaces on the same inode?
> > 
> > Don't take this as me being contrary, I really want to understand
> > how this makes for a better LSM, not just a bigger one.
> 
> I'll try again to explain, but everything below has been said previously
> in this discussion.

Never hurts to review the bidding.

> inode_setsecctx:  Change the security context of an inode.  Updates the
> incore security context managed by the security module and invokes the
> fs code as needed (via __vfs_setxattr_noperm) to update any backing
> xattrs that represent the context.  Example usage:  NFS server invokes
> this hook to change the security context in its incore inode and on the
> backing filesystem to a value provided by the client on a SETATTR
> operation.
> 
> inode_notifysecctx:  Notify the security module of what the security
> context of an inode should be.  Initializes the incore security context
> managed by the security module for this inode.

I'm not convinced that building a mechanism into the LSM that
prohibits one from maintaining secctx integrity is a good thing.
I suppose an LSM that cares about such things can always refuse
to go along with the inode_notifysecctx hook semantics. That
will mean such an LSM couldn't support NFS. Oh well.

> Example usage:  NFS
> client invokes this hook to initialize the security context in its
> incore inode to the value provided by the server for the file when the
> server returned the file's attributes to the client.
>  
> > > The other model I suppose would be something more along the lines of
> > > David Howell's interfaces for creating a task security struct with a
> > > particular value and then letting the caller set ->security directly.
> > > In this case, it would be creating an inode security struct with a
> > > particular value and then letting the fs code set inode->i_security
> > > directly.  That seems non-optimal though for this situation (in David's
> > > case, the setup of the task security struct happens once early on, and
> > > then the swapping of the task security pointer happens later when
> > > performing actions that shouldn't be treated as happening under the
> > > current task's credentials). 
> > 
> > David has said, unless I'm remembering incorrectly again, that he
> > would expect NFS to use his scheme. I would be happier with a single
> > scheme than this pair. Which of the real/effective secctx values
> > would be affected by each of these interfaces? Maybe the right
> > thing is to have setsecctx hit the real and notifysecctx the
> > effective. Maybe that's a dumb idea. I hope that the interactions
> > between those schemes can be worked out before either gets adopted.
> > If not, there's likely to be tears.
> 
> You're confusing the task security credentials with the inode security
> context again.  David's work is only dealing with assuming different
> task credentials than the current process.  Not managing the inode
> security contexts.

Err, yeah. If I've got two task blobs and two ways to deal with
inode blobs there are more ways to get it wrong than if there is
only one way to deal with exactly one blob. I'm not confusing
anything, I'm looking at the implications of all this complexity
and, although some people don't seem to mind if the internals of
Linux start to look like the spagetti factory kitchen on all you
can eat night, I do.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Dave Quigley]

Actually I think I might have a solution to this. The inode structure
has a list_head of all dentries that reference it. On CIFS (correct me
if I am wrong) wouldn't this list consist of only one entry, or would
there be more in the event that the same CIFS share is mounted in
different places? Even if there might be more than one I don't think
that would be an issue since we don't have a vfsmount structure so we
can't effectively tell with a dentry alone where in the tree the share
is mounted (and it shouldn't matter anyway since it seems the operation
is full_path based on the share). Anyone have any comments on this
approach? 

Dave 


On Fri, 2008-03-07 at 11:06 -0500, Dave Quigley wrote:
> So I have converted all the xattr internals over to an inode from a
> dentry but there is one issue with that. To set EAs on CIFS they need a
> full path for the file. I don't think we can reconcile using inodes in
> the vfs operation with CIFS needing a path. If you have a suggestion on
> how to handle this I'm more than willing to listen. Everything else
> however seems to be a trivial change.
> 
> Dave
> 
> On Fri, 2008-03-07 at 11:03 +0100, Christoph Hellwig wrote:
> > On Thu, Mar 06, 2008 at 12:13:58PM -0500, Dave Quigley wrote:
> > > So are we keeping the dentry parameter for these calls, or am I changing
> > > them over to an inode. If it is going to use an inode this means I need
> > > to change the parameters for the xattr code. Is there a reason why the
> > > xattr code takes dentries instead of an inode? 
> > 
> > Ah, that's the reason why you use dentries.    Either keep the dentry
> > in the call that does the xattr modification for now and document that
> > why you're doing it, or if you feel eager fix up the xattr interface.
> > 
> > In fact the new fine-grained xattr interface already only passed inodes
> > which is what the inode operations should have been doing aswell -
> > xattrs are a concept tied to the inode and not in any way to a
> > hiearchical pathname component.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Miklos Szeredi]

> > > This isn't hiding information from the lower layers. The only use of the
> > > dentry is much higher up in the call chain. If you take a look at
> > > sys_chmod (another inode attr modifying call) the dentry is really only
> > > used in
> > > 
> > > sys_chmod->chown_common->notify_change->fsnotify_change
> > 
> > And i_op->setattr().
> 
> Which is in the same boat as setxattr since most filesystems just grab
> the inode from the dentry that is passed in (although I didn't look as
> extensively as I did with setxattr). This is another example of
> needlessly passing a dentry where an inode is sufficient and correct. So
> once again the only real purpose for the dentry to be there is
> fsnotify_change. 
> 
> I also don't see a reason for getattr to take a vfsmount and a dentry.

About vfsmount, I have no idea.

> Even fuse_getattr does nothing with the vfsmount and only pulls the
> inode from the dentry to pass into fuse_do_getattr(which takes an
> inode).

Fuse is a strange beast, it uses identifiers stored in the inode to
communicate with userspace, but then converts them back to paths on
the library interface (there's an alternative, much less popular API,
where filesystems may use the IDs natively).

LUFS was a project very similar to fuse, and it used paths on the
kernel interface.  The project died, but not because it was
fundamentally flawed, but because fuse was more stable, and better in
other respects.  Both projects used the "path based API" concept,
which distinguished them from eariler attempts at userspace
filesystems, and which made both of them very popular.

> The libfs code for simple_getattr does nothing with them as
> well. Can anyone cite a real use for all of this? It seems that the
> pervasiveness of dentries in all the file system code isn't justified.
> Note I haven't looked through every file system (yet) but from what I've
> seen there are no real users of these dentries except for CIFS.

Right, and while CIFS is not the cleanest codebases in the kernel, to
say the least, this particular usage of dentries is perfectly valid.

> > > The operations that actually change the inode metadata on disk do not
> > > touch the dentry at all except to get the inode(rightly so since it is
> > > an INODE operation). 
> > 
> > "Disk" and "inode" are concepts specific to a certain class of
> > filesystems, but make no sense for a different set.  What makes sense
> > for all filesystems is the hierarchy of path components, which is what
> > dentries represent.
> 
> 
> Would you care to elaborate? Perhaps an example in tree (or out). Path
> names are nothing but a user friendly way of telling the file system
> which inode you want.

FAT?  AFAIK there isn't any inode, metadata is stored directly in the
directory entry.  I know, fat doesn't use dentries either, but that's
beside the point.

The struct inode is just a way to cache metadata (and whatever the
filesystem wants), and dentries are a way to cache the names.  Using
the name is certainly a valid thing for a filesystem to do, so passing
down the dentry is the right thing to do.

> I've even had someone ask me once if they could just open a file by
> inode.

Yes, comes up once in a while, and it's a rather stupid idea.
Userspace shouldn't know about inodes at all.  But because of hard
links it does need i_ino, which has been a constant source of
headaches over the years.

Miklos

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Brad Boyer]

On Fri, Mar 07, 2008 at 10:46:40PM +0100, Miklos Szeredi wrote:
> > > "Disk" and "inode" are concepts specific to a certain class of
> > > filesystems, but make no sense for a different set.  What makes sense
> > > for all filesystems is the hierarchy of path components, which is what
> > > dentries represent.
> > 
> > Would you care to elaborate? Perhaps an example in tree (or out). Path
> > names are nothing but a user friendly way of telling the file system
> > which inode you want.
> 
> FAT?  AFAIK there isn't any inode, metadata is stored directly in the
> directory entry.  I know, fat doesn't use dentries either, but that's
> beside the point.
> 
> The struct inode is just a way to cache metadata (and whatever the
> filesystem wants), and dentries are a way to cache the names.  Using
> the name is certainly a valid thing for a filesystem to do, so passing
> down the dentry is the right thing to do.

A better example in terms of having more unix-like visible use would be
hfsplus. It is even used as the native file system of a unix variant
(apple's osx/darwin). For Linux, we synthetically create the whole
dentry and inode separation. On disk, there is no logical separation.
The primary key to find the metadata is parent directory id + filename,
while the catalog id number (the equivalent of an inode number) is a
secondary key with a separate index pointing to the real primary key.
Structurally, this is all kept in a single catalog tree for the entire
volume, and you build a key and do a search. If you look at the code
for iget in hfsplus, you will find that it creates the secondary key
of just the "inode" number and searches on that in the catalog. The
result of that search is the thread record containing the real key
consisting of the parent id and filename, which is then used to search
for the actual data record. As a consequence of this structure, hard
links are a horrible hack and are more like symlinks internally.

Just to summarize, on hfsplus the information in the dentry is what
we need to load the data for struct inode, while it takes extra work
to find the data on disk if we don't have the information from the
dentry. If you look at the code for hfsplus, it tries to act like
it is a normal unix filesystem as much as possible but that does
cause extra searches in the catalog.

	Brad Boyer
	flar@allandria.com

Re: [PATCH 2/2] LSM/SELinux: inode_{get,set}secctx hooks to access LSM security context information.

[Christoph Hellwig]

Sorry, I'm a bit busy currently so I can't really heep up with the
heated discussion here.  But having a problem in cifs and the discussion
here should be a good enough reason to make the hook take a dentry for
now, maybe with a little comment that it's only because the xattr
helpers need it.   We can still sort this out later without blocking
your project.

Re: [PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[Stephen Smalley]

On Fri, 2008-03-07 at 13:13 -0800, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > 
> > On Fri, 2008-03-07 at 11:48 -0800, Casey Schaufler wrote:
> > > --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> > > 
> > > > 
> > > > ...
> > > > > So this is a way for filesystem code to pass information to an LSM
> > > > > without specifying semantics. Is there an expectation that
> > > > > inode_getsecctx return the value sent by inode_notifysecctx, or
> > > > > would you expect the "notify" secctx to be stored elsewhere?
> > > > 
> > > > The former (getsecctx should return the value sent by notifysecctx).
> > > > Not a separate value.
> > > 
> > > Now that took me by surprise.
> > > 
> > > I spent a good deal of time working with POSIX, so my perspective
> > > may be a bit twisted, but I looks to me that from an interface
> > > standpoint, inode_setsecctx and inode_notifysecctx are
> > > indistinguishable. How would the man pages for the two differ?
> > > Would you ever use both interfaces on the same inode?
> > > 
> > > Don't take this as me being contrary, I really want to understand
> > > how this makes for a better LSM, not just a bigger one.
> > 
> > I'll try again to explain, but everything below has been said previously
> > in this discussion.
> 
> Never hurts to review the bidding.
> 
> > inode_setsecctx:  Change the security context of an inode.  Updates the
> > incore security context managed by the security module and invokes the
> > fs code as needed (via __vfs_setxattr_noperm) to update any backing
> > xattrs that represent the context.  Example usage:  NFS server invokes
> > this hook to change the security context in its incore inode and on the
> > backing filesystem to a value provided by the client on a SETATTR
> > operation.
> > 
> > inode_notifysecctx:  Notify the security module of what the security
> > context of an inode should be.  Initializes the incore security context
> > managed by the security module for this inode.
> 
> I'm not convinced that building a mechanism into the LSM that
> prohibits one from maintaining secctx integrity is a good thing.
> I suppose an LSM that cares about such things can always refuse
> to go along with the inode_notifysecctx hook semantics. That
> will mean such an LSM couldn't support NFS. Oh well.

I don't understand how this prohibits one from maintaining secctx
integrity.  And you continue to respond in a non-constructive manner
that suggests you are more interested in arguing that in solving
problems.  If not, please demonstrate otherwise.

As I've already said, the NFS client code directly sets the i_uid in the
in-core inode from the fattr->uid supplied by the server to reflect the
state of the server's inode.  All we are doing here is providing a
mechanism for it to do likewise for the inode security context.  That is
what inode_notifysecctx() is about.  If you don't like the hook name,
feel free to suggest another one - I already asked for suggestions on
that too.

hch already indicated that he was ok with the basic idea,
http://marc.info/?l=linux-fsdevel&m=120481283810898&w=2

> 
> > Example usage:  NFS
> > client invokes this hook to initialize the security context in its
> > incore inode to the value provided by the server for the file when the
> > server returned the file's attributes to the client.
> >  
> > > > The other model I suppose would be something more along the lines of
> > > > David Howell's interfaces for creating a task security struct with a
> > > > particular value and then letting the caller set ->security directly.
> > > > In this case, it would be creating an inode security struct with a
> > > > particular value and then letting the fs code set inode->i_security
> > > > directly.  That seems non-optimal though for this situation (in David's
> > > > case, the setup of the task security struct happens once early on, and
> > > > then the swapping of the task security pointer happens later when
> > > > performing actions that shouldn't be treated as happening under the
> > > > current task's credentials). 
> > > 
> > > David has said, unless I'm remembering incorrectly again, that he
> > > would expect NFS to use his scheme. I would be happier with a single
> > > scheme than this pair. Which of the real/effective secctx values
> > > would be affected by each of these interfaces? Maybe the right
> > > thing is to have setsecctx hit the real and notifysecctx the
> > > effective. Maybe that's a dumb idea. I hope that the interactions
> > > between those schemes can be worked out before either gets adopted.
> > > If not, there's likely to be tears.
> > 
> > You're confusing the task security credentials with the inode security
> > context again.  David's work is only dealing with assuming different
> > task credentials than the current process.  Not managing the inode
> > security contexts.
> 
> Err, yeah. If I've got two task blobs and two ways to deal with
> inode blobs there are more ways to get it wrong than if there is
> only one way to deal with exactly one blob. I'm not confusing
> anything, I'm looking at the implications of all this complexity
> and, although some people don't seem to mind if the internals of
> Linux start to look like the spagetti factory kitchen on all you
> can eat night, I do.

No, you are confusing things - the fact that you suggested that the
inode_setsecctx would affect the real and inode_notifysecctx would
affect the effective shows that you are conflating the two ideas.  And
they are quite different.  David's work enables a kernel service to
assume a particular task context for a particular operation, such as
cachefiles assuming a different context than the current process when
accessing the cache, and nfsd assuming the context of the client process
when performing operations on its behalf.  These inode hooks enable the
server side to set the inode security context in a generic way (w/o
knowing what attribute or attributes or other mechanism may be used for
storage, as per your earlier request) and enable the client side to
update its incore inode security context to reflect the server's inode
state.

Please, be constructive and take the time to actually read what we have
written and to think about the code.

-- 
Stephen Smalley
National Security Agency

[PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[David P. Quigley]

This factors out the part of the vfs_setxattr function that performs the
setting of the xattr and its notification. This is needed so the SELinux
implementation of inode_setsecctx can handle the setting of it's xattr while
maintaining the proper separation of layers.

Signed-off-by: David P. Quigley <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
---
 fs/xattr.c            |   55 +++++++++++++++++++++++++++++++++++++-----------
 include/linux/xattr.h |    3 +-
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 3acab16..464265e 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -67,22 +67,28 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 	return permission(inode, mask, NULL);
 }
 
-int
-vfs_setxattr(struct dentry *dentry, char *name, void *value,
+/**
+ *  __vfs_setxattr_noperm - perform setxattr operation without performing
+ *  permission checks.
+ *
+ *  @dentry - object to perform setxattr on
+ *  @name - xattr name to set
+ *  @value - value to set @name to
+ *  @size - size of @value
+ *  @flags
+ *
+ *  returns the result of the internel setxattr or setsecurity operations.
+ *
+ *  This function requires the the caller locks the inode's i_mutex before it
+ *  is executed. It also that the caller will make the appropriate permission
+ *  checks.
+ */
+int __vfs_setxattr_noperm(struct dentry *dentry, char *name, void *value,
 		size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
-
-	error = xattr_permission(inode, name, MAY_WRITE);
-	if (error)
-		return error;
-
-	mutex_lock(&inode->i_mutex);
-	error = security_inode_setxattr(dentry, name, value, size, flags);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
+	int error = -EOPNOTSUPP;
+	
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
@@ -98,6 +104,29 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
 		if (!error)
 			fsnotify_xattr(dentry);
 	}
+
+	return error;
+}
+
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+		size_t size, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
+	mutex_lock(&inode->i_mutex);
+	error = security_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+
+	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
+
 out:
 	mutex_unlock(&inode->i_mutex);
 	return error;
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index df6b95d..324c792 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -49,7 +49,8 @@ struct xattr_handler {
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
-int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
+int __vfs_setxattr_noperm(struct dentry *, char *, void *, size_t, int);
+int do_setxattr(struct dentry *, char *, void *, size_t, int);
 int vfs_removexattr(struct dentry *, char *);
 
 ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
-- 
1.5.4.1

[PATCH 1/2] VFS: Factor out part of vfs_setxattr so it can be called from the SELinux hook for inode_setsecctx.

[David P. Quigley]

This factors out the part of the vfs_setxattr function that performs the
setting of the xattr and its notification. This is needed so the SELinux
implementation of inode_setsecctx can handle the setting of it's xattr while
maintaining the proper separation of layers.

Signed-off-by: David P. Quigley <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
---
 fs/xattr.c            |   57 +++++++++++++++++++++++++++++++++++++------------
 include/linux/xattr.h |    1 +
 2 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index f7062da..dd349ea 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -66,22 +66,28 @@ xattr_permission(struct inode *inode, const char *name, int mask)
 	return permission(inode, mask, NULL);
 }
 
-int
-vfs_setxattr(struct dentry *dentry, char *name, void *value,
-		size_t size, int flags)
+/**
+ *  __vfs_setxattr_noperm - perform setxattr operation without performing
+ *  permission checks.
+ *
+ *  @dentry - object to perform setxattr on
+ *  @name - xattr name to set
+ *  @value - value to set @name to
+ *  @size - size of @value
+ *  @flags - flags to pass into filesystem operations
+ *
+ *  returns the result of the internal setxattr or setsecurity operations.
+ *
+ *  This function requires the caller to lock the inode's i_mutex before it
+ *  is executed. It also assumes that the caller will make the appropriate 
+ *  permission checks.
+ */
+int __vfs_setxattr_noperm(struct dentry *dentry, const char *name,
+		const void *value, size_t size, int flags)
 {
 	struct inode *inode = dentry->d_inode;
-	int error;
-
-	error = xattr_permission(inode, name, MAY_WRITE);
-	if (error)
-		return error;
-
-	mutex_lock(&inode->i_mutex);
-	error = security_inode_setxattr(dentry, name, value, size, flags);
-	if (error)
-		goto out;
-	error = -EOPNOTSUPP;
+	int error = -EOPNOTSUPP;
+	
 	if (inode->i_op->setxattr) {
 		error = inode->i_op->setxattr(dentry, name, value, size, flags);
 		if (!error) {
@@ -97,6 +103,29 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
 		if (!error)
 			fsnotify_xattr(dentry);
 	}
+
+	return error;
+}
+
+
+int
+vfs_setxattr(struct dentry *dentry, char *name, void *value,
+		size_t size, int flags)
+{
+	struct inode *inode = dentry->d_inode;
+	int error;
+
+	error = xattr_permission(inode, name, MAY_WRITE);
+	if (error)
+		return error;
+
+	mutex_lock(&inode->i_mutex);
+	error = security_inode_setxattr(dentry, name, value, size, flags);
+	if (error)
+		goto out;
+
+	error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
+
 out:
 	mutex_unlock(&inode->i_mutex);
 	return error;
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index df6b95d..b23d6a8 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -49,6 +49,7 @@ struct xattr_handler {
 ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
 ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
 ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
+int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, size_t, int);
 int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
 int vfs_removexattr(struct dentry *, char *);
 
-- 
1.5.4.1