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?
> 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().
> 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.
> 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;
> 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.
--
paul-moore.com
