[RFC]Cleanup inode_getsecurity and callers for use with labeled NFS

[RFC]Cleanup inode_getsecurity and callers for use with labeled NFS

[David P. Quigley]

These patches modify inode_getsecurity and its callers to use the format
similar to the one used by getprocattr which was proposed by Al Viro. They
also rework vfs_getxattr to avoid unnecessary calls to the LSM. These patches
are to support the ongoing work on Labeled NFS.

The first patch converts inode_getsecurity to return the security blob via a
parameter and the length of the blob via the function's return value. This
removes the need for the called to guess the length of the security blob. The
patch also removed the unused err parameter. All of the callers are updated to
use a helper function named xattr_getsecurity which keeps the xattr style
interface and performs generic checks which were originally in the security
modules.

The second patch reworks vfs_getxattr to unconditionally call to the LSM in
the case that we are requesting a security namespace attribute. Originally we
would grab the value from disk then clobber it with the one from the LSM. This
way we can grab it directly from the LSM and in the case when there is no
module loaded or it does not support inode_getsecurity we fall back on the
inode getxattr operation.

David Quigley



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

[PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

From: David P. Quigley <dpquigl@tycho.nsa.gov>

This patch modifies the interface to inode_getsecurity to have the function
return a buffer containing the security blob and its length via parameters
instead of relying on the calling function to give it an appropriately sized
buffer. Security blobs obtained with this function should be freed using the
release_secctx LSM hook. This alleviates the problem of the caller having to
guess a length and preallocate a buffer for this function allowing it to be
used elsewhere for Labeled NFS. The patch also removed the unused err
parameter. The same conversion is similar to the one used by Al Viro for the
security_getprocattr hook.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/xattr.c               |   27 +++++++++++++++++++++++++--
 include/linux/security.h |   27 ++++++++++++++-------------
 include/linux/xattr.h    |    1 +
 mm/shmem.c               |    3 +--
 security/dummy.c         |    4 +++-
 security/selinux/hooks.c |   39 +++++++++++----------------------------
 6 files changed, 55 insertions(+), 46 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index a44fd92..6d59a72 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -105,6 +105,30 @@ out:
 EXPORT_SYMBOL_GPL(vfs_setxattr);
 
 ssize_t
+xattr_getsecurity(struct inode *inode, const char *name, void *value,
+			size_t size)
+{
+	void *buffer = NULL;
+	ssize_t len;
+
+	len = security_inode_getsecurity(inode, name, &buffer);
+	if (len < 0)
+		return len;
+	if (!value || !size) {
+		goto out;
+	}
+	if (size < len) {
+		len = -ERANGE;
+		goto out;
+	}
+	memcpy(value, buffer, len);
+out:
+	security_release_secctx(buffer, len);
+	return len;
+}
+EXPORT_SYMBOL_GPL(xattr_getsecurity);
+
+ssize_t
 vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 {
 	struct inode *inode = dentry->d_inode;
@@ -126,8 +150,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
-		int ret = security_inode_getsecurity(inode, suffix, value,
-						     size, error);
+		int ret = xattr_getsecurity(inode, suffix, value, size);
 		/*
 		 * Only overwrite the return value if a security module
 		 * is actually active.
diff --git a/include/linux/security.h b/include/linux/security.h
index c11dc8a..b6dfa0a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -391,15 +391,11 @@ struct request_sock;
  * 	identified by @name for @dentry.
  * 	Return 0 if permission is granted.
  * @inode_getsecurity:
- *	Copy the extended attribute representation of the security label 
- *	associated with @name for @inode into @buffer.  @buffer may be
- *	NULL to request the size of the buffer required.  @size indicates
- *	the size of @buffer in bytes.  Note that @name is the remainder
- *	of the attribute name after the security. prefix has been removed.
- *	@err is the return value from the preceding fs getxattr call,
- *	and can be used by the security module to determine whether it
- *	should try and canonicalize the attribute value.
- *	Return number of bytes used/required on success.
+ *	Retrieve a copy of the extended attribute representation of the
+ *	security label associated with @name for @inode via @buffer.  Note that
+ *	@name is the remainder of the attribute name after the security. prefix
+ *	has been removed.  
+ *	Return size of buffer on success.
  * @inode_setsecurity:
  *	Set the security label associated with @name for @inode from the
  *	extended attribute value @value.  @size indicates the size of the
@@ -1232,7 +1228,8 @@ struct security_operations {
 	int (*inode_listxattr) (struct dentry *dentry);
 	int (*inode_removexattr) (struct dentry *dentry, char *name);
 	const char *(*inode_xattr_getsuffix) (void);
-  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
+	int (*inode_getsecurity)(const struct inode *inode, const char *name,
+					void **buffer);
   	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
   	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
 
@@ -1772,11 +1769,13 @@ static inline const char *security_inode_xattr_getsuffix(void)
 	return security_ops->inode_xattr_getsuffix();
 }
 
-static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static inline int security_inode_getsecurity(const struct inode *inode,
+						const char *name,
+						void **buffer)
 {
 	if (unlikely (IS_PRIVATE (inode)))
 		return 0;
-	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
+	return security_ops->inode_getsecurity(inode, name, buffer);
 }
 
 static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
@@ -2458,7 +2457,9 @@ static inline const char *security_inode_xattr_getsuffix (void)
 	return NULL ;
 }
 
-static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static inline int security_inode_getsecurity(const struct inode *inode,
+						const char *name,
+						void **buffer)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index def131a..df6b95d 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -46,6 +46,7 @@ struct xattr_handler {
 		   size_t size, int flags);
 };
 
+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);
diff --git a/mm/shmem.c b/mm/shmem.c
index fcd19d3..5ed3a66 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1958,8 +1958,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	return security_inode_getsecurity(inode, name, buffer, size,
-					  -EOPNOTSUPP);
+	return xattr_getsecurity(inode, name, buffer, size);
 }
 
 static int shmem_xattr_security_set(struct inode *inode, const char *name,
diff --git a/security/dummy.c b/security/dummy.c
index 19d813d..b1d28b4 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -377,7 +377,9 @@ static int dummy_inode_removexattr (struct dentry *dentry, char *name)
 	return 0;
 }
 
-static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int dummy_inode_getsecurity(const struct inode *inode,
+					const char *name,
+					void **buffer)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 6237933..bd51f74 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -125,32 +125,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
 
 static struct kmem_cache *sel_inode_cache;
 
-/* Return security context for a given sid or just the context 
-   length if the buffer is null or length is 0 */
-static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
-{
-	char *context;
-	unsigned len;
-	int rc;
-
-	rc = security_sid_to_context(sid, &context, &len);
-	if (rc)
-		return rc;
-
-	if (!buffer || !size)
-		goto getsecurity_exit;
-
-	if (size < len) {
-		len = -ERANGE;
-		goto getsecurity_exit;
-	}
-	memcpy(buffer, context, len);
-
-getsecurity_exit:
-	kfree(context);
-	return len;
-}
-
 /* Allocate and free functions for each kind of security blob. */
 
 static int task_alloc_security(struct task_struct *task)
@@ -2418,14 +2392,23 @@ static const char *selinux_inode_xattr_getsuffix(void)
  *
  * Permission check is handled by selinux_inode_getxattr hook.
  */
-static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int selinux_inode_getsecurity(const struct inode *inode,
+					const char *name,
+					void **buffer)
 {
+	u32 size;
+	int error;
 	struct inode_security_struct *isec = inode->i_security;
 
 	if (strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
 
-	return selinux_getsecurity(isec->sid, buffer, size);
+	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
+	if (error)
+		return error;
+	error = size;
+	return error;
+	
 }
 
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,
-- 
1.5.2.2


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

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Casey Schaufler]

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

> From: David P. Quigley <dpquigl@tycho.nsa.gov>
> 
> This patch modifies the interface to inode_getsecurity to have the function
> return a buffer containing the security blob and its length via parameters
> instead of relying on the calling function to give it an appropriately sized
> buffer. Security blobs obtained with this function should be freed using the
> release_secctx LSM hook.

You are making the assumption that the LSM is going to return a secctx
for all possible named attributes. The caller already has to know the
name of the interesting attribute, that should imply that the caller
has enough information to get the right size for a buffer. I don't think
much of interfaces that require you to allocate memory that you're
just going to throw away after you glance at the data, especially in
the file system lookup path.

> This alleviates the problem of the caller having to
> guess a length and preallocate a buffer for this function allowing it to be
> used elsewhere for Labeled NFS. The patch also removed the unused err
> parameter. The same conversion is similar to the one used by Al Viro for the
> security_getprocattr hook.

These are very different use frequency cases.



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.

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-08-17 at 12:37 -0700, Casey Schaufler wrote:
> --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> 
> > From: David P. Quigley <dpquigl@tycho.nsa.gov>
> > 
> > This patch modifies the interface to inode_getsecurity to have the function
> > return a buffer containing the security blob and its length via parameters
> > instead of relying on the calling function to give it an appropriately sized
> > buffer. Security blobs obtained with this function should be freed using the
> > release_secctx LSM hook.
> 
> You are making the assumption that the LSM is going to return a secctx
> for all possible named attributes. The caller already has to know the
> name of the interesting attribute, that should imply that the caller
> has enough information to get the right size for a buffer. 
But why introduce an additional call to get the information when the
interface can just give it to you? Also the only named attribute that
the LSM should be returning is its security information in the void *
blob as your interfaces propose. Perhaps release_secctx should be
renamed to release_security or security_release to make it sound more
inline with these functions. In your case the implementation for this is
trivial and has little to no overhead. In the case of getsecurity you
just return your internal pointer. In the case of release_secctx (or
other name for it) you just have a nop.

> I don't think
> much of interfaces that require you to allocate memory that you're
> just going to throw away after you glance at the data, especially in
> the file system lookup path.

Sorry but I'm having problems parsing this. However if I understand what
I think your trying to say is that not many interfaces have you allocate
the buffer, return a value, quickly look at it, and toss it out. This
isn't really what this is used for. The idea here is to make
inode_getsecurity work as a way of getting an object with an interface
for managing its lifecycle from the security module. There are other
places where this functionality is useful outside of the getxattr
functions and to request that other parts of the kernel either perform
magic guessing on the security blob length or make the call to grab the
length first doesn't seem efficient to me. 

> 
> > This alleviates the problem of the caller having to
> > guess a length and preallocate a buffer for this function allowing it to be
> > used elsewhere for Labeled NFS. The patch also removed the unused err
> > parameter. The same conversion is similar to the one used by Al Viro for the
> > security_getprocattr hook.
> 
> These are very different use frequency cases.

Which one are you arguing is more frequent than the other? 

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

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-08-17 at 12:37 -0700, Casey Schaufler wrote:
> --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> 
> > From: David P. Quigley <dpquigl@tycho.nsa.gov>
> > 
> > This patch modifies the interface to inode_getsecurity to have the function
> > return a buffer containing the security blob and its length via parameters
> > instead of relying on the calling function to give it an appropriately sized
> > buffer. Security blobs obtained with this function should be freed using the
> > release_secctx LSM hook.
> 
> You are making the assumption that the LSM is going to return a secctx
> for all possible named attributes. The caller already has to know the
> name of the interesting attribute, that should imply that the caller
> has enough information to get the right size for a buffer. I don't think
> much of interfaces that require you to allocate memory that you're
> just going to throw away after you glance at the data, especially in
> the file system lookup path.

Ok sorry I read this a couple of more times and I understand this now.
Your issue isn't a concern. This is going to be done somewhere for an
lsm since it has to allocate space to store the information internally
and then copy it into the buffer provided by getxattr. In your case
there isn't even an allocation and freeing of data here. In SELinux we
had to allocate a string to store the converted sid in and then copy it
up into the buffer internally. Regardless a copy is going to happen the
question is where.

> > This alleviates the problem of the caller having to
> > guess a length and preallocate a buffer for this function allowing it to be
> > used elsewhere for Labeled NFS. The patch also removed the unused err
> > parameter. The same conversion is similar to the one used by Al Viro for the
> > security_getprocattr hook.
> 
> These are very different use frequency cases.
> 
> 
> 
> 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.

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Casey Schaufler]

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

> On Fri, 2007-08-17 at 12:37 -0700, Casey Schaufler wrote:
> > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > 
> > > From: David P. Quigley <dpquigl@tycho.nsa.gov>
> > > 
> > > This patch modifies the interface to inode_getsecurity to have the
> function
> > > return a buffer containing the security blob and its length via
> parameters
> > > instead of relying on the calling function to give it an appropriately
> sized
> > > buffer. Security blobs obtained with this function should be freed using
> the
> > > release_secctx LSM hook.
> > 
> > You are making the assumption that the LSM is going to return a secctx
> > for all possible named attributes. The caller already has to know the
> > name of the interesting attribute, that should imply that the caller
> > has enough information to get the right size for a buffer. I don't think
> > much of interfaces that require you to allocate memory that you're
> > just going to throw away after you glance at the data, especially in
> > the file system lookup path.
> 
> Ok sorry I read this a couple of more times and I understand this now.
> Your issue isn't a concern. This is going to be done somewhere for an
> lsm since it has to allocate space to store the information internally
> and then copy it into the buffer provided by getxattr. In your case
> there isn't even an allocation and freeing of data here. In SELinux we
> had to allocate a string to store the converted sid in and then copy it
> up into the buffer internally. Regardless a copy is going to happen the
> question is where.

I checked and indeed everyone who calls this is allocating a
buffer anyway (no one is using the stack) so replacing the
caller allocated buffer with an LSM managed scheme ought not
to be a concern.

> > > This alleviates the problem of the caller having to
> > > guess a length and preallocate a buffer for this function allowing it to
> be
> > > used elsewhere for Labeled NFS. The patch also removed the unused err
> > > parameter. The same conversion is similar to the one used by Al Viro for
> the
> > > security_getprocattr hook.
> > 
> > These are very different use frequency cases.
> > 
> > 
> > 
> > Casey Schaufler
> > casey@schaufler-ca.com
> 
> 
> 


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.

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

If there are no other concerns with this I'd like to propose that it be
queued up for the 2.6.24 merge window. If you want I can repost the
patches.

Dave Quigley

On Fri, 2007-08-17 at 14:21 -0700, Casey Schaufler wrote:
> --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> 
> > On Fri, 2007-08-17 at 12:37 -0700, Casey Schaufler wrote:
> > > --- "David P. Quigley" <dpquigl@tycho.nsa.gov> wrote:
> > > 
> > > > From: David P. Quigley <dpquigl@tycho.nsa.gov>
> > > > 
> > > > This patch modifies the interface to inode_getsecurity to have the
> > function
> > > > return a buffer containing the security blob and its length via
> > parameters
> > > > instead of relying on the calling function to give it an appropriately
> > sized
> > > > buffer. Security blobs obtained with this function should be freed using
> > the
> > > > release_secctx LSM hook.
> > > 
> > > You are making the assumption that the LSM is going to return a secctx
> > > for all possible named attributes. The caller already has to know the
> > > name of the interesting attribute, that should imply that the caller
> > > has enough information to get the right size for a buffer. I don't think
> > > much of interfaces that require you to allocate memory that you're
> > > just going to throw away after you glance at the data, especially in
> > > the file system lookup path.
> > 
> > Ok sorry I read this a couple of more times and I understand this now.
> > Your issue isn't a concern. This is going to be done somewhere for an
> > lsm since it has to allocate space to store the information internally
> > and then copy it into the buffer provided by getxattr. In your case
> > there isn't even an allocation and freeing of data here. In SELinux we
> > had to allocate a string to store the converted sid in and then copy it
> > up into the buffer internally. Regardless a copy is going to happen the
> > question is where.
> 
> I checked and indeed everyone who calls this is allocating a
> buffer anyway (no one is using the stack) so replacing the
> caller allocated buffer with an LSM managed scheme ought not
> to be a concern.
> 
> > > > This alleviates the problem of the caller having to
> > > > guess a length and preallocate a buffer for this function allowing it to
> > be
> > > > used elsewhere for Labeled NFS. The patch also removed the unused err
> > > > parameter. The same conversion is similar to the one used by Al Viro for
> > the
> > > > security_getprocattr hook.
> > > 
> > > These are very different use frequency cases.
> > > 
> > > 
> > > 
> > > Casey Schaufler
> > > casey@schaufler-ca.com
> > 
> > 
> > 
> 
> 
> 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.


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

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Casey Schaufler]

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

> If there are no other concerns with this I'd like to propose that it be
> queued up for the 2.6.24 merge window. If you want I can repost the
> patches.

My concerns were addressed by education. Proceed as you will.

Oh, and you can queue up the Smack patch while you're at it.

(Joshua says I should put a smiley on when I say things
like this. :) )


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.

[PATCH 2/2] VFS: Reorder vfs_getxattr to avoid unnecessary calls to the LSM

[David P. Quigley]

From: David P. Quigley <dpquigl@tycho.nsa.gov>

	Originally vfs_getxattr would pull the security xattr variable using
the inode getxattr handle and then proceed to clobber it with a subsequent call
to the LSM. This patch reorders the two operations such that when the xattr
requested is in the security namespace it first attempts to grab the value from
the LSM directly. If it fails to obtain the value because there is no module
present or the module does not support the operation it will fall back to using
the inode getxattr operation. In the event that both are inaccessible it
returns EOPNOTSUPP.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/xattr.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 6d59a72..e04a030 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -142,11 +142,6 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 	if (error)
 		return error;
 
-	if (inode->i_op->getxattr)
-		error = inode->i_op->getxattr(dentry, name, value, size);
-	else
-		error = -EOPNOTSUPP;
-
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
@@ -155,9 +150,15 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 		 * Only overwrite the return value if a security module
 		 * is actually active.
 		 */
-		if (ret != -EOPNOTSUPP)
-			error = ret;
+		if (ret == -EOPNOTSUPP)
+			goto nolsm;
+		return ret;
 	}
+nolsm:
+	if (inode->i_op->getxattr)
+		error = inode->i_op->getxattr(dentry, name, value, size);
+	else
+		error = -EOPNOTSUPP;
 
 	return error;
 }
-- 
1.5.2.2


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

[RFC 0/2] getsecurity/vfs_getxattr cleanup

[David P. Quigley]

This patch series addresses two concerns. Currently when a developer
wishes to obtain a security blob from the LSM he/she has to guess at the
length of the blob being returned. We modify security_inode_getsecurity
to return an appropriately sized buffer populated with the security
information and the length of that buffer. This is similar to the
approach taken by Al Viro for the security_getprocattr hook. 

The second concern that this patch set addresses is that vfs_getxattr
reads the security xattr using inode_getxattr and then proceeds to
clobber it with a subsequent call to the LSM. This is fixed by
reordering vfs_getxattr.

The series applies on top of 2.6.23 aka git hash
bbf25010f1a6b761914430f5fca081ec8c7accd1

[PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

	This patch modifies the interface to inode_getsecurity to have the
function return a buffer containing the security blob and its length via
parameters instead of relying on the calling function to give it an
appropriately sized buffer. Security blobs obtained with this function
should be freed using the release_secctx LSM hook. This alleviates the
problem of the caller having to guess a length and preallocate a buffer
for this function allowing it to be used elsewhere for Labeled NFS. The
patch also removed the unused err parameter. The conversion is similar
to the one performed by Al Viro for the security_getprocattr hook.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/xattr.c               |   26 ++++++++++++++++++++++++--
 include/linux/security.h |   27 ++++++++++++++-------------
 include/linux/xattr.h    |    1 +
 mm/shmem.c               |    3 +--
 security/dummy.c         |    4 +++-
 security/selinux/hooks.c |   38 ++++++++++----------------------------
 6 files changed, 53 insertions(+), 46 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index a44fd92..d45c7ef 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -105,6 +105,29 @@ out:
 EXPORT_SYMBOL_GPL(vfs_setxattr);
 
 ssize_t
+xattr_getsecurity(struct inode *inode, const char *name, void *value,
+			size_t size)
+{
+	void *buffer = NULL;
+	ssize_t len;
+
+	len = security_inode_getsecurity(inode, name, &buffer);
+	if (len < 0)
+		return len;
+	if (!value || !size)
+		goto out;
+	if (size < len) {
+		len = -ERANGE;
+		goto out;
+	}
+	memcpy(value, buffer, len);
+out:
+	security_release_secctx(buffer, len);
+	return len;
+}
+EXPORT_SYMBOL_GPL(xattr_getsecurity);
+
+ssize_t
 vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 {
 	struct inode *inode = dentry->d_inode;
@@ -126,8 +149,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
-		int ret = security_inode_getsecurity(inode, suffix, value,
-						     size, error);
+		int ret = xattr_getsecurity(inode, suffix, value, size);
 		/*
 		 * Only overwrite the return value if a security module
 		 * is actually active.
diff --git a/include/linux/security.h b/include/linux/security.h
index 1a15526..8658929 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -391,15 +391,11 @@ struct request_sock;
  * 	identified by @name for @dentry.
  * 	Return 0 if permission is granted.
  * @inode_getsecurity:
- *	Copy the extended attribute representation of the security label 
- *	associated with @name for @inode into @buffer.  @buffer may be
- *	NULL to request the size of the buffer required.  @size indicates
- *	the size of @buffer in bytes.  Note that @name is the remainder
- *	of the attribute name after the security. prefix has been removed.
- *	@err is the return value from the preceding fs getxattr call,
- *	and can be used by the security module to determine whether it
- *	should try and canonicalize the attribute value.
- *	Return number of bytes used/required on success.
+ *	Retrieve a copy of the extended attribute representation of the
+ *	security label associated with @name for @inode via @buffer.  Note that
+ *	@name is the remainder of the attribute name after the security prefix
+ *	has been removed.
+ *	Return size of buffer on success.
  * @inode_setsecurity:
  *	Set the security label associated with @name for @inode from the
  *	extended attribute value @value.  @size indicates the size of the
@@ -1233,7 +1229,8 @@ struct security_operations {
 	int (*inode_listxattr) (struct dentry *dentry);
 	int (*inode_removexattr) (struct dentry *dentry, char *name);
 	const char *(*inode_xattr_getsuffix) (void);
-  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
+	int (*inode_getsecurity)(const struct inode *inode, const char *name,
+					void **buffer);
   	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
   	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
 
@@ -1777,11 +1774,13 @@ static inline const char *security_inode_xattr_getsuffix(void)
 	return security_ops->inode_xattr_getsuffix();
 }
 
-static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static inline int security_inode_getsecurity(const struct inode *inode,
+						const char *name,
+						void **buffer)
 {
 	if (unlikely (IS_PRIVATE (inode)))
 		return 0;
-	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
+	return security_ops->inode_getsecurity(inode, name, buffer);
 }
 
 static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
@@ -2468,7 +2467,9 @@ static inline const char *security_inode_xattr_getsuffix (void)
 	return NULL ;
 }
 
-static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static inline int security_inode_getsecurity(const struct inode *inode,
+						const char *name,
+						void **buffer)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index def131a..df6b95d 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -46,6 +46,7 @@ struct xattr_handler {
 		   size_t size, int flags);
 };
 
+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);
diff --git a/mm/shmem.c b/mm/shmem.c
index fcd19d3..5ed3a66 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1958,8 +1958,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	return security_inode_getsecurity(inode, name, buffer, size,
-					  -EOPNOTSUPP);
+	return xattr_getsecurity(inode, name, buffer, size);
 }
 
 static int shmem_xattr_security_set(struct inode *inode, const char *name,
diff --git a/security/dummy.c b/security/dummy.c
index 853ec22..6983db9 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -377,7 +377,9 @@ static int dummy_inode_removexattr (struct dentry *dentry, char *name)
 	return 0;
 }
 
-static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int dummy_inode_getsecurity(const struct inode *inode,
+					const char *name,
+					void **buffer)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 0753b20..2756c99 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -125,32 +125,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
 
 static struct kmem_cache *sel_inode_cache;
 
-/* Return security context for a given sid or just the context 
-   length if the buffer is null or length is 0 */
-static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
-{
-	char *context;
-	unsigned len;
-	int rc;
-
-	rc = security_sid_to_context(sid, &context, &len);
-	if (rc)
-		return rc;
-
-	if (!buffer || !size)
-		goto getsecurity_exit;
-
-	if (size < len) {
-		len = -ERANGE;
-		goto getsecurity_exit;
-	}
-	memcpy(buffer, context, len);
-
-getsecurity_exit:
-	kfree(context);
-	return len;
-}
-
 /* Allocate and free functions for each kind of security blob. */
 
 static int task_alloc_security(struct task_struct *task)
@@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
  *
  * Permission check is handled by selinux_inode_getxattr hook.
  */
-static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int selinux_inode_getsecurity(const struct inode *inode,
+					const char *name,
+					void **buffer)
 {
+	u32 size;
+	int error;
 	struct inode_security_struct *isec = inode->i_security;
 
 	if (strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
 
-	return selinux_getsecurity(isec->sid, buffer, size);
+	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
+	if (error)
+		return error;
+	error = size;
+	return error;
 }
 
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[James Morris]

On Mon, 22 Oct 2007, David P. Quigley wrote:

> +static inline int security_inode_getsecurity(const struct inode *inode,
> +						const char *name,
> +						void **buffer)

It's better to keep function declarations on one line if possible (the 
80-col rule can be broken for this).

But in any case, it looks ok to me.


Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> 	This patch modifies the interface to inode_getsecurity to have the
> function return a buffer containing the security blob and its length via
> parameters instead of relying on the calling function to give it an
> appropriately sized buffer. Security blobs obtained with this function
> should be freed using the release_secctx LSM hook. This alleviates the
> problem of the caller having to guess a length and preallocate a buffer
> for this function allowing it to be used elsewhere for Labeled NFS. The
> patch also removed the unused err parameter. The conversion is similar
> to the one performed by Al Viro for the security_getprocattr hook.
> 
> Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> ---
>  fs/xattr.c               |   26 ++++++++++++++++++++++++--
>  include/linux/security.h |   27 ++++++++++++++-------------
>  include/linux/xattr.h    |    1 +
>  mm/shmem.c               |    3 +--
>  security/dummy.c         |    4 +++-
>  security/selinux/hooks.c |   38 ++++++++++----------------------------

(Hmm, I was about to ask if this diffstat could be complete, as it
doesn't have for instance security/security.c, but I guess this predates
the staticlsm patch...)

>  6 files changed, 53 insertions(+), 46 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index a44fd92..d45c7ef 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -105,6 +105,29 @@ out:
>  EXPORT_SYMBOL_GPL(vfs_setxattr);
>  
>  ssize_t
> +xattr_getsecurity(struct inode *inode, const char *name, void *value,
> +			size_t size)
> +{
> +	void *buffer = NULL;
> +	ssize_t len;
> +
> +	len = security_inode_getsecurity(inode, name, &buffer);
> +	if (len < 0)
> +		return len;
> +	if (!value || !size)
> +		goto out;
> +	if (size < len) {
> +		len = -ERANGE;
> +		goto out;
> +	}
> +	memcpy(value, buffer, len);
> +out:
> +	security_release_secctx(buffer, len);

This is mighty misleading in -ERANGE case :)  I realize that
selinux_release_secctx() ignores len anyway.  But given the description
in security.h, I'd say either you need to keep the actual length
allocated and pass that in here, or (probably better) have another patch
remove the second argument from security_release_secctx().

> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(xattr_getsecurity);
> +
> +ssize_t
>  vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
>  {
>  	struct inode *inode = dentry->d_inode;
> @@ -126,8 +149,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
>  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
>  				XATTR_SECURITY_PREFIX_LEN)) {
>  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> -		int ret = security_inode_getsecurity(inode, suffix, value,
> -						     size, error);
> +		int ret = xattr_getsecurity(inode, suffix, value, size);
>  		/*
>  		 * Only overwrite the return value if a security module
>  		 * is actually active.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1a15526..8658929 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -391,15 +391,11 @@ struct request_sock;
>   * 	identified by @name for @dentry.
>   * 	Return 0 if permission is granted.
>   * @inode_getsecurity:
> - *	Copy the extended attribute representation of the security label 
> - *	associated with @name for @inode into @buffer.  @buffer may be
> - *	NULL to request the size of the buffer required.  @size indicates
> - *	the size of @buffer in bytes.  Note that @name is the remainder
> - *	of the attribute name after the security. prefix has been removed.
> - *	@err is the return value from the preceding fs getxattr call,
> - *	and can be used by the security module to determine whether it
> - *	should try and canonicalize the attribute value.
> - *	Return number of bytes used/required on success.
> + *	Retrieve a copy of the extended attribute representation of the
> + *	security label associated with @name for @inode via @buffer.  Note that
> + *	@name is the remainder of the attribute name after the security prefix
> + *	has been removed.
> + *	Return size of buffer on success.
>   * @inode_setsecurity:
>   *	Set the security label associated with @name for @inode from the
>   *	extended attribute value @value.  @size indicates the size of the
> @@ -1233,7 +1229,8 @@ struct security_operations {
>  	int (*inode_listxattr) (struct dentry *dentry);
>  	int (*inode_removexattr) (struct dentry *dentry, char *name);
>  	const char *(*inode_xattr_getsuffix) (void);
> -  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
> +	int (*inode_getsecurity)(const struct inode *inode, const char *name,
> +					void **buffer);
>    	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
>    	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
>  
> @@ -1777,11 +1774,13 @@ static inline const char *security_inode_xattr_getsuffix(void)
>  	return security_ops->inode_xattr_getsuffix();
>  }
>  
> -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static inline int security_inode_getsecurity(const struct inode *inode,
> +						const char *name,
> +						void **buffer)
>  {
>  	if (unlikely (IS_PRIVATE (inode)))
>  		return 0;
> -	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
> +	return security_ops->inode_getsecurity(inode, name, buffer);
>  }
>  
>  static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> @@ -2468,7 +2467,9 @@ static inline const char *security_inode_xattr_getsuffix (void)
>  	return NULL ;
>  }
>  
> -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static inline int security_inode_getsecurity(const struct inode *inode,
> +						const char *name,
> +						void **buffer)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index def131a..df6b95d 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -46,6 +46,7 @@ struct xattr_handler {
>  		   size_t size, int flags);
>  };
>  
> +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);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index fcd19d3..5ed3a66 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1958,8 +1958,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
>  {
>  	if (strcmp(name, "") == 0)
>  		return -EINVAL;
> -	return security_inode_getsecurity(inode, name, buffer, size,
> -					  -EOPNOTSUPP);
> +	return xattr_getsecurity(inode, name, buffer, size);
>  }
>  
>  static int shmem_xattr_security_set(struct inode *inode, const char *name,
> diff --git a/security/dummy.c b/security/dummy.c
> index 853ec22..6983db9 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -377,7 +377,9 @@ static int dummy_inode_removexattr (struct dentry *dentry, char *name)
>  	return 0;
>  }
>  
> -static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static int dummy_inode_getsecurity(const struct inode *inode,
> +					const char *name,
> +					void **buffer)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 0753b20..2756c99 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -125,32 +125,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
>  
>  static struct kmem_cache *sel_inode_cache;
>  
> -/* Return security context for a given sid or just the context 
> -   length if the buffer is null or length is 0 */
> -static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> -{
> -	char *context;
> -	unsigned len;
> -	int rc;
> -
> -	rc = security_sid_to_context(sid, &context, &len);
> -	if (rc)
> -		return rc;
> -
> -	if (!buffer || !size)
> -		goto getsecurity_exit;
> -
> -	if (size < len) {
> -		len = -ERANGE;
> -		goto getsecurity_exit;
> -	}
> -	memcpy(buffer, context, len);
> -
> -getsecurity_exit:
> -	kfree(context);
> -	return len;
> -}
> -
>  /* Allocate and free functions for each kind of security blob. */
>  
>  static int task_alloc_security(struct task_struct *task)
> @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
>   *
>   * Permission check is handled by selinux_inode_getxattr hook.
>   */
> -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static int selinux_inode_getsecurity(const struct inode *inode,
> +					const char *name,
> +					void **buffer)
>  {
> +	u32 size;
> +	int error;
>  	struct inode_security_struct *isec = inode->i_security;
>  
>  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
>  		return -EOPNOTSUPP;
>  
> -	return selinux_getsecurity(isec->sid, buffer, size);
> +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);

The only other downside I see here is that when the user just passes in
NULL for a buffer, security_sid_to_context() will still
kmalloc the buffer only to have it immediately freed by
xattr_getsecurity() through release_secctx().  I trust that isn't seen
as any major performance impact?

thanks,
-serge

> +	if (error)
> +		return error;
> +	error = size;
> +	return error;
>  }
>  
>  static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> 
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > 	This patch modifies the interface to inode_getsecurity to have the
> > function return a buffer containing the security blob and its length via
> > parameters instead of relying on the calling function to give it an
> > appropriately sized buffer. Security blobs obtained with this function
> > should be freed using the release_secctx LSM hook. This alleviates the
> > problem of the caller having to guess a length and preallocate a buffer
> > for this function allowing it to be used elsewhere for Labeled NFS. The
> > patch also removed the unused err parameter. The conversion is similar
> > to the one performed by Al Viro for the security_getprocattr hook.
> > 
> > Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> > ---
> >  fs/xattr.c               |   26 ++++++++++++++++++++++++--
> >  include/linux/security.h |   27 ++++++++++++++-------------
> >  include/linux/xattr.h    |    1 +
> >  mm/shmem.c               |    3 +--
> >  security/dummy.c         |    4 +++-
> >  security/selinux/hooks.c |   38 ++++++++++----------------------------
> 
> (Hmm, I was about to ask if this diffstat could be complete, as it
> doesn't have for instance security/security.c, but I guess this predates
> the staticlsm patch...)

It wouldn't be much effort to rebase this patch against Linus's latest
tree. I am assuming that the static lsm patch is in there based on the
recent discussion on LKML?

> 
> >  6 files changed, 53 insertions(+), 46 deletions(-)
> > 
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index a44fd92..d45c7ef 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -105,6 +105,29 @@ out:
> >  EXPORT_SYMBOL_GPL(vfs_setxattr);
> >  
> >  ssize_t
> > +xattr_getsecurity(struct inode *inode, const char *name, void *value,
> > +			size_t size)
> > +{
> > +	void *buffer = NULL;
> > +	ssize_t len;
> > +
> > +	len = security_inode_getsecurity(inode, name, &buffer);
> > +	if (len < 0)
> > +		return len;
> > +	if (!value || !size)
> > +		goto out;
> > +	if (size < len) {
> > +		len = -ERANGE;
> > +		goto out;
> > +	}
> > +	memcpy(value, buffer, len);
> > +out:
> > +	security_release_secctx(buffer, len);
> 
> This is mighty misleading in -ERANGE case :)  I realize that
> selinux_release_secctx() ignores len anyway.  But given the description
> in security.h, I'd say either you need to keep the actual length
> allocated and pass that in here, or (probably better) have another patch
> remove the second argument from security_release_secctx().

I would consider this a bug on my part. I don't think we can get rid of
the len argument to security_release_secctx because it is supposed to be
a generic destructor for security blobs. If the module always returned a
fixed length blob we could remove it but there is no telling what one of
the many new LSMs to come will do. 

> 
> > +	return len;
> > +}
> > +EXPORT_SYMBOL_GPL(xattr_getsecurity);
> > +
> > +ssize_t
> >  vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
> >  {
> >  	struct inode *inode = dentry->d_inode;
> > @@ -126,8 +149,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
> >  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
> >  				XATTR_SECURITY_PREFIX_LEN)) {
> >  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> > -		int ret = security_inode_getsecurity(inode, suffix, value,
> > -						     size, error);
> > +		int ret = xattr_getsecurity(inode, suffix, value, size);
> >  		/*
> >  		 * Only overwrite the return value if a security module
> >  		 * is actually active.
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 1a15526..8658929 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -391,15 +391,11 @@ struct request_sock;
> >   * 	identified by @name for @dentry.
> >   * 	Return 0 if permission is granted.
> >   * @inode_getsecurity:
> > - *	Copy the extended attribute representation of the security label 
> > - *	associated with @name for @inode into @buffer.  @buffer may be
> > - *	NULL to request the size of the buffer required.  @size indicates
> > - *	the size of @buffer in bytes.  Note that @name is the remainder
> > - *	of the attribute name after the security. prefix has been removed.
> > - *	@err is the return value from the preceding fs getxattr call,
> > - *	and can be used by the security module to determine whether it
> > - *	should try and canonicalize the attribute value.
> > - *	Return number of bytes used/required on success.
> > + *	Retrieve a copy of the extended attribute representation of the
> > + *	security label associated with @name for @inode via @buffer.  Note that
> > + *	@name is the remainder of the attribute name after the security prefix
> > + *	has been removed.
> > + *	Return size of buffer on success.
> >   * @inode_setsecurity:
> >   *	Set the security label associated with @name for @inode from the
> >   *	extended attribute value @value.  @size indicates the size of the
> > @@ -1233,7 +1229,8 @@ struct security_operations {
> >  	int (*inode_listxattr) (struct dentry *dentry);
> >  	int (*inode_removexattr) (struct dentry *dentry, char *name);
> >  	const char *(*inode_xattr_getsuffix) (void);
> > -  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
> > +	int (*inode_getsecurity)(const struct inode *inode, const char *name,
> > +					void **buffer);
> >    	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
> >    	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
> >  
> > @@ -1777,11 +1774,13 @@ static inline const char *security_inode_xattr_getsuffix(void)
> >  	return security_ops->inode_xattr_getsuffix();
> >  }
> >  
> > -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static inline int security_inode_getsecurity(const struct inode *inode,
> > +						const char *name,
> > +						void **buffer)
> >  {
> >  	if (unlikely (IS_PRIVATE (inode)))
> >  		return 0;
> > -	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
> > +	return security_ops->inode_getsecurity(inode, name, buffer);
> >  }
> >  
> >  static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> > @@ -2468,7 +2467,9 @@ static inline const char *security_inode_xattr_getsuffix (void)
> >  	return NULL ;
> >  }
> >  
> > -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static inline int security_inode_getsecurity(const struct inode *inode,
> > +						const char *name,
> > +						void **buffer)
> >  {
> >  	return -EOPNOTSUPP;
> >  }
> > diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> > index def131a..df6b95d 100644
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -46,6 +46,7 @@ struct xattr_handler {
> >  		   size_t size, int flags);
> >  };
> >  
> > +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);
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index fcd19d3..5ed3a66 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -1958,8 +1958,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
> >  {
> >  	if (strcmp(name, "") == 0)
> >  		return -EINVAL;
> > -	return security_inode_getsecurity(inode, name, buffer, size,
> > -					  -EOPNOTSUPP);
> > +	return xattr_getsecurity(inode, name, buffer, size);
> >  }
> >  
> >  static int shmem_xattr_security_set(struct inode *inode, const char *name,
> > diff --git a/security/dummy.c b/security/dummy.c
> > index 853ec22..6983db9 100644
> > --- a/security/dummy.c
> > +++ b/security/dummy.c
> > @@ -377,7 +377,9 @@ static int dummy_inode_removexattr (struct dentry *dentry, char *name)
> >  	return 0;
> >  }
> >  
> > -static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static int dummy_inode_getsecurity(const struct inode *inode,
> > +					const char *name,
> > +					void **buffer)
> >  {
> >  	return -EOPNOTSUPP;
> >  }
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 0753b20..2756c99 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -125,32 +125,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
> >  
> >  static struct kmem_cache *sel_inode_cache;
> >  
> > -/* Return security context for a given sid or just the context 
> > -   length if the buffer is null or length is 0 */
> > -static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> > -{
> > -	char *context;
> > -	unsigned len;
> > -	int rc;
> > -
> > -	rc = security_sid_to_context(sid, &context, &len);
> > -	if (rc)
> > -		return rc;
> > -
> > -	if (!buffer || !size)
> > -		goto getsecurity_exit;
> > -
> > -	if (size < len) {
> > -		len = -ERANGE;
> > -		goto getsecurity_exit;
> > -	}
> > -	memcpy(buffer, context, len);
> > -
> > -getsecurity_exit:
> > -	kfree(context);
> > -	return len;
> > -}
> > -
> >  /* Allocate and free functions for each kind of security blob. */
> >  
> >  static int task_alloc_security(struct task_struct *task)
> > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> >   *
> >   * Permission check is handled by selinux_inode_getxattr hook.
> >   */
> > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > +static int selinux_inode_getsecurity(const struct inode *inode,
> > +					const char *name,
> > +					void **buffer)
> >  {
> > +	u32 size;
> > +	int error;
> >  	struct inode_security_struct *isec = inode->i_security;
> >  
> >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> >  		return -EOPNOTSUPP;
> >  
> > -	return selinux_getsecurity(isec->sid, buffer, size);
> > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> 
> The only other downside I see here is that when the user just passes in
> NULL for a buffer, security_sid_to_context() will still
> kmalloc the buffer only to have it immediately freed by
> xattr_getsecurity() through release_secctx().  I trust that isn't seen
> as any major performance impact?

There is no way to avoid this in the SELinux case. SELinux doesn't store
the sid to string mapping directly. Rather it takes the sid and then
builds the string from fields in the related structure. So regardless
this data is being allocated internally. The only issue I potentially
see is that if someone passes in null expecting just to get the length
we are actually returning a value. However we are changing the semantics
of the function so the old semantics are no longer valid.

In the case of SMACK this is equally irrelevant since there is no
allocation at all just the returning of a pointer into a table via
buffer. I don't know enough about other potential LSMs to comment on the
impact on their code bases.

> 
> thanks,
> -serge
> 
> > +	if (error)
> > +		return error;
> > +	error = size;
> > +	return error;
> >  }
> >  
> >  static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> > 
> > -
> > 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
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > >  static int task_alloc_security(struct task_struct *task)
> > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > >   *
> > >   * Permission check is handled by selinux_inode_getxattr hook.
> > >   */
> > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > +					const char *name,
> > > +					void **buffer)
> > >  {
> > > +	u32 size;
> > > +	int error;
> > >  	struct inode_security_struct *isec = inode->i_security;
> > >  
> > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > >  		return -EOPNOTSUPP;
> > >  
> > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > 
> > The only other downside I see here is that when the user just passes in
> > NULL for a buffer, security_sid_to_context() will still
> > kmalloc the buffer only to have it immediately freed by
> > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > as any major performance impact?
> 
> There is no way to avoid this in the SELinux case. SELinux doesn't store
> the sid to string mapping directly. Rather it takes the sid and then
> builds the string from fields in the related structure. So regardless
> this data is being allocated internally. The only issue I potentially
> see is that if someone passes in null expecting just to get the length
> we are actually returning a value. However we are changing the semantics
> of the function so the old semantics are no longer valid.

Hmm?  Which semantics are no longer valid?

You're changing the semantincs of the in-kernel API, but userspace can
still send in NULL to query the length of the buffer needed.  So if
userspace does two getxattrs, one to get the length, then another to get
the value, selinux will be kmallocing twice.

For a file manager doing a listing on a huge directory and wanting to
list the selinux type, i could see that being a performance issue.  Of
course they could get around that by sending in a 'reasonably large'
buffer for a first try.

-serge

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Stephen Smalley]

On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > >  static int task_alloc_security(struct task_struct *task)
> > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > >   *
> > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > >   */
> > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > +					const char *name,
> > > > +					void **buffer)
> > > >  {
> > > > +	u32 size;
> > > > +	int error;
> > > >  	struct inode_security_struct *isec = inode->i_security;
> > > >  
> > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > >  		return -EOPNOTSUPP;
> > > >  
> > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > 
> > > The only other downside I see here is that when the user just passes in
> > > NULL for a buffer, security_sid_to_context() will still
> > > kmalloc the buffer only to have it immediately freed by
> > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > as any major performance impact?
> > 
> > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > the sid to string mapping directly. Rather it takes the sid and then
> > builds the string from fields in the related structure. So regardless
> > this data is being allocated internally. The only issue I potentially
> > see is that if someone passes in null expecting just to get the length
> > we are actually returning a value. However we are changing the semantics
> > of the function so the old semantics are no longer valid.
> 
> Hmm?  Which semantics are no longer valid?
> 
> You're changing the semantincs of the in-kernel API, but userspace can
> still send in NULL to query the length of the buffer needed.  So if
> userspace does two getxattrs, one to get the length, then another to get
> the value, selinux will be kmallocing twice.
> 
> For a file manager doing a listing on a huge directory and wanting to
> list the selinux type, i could see that being a performance issue.  Of
> course they could get around that by sending in a 'reasonably large'
> buffer for a first try.

That's what current userland does. libselinux always tries with an
initial buffer first (and usually succeeds), thereby avoiding the second
call to getxattr in the common case.

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting Stephen Smalley (sds@tycho.nsa.gov):
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > >  static int task_alloc_security(struct task_struct *task)
> > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > >   *
> > > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > > >   */
> > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > +					const char *name,
> > > > > +					void **buffer)
> > > > >  {
> > > > > +	u32 size;
> > > > > +	int error;
> > > > >  	struct inode_security_struct *isec = inode->i_security;
> > > > >  
> > > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > >  		return -EOPNOTSUPP;
> > > > >  
> > > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > > 
> > > > The only other downside I see here is that when the user just passes in
> > > > NULL for a buffer, security_sid_to_context() will still
> > > > kmalloc the buffer only to have it immediately freed by
> > > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > > as any major performance impact?
> > > 
> > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > the sid to string mapping directly. Rather it takes the sid and then
> > > builds the string from fields in the related structure. So regardless
> > > this data is being allocated internally. The only issue I potentially
> > > see is that if someone passes in null expecting just to get the length
> > > we are actually returning a value. However we are changing the semantics
> > > of the function so the old semantics are no longer valid.
> > 
> > Hmm?  Which semantics are no longer valid?
> > 
> > You're changing the semantincs of the in-kernel API, but userspace can
> > still send in NULL to query the length of the buffer needed.  So if
> > userspace does two getxattrs, one to get the length, then another to get
> > the value, selinux will be kmallocing twice.
> > 
> > For a file manager doing a listing on a huge directory and wanting to
> > list the selinux type, i could see that being a performance issue.  Of
> > course they could get around that by sending in a 'reasonably large'
> > buffer for a first try.
> 
> That's what current userland does. libselinux always tries with an
> initial buffer first (and usually succeeds), thereby avoiding the second
> call to getxattr in the common case.

Ok - I figured for doing thousands of these in one directory listing
that could waste quite a bit of memory, but since (as i check) selinux
has always done a kmalloc for every getsecurity call, I guess it's a
fair tradeoff

thanks,
-serge

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > >  static int task_alloc_security(struct task_struct *task)
> > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > >   *
> > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > >   */
> > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > +					const char *name,
> > > > +					void **buffer)
> > > >  {
> > > > +	u32 size;
> > > > +	int error;
> > > >  	struct inode_security_struct *isec = inode->i_security;
> > > >  
> > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > >  		return -EOPNOTSUPP;
> > > >  
> > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > 
> > > The only other downside I see here is that when the user just passes in
> > > NULL for a buffer, security_sid_to_context() will still
> > > kmalloc the buffer only to have it immediately freed by
> > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > as any major performance impact?
> > 
> > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > the sid to string mapping directly. Rather it takes the sid and then
> > builds the string from fields in the related structure. So regardless
> > this data is being allocated internally. The only issue I potentially
> > see is that if someone passes in null expecting just to get the length
> > we are actually returning a value. However we are changing the semantics
> > of the function so the old semantics are no longer valid.
> 
> Hmm?  Which semantics are no longer valid?
> 
> You're changing the semantincs of the in-kernel API, but userspace can
> still send in NULL to query the length of the buffer needed.  So if
> userspace does two getxattrs, one to get the length, then another to get
> the value, selinux will be kmallocing twice.


We are changing the semantics of the LSM hook. From a memory allocation
stand point the only difference is who allocates the buffer. Before this
patch we would allocate one outside of the LSM hook and then fill it
(even if null were passed). Since the LSM hook is in a better position
to accurately create this buffer we pushed the allocation to it.

> For a file manager doing a listing on a huge directory and wanting to
> list the selinux type, i could see that being a performance issue.  Of
> course they could get around that by sending in a 'reasonably large'
> buffer for a first try.

I'd agree but these allocations have always been happening and no one
has seen a problem with it. There should be the same number of
allocations as before the question is where are they done.

> 
> -serge

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-10-26 at 11:13 -0400, David P. Quigley wrote:
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > >  static int task_alloc_security(struct task_struct *task)
> > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > >   *
> > > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > > >   */
> > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > +					const char *name,
> > > > > +					void **buffer)
> > > > >  {
> > > > > +	u32 size;
> > > > > +	int error;
> > > > >  	struct inode_security_struct *isec = inode->i_security;
> > > > >  
> > > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > >  		return -EOPNOTSUPP;
> > > > >  
> > > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > > 
> > > > The only other downside I see here is that when the user just passes in
> > > > NULL for a buffer, security_sid_to_context() will still
> > > > kmalloc the buffer only to have it immediately freed by
> > > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > > as any major performance impact?
> > > 
> > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > the sid to string mapping directly. Rather it takes the sid and then
> > > builds the string from fields in the related structure. So regardless
> > > this data is being allocated internally. The only issue I potentially
> > > see is that if someone passes in null expecting just to get the length
> > > we are actually returning a value. However we are changing the semantics
> > > of the function so the old semantics are no longer valid.
> > 
> > Hmm?  Which semantics are no longer valid?
> > 
> > You're changing the semantincs of the in-kernel API, but userspace can
> > still send in NULL to query the length of the buffer needed.  So if
> > userspace does two getxattrs, one to get the length, then another to get
> > the value, selinux will be kmallocing twice.
> 
> 
> We are changing the semantics of the LSM hook. From a memory allocation
> stand point the only difference is who allocates the buffer. Before this
> patch we would allocate one outside of the LSM hook and then fill it
> (even if null were passed). Since the LSM hook is in a better position
> to accurately create this buffer we pushed the allocation to it.
> 
> > For a file manager doing a listing on a huge directory and wanting to
> > list the selinux type, i could see that being a performance issue.  Of
> > course they could get around that by sending in a 'reasonably large'
> > buffer for a first try.
> 
> I'd agree but these allocations have always been happening and no one
> has seen a problem with it. There should be the same number of
> allocations as before the question is where are they done.

This statement applies to SELinux. In actuality it is possible for an
LSM to have less allocation than before with this model.

> 
> > 
> > -serge
> 
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > >  static int task_alloc_security(struct task_struct *task)
> > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > >   *
> > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > >   */
> > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > +					const char *name,
> > > > +					void **buffer)
> > > >  {
> > > > +	u32 size;
> > > > +	int error;
> > > >  	struct inode_security_struct *isec = inode->i_security;
> > > >  
> > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > >  		return -EOPNOTSUPP;
> > > >  
> > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > 
> > > The only other downside I see here is that when the user just passes in
> > > NULL for a buffer, security_sid_to_context() will still
> > > kmalloc the buffer only to have it immediately freed by
> > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > as any major performance impact?
> > 
> > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > the sid to string mapping directly. Rather it takes the sid and then
> > builds the string from fields in the related structure. So regardless
> > this data is being allocated internally. The only issue I potentially
> > see is that if someone passes in null expecting just to get the length
> > we are actually returning a value. However we are changing the semantics
> > of the function so the old semantics are no longer valid.
> 
> Hmm?  Which semantics are no longer valid?
> 
> You're changing the semantincs of the in-kernel API, but userspace can
> still send in NULL to query the length of the buffer needed.  So if
> userspace does two getxattrs, one to get the length, then another to get
> the value, selinux will be kmallocing twice.
> 
> For a file manager doing a listing on a huge directory and wanting to
> list the selinux type, i could see that being a performance issue.  Of
> course they could get around that by sending in a 'reasonably large'
> buffer for a first try.
> 

Ok lets start this line of thought over again since it has been a while
since I wrote the patches and got almost no sleep last night. 

Your concerns are that we are double allocating buffers one of which we
are just going to immediately free after a copy. So inside the SELinux
helper function there was what I saw as generic code for handling
xattrs. This can be seen in the new function xattr_getsecurity which use
to be internal to SELinux (selinux_getsecurity). What we are doing is
grabbing the string which internally is being allocated anyway and if
our buffer passed in for the copy is null we just goto out returning the
length and freeing the buffer. So here is our standard null handling
that we had before. In LSMs where there is no internal allocation to
handle the getsecurity call this should introduce almost no overhead.
For example in Casey's latest SMACK patch he has a table of the label
strings and he can pass a pointer directly into that table back via the
security_inode_getsecurity hook. 
	In addition to this completes the lifecycle management that
security_release_secctx attempts to perform. It doesn't seem right that
if we require security_release_secctx to free the data we obtained from
security_inode_getsecurity that the data buffer should be allocated
outside of security_inode_getsecurity.

I hope this clears up any questions/concerns.

> -serge
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > >  static int task_alloc_security(struct task_struct *task)
> > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > >   *
> > > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > > >   */
> > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > +					const char *name,
> > > > > +					void **buffer)
> > > > >  {
> > > > > +	u32 size;
> > > > > +	int error;
> > > > >  	struct inode_security_struct *isec = inode->i_security;
> > > > >  
> > > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > >  		return -EOPNOTSUPP;
> > > > >  
> > > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > > 
> > > > The only other downside I see here is that when the user just passes in
> > > > NULL for a buffer, security_sid_to_context() will still
> > > > kmalloc the buffer only to have it immediately freed by
> > > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > > as any major performance impact?
> > > 
> > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > the sid to string mapping directly. Rather it takes the sid and then
> > > builds the string from fields in the related structure. So regardless
> > > this data is being allocated internally. The only issue I potentially
> > > see is that if someone passes in null expecting just to get the length
> > > we are actually returning a value. However we are changing the semantics
> > > of the function so the old semantics are no longer valid.
> > 
> > Hmm?  Which semantics are no longer valid?
> > 
> > You're changing the semantincs of the in-kernel API, but userspace can
> > still send in NULL to query the length of the buffer needed.  So if
> > userspace does two getxattrs, one to get the length, then another to get
> > the value, selinux will be kmallocing twice.
> > 
> > For a file manager doing a listing on a huge directory and wanting to
> > list the selinux type, i could see that being a performance issue.  Of
> > course they could get around that by sending in a 'reasonably large'
> > buffer for a first try.
> > 
> 
> Ok lets start this line of thought over again since it has been a while
> since I wrote the patches and got almost no sleep last night. 
> 
> Your concerns are that we are double allocating buffers one of which we
> are just going to immediately free after a copy. So inside the SELinux
> helper function there was what I saw as generic code for handling
> xattrs. This can be seen in the new function xattr_getsecurity which use
> to be internal to SELinux (selinux_getsecurity). What we are doing is
> grabbing the string which internally is being allocated anyway and if
> our buffer passed in for the copy is null we just goto out returning the
> length and freeing the buffer. So here is our standard null handling
> that we had before. In LSMs where there is no internal allocation to
> handle the getsecurity call this should introduce almost no overhead.

Ah, thanks, you reminded me of what I was trying to point out.

SMACK won't do allocations so it's ok.  SELinux will do allocations
in any case so it's ok.  So in terms of current users it's fine, so I
don't want to complaint too loudly.

But the now-generic xattr_getsecurity() call passes in 'buffer' from its
stack, with no indication to the LSM of whether userspace passed in NULL
or a buffer.  So if there *were* an lsm which had to allocate space to
return data, but didn't want to do so when the user just asked for the
length of the data, then that LSM would be out of luck.

So would you object to passing in a boolean telling the LSM whether the
user had passed in a buffer in which to return data or not?

thanks,
-serge

> For example in Casey's latest SMACK patch he has a table of the label
> strings and he can pass a pointer directly into that table back via the
> security_inode_getsecurity hook. 
> 	In addition to this completes the lifecycle management that
> security_release_secctx attempts to perform. It doesn't seem right that
> if we require security_release_secctx to free the data we obtained from
> security_inode_getsecurity that the data buffer should be allocated
> outside of security_inode_getsecurity.
> 
> I hope this clears up any questions/concerns.
> 
> > -serge
> > -
> > 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
> 
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-10-26 at 11:36 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > On Fri, 2007-10-26 at 10:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > > >  static int task_alloc_security(struct task_struct *task)
> > > > > > @@ -2423,14 +2397,22 @@ static const char *selinux_inode_xattr_getsuffix(void)
> > > > > >   *
> > > > > >   * Permission check is handled by selinux_inode_getxattr hook.
> > > > > >   */
> > > > > > -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> > > > > > +static int selinux_inode_getsecurity(const struct inode *inode,
> > > > > > +					const char *name,
> > > > > > +					void **buffer)
> > > > > >  {
> > > > > > +	u32 size;
> > > > > > +	int error;
> > > > > >  	struct inode_security_struct *isec = inode->i_security;
> > > > > >  
> > > > > >  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
> > > > > >  		return -EOPNOTSUPP;
> > > > > >  
> > > > > > -	return selinux_getsecurity(isec->sid, buffer, size);
> > > > > > +	error = security_sid_to_context(isec->sid, (char **)buffer, &size);
> > > > > 
> > > > > The only other downside I see here is that when the user just passes in
> > > > > NULL for a buffer, security_sid_to_context() will still
> > > > > kmalloc the buffer only to have it immediately freed by
> > > > > xattr_getsecurity() through release_secctx().  I trust that isn't seen
> > > > > as any major performance impact?
> > > > 
> > > > There is no way to avoid this in the SELinux case. SELinux doesn't store
> > > > the sid to string mapping directly. Rather it takes the sid and then
> > > > builds the string from fields in the related structure. So regardless
> > > > this data is being allocated internally. The only issue I potentially
> > > > see is that if someone passes in null expecting just to get the length
> > > > we are actually returning a value. However we are changing the semantics
> > > > of the function so the old semantics are no longer valid.
> > > 
> > > Hmm?  Which semantics are no longer valid?
> > > 
> > > You're changing the semantincs of the in-kernel API, but userspace can
> > > still send in NULL to query the length of the buffer needed.  So if
> > > userspace does two getxattrs, one to get the length, then another to get
> > > the value, selinux will be kmallocing twice.
> > > 
> > > For a file manager doing a listing on a huge directory and wanting to
> > > list the selinux type, i could see that being a performance issue.  Of
> > > course they could get around that by sending in a 'reasonably large'
> > > buffer for a first try.
> > > 
> > 
> > Ok lets start this line of thought over again since it has been a while
> > since I wrote the patches and got almost no sleep last night. 
> > 
> > Your concerns are that we are double allocating buffers one of which we
> > are just going to immediately free after a copy. So inside the SELinux
> > helper function there was what I saw as generic code for handling
> > xattrs. This can be seen in the new function xattr_getsecurity which use
> > to be internal to SELinux (selinux_getsecurity). What we are doing is
> > grabbing the string which internally is being allocated anyway and if
> > our buffer passed in for the copy is null we just goto out returning the
> > length and freeing the buffer. So here is our standard null handling
> > that we had before. In LSMs where there is no internal allocation to
> > handle the getsecurity call this should introduce almost no overhead.
> 
> Ah, thanks, you reminded me of what I was trying to point out.
> 
> SMACK won't do allocations so it's ok.  SELinux will do allocations
> in any case so it's ok.  So in terms of current users it's fine, so I
> don't want to complaint too loudly.
> 
> But the now-generic xattr_getsecurity() call passes in 'buffer' from its
> stack, with no indication to the LSM of whether userspace passed in NULL
> or a buffer.  So if there *were* an lsm which had to allocate space to
> return data, but didn't want to do so when the user just asked for the
> length of the data, then that LSM would be out of luck.
> 
> So would you object to passing in a boolean telling the LSM whether the
> user had passed in a buffer in which to return data or not?

I don't see why we couldn't add a bool. I am just wondering are there
really use-cases where the developer is going to need this though.
Unfortunately I'm not all knowing so I can't foresee what us "Insane"
security people will do so I think it's a reasonable addition. I'll
rebase the patch and make these changes and hopefully have a new
revision to the list before the end of the day.

Dave

> 
> thanks,
> -serge
> 
> > For example in Casey's latest SMACK patch he has a table of the label
> > strings and he can pass a pointer directly into that table back via the
> > security_inode_getsecurity hook. 
> > 	In addition to this completes the lifecycle management that
> > security_release_secctx attempts to perform. It doesn't seem right that
> > if we require security_release_secctx to free the data we obtained from
> > security_inode_getsecurity that the data buffer should be allocated
> > outside of security_inode_getsecurity.
> > 
> > I hope this clears up any questions/concerns.
> > 
> > > -serge
> > > -
> > > 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
> > 
> > -
> > 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > 	This patch modifies the interface to inode_getsecurity to have the
> > > function return a buffer containing the security blob and its length via
> > > parameters instead of relying on the calling function to give it an
> > > appropriately sized buffer. Security blobs obtained with this function
> > > should be freed using the release_secctx LSM hook. This alleviates the
> > > problem of the caller having to guess a length and preallocate a buffer
> > > for this function allowing it to be used elsewhere for Labeled NFS. The
> > > patch also removed the unused err parameter. The conversion is similar
> > > to the one performed by Al Viro for the security_getprocattr hook.
> > > 
> > > Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> > > ---
> > >  fs/xattr.c               |   26 ++++++++++++++++++++++++--
> > >  include/linux/security.h |   27 ++++++++++++++-------------
> > >  include/linux/xattr.h    |    1 +
> > >  mm/shmem.c               |    3 +--
> > >  security/dummy.c         |    4 +++-
> > >  security/selinux/hooks.c |   38 ++++++++++----------------------------
> > 
> > (Hmm, I was about to ask if this diffstat could be complete, as it
> > doesn't have for instance security/security.c, but I guess this predates
> > the staticlsm patch...)
> 
> It wouldn't be much effort to rebase this patch against Linus's latest
> tree. I am assuming that the static lsm patch is in there based on the
> recent discussion on LKML?

Oh, sorry for the two emails.

Yeah it's in 2.6.24.  So a rebase will be necessary anyway.  I was just
saying I was too lazy to find another tree against which to check that
you didn't miss any getsecurity calls (hidden under some exotic .config)
to change their arguments  :)

-serge

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Fri, 2007-10-26 at 10:07 -0500, Serge E. Hallyn wrote:
> Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > On Thu, 2007-10-25 at 19:02 -0500, Serge E. Hallyn wrote:
> > > Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> > > > 	This patch modifies the interface to inode_getsecurity to have the
> > > > function return a buffer containing the security blob and its length via
> > > > parameters instead of relying on the calling function to give it an
> > > > appropriately sized buffer. Security blobs obtained with this function
> > > > should be freed using the release_secctx LSM hook. This alleviates the
> > > > problem of the caller having to guess a length and preallocate a buffer
> > > > for this function allowing it to be used elsewhere for Labeled NFS. The
> > > > patch also removed the unused err parameter. The conversion is similar
> > > > to the one performed by Al Viro for the security_getprocattr hook.
> > > > 
> > > > Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
> > > > ---
> > > >  fs/xattr.c               |   26 ++++++++++++++++++++++++--
> > > >  include/linux/security.h |   27 ++++++++++++++-------------
> > > >  include/linux/xattr.h    |    1 +
> > > >  mm/shmem.c               |    3 +--
> > > >  security/dummy.c         |    4 +++-
> > > >  security/selinux/hooks.c |   38 ++++++++++----------------------------
> > > 
> > > (Hmm, I was about to ask if this diffstat could be complete, as it
> > > doesn't have for instance security/security.c, but I guess this predates
> > > the staticlsm patch...)
> > 
> > It wouldn't be much effort to rebase this patch against Linus's latest
> > tree. I am assuming that the static lsm patch is in there based on the
> > recent discussion on LKML?
> 
> Oh, sorry for the two emails.
> 
> Yeah it's in 2.6.24.  So a rebase will be necessary anyway.  I was just
> saying I was too lazy to find another tree against which to check that
> you didn't miss any getsecurity calls (hidden under some exotic .config)
> to change their arguments  :)

I used the LXR to get all uses of getsecurity so I am pretty sure I have
them all.

> 
> -serge
> -
> 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 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[James Morris]

On Fri, 26 Oct 2007, Serge E. Hallyn wrote:

> > It wouldn't be much effort to rebase this patch against Linus's latest
> > tree. I am assuming that the static lsm patch is in there based on the
> > recent discussion on LKML?
> 
> Oh, sorry for the two emails.
> 
> Yeah it's in 2.6.24.  So a rebase will be necessary anyway. 

That code may still change -- Arjan's update, for example.

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

On Sat, 2007-10-27 at 08:14 +1000, James Morris wrote:
> On Fri, 26 Oct 2007, Serge E. Hallyn wrote:
> 
> > > It wouldn't be much effort to rebase this patch against Linus's latest
> > > tree. I am assuming that the static lsm patch is in there based on the
> > > recent discussion on LKML?
> > 
> > Oh, sorry for the two emails.
> > 
> > Yeah it's in 2.6.24.  So a rebase will be necessary anyway. 
> 
> That code may still change -- Arjan's update, for example.
> 

What do you think should be done with this patch set? We have already
gone past the 2.6.24 merge window so it seems as if it will have to be
put into 2.6.25. Should I wait until 2.6.24 is released and repost it
then? Is it possible for them to get into 2.6.24 since its early in the
rc cycle?

Dave

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[James Morris]

On Wed, 31 Oct 2007, David P. Quigley wrote:

> On Sat, 2007-10-27 at 08:14 +1000, James Morris wrote:
> > On Fri, 26 Oct 2007, Serge E. Hallyn wrote:
> > 
> > > > It wouldn't be much effort to rebase this patch against Linus's latest
> > > > tree. I am assuming that the static lsm patch is in there based on the
> > > > recent discussion on LKML?
> > > 
> > > Oh, sorry for the two emails.
> > > 
> > > Yeah it's in 2.6.24.  So a rebase will be necessary anyway. 
> > 
> > That code may still change -- Arjan's update, for example.
> > 
> 
> What do you think should be done with this patch set? We have already
> gone past the 2.6.24 merge window so it seems as if it will have to be
> put into 2.6.25. Should I wait until 2.6.24 is released and repost it
> then? Is it possible for them to get into 2.6.24 since its early in the
> rc cycle?

Not as far as I'm aware.


-- 
James Morris
<jmorris@namei.org>

[PATCH 0/2] getsecurity/vfs_getxattr cleanup V2

[David P. Quigley]

This patch series addresses two concerns. Currently when a developer
wishes to obtain a security blob from the LSM he/she has to guess at the
length of the blob being returned. We modify security_inode_getsecurity
to return an appropriately sized buffer populated with the security
information and the length of that buffer. This is similar to the
approach taken by Al Viro for the security_getprocattr hook. 

The second concern that this patch set addresses is that vfs_getxattr
reads the security xattr using inode_getxattr and then proceeds to
clobber it with a subsequent call to the LSM. This is fixed by
reordering vfs_getxattr.

The difference between this patch and version one can be seen in two places.
As per James Morris's suggestion function declarations that were split into
multiple lines because they were larger than 80 characters in length have been
merged into one line. Second as per Serge's comments security_inode_getsecurity
and the LSM hook inode_getsecurity take a bool to indicate if the function
should allocate the buffer and return the length or just return the length.  

This patch should apply on top of 2.6.24-rc1 and will definitely apply on git
commit hash ec3b67c11df42362ccda81261d62829042f223f0

If all concerns have been addressed I would propose the patches be added into -mm.

[PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[David P. Quigley]

This patch modifies the interface to inode_getsecurity to have the function
return a buffer containing the security blob and its length via parameters
instead of relying on the calling function to give it an appropriately sized
buffer. Security blobs obtained with this function should be freed using the
release_secctx LSM hook. This alleviates the problem of the caller having to
guess a length and preallocate a buffer for this function allowing it to be
used elsewhere for Labeled NFS. The patch also removed the unused err
parameter. The conversion is similar to the one performed by Al Viro for the
security_getprocattr hook.

Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>
---
 fs/xattr.c               |   30 ++++++++++++++++++++++++++++--
 include/linux/security.h |   21 +++++++++------------
 include/linux/xattr.h    |    1 +
 mm/shmem.c               |    3 +--
 security/dummy.c         |    2 +-
 security/security.c      |    4 ++--
 security/selinux/hooks.c |   45 ++++++++++++++++-----------------------------
 7 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c
index 6645b73..56b5b88 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -105,6 +105,33 @@ out:
 EXPORT_SYMBOL_GPL(vfs_setxattr);
 
 ssize_t
+xattr_getsecurity(struct inode *inode, const char *name, void *value,
+			size_t size)
+{
+	void *buffer = NULL;
+	ssize_t len;
+
+	if (!value || !size) {
+		len = security_inode_getsecurity(inode, name, &buffer, false);
+		goto out_noalloc;
+	}
+
+	len = security_inode_getsecurity(inode, name, &buffer, true);
+	if (len < 0)
+		return len;
+	if (size < len) {
+		len = -ERANGE;
+		goto out;
+	}
+	memcpy(value, buffer, len);
+out:
+	security_release_secctx(buffer, len);
+out_noalloc:
+	return len;
+}
+EXPORT_SYMBOL_GPL(xattr_getsecurity);
+
+ssize_t
 vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 {
 	struct inode *inode = dentry->d_inode;
@@ -126,8 +153,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
 	if (!strncmp(name, XATTR_SECURITY_PREFIX,
 				XATTR_SECURITY_PREFIX_LEN)) {
 		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
-		int ret = security_inode_getsecurity(inode, suffix, value,
-						     size, error);
+		int ret = xattr_getsecurity(inode, suffix, value, size);
 		/*
 		 * Only overwrite the return value if a security module
 		 * is actually active.
diff --git a/include/linux/security.h b/include/linux/security.h
index ac05083..3c4c91e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -404,15 +404,12 @@ struct request_sock;
  * 	identified by @name for @dentry.
  * 	Return 0 if permission is granted.
  * @inode_getsecurity:
- *	Copy the extended attribute representation of the security label 
- *	associated with @name for @inode into @buffer.  @buffer may be
- *	NULL to request the size of the buffer required.  @size indicates
- *	the size of @buffer in bytes.  Note that @name is the remainder
- *	of the attribute name after the security. prefix has been removed.
- *	@err is the return value from the preceding fs getxattr call,
- *	and can be used by the security module to determine whether it
- *	should try and canonicalize the attribute value.
- *	Return number of bytes used/required on success.
+ *	Retrieve a copy of the extended attribute representation of the
+ *	security label associated with @name for @inode via @buffer.  Note that
+ *	@name is the remainder of the attribute name after the security prefix
+ *	has been removed. @alloc is used to specify of the call should return a
+ *	value via the buffer or just the value length Return size of buffer on
+ *	success.
  * @inode_setsecurity:
  *	Set the security label associated with @name for @inode from the
  *	extended attribute value @value.  @size indicates the size of the
@@ -1275,7 +1272,7 @@ struct security_operations {
 	int (*inode_removexattr) (struct dentry *dentry, char *name);
 	int (*inode_need_killpriv) (struct dentry *dentry);
 	int (*inode_killpriv) (struct dentry *dentry);
-  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
+	int (*inode_getsecurity)(const struct inode *inode, const char *name, void **buffer, bool alloc);
   	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
   	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
 
@@ -1529,7 +1526,7 @@ int security_inode_listxattr(struct dentry *dentry);
 int security_inode_removexattr(struct dentry *dentry, char *name);
 int security_inode_need_killpriv(struct dentry *dentry);
 int security_inode_killpriv(struct dentry *dentry);
-int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
+int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc);
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 int security_file_permission(struct file *file, int mask);
@@ -1933,7 +1930,7 @@ static inline int security_inode_killpriv(struct dentry *dentry)
 	return cap_inode_killpriv(dentry);
 }
 
-static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index def131a..df6b95d 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -46,6 +46,7 @@ struct xattr_handler {
 		   size_t size, int flags);
 };
 
+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);
diff --git a/mm/shmem.c b/mm/shmem.c
index 404e53b..4c53460 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1980,8 +1980,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
 {
 	if (strcmp(name, "") == 0)
 		return -EINVAL;
-	return security_inode_getsecurity(inode, name, buffer, size,
-					  -EOPNOTSUPP);
+	return xattr_getsecurity(inode, name, buffer, size);
 }
 
 static int shmem_xattr_security_set(struct inode *inode, const char *name,
diff --git a/security/dummy.c b/security/dummy.c
index 6d895ad..7de65dc 100644
--- a/security/dummy.c
+++ b/security/dummy.c
@@ -384,7 +384,7 @@ static int dummy_inode_killpriv(struct dentry *dentry)
 	return 0;
 }
 
-static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/security.c b/security/security.c
index 0e1f1f1..39de3f4 100644
--- a/security/security.c
+++ b/security/security.c
@@ -478,11 +478,11 @@ int security_inode_killpriv(struct dentry *dentry)
 	return security_ops->inode_killpriv(dentry);
 }
 
-int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
 {
 	if (unlikely(IS_PRIVATE(inode)))
 		return 0;
-	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
+	return security_ops->inode_getsecurity(inode, name, buffer, alloc);
 }
 
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9f3124b..2ae7766 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -127,32 +127,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
 
 static struct kmem_cache *sel_inode_cache;
 
-/* Return security context for a given sid or just the context 
-   length if the buffer is null or length is 0 */
-static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
-{
-	char *context;
-	unsigned len;
-	int rc;
-
-	rc = security_sid_to_context(sid, &context, &len);
-	if (rc)
-		return rc;
-
-	if (!buffer || !size)
-		goto getsecurity_exit;
-
-	if (size < len) {
-		len = -ERANGE;
-		goto getsecurity_exit;
-	}
-	memcpy(buffer, context, len);
-
-getsecurity_exit:
-	kfree(context);
-	return len;
-}
-
 /* Allocate and free functions for each kind of security blob. */
 
 static int task_alloc_security(struct task_struct *task)
@@ -2416,14 +2390,27 @@ static int selinux_inode_removexattr (struct dentry *dentry, char *name)
  *
  * Permission check is handled by selinux_inode_getxattr hook.
  */
-static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
+static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
 {
+	u32 size;
+	int error;
+	char *context = NULL;
 	struct inode_security_struct *isec = inode->i_security;
 
 	if (strcmp(name, XATTR_SELINUX_SUFFIX))
 		return -EOPNOTSUPP;
-
-	return selinux_getsecurity(isec->sid, buffer, size);
+
+	error = security_sid_to_context(isec->sid, &context, &size);
+	if (error)
+		return error;
+	error = size;
+	if (alloc) {
+		*buffer = context;
+		goto out_nofree;
+	}
+	kfree(context);
+out_nofree:
+	return error;
 }
 
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,
-- 
1.5.3.4

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[James Morris]

On Thu, 1 Nov 2007, David P. Quigley wrote:

> This patch modifies the interface to inode_getsecurity to have the function
> return a buffer containing the security blob and its length via parameters
> instead of relying on the calling function to give it an appropriately sized
> buffer. Security blobs obtained with this function should be freed using the
> release_secctx LSM hook. This alleviates the problem of the caller having to
> guess a length and preallocate a buffer for this function allowing it to be
> used elsewhere for Labeled NFS. The patch also removed the unused err
> parameter. The conversion is similar to the one performed by Al Viro for the
> security_getprocattr hook.
> 
> Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>

Acked-by: James Morris <jmorris@namei.org>

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/2] VFS/Security: Rework inode_getsecurity and callers to return resulting buffer

[Serge E. Hallyn]

Quoting David P. Quigley (dpquigl@tycho.nsa.gov):
> This patch modifies the interface to inode_getsecurity to have the function
> return a buffer containing the security blob and its length via parameters
> instead of relying on the calling function to give it an appropriately sized
> buffer. Security blobs obtained with this function should be freed using the
> release_secctx LSM hook. This alleviates the problem of the caller having to
> guess a length and preallocate a buffer for this function allowing it to be
> used elsewhere for Labeled NFS. The patch also removed the unused err
> parameter. The conversion is similar to the one performed by Al Viro for the
> security_getprocattr hook.
> 
> Signed-off-by: David P. Quigley <dpquigl@tycho.nsa.gov>

Looks good.  Looks like it's already hit -mm, but anyway

Acked-by: Serge Hallyn <serue@us.ibm.com>

thanks,
-serge

> ---
>  fs/xattr.c               |   30 ++++++++++++++++++++++++++++--
>  include/linux/security.h |   21 +++++++++------------
>  include/linux/xattr.h    |    1 +
>  mm/shmem.c               |    3 +--
>  security/dummy.c         |    2 +-
>  security/security.c      |    4 ++--
>  security/selinux/hooks.c |   45 ++++++++++++++++-----------------------------
>  7 files changed, 58 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 6645b73..56b5b88 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -105,6 +105,33 @@ out:
>  EXPORT_SYMBOL_GPL(vfs_setxattr);
> 
>  ssize_t
> +xattr_getsecurity(struct inode *inode, const char *name, void *value,
> +			size_t size)
> +{
> +	void *buffer = NULL;
> +	ssize_t len;
> +
> +	if (!value || !size) {
> +		len = security_inode_getsecurity(inode, name, &buffer, false);
> +		goto out_noalloc;
> +	}
> +
> +	len = security_inode_getsecurity(inode, name, &buffer, true);
> +	if (len < 0)
> +		return len;
> +	if (size < len) {
> +		len = -ERANGE;
> +		goto out;
> +	}
> +	memcpy(value, buffer, len);
> +out:
> +	security_release_secctx(buffer, len);
> +out_noalloc:
> +	return len;
> +}
> +EXPORT_SYMBOL_GPL(xattr_getsecurity);
> +
> +ssize_t
>  vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
>  {
>  	struct inode *inode = dentry->d_inode;
> @@ -126,8 +153,7 @@ vfs_getxattr(struct dentry *dentry, char *name, void *value, size_t size)
>  	if (!strncmp(name, XATTR_SECURITY_PREFIX,
>  				XATTR_SECURITY_PREFIX_LEN)) {
>  		const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> -		int ret = security_inode_getsecurity(inode, suffix, value,
> -						     size, error);
> +		int ret = xattr_getsecurity(inode, suffix, value, size);
>  		/*
>  		 * Only overwrite the return value if a security module
>  		 * is actually active.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ac05083..3c4c91e 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -404,15 +404,12 @@ struct request_sock;
>   * 	identified by @name for @dentry.
>   * 	Return 0 if permission is granted.
>   * @inode_getsecurity:
> - *	Copy the extended attribute representation of the security label 
> - *	associated with @name for @inode into @buffer.  @buffer may be
> - *	NULL to request the size of the buffer required.  @size indicates
> - *	the size of @buffer in bytes.  Note that @name is the remainder
> - *	of the attribute name after the security. prefix has been removed.
> - *	@err is the return value from the preceding fs getxattr call,
> - *	and can be used by the security module to determine whether it
> - *	should try and canonicalize the attribute value.
> - *	Return number of bytes used/required on success.
> + *	Retrieve a copy of the extended attribute representation of the
> + *	security label associated with @name for @inode via @buffer.  Note that
> + *	@name is the remainder of the attribute name after the security prefix
> + *	has been removed. @alloc is used to specify of the call should return a
> + *	value via the buffer or just the value length Return size of buffer on
> + *	success.
>   * @inode_setsecurity:
>   *	Set the security label associated with @name for @inode from the
>   *	extended attribute value @value.  @size indicates the size of the
> @@ -1275,7 +1272,7 @@ struct security_operations {
>  	int (*inode_removexattr) (struct dentry *dentry, char *name);
>  	int (*inode_need_killpriv) (struct dentry *dentry);
>  	int (*inode_killpriv) (struct dentry *dentry);
> -  	int (*inode_getsecurity)(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
> +	int (*inode_getsecurity)(const struct inode *inode, const char *name, void **buffer, bool alloc);
>    	int (*inode_setsecurity)(struct inode *inode, const char *name, const void *value, size_t size, int flags);
>    	int (*inode_listsecurity)(struct inode *inode, char *buffer, size_t buffer_size);
> 
> @@ -1529,7 +1526,7 @@ int security_inode_listxattr(struct dentry *dentry);
>  int security_inode_removexattr(struct dentry *dentry, char *name);
>  int security_inode_need_killpriv(struct dentry *dentry);
>  int security_inode_killpriv(struct dentry *dentry);
> -int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err);
> +int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc);
>  int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
>  int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
>  int security_file_permission(struct file *file, int mask);
> @@ -1933,7 +1930,7 @@ static inline int security_inode_killpriv(struct dentry *dentry)
>  	return cap_inode_killpriv(dentry);
>  }
> 
> -static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static inline int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index def131a..df6b95d 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -46,6 +46,7 @@ struct xattr_handler {
>  		   size_t size, int flags);
>  };
> 
> +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);
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 404e53b..4c53460 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1980,8 +1980,7 @@ static int shmem_xattr_security_get(struct inode *inode, const char *name,
>  {
>  	if (strcmp(name, "") == 0)
>  		return -EINVAL;
> -	return security_inode_getsecurity(inode, name, buffer, size,
> -					  -EOPNOTSUPP);
> +	return xattr_getsecurity(inode, name, buffer, size);
>  }
> 
>  static int shmem_xattr_security_set(struct inode *inode, const char *name,
> diff --git a/security/dummy.c b/security/dummy.c
> index 6d895ad..7de65dc 100644
> --- a/security/dummy.c
> +++ b/security/dummy.c
> @@ -384,7 +384,7 @@ static int dummy_inode_killpriv(struct dentry *dentry)
>  	return 0;
>  }
> 
> -static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static int dummy_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
>  {
>  	return -EOPNOTSUPP;
>  }
> diff --git a/security/security.c b/security/security.c
> index 0e1f1f1..39de3f4 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -478,11 +478,11 @@ int security_inode_killpriv(struct dentry *dentry)
>  	return security_ops->inode_killpriv(dentry);
>  }
> 
> -int security_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +int security_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
>  {
>  	if (unlikely(IS_PRIVATE(inode)))
>  		return 0;
> -	return security_ops->inode_getsecurity(inode, name, buffer, size, err);
> +	return security_ops->inode_getsecurity(inode, name, buffer, alloc);
>  }
> 
>  int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9f3124b..2ae7766 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -127,32 +127,6 @@ static DEFINE_SPINLOCK(sb_security_lock);
> 
>  static struct kmem_cache *sel_inode_cache;
> 
> -/* Return security context for a given sid or just the context 
> -   length if the buffer is null or length is 0 */
> -static int selinux_getsecurity(u32 sid, void *buffer, size_t size)
> -{
> -	char *context;
> -	unsigned len;
> -	int rc;
> -
> -	rc = security_sid_to_context(sid, &context, &len);
> -	if (rc)
> -		return rc;
> -
> -	if (!buffer || !size)
> -		goto getsecurity_exit;
> -
> -	if (size < len) {
> -		len = -ERANGE;
> -		goto getsecurity_exit;
> -	}
> -	memcpy(buffer, context, len);
> -
> -getsecurity_exit:
> -	kfree(context);
> -	return len;
> -}
> -
>  /* Allocate and free functions for each kind of security blob. */
> 
>  static int task_alloc_security(struct task_struct *task)
> @@ -2416,14 +2390,27 @@ static int selinux_inode_removexattr (struct dentry *dentry, char *name)
>   *
>   * Permission check is handled by selinux_inode_getxattr hook.
>   */
> -static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void *buffer, size_t size, int err)
> +static int selinux_inode_getsecurity(const struct inode *inode, const char *name, void **buffer, bool alloc)
>  {
> +	u32 size;
> +	int error;
> +	char *context = NULL;
>  	struct inode_security_struct *isec = inode->i_security;
> 
>  	if (strcmp(name, XATTR_SELINUX_SUFFIX))
>  		return -EOPNOTSUPP;
> -
> -	return selinux_getsecurity(isec->sid, buffer, size);
> +
> +	error = security_sid_to_context(isec->sid, &context, &size);
> +	if (error)
> +		return error;
> +	error = size;
> +	if (alloc) {
> +		*buffer = context;
> +		goto out_nofree;
> +	}
> +	kfree(context);
> +out_nofree:
> +	return error;
>  }
> 
>  static int selinux_inode_setsecurity(struct inode *inode, const char *name,
> -- 
> 1.5.3.4
>