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

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

[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 three new hooks
inode_{get,set}secctx, and inode_notifysecctx.

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 sets both the in-core and on-disk store for the particular
inode. The third hook is used to notify the in-core inode of a change to it's
security state.

This is the third revision of this patch which takes into account concerns by
Casey Schaufler, and Christop Hellwig.

fs/xattr.c               |   55 +++++++++++++++++++++++++++++++++++-----------
 include/linux/security.h |   37 +++++++++++++++++++++++++++++++
 include/linux/xattr.h    |    3 +-
 security/dummy.c         |   17 ++++++++++++++
 security/security.c      |   18 +++++++++++++++
 security/selinux/hooks.c |   32 ++++++++++++++++++++++++++-
 6 files changed, 147 insertions(+), 15 deletions(-)

[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 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-+05T5uksL2qpZYMLLGbcSA@public.gmane.org>
---
 include/linux/security.h |   37 +++++++++++++++++++++++++++++++++++++
 security/dummy.c         |   17 +++++++++++++++++
 security/security.c      |   18 ++++++++++++++++++
 security/selinux/hooks.c |   32 +++++++++++++++++++++++++++++++-
 4 files changed, 103 insertions(+), 1 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index b07357c..220ec46 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1224,6 +1224,23 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@secdata contains the security context.
  *	@seclen contains the length of the security context.
  *
+ * @inode_notifysecctx:
+ * 	Set the incore security context of an inode.
+ * 	@dentry contains the inode we wish to set the security context of.
+ * 	@ctx contains the string which we wish to set in the inode.
+ * 	@ctxlen contains the length of @ctx.
+ *
+ * @inode_setsecctx:
+ * 	Sets both the incore and persistant form of the inode's security context.
+ * 	@dentry contains the inode we wish to set the security context of.
+ * 	@ctx contains the string which we wish to set in the inode.
+ * 	@ctxlen contains the length of @ctx.
+ *
+ * @inode_getsecctx:
+ * 	Returns a string containing all relavent security context information 	
+ * 	@dentry contains the inode we wish to set the security context of.
+ *	@ctx is a pointer to place the allocated security context should be placed.
+ *	@ctxlen points to the place to put the length of @ctx.	
  * This is the main security structure.
  */
 struct security_operations {
@@ -1414,6 +1431,10 @@ struct security_operations {
 	int (*secctx_to_secid)(char *secdata, u32 seclen, u32 *secid);
 	void (*release_secctx)(char *secdata, u32 seclen);
 
+	int (*inode_notifysecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
+	int (*inode_setsecctx)(struct dentry *dentry, void *ctx, u32 ctxlen);
+	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);
@@ -1653,6 +1674,9 @@ 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_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
+int security_inode_getsecctx(struct dentry *dentry, void **ctx, u32 *ctxlen);
 #else /* CONFIG_SECURITY */
 struct security_mnt_opts {
 };
@@ -2365,6 +2389,19 @@ static inline int security_secctx_to_secid(char *secdata,
 static inline void security_release_secctx(char *secdata, u32 seclen)
 {
 }
+
+static inline int security_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+static inline int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	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 78d8f92..872b45c 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -962,6 +962,20 @@ static void dummy_release_secctx(char *secdata, u32 seclen)
 {
 }
 
+static int dummy_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return -EOPNOTSUPP;
+}
+static int dummy_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	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)
@@ -1121,6 +1135,9 @@ 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_notifysecctx);
+	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 b1387a6..39ed78f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -852,6 +852,24 @@ void security_release_secctx(char *secdata, u32 seclen)
 }
 EXPORT_SYMBOL(security_release_secctx);
 
+int security_inode_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return security_ops->inode_notifysecctx(dentry, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_inode_notifysecctx);
+
+int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
+{
+	return security_ops->inode_setsecctx(dentry, ctx, ctxlen);
+}
+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 4bf4807..5ce6bbe 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"
@@ -5174,6 +5175,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_notifysecctx(struct dentry *dentry, void *ctx, u32 ctxlen, bool setdisk)
+{
+	struct inode *inode = dentry->d_inode;
+
+	return selinux_inode_setsecurity(inode, XATTR_SELINUX_SUFFIX, ctx, ctxlen, 0);
+	
+}
+
+/*
+ *	This hook requires that the inode i_mutex be locked
+ */
+static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen, bool setdisk)
+{
+	return __vfs_setxattr_noperm(dentry, XATTR_NAME_SELINUX, ctx, ctxlen, 0); 
+}
+
+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,
@@ -5364,7 +5392,9 @@ 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_notifysecctx =		selinux_inode_notifysecctx,
+	.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: [RFC]Introduce generalized hooks for getting and setting inode secctx v3

[Casey Schaufler]

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

> 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.

Why is this important? I'm perfectly willing to believe that it is,
but I would hesitate to say that it's completely obvious to the
casual observer. After all, we've gotten by with things the way they
are for some time. Perhaps you could describe the use to which you
would be putting this. I expect that if I go through the backlog
discussions on or about this topic I could probably make some
logical assumptions about what you want to do, but it will save
everyone some time if you could spell it out here.

> Second it introduces three new hooks
> inode_{get,set}secctx, and inode_notifysecctx.
>
> 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 sets both the in-core and on-disk store for the particular
> inode. The third hook is used to notify the in-core inode of a change to it's
> security state.

I still dislike having an interface that explicitly disallows
security attribute integrity. That does not help me feel secure.

> This is the third revision of this patch which takes into account concerns by
> Casey Schaufler, and Christop Hellwig.



Casey Schaufler
casey@schaufler-ca.com

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

[James Morris]

On Wed, 19 Mar 2008, Casey Schaufler wrote:

> I still dislike having an interface that explicitly disallows
> security attribute integrity. That does not help me feel secure.

What do you mean?


-- 
James Morris
<jmorris-gx6/JNMH7DfYtjvyW6yDsg@public.gmane.org>

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

[Stephen Smalley]

On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> --- "David P. Quigley" <dpquigl-+05T5uksL2qpZYMLLGbcSA@public.gmane.org> wrote:
> 
> > 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.
> 
> Why is this important? I'm perfectly willing to believe that it is,
> but I would hesitate to say that it's completely obvious to the
> casual observer. After all, we've gotten by with things the way they
> are for some time. Perhaps you could describe the use to which you
> would be putting this. I expect that if I go through the backlog
> discussions on or about this topic I could probably make some
> logical assumptions about what you want to do, but it will save
> everyone some time if you could spell it out here.
> 
> > Second it introduces three new hooks
> > inode_{get,set}secctx, and inode_notifysecctx.
> >
> > 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 sets both the in-core and on-disk store for the particular
> > inode. The third hook is used to notify the in-core inode of a change to it's
> > security state.
> 
> I still dislike having an interface that explicitly disallows
> security attribute integrity. That does not help me feel secure.

Which part of our prior explanations of this functionality didn't you
understand?

http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2

I would agree though that the final patch submission ought to include
some of that prior explanation in its patch description for historical
purposes.

-- 
Stephen Smalley
National Security Agency

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

[Casey Schaufler]

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

> 
> On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > 
> > > 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.
> > 
> > Why is this important? I'm perfectly willing to believe that it is,
> > but I would hesitate to say that it's completely obvious to the
> > casual observer. After all, we've gotten by with things the way they
> > are for some time. Perhaps you could describe the use to which you
> > would be putting this. I expect that if I go through the backlog
> > discussions on or about this topic I could probably make some
> > logical assumptions about what you want to do, but it will save
> > everyone some time if you could spell it out here.
> > 
> > > Second it introduces three new hooks
> > > inode_{get,set}secctx, and inode_notifysecctx.
> > >
> > > 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 sets both the in-core and on-disk store for the
> particular
> > > inode. The third hook is used to notify the in-core inode of a change to
> it's
> > > security state.
> > 
> > I still dislike having an interface that explicitly disallows
> > security attribute integrity. That does not help me feel secure.
> 
> Which part of our prior explanations of this functionality didn't you
> understand?

Oh, cut the crap. What part of my explainations don't you understand?

I understand the functionality. That is not my point. My point is
that inode_notifysecctx() explicitly prohibits the LSM from providing
integrity of the security attributes by introducing a differentiation
between the "in-core" and "on-disk" values, and making it explicit
that the one is set, but not the other.

Clearly this is the direction you intend to go. Have fun with it.
I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
it has happened before.

> http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2
> 
> I would agree though that the final patch submission ought to include
> some of that prior explanation in its patch description for historical
> purposes.

Yes indeed.

Thank you.


Casey Schaufler
casey@schaufler-ca.com

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

[Stephen Smalley]

On Wed, 2008-03-19 at 08:11 -0700, Casey Schaufler wrote:
> --- Stephen Smalley <sds@tycho.nsa.gov> wrote:
> 
> > 
> > On Wed, 2008-03-19 at 06:38 -0700, Casey Schaufler wrote:
> > > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > > 
> > > > 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.
> > > 
> > > Why is this important? I'm perfectly willing to believe that it is,
> > > but I would hesitate to say that it's completely obvious to the
> > > casual observer. After all, we've gotten by with things the way they
> > > are for some time. Perhaps you could describe the use to which you
> > > would be putting this. I expect that if I go through the backlog
> > > discussions on or about this topic I could probably make some
> > > logical assumptions about what you want to do, but it will save
> > > everyone some time if you could spell it out here.
> > > 
> > > > Second it introduces three new hooks
> > > > inode_{get,set}secctx, and inode_notifysecctx.
> > > >
> > > > 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 sets both the in-core and on-disk store for the
> > particular
> > > > inode. The third hook is used to notify the in-core inode of a change to
> > it's
> > > > security state.
> > > 
> > > I still dislike having an interface that explicitly disallows
> > > security attribute integrity. That does not help me feel secure.
> > 
> > Which part of our prior explanations of this functionality didn't you
> > understand?
> 
> Oh, cut the crap. What part of my explainations don't you understand?
> 
> I understand the functionality. That is not my point. My point is
> that inode_notifysecctx() explicitly prohibits the LSM from providing
> integrity of the security attributes by introducing a differentiation
> between the "in-core" and "on-disk" values, and making it explicit
> that the one is set, but not the other.
> 
> Clearly this is the direction you intend to go. Have fun with it.
> I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
> it has happened before.

It is absolutely no different than the way that uids and other inode
attributes are handled; NFS client has to update the incore inode state
based on the server's information and that is not the same as setting it
in the backing filesystem.  I've only said that two or three times.

> > http://marc.info/?l=linux-fsdevel&m=120515271614741&w=2
> > 
> > I would agree though that the final patch submission ought to include
> > some of that prior explanation in its patch description for historical
> > purposes.
> 
> Yes indeed.
> 
> Thank you.
> 
> 
> Casey Schaufler
> casey@schaufler-ca.com
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.
-- 
Stephen Smalley
National Security Agency

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

[James Morris]

On Wed, 19 Mar 2008, Casey Schaufler wrote:

> Oh, cut the crap. What part of my explainations don't you understand?
> 
> I understand the functionality. That is not my point. My point is
> that inode_notifysecctx() explicitly prohibits the LSM from providing
> integrity of the security attributes by introducing a differentiation
> between the "in-core" and "on-disk" values, and making it explicit
> that the one is set, but not the other.
> 
> Clearly this is the direction you intend to go. Have fun with it.
> I've raised the issue, y'all aren't seeing it. Maybe I'm wrong,
> it has happened before.

Please stop trolling.


- James
-- 
James Morris
<jmorris@namei.org>

[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 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 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 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 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 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 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 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 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 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.

[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.

[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.