[PATCH 0/2] getsecurity/vfs_getxattr cleanup V2

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

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

[David P. Quigley]

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 56b5b88..91c7929 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -145,11 +145,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;
@@ -158,9 +153,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.3.4

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

[James Morris]

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

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

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


-- 
James Morris
<jmorris@namei.org>

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

[Serge E. Hallyn]

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

No change from last time, so again

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

thanks,
-serge

> ---
>  fs/xattr.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 56b5b88..91c7929 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -145,11 +145,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;
> @@ -158,9 +153,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.3.4
> 

[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 2/2] VFS: Reorder vfs_getxattr to avoid unnecessary calls to the LSM

[David P. Quigley]

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 d45c7ef..e5e7fb8 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -141,11 +141,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;
@@ -154,9 +149,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;
 }

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

[James Morris]

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

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

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


-- 
James Morris
<jmorris@namei.org>

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

[Serge E. Hallyn]

Quoting James Morris (jmorris@namei.org):
> On Mon, 22 Oct 2007, David P. Quigley wrote:
> 
> > 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>
> 
> Acked-by: James Morris <jmorris@namei.org>

(not that it matters much, esp with selinux being the only current user,
but)

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

Makes sense and looks good.

thanks,
-serge