Re: [PATCH v4 4/20] lsm: Refactor return value of LSM hook inode_listsecurity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/19/2024 10:08 AM, Paul Moore wrote:
On Jul 11, 2024 Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:

To be consistent with most LSM hooks, convert the return value of
hook inode_listsecurity to 0 or a negative error code.

Before:
- Hook inode_listsecurity returns number of bytes used/required on
   success or a negative error code on failure.

After:
- Hook inode_listsecurity returns 0 on success or a negative error
   code on failure. An output parameter @bytes is introduced to hold
   the number of bytes used/required on success.

Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
---
  fs/nfs/nfs4proc.c             |  5 ++++-
  fs/xattr.c                    |  5 ++++-
  include/linux/lsm_hook_defs.h |  2 +-
  include/linux/security.h      |  7 ++++---
  net/socket.c                  |  9 +++++----
  security/security.c           | 29 +++++++++++++++++++++++++----
  security/selinux/hooks.c      |  8 +++++---
  security/smack/smack_lsm.c    |  6 ++++--
  8 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index a691fa10b3e9..6d75758ba3d5 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7848,10 +7848,13 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
  static ssize_t
  nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len)
  {
+	size_t bytes;
  	int len = 0;
if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) {
-		len = security_inode_listsecurity(inode, list, list_len);
+		len = security_inode_listsecurity(inode, list, list_len, &bytes);
+		if (!len)
+			len = bytes;
  		if (len >= 0 && list_len && len > list_len)
  			return -ERANGE;
  	}

See my comments below.

diff --git a/fs/xattr.c b/fs/xattr.c
index f4e3bedf7272..ab7d7123a016 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -485,6 +485,7 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
  {
  	struct inode *inode = d_inode(dentry);
  	ssize_t error;
+	size_t bytes;
error = security_inode_listxattr(dentry);
  	if (error)
@@ -493,7 +494,9 @@ vfs_listxattr(struct dentry *dentry, char *list, size_t size)
  	if (inode->i_op->listxattr) {
  		error = inode->i_op->listxattr(dentry, list, size);
  	} else {
-		error = security_inode_listsecurity(inode, list, size);
+		error = security_inode_listsecurity(inode, list, size, &bytes);
+		if (!error)
+			error = bytes;
  		if (size && error > size)
  			error = -ERANGE;

More on this below, but since the buffer length is fixed we are
already going to have to do a length comparison in the LSMs, why not
do the check and return -ERANGE there?


Sounds great, thanks

diff --git a/net/socket.c b/net/socket.c
index e416920e9399..43f0e3c9a6e0 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -571,12 +571,13 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
  static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
  				size_t size)
  {
-	ssize_t len;
+	int err;
+	size_t len;
  	ssize_t used = 0;
- len = security_inode_listsecurity(d_inode(dentry), buffer, size);
-	if (len < 0)
-		return len;
+	err = security_inode_listsecurity(d_inode(dentry), buffer, size, &len);
+	if (err < 0)
+		return err;
  	used += len;
  	if (buffer) {
  		if (size < used)

It doesn't show in the patch/diff, but if the LSM hook handles the length
comparison we can simplify the -ERANGE code in sockfs_listxattr().


Will do

diff --git a/security/security.c b/security/security.c
index 614f14cbfff7..26eea8f4cd74 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2597,20 +2597,41 @@ int security_inode_setsecurity(struct inode *inode, const char *name,
   * @inode: inode
   * @buffer: buffer
   * @buffer_size: size of buffer
+ * @bytes: number of bytes used/required
   *
   * Copy the extended attribute names for the security labels associated with
   * @inode into @buffer.  The maximum size of @buffer is specified by
   * @buffer_size.  @buffer may be NULL to request the size of the buffer
   * required.
   *
- * Return: Returns number of bytes used/required on success.
+ * Return: Returns 0 on success or a negative error code on failure.
   */
  int security_inode_listsecurity(struct inode *inode,
-				char *buffer, size_t buffer_size)
+				char *buffer, size_t buffer_size,
+				size_t *bytes)
  {
+	int rc;
+	size_t used;
+	struct security_hook_list *hp;
+
  	if (unlikely(IS_PRIVATE(inode)))
-		return 0;
-	return call_int_hook(inode_listsecurity, inode, buffer, buffer_size);
+		return *bytes = 0;
+
+	used = 0;
+	hlist_for_each_entry(hp, &security_hook_heads.inode_listsecurity,
+			     list) {
+		rc = hp->hook.inode_listsecurity(inode, buffer, buffer_size,
+						 &used);
+		if (rc < 0)
+			return rc;
+		if (used != 0)
+			break;
+	}
+
+	*bytes = used;
+
+	return 0;
+
  }
  EXPORT_SYMBOL(security_inode_listsecurity);

For reasons associated with the static_call work, we really need to
limit ourselves to the call_{int,void}_hook() macros on any new code.
The good news is that I think we can do that here as the existing
code isn't multi-LSM friendly.


Thanks for pointing that out. Good to know the current code is acceptable.

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 70792bba24d9..5dedd3917d57 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3481,16 +3481,18 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
  	return 0;
  }
-static int selinux_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
+static int selinux_inode_listsecurity(struct inode *inode, char *buffer,
+				      size_t buffer_size, size_t *bytes)
  {
  	const int len = sizeof(XATTR_NAME_SELINUX);
if (!selinux_initialized())
-		return 0;
+		return *bytes = 0;
if (buffer && len <= buffer_size)
  		memcpy(buffer, XATTR_NAME_SELINUX, len);
-	return len;
+	*bytes = len;
+	return 0;
  }

Let's do something like below so we can catch -ERANGE in the LSMs
themselves.

   if (!selinux_initialized())
     return *bytes = 0;

   *bytes = sizeof(XATTR_NAME_SELINUX);
   if (len > buffer_size);
     return -ERANGE;
   if (buffer)
     memcpy(buffer, XATTR_NAME_SELINUX, *bytes);
return 0;


Will do

  static void selinux_inode_getsecid(struct inode *inode, u32 *secid)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index e7a5f6fd9a2d..6f73906bf7ea 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1611,16 +1611,18 @@ static int smack_inode_getsecurity(struct mnt_idmap *idmap,
   * @inode: the object
   * @buffer: where they go
   * @buffer_size: size of buffer
+ * @bytes: number of data bytes in buffer
   */
  static int smack_inode_listsecurity(struct inode *inode, char *buffer,
-				    size_t buffer_size)
+				    size_t buffer_size, size_t *bytes)
  {
  	int len = sizeof(XATTR_NAME_SMACK);
if (buffer != NULL && len <= buffer_size)
  		memcpy(buffer, XATTR_NAME_SMACK, len);
- return len;
+	*bytes = len;
+	return 0;
  }

A similar approach could be used here.


Will do

--
paul-moore.com






[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux