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

Commit Message

Xu Kuohai July 11, 2024, 11:18 a.m. UTC

From: Xu Kuohai <[e-mail]>

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

Before:
- Hook getprocattr returns length of value on success or a negative
  error code on failure.

After:
- Hook getprocattr returns 0 on success or a negative error code on
  failure. An output parameter @len is introduced to hold the
  length of value on success.

Signed-off-by: Xu Kuohai <[e-mail]>
---
 fs/proc/base.c                |  5 ++++-
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  5 +++--
 security/apparmor/lsm.c       |  7 +++++--
 security/security.c           |  8 +++++---
 security/selinux/hooks.c      | 16 +++++++++-------
 security/smack/smack_lsm.c    | 11 ++++++-----
 7 files changed, 33 insertions(+), 21 deletions(-)

Comments

Paul Moore July 19, 2024, 2:08 a.m. UTC | #1

On Jul 11, 2024 Xu Kuohai <[e-mail]> wrote:
> 
> To be consistent with most LSM hooks, convert the return value of
> hook getprocattr to 0 or a negative error code.
> 
> Before:
> - Hook getprocattr returns length of value on success or a negative
>   error code on failure.
> 
> After:
> - Hook getprocattr returns 0 on success or a negative error code on
>   failure. An output parameter @len is introduced to hold the
>   length of value on success.
> 
> Signed-off-by: Xu Kuohai <[e-mail]>
> ---
>  fs/proc/base.c                |  5 ++++-
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  5 +++--
>  security/apparmor/lsm.c       |  7 +++++--
>  security/security.c           |  8 +++++---
>  security/selinux/hooks.c      | 16 +++++++++-------
>  security/smack/smack_lsm.c    | 11 ++++++-----
>  7 files changed, 33 insertions(+), 21 deletions(-)

The patch 07/20 comments also apply here.

--
paul-moore.com

Xu Kuohai July 20, 2024, 9:30 a.m. UTC | #2

On 7/19/2024 10:08 AM, Paul Moore wrote:
> On Jul 11, 2024 Xu Kuohai <[e-mail]> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook getprocattr to 0 or a negative error code.
>>
>> Before:
>> - Hook getprocattr returns length of value on success or a negative
>>    error code on failure.
>>
>> After:
>> - Hook getprocattr returns 0 on success or a negative error code on
>>    failure. An output parameter @len is introduced to hold the
>>    length of value on success.
>>
>> Signed-off-by: Xu Kuohai <[e-mail]>
>> ---
>>   fs/proc/base.c                |  5 ++++-
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      |  5 +++--
>>   security/apparmor/lsm.c       |  7 +++++--
>>   security/security.c           |  8 +++++---
>>   security/selinux/hooks.c      | 16 +++++++++-------
>>   security/smack/smack_lsm.c    | 11 ++++++-----
>>   7 files changed, 33 insertions(+), 21 deletions(-)
> 
> The patch 07/20 comments also apply here.
>

OK, will drop this patch.

> --
> paul-moore.com
> 
>

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 9e1cf6cc674d..516a00f6ce36 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2721,13 +2721,16 @@  static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
 	char *p = NULL;
 	ssize_t length;
 	struct task_struct *task = get_proc_task(inode);
+	u32 n;
 
 	if (!task)
 		return -ESRCH;
 
 	length = security_getprocattr(task, PROC_I(inode)->op.lsmid,
 				      file->f_path.dentry->d_name.name,
-				      &p);
+				      &p, &n);
+	if (!length)
+		length = n;
 	put_task_struct(task);
 	if (length > 0)
 		length = simple_read_from_buffer(buf, count, ppos, p, length);
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 2628514bb19c..b0e3cf3fc33f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -286,7 +286,7 @@  LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
 LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr,
 	 struct lsm_ctx *ctx, u32 size, u32 flags)
 LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
-	 char **value)
+	 char **value, u32 *len)
 LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size,
 	 size_t *wbytes)
 LSM_HOOK(int, 0, ismaclabel, const char *name)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1f1a9696e65d..616047030a89 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -495,7 +495,7 @@  int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 			 u32 size, u32 flags);
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
-			 char **value);
+			 char **value, u32 *len);
 int security_setprocattr(int lsmid, const char *name, void *value, size_t size,
 			 size_t *wbytes);
 int security_netlink_send(struct sock *sk, struct sk_buff *skb);
@@ -1435,7 +1435,8 @@  static inline int security_setselfattr(unsigned int attr,
 }
 
 static inline int security_getprocattr(struct task_struct *p, int lsmid,
-				       const char *name, char **value)
+				       const char *name, char **value,
+				       u32 *len)
 {
 	return -EINVAL;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6c8b1f8c5781..0454f3f1af06 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -820,7 +820,7 @@  static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 }
 
 static int apparmor_getprocattr(struct task_struct *task, const char *name,
-				char **value)
+				char **value, u32 *len)
 {
 	int error = -ENOENT;
 	/* released below */
@@ -843,7 +843,10 @@  static int apparmor_getprocattr(struct task_struct *task, const char *name,
 	aa_put_label(label);
 	put_cred(cred);
 
-	return error;
+	if (error < 0)
+		return error;
+	*len = error;
+	return 0;
 }
 
 static int do_setattr(u64 attr, void *value, size_t size)
diff --git a/security/security.c b/security/security.c
index 9685096dbf16..9dd2ae6cf763 100644
--- a/security/security.c
+++ b/security/security.c
@@ -4117,20 +4117,22 @@  int security_setselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
  * @lsmid: LSM identification
  * @name: attribute name
  * @value: attribute value
+ * @len: length of @value
  *
  * Read attribute @name for task @p and store it into @value if allowed.
  *
- * Return: Returns the length of @value on success, a negative value otherwise.
+ * Return: Returns 0 on success or a negative error code on failure.
+ *         @len is set to the length of @value on success.
  */
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
-			 char **value)
+			 char **value, u32 *len)
 {
 	struct security_hook_list *hp;
 
 	hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
 		if (lsmid != 0 && lsmid != hp->lsmid->id)
 			continue;
-		return hp->hook.getprocattr(p, name, value);
+		return hp->hook.getprocattr(p, name, value, len);
 	}
 	return LSM_RET_DEFAULT(getprocattr);
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 7a73f3710025..16cd336aab3d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6574,19 +6574,21 @@  static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
 	return rc;
 }
 
-static int selinux_getprocattr(struct task_struct *p,
-			       const char *name, char **value)
+static int selinux_getprocattr(struct task_struct *p, const char *name,
+			       char **value, u32 *len)
 {
 	unsigned int attr = lsm_name_to_attr(name);
-	int rc;
+	int rc = -EINVAL;
 
 	if (attr) {
 		rc = selinux_lsm_getattr(attr, p, value);
-		if (rc != -EOPNOTSUPP)
-			return rc;
+		if (rc == -EOPNOTSUPP)
+			rc = -EINVAL;
 	}
-
-	return -EINVAL;
+	if (rc < 0)
+		return rc;
+	*len = rc;
+	return 0;
 }
 
 static int selinux_setprocattr(const char *name, void *value, size_t size,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4265f2639106..8a352bd05565 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3681,16 +3681,17 @@  static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
  * @p: the object task
  * @name: the name of the attribute in /proc/.../attr
  * @value: where to put the result
+ * @len: where to put the length of the result
  *
  * Places a copy of the task Smack into value
  *
- * Returns the length of the smack label or an error code
+ * Returns 0 on success or a negative error code on failure.
  */
-static int smack_getprocattr(struct task_struct *p, const char *name, char **value)
+static int smack_getprocattr(struct task_struct *p, const char *name,
+			     char **value, u32 *len)
 {
 	struct smack_known *skp = smk_of_task_struct_obj(p);
 	char *cp;
-	int slen;
 
 	if (strcmp(name, "current") != 0)
 		return -EINVAL;
@@ -3699,9 +3700,9 @@  static int smack_getprocattr(struct task_struct *p, const char *name, char **val
 	if (cp == NULL)
 		return -ENOMEM;
 
-	slen = strlen(cp);
+	*len = strlen(cp);
 	*value = cp;
-	return slen;
+	return 0;
 }
 
 /**