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

On 7/19/2024 10:08 AM, Paul Moore wrote:

On Jul 11, 2024 Xu Kuohai xukuohai@huaweicloud.com 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@huawei.com

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