diff mbox series

[bpf-next,v4,04/20] lsm: Refactor return value of LSM hook inode_listsecurity

Message ID 20240711111908.3817636-5-xukuohai@huaweicloud.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Add return value range check for BPF LSM | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1044 this patch: 1044
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 4 maintainers not CCed: trondmy@kernel.org linux-nfs@vger.kernel.org linux-fsdevel@vger.kernel.org jack@suse.cz
netdev/build_clang success Errors and warnings before: 1112 this patch: 1112
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 7721 this patch: 7721
netdev/checkpatch warning WARNING: line length of 102 exceeds 80 columns WARNING: line length of 81 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 173 this patch: 173
netdev/source_inline success Was 0 now: 0

Commit Message

Xu Kuohai July 11, 2024, 11:18 a.m. UTC
From: Xu Kuohai <xukuohai@huawei.com>

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

Comments

Paul Moore July 19, 2024, 2:08 a.m. UTC | #1
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?

> 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
Xu Kuohai July 20, 2024, 9:29 a.m. UTC | #2
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
>
diff mbox series

Patch

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;
 	}
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;
 	}
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 4f056f2613af..1b7761ae2777 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -174,7 +174,7 @@  LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
 LSM_HOOK(int, -EOPNOTSUPP, inode_setsecurity, struct inode *inode,
 	 const char *name, const void *value, size_t size, int flags)
 LSM_HOOK(int, 0, inode_listsecurity, struct inode *inode, char *buffer,
-	 size_t buffer_size)
+	 size_t buffer_size, size_t *bytes)
 LSM_HOOK(void, LSM_RET_VOID, inode_getsecid, struct inode *inode, u32 *secid)
 LSM_HOOK(int, 0, inode_copy_up, struct dentry *src, struct cred **new)
 LSM_HOOK(int, -EOPNOTSUPP, inode_copy_up_xattr, struct dentry *src,
diff --git a/include/linux/security.h b/include/linux/security.h
index b6d296d21438..0ed53e232c4d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -395,7 +395,7 @@  int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
 			       bool alloc, void **buffer, u32 *len);
 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_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size, size_t *bytes);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
 int security_inode_copy_up(struct dentry *src, struct cred **new);
 int security_inode_copy_up_xattr(struct dentry *src, const char *name);
@@ -1007,9 +1007,10 @@  static inline int security_inode_setsecurity(struct inode *inode, const char *na
 	return -EOPNOTSUPP;
 }
 
-static inline int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
+static inline int security_inode_listsecurity(struct inode *inode, char *buffer,
+					      size_t buffer_size, size_t *bytes)
 {
-	return 0;
+	return *bytes = 0;
 }
 
 static inline void security_inode_getsecid(struct inode *inode, u32 *secid)
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)
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);
 
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;
 }
 
 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;
 }
 
 /**