diff mbox series

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

Message ID 20240711111908.3817636-7-xukuohai@huaweicloud.com (mailing list archive)
State New
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 getselfattr to 0 or a negative error code.

Before:
- Hook getselfattr returns number of attributes found on success
  or a negative error code on failure.

After:
- Hook getselfattr returns 0 on success or a negative error code
  on failure. An output parameter @nattr is introduced to hold
  the number of attributes found on success.

Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
---
 include/linux/lsm_hook_defs.h |  2 +-
 include/linux/security.h      |  5 +++--
 security/apparmor/lsm.c       |  5 +++--
 security/lsm_syscalls.c       |  6 +++++-
 security/security.c           | 18 +++++++++++-------
 security/selinux/hooks.c      | 13 +++++++++----
 security/smack/smack_lsm.c    | 13 +++++++++----
 7 files changed, 41 insertions(+), 21 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 getselfattr to 0 or a negative error code.
> 
> Before:
> - Hook getselfattr returns number of attributes found on success
>   or a negative error code on failure.
> 
> After:
> - Hook getselfattr returns 0 on success or a negative error code
>   on failure. An output parameter @nattr is introduced to hold
>   the number of attributes found on success.
> 
> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
> ---
>  include/linux/lsm_hook_defs.h |  2 +-
>  include/linux/security.h      |  5 +++--
>  security/apparmor/lsm.c       |  5 +++--
>  security/lsm_syscalls.c       |  6 +++++-
>  security/security.c           | 18 +++++++++++-------
>  security/selinux/hooks.c      | 13 +++++++++----
>  security/smack/smack_lsm.c    | 13 +++++++++----
>  7 files changed, 41 insertions(+), 21 deletions(-)

The getselfattr hook is different from the majority of the other LSM
hooks as getselfattr is used as part of lsm_get_self_attr(2) syscall and
not by other subsystems within the kernel.  Let's leave it as-is for now
as it is sufficiently special case that a deviation is okay.

--
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 <xukuohai@huaweicloud.com> wrote:
>>
>> To be consistent with most LSM hooks, convert the return value of
>> hook getselfattr to 0 or a negative error code.
>>
>> Before:
>> - Hook getselfattr returns number of attributes found on success
>>    or a negative error code on failure.
>>
>> After:
>> - Hook getselfattr returns 0 on success or a negative error code
>>    on failure. An output parameter @nattr is introduced to hold
>>    the number of attributes found on success.
>>
>> Signed-off-by: Xu Kuohai <xukuohai@huawei.com>
>> ---
>>   include/linux/lsm_hook_defs.h |  2 +-
>>   include/linux/security.h      |  5 +++--
>>   security/apparmor/lsm.c       |  5 +++--
>>   security/lsm_syscalls.c       |  6 +++++-
>>   security/security.c           | 18 +++++++++++-------
>>   security/selinux/hooks.c      | 13 +++++++++----
>>   security/smack/smack_lsm.c    | 13 +++++++++----
>>   7 files changed, 41 insertions(+), 21 deletions(-)
> 
> The getselfattr hook is different from the majority of the other LSM
> hooks as getselfattr is used as part of lsm_get_self_attr(2) syscall and
> not by other subsystems within the kernel.  Let's leave it as-is for now
> as it is sufficiently special case that a deviation is okay.
>

Got it, thanks

> --
> paul-moore.com
> 
>
diff mbox series

Patch

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 1b7761ae2777..dbc16f14f42f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -282,7 +282,7 @@  LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
 LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
 	 struct inode *inode)
 LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int attr,
-	 struct lsm_ctx __user *ctx, u32 *size, u32 flags)
+	 struct lsm_ctx __user *ctx, u32 *size, u32 flags, u32 *nattr)
 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,
diff --git a/include/linux/security.h b/include/linux/security.h
index 0ed53e232c4d..96a63e132abf 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -491,7 +491,7 @@  int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
 			unsigned nsops, int alter);
 void security_d_instantiate(struct dentry *dentry, struct inode *inode);
 int security_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			 u32 __user *size, u32 flags);
+			 u32 __user *size, u32 flags, u32 *nattr);
 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,
@@ -1420,7 +1420,8 @@  static inline void security_d_instantiate(struct dentry *dentry,
 
 static inline int security_getselfattr(unsigned int attr,
 				       struct lsm_ctx __user *ctx,
-				       size_t __user *size, u32 flags)
+				       size_t __user *size, u32 flags,
+				       u32 *nattr)
 {
 	return -EOPNOTSUPP;
 }
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 6239777090c4..72dd09993f28 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -779,7 +779,7 @@  static int apparmor_sb_pivotroot(const struct path *old_path,
 }
 
 static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
-				u32 *size, u32 flags)
+				u32 *size, u32 flags, u32 *nattr)
 {
 	int error = -ENOENT;
 	struct aa_task_ctx *ctx = task_ctx(current);
@@ -815,7 +815,8 @@  static int apparmor_getselfattr(unsigned int attr, struct lsm_ctx __user *lx,
 
 	if (error < 0)
 		return error;
-	return 1;
+	*nattr = 1;
+	return 0;
 }
 
 static int apparmor_getprocattr(struct task_struct *task, const char *name,
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 8440948a690c..845866f94b03 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -77,7 +77,11 @@  SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
 SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
 		ctx, u32 __user *, size, u32, flags)
 {
-	return security_getselfattr(attr, ctx, size, flags);
+	int rc;
+	u32 nattr;
+
+	rc = security_getselfattr(attr, ctx, size, flags, &nattr);
+	return rc < 0 ? rc : nattr;
 }
 
 /**
diff --git a/security/security.c b/security/security.c
index 12215ca286af..095e78efcb32 100644
--- a/security/security.c
+++ b/security/security.c
@@ -3969,21 +3969,23 @@  EXPORT_SYMBOL(security_d_instantiate);
  * @flags: special handling options. LSM_FLAG_SINGLE indicates that only
  * attributes associated with the LSM identified in the passed @ctx be
  * reported.
+ * @nattr: number of attributes found on success
  *
  * A NULL value for @uctx can be used to get both the number of attributes
  * and the size of the data.
  *
- * Returns the number of attributes found on success, negative value
- * on error. @size is reset to the total size of the data.
- * If @size is insufficient to contain the data -E2BIG is returned.
+ * Returns 0 on success, a negative error code on failure. @size is reset
+ * to the total size of the data. If @size is insufficient to contain the
+ * data -E2BIG is returned.
  */
 int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
-			 u32 __user *size, u32 flags)
+			 u32 __user *size, u32 flags, u32 *nattr)
 {
 	struct security_hook_list *hp;
 	struct lsm_ctx lctx = { .id = LSM_ID_UNDEF, };
 	u8 __user *base = (u8 __user *)uctx;
 	u32 entrysize;
+	u32 entrycount;
 	u32 total = 0;
 	u32 left;
 	bool toobig = false;
@@ -4024,7 +4026,8 @@  int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 		entrysize = left;
 		if (base)
 			uctx = (struct lsm_ctx __user *)(base + total);
-		rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags);
+		rc = hp->hook.getselfattr(attr, uctx, &entrysize, flags,
+					  &entrycount);
 		if (rc == -EOPNOTSUPP) {
 			rc = 0;
 			continue;
@@ -4039,7 +4042,7 @@  int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 			left -= entrysize;
 
 		total += entrysize;
-		count += rc;
+		count += entrycount;
 		if (single)
 			break;
 	}
@@ -4047,9 +4050,10 @@  int security_getselfattr(unsigned int attr, struct lsm_ctx __user *uctx,
 		return -EFAULT;
 	if (toobig)
 		return -E2BIG;
+	*nattr = count;
 	if (count == 0)
 		return LSM_RET_DEFAULT(getselfattr);
-	return count;
+	return 0;
 }
 
 /*
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f9a6637dfd78..0d35bb93baca 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6536,15 +6536,17 @@  static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
  * @ctx: buffer to receive the result
  * @size: buffer size (input), buffer size used (output)
  * @flags: unused
+ * @nattr: number of attributes found on success.
  *
  * Fill the passed user space @ctx with the details of the requested
  * attribute.
  *
- * Returns the number of attributes on success, an error code otherwise.
- * There will only ever be one attribute.
+ * Returns 0 on success or a negative error code on failure.
+ * There will only ever be one attribute, so @nattr is set to
+ * 1 on success.
  */
 static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			       u32 *size, u32 flags)
+			       u32 *size, u32 flags, u32 *nattr)
 {
 	int rc;
 	char *val = NULL;
@@ -6555,7 +6557,10 @@  static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 		return val_len;
 	rc = lsm_fill_user_ctx(ctx, size, val, val_len, LSM_ID_SELINUX, 0);
 	kfree(val);
-	return (!rc ? 1 : rc);
+	if (rc < 0)
+		return rc;
+	*nattr = 1;
+	return 0;
 }
 
 static int selinux_setselfattr(unsigned int attr, struct lsm_ctx *ctx,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ae8f1c2d0ca6..63d9c5f456c1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3648,15 +3648,17 @@  static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
  * @ctx: buffer to receive the result
  * @size: available size in, actual size out
  * @flags: unused
+ * @nattr: number of attributes found on success
  *
  * Fill the passed user space @ctx with the details of the requested
  * attribute.
  *
- * Returns the number of attributes on success, an error code otherwise.
- * There will only ever be one attribute.
+ * Returns 0 on success or a ngetaive error code on failure.
+ * There will only ever be one attribute, so @nattr is set to
+ * 1 on success.
  */
 static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
-			     u32 *size, u32 flags)
+			     u32 *size, u32 flags, u32 *nattr)
 {
 	int rc;
 	struct smack_known *skp;
@@ -3668,7 +3670,10 @@  static int smack_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
 	rc = lsm_fill_user_ctx(ctx, size,
 			       skp->smk_known, strlen(skp->smk_known) + 1,
 			       LSM_ID_SMACK, 0);
-	return (!rc ? 1 : rc);
+	if (rc < 0)
+		return rc;
+	*nattr = 1;
+	return 0;
 }
 
 /**