Re: [PATCH bpf-next v4 03/20] lsm: Refactor return value of LSM hook inode_getsecurity

On 7/12/2024 9:31 PM, Simon Horman wrote:

On Thu, Jul 11, 2024 at 07:18:51PM +0800, Xu Kuohai wrote:

...

From: Xu Kuohai xukuohai@huawei.com

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

Before:

• Hook inode_getsecurity returns size of buffer on success or a negative error code on failure.

After:

• Hook inode_getsecurity returns 0 on success or a negative error code on failure. An output parameter @len is introduced to hold the buffer size on success.

Signed-off-by: Xu Kuohai xukuohai@huawei.com

fs/xattr.c | 19 ++++++++++--------- include/linux/lsm_hook_defs.h | 3 ++- include/linux/security.h | 12 ++++++------ security/commoncap.c | 9 ++++++--- security/security.c | 11 ++++++----- security/selinux/hooks.c | 16 ++++++---------- security/smack/smack_lsm.c | 14 +++++++------- 7 files changed, 43 insertions(+), 41 deletions(-)

diff --git a/fs/xattr.c b/fs/xattr.c index f8b643f91a98..f4e3bedf7272 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -339,27 +339,28 @@ xattr_getsecurity(struct mnt_idmap *idmap, struct inode *inode, const char *name, void *value, size_t size) { void *buffer = NULL;

• ssize_t len;

• int error;
• u32 len;

if (!value || !size) {

• len = security_inode_getsecurity(idmap, inode, name,
  

• 				 &buffer, false);
  

• error = security_inode_getsecurity(idmap, inode, name,
  

• 				   false, &buffer, &len);
  

  goto out_noalloc; }

• len = security_inode_getsecurity(idmap, inode, name, &buffer,

• 			 true);
  

• if (len < 0)

• return len;
  

• error = security_inode_getsecurity(idmap, inode, name, true,

• 			   &buffer, &len);
  

• if (error)

• return error;
  

  if (size < len) {

• len = -ERANGE;
  

• error = -ERANGE;
  

  goto out; } memcpy(value, buffer, len); out: kfree(buffer); out_noalloc:

• return len;

• return error < 0 ? error : len;

Hi Xu Kuohai,

len is an unsigned 32-bit entity, but the return type of this function is an unsigned value (ssize_t). So in theory, if len is very large, a negative error value error will be returned.

...

}

Similarly for the handling of nattr in lsm_get_self_attr in lsm_syscalls.c in a subsequent patch.

Flagged by Smatch.

...

OK, indeed there is no standardization that says ssize_t must be wider than u32. I'll look into adding overflow checks. Thanks.