diff mbox series

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

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

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

Comments

Simon Horman July 12, 2024, 1:31 p.m. UTC | #1 
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.

...
Xu Kuohai July 13, 2024, 8:07 a.m. UTC | #2 
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.
Paul Moore July 19, 2024, 2:08 a.m. UTC | #3 
On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote:
> 
> 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(-)

Aside from Simon's concern over variable types, I saw a few other issues
when looking at this patch (below).

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 17d6188d22cf..ff82e2ab6f8f 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -485,7 +485,10 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
>  	}
>  out_free:
>  	kfree(tmpbuf);
> -	return size;
> +	if (size < 0)
> +		return size;
> +	*len = size;
> +	return 0;
>  }

We should do a better job converting cap_inode_getsecurity(), create a
new local variable, e.g. 'int error', and use it to store and return the
error code instead of reusing @size.  I understand that what you've done
is easier, but I'd prefer to see it done properly.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9cd5a8f1f6a3..70792bba24d9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3407,7 +3407,7 @@ static int selinux_path_notify(const struct path *path, u64 mask,
>   */
>  static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>  				     struct inode *inode, const char *name,
> -				     void **buffer, bool alloc)
> +				     bool alloc, void **buffer, u32 *len)
>  {
>  	u32 size;
>  	int error;
> @@ -3440,14 +3440,14 @@ static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>  						&context, &size);
>  	if (error)
>  		return error;
> -	error = size;
> +	*len = size;

Depending on how you choose to resolve the variable type issue, you may
be able to pass @len directly to security_sid_to_context().

>  	if (alloc) {
>  		*buffer = context;
>  		goto out_nofree;
>  	}
>  	kfree(context);
>  out_nofree:
> -	return error;
> +	return 0;
>  }


--
paul-moore.com
Xu Kuohai July 20, 2024, 9:28 a.m. UTC | #4 
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_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(-)
> 
> Aside from Simon's concern over variable types, I saw a few other issues
> when looking at this patch (below).
> 
>> diff --git a/security/commoncap.c b/security/commoncap.c
>> index 17d6188d22cf..ff82e2ab6f8f 100644
>> --- a/security/commoncap.c
>> +++ b/security/commoncap.c
>> @@ -485,7 +485,10 @@ int cap_inode_getsecurity(struct mnt_idmap *idmap,
>>   	}
>>   out_free:
>>   	kfree(tmpbuf);
>> -	return size;
>> +	if (size < 0)
>> +		return size;
>> +	*len = size;
>> +	return 0;
>>   }
> 
> We should do a better job converting cap_inode_getsecurity(), create a
> new local variable, e.g. 'int error', and use it to store and return the
> error code instead of reusing @size.  I understand that what you've done
> is easier, but I'd prefer to see it done properly.
>

Got it

>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9cd5a8f1f6a3..70792bba24d9 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3407,7 +3407,7 @@ static int selinux_path_notify(const struct path *path, u64 mask,
>>    */
>>   static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>>   				     struct inode *inode, const char *name,
>> -				     void **buffer, bool alloc)
>> +				     bool alloc, void **buffer, u32 *len)
>>   {
>>   	u32 size;
>>   	int error;
>> @@ -3440,14 +3440,14 @@ static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
>>   						&context, &size);
>>   	if (error)
>>   		return error;
>> -	error = size;
>> +	*len = size;
> 
> Depending on how you choose to resolve the variable type issue, you may
> be able to pass @len directly to security_sid_to_context().
>

Sounds great

>>   	if (alloc) {
>>   		*buffer = context;
>>   		goto out_nofree;
>>   	}
>>   	kfree(context);
>>   out_nofree:
>> -	return error;
>> +	return 0;
>>   }
> 
> 
> --
> paul-moore.com
> 
>
diff mbox series

Patch

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;
 }
 
 /*
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 964849de424b..4f056f2613af 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -169,7 +169,8 @@  LSM_HOOK(int, 0, inode_need_killpriv, struct dentry *dentry, bool *need)
 LSM_HOOK(int, 0, inode_killpriv, struct mnt_idmap *idmap,
 	 struct dentry *dentry)
 LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct mnt_idmap *idmap,
-	 struct inode *inode, const char *name, void **buffer, bool alloc)
+	 struct inode *inode, const char *name, bool alloc, void **buffer,
+	 u32 *len)
 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,
diff --git a/include/linux/security.h b/include/linux/security.h
index 1614ef5b2dd2..b6d296d21438 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -164,8 +164,8 @@  int cap_inode_removexattr(struct mnt_idmap *idmap,
 int cap_inode_need_killpriv(struct dentry *dentry, bool *need);
 int cap_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int cap_inode_getsecurity(struct mnt_idmap *idmap,
-			  struct inode *inode, const char *name, void **buffer,
-			  bool alloc);
+			  struct inode *inode, const char *name, bool alloc,
+			  void **buffer, u32 *len);
 extern int cap_mmap_addr(unsigned long addr);
 extern int cap_mmap_file(struct file *file, unsigned long reqprot,
 			 unsigned long prot, unsigned long flags);
@@ -393,7 +393,7 @@  int security_inode_need_killpriv(struct dentry *dentry, int *attr);
 int security_inode_killpriv(struct mnt_idmap *idmap, struct dentry *dentry);
 int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
-			       void **buffer, bool alloc);
+			       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);
 void security_inode_getsecid(struct inode *inode, u32 *secid);
@@ -996,10 +996,10 @@  static inline int security_inode_killpriv(struct mnt_idmap *idmap,
 
 static inline int security_inode_getsecurity(struct mnt_idmap *idmap,
 					     struct inode *inode,
-					     const char *name, void **buffer,
-					     bool alloc)
+					     const char *name, bool alloc,
+					     void **buffer, u32 *len)
 {
-	return cap_inode_getsecurity(idmap, inode, name, buffer, alloc);
+	return cap_inode_getsecurity(idmap, inode, name, alloc, buffer, len);
 }
 
 static inline int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
diff --git a/security/commoncap.c b/security/commoncap.c
index 17d6188d22cf..ff82e2ab6f8f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -383,8 +383,8 @@  static bool is_v3header(int size, const struct vfs_cap_data *cap)
  * so that's good.
  */
 int cap_inode_getsecurity(struct mnt_idmap *idmap,
-			  struct inode *inode, const char *name, void **buffer,
-			  bool alloc)
+			  struct inode *inode, const char *name,
+			  bool alloc, void **buffer, u32 *len)
 {
 	int size;
 	kuid_t kroot;
@@ -485,7 +485,10 @@  int cap_inode_getsecurity(struct mnt_idmap *idmap,
 	}
 out_free:
 	kfree(tmpbuf);
-	return size;
+	if (size < 0)
+		return size;
+	*len = size;
+	return 0;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index a4abcd86eb36..614f14cbfff7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2544,8 +2544,9 @@  int security_inode_killpriv(struct mnt_idmap *idmap,
  * @idmap: idmap of the mount
  * @inode: inode
  * @name: xattr name
- * @buffer: security label buffer
  * @alloc: allocation flag
+ * @buffer: security label buffer
+ * @len: security label length
  *
  * Retrieve a copy of the extended attribute representation of the security
  * label associated with @name for @inode via @buffer.  Note that @name is the
@@ -2553,17 +2554,17 @@  int security_inode_killpriv(struct mnt_idmap *idmap,
  * @alloc is used to specify if the call should return a value via the buffer
  * or just the value length.
  *
- * Return: Returns size of buffer on success.
+ * Return: Returns 0 on success or a negative error code on failure.
  */
 int security_inode_getsecurity(struct mnt_idmap *idmap,
 			       struct inode *inode, const char *name,
-			       void **buffer, bool alloc)
+			       bool alloc, void **buffer, u32 *len)
 {
 	if (unlikely(IS_PRIVATE(inode)))
 		return LSM_RET_DEFAULT(inode_getsecurity);
 
-	return call_int_hook(inode_getsecurity, idmap, inode, name, buffer,
-			     alloc);
+	return call_int_hook(inode_getsecurity, idmap, inode, name, alloc,
+			     buffer, len);
 }
 
 /**
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9cd5a8f1f6a3..70792bba24d9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3407,7 +3407,7 @@  static int selinux_path_notify(const struct path *path, u64 mask,
  */
 static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
 				     struct inode *inode, const char *name,
-				     void **buffer, bool alloc)
+				     bool alloc, void **buffer, u32 *len)
 {
 	u32 size;
 	int error;
@@ -3440,14 +3440,14 @@  static int selinux_inode_getsecurity(struct mnt_idmap *idmap,
 						&context, &size);
 	if (error)
 		return error;
-	error = size;
+	*len = size;
 	if (alloc) {
 		*buffer = context;
 		goto out_nofree;
 	}
 	kfree(context);
 out_nofree:
-	return error;
+	return 0;
 }
 
 static int selinux_inode_setsecurity(struct inode *inode, const char *name,
@@ -6644,13 +6644,9 @@  static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
 
 static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
 {
-	int len = 0;
-	len = selinux_inode_getsecurity(&nop_mnt_idmap, inode,
-					XATTR_SELINUX_SUFFIX, ctx, true);
-	if (len < 0)
-		return len;
-	*ctxlen = len;
-	return 0;
+	return selinux_inode_getsecurity(&nop_mnt_idmap, inode,
+					 XATTR_SELINUX_SUFFIX,
+					 true, ctx, ctxlen);
 }
 #ifdef CONFIG_KEYS
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f5cbec1e6a92..e7a5f6fd9a2d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1543,14 +1543,15 @@  static int smack_inode_remove_acl(struct mnt_idmap *idmap,
  * @idmap: idmap of the mount
  * @inode: the object
  * @name: attribute name
- * @buffer: where to put the result
  * @alloc: duplicate memory
+ * @buffer: where to put the result
+ * @len: where to put the result length
  *
- * Returns the size of the attribute or an error code
+ * Returns 0 on success or a negative error code on failure
  */
 static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 				   struct inode *inode, const char *name,
-				   void **buffer, bool alloc)
+				   bool alloc, void **buffer, u32 *len)
 {
 	struct socket_smack *ssp;
 	struct socket *sock;
@@ -1558,7 +1559,6 @@  static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 	struct inode *ip = inode;
 	struct smack_known *isp;
 	struct inode_smack *ispp;
-	size_t label_len;
 	char *label = NULL;
 
 	if (strcmp(name, XATTR_SMACK_SUFFIX) == 0) {
@@ -1594,15 +1594,15 @@  static int smack_inode_getsecurity(struct mnt_idmap *idmap,
 	if (!label)
 		label = isp->smk_known;
 
-	label_len = strlen(label);
-
 	if (alloc) {
 		*buffer = kstrdup(label, GFP_KERNEL);
 		if (*buffer == NULL)
 			return -ENOMEM;
 	}
 
-	return label_len;
+	*len = strlen(label);
+
+	return 0;
 }