Re: [PATCH v7 04/11] LSM: syscalls for current process attributes
From: Paul Moore <paul@paul-moore.com>
Date: 2023-03-30 01:12:45
Also in:
linux-security-module, lkml
On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler [off-list ref] wrote:
Create a system call lsm_get_self_attr() to provide the security module maintained attributes of the current process. Create a system call lsm_set_self_attr() to set a security module maintained attribute of the current process. Historically these attributes have been exposed to user space via entries in procfs under /proc/self/attr. The attribute value is provided in a lsm_ctx structure. The structure identifys the size of the attribute, and the attribute value. The format
"identifies"
of the attribute value is defined by the security module. A flags field
is included for LSM specific information. It is currently unused and must
be 0. The total size of the data, including the lsm_ctx structure and any
padding, is maintained as well.
struct lsm_ctx {
__u64 id;
__u64 flags;
__u64 len;
__u64 ctx_len;
__u8 ctx[];
};
Two new LSM hooks are used to interface with the LSMs.
security_getselfattr() collects the lsm_ctx values from the
LSMs that support the hook, accounting for space requirements.
security_setselfattr() identifies which LSM the attribute is
intended for and passes it along.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
Documentation/userspace-api/lsm.rst | 15 +++++
include/linux/lsm_hook_defs.h | 4 ++
include/linux/lsm_hooks.h | 9 +++
include/linux/security.h | 19 ++++++
include/linux/syscalls.h | 5 ++
include/uapi/linux/lsm.h | 33 ++++++++++
kernel/sys_ni.c | 4 ++
security/Makefile | 1 +
security/lsm_syscalls.c | 55 ++++++++++++++++
security/security.c | 97 +++++++++++++++++++++++++++++
10 files changed, 242 insertions(+)
create mode 100644 security/lsm_syscalls.c...
quoted hunk ↗ jump to hunk
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 32285ce65419..3c2c4916bd53 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h@@ -503,6 +504,14 @@ * and writing the xattrs as this hook is merely a filter. * @d_instantiate: * Fill in @inode security information for a @dentry if allowed. + * @getselfattr: + * Read attribute @attr for the current process and store it into @ctx. + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported, + * or another negative value otherwise. + * @setselfattr: + * Set attribute @attr for the current process. + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported, + * or another negative value otherwise. * @getprocattr: * Read attribute @name for process @p and store it into @value if allowed. * Return the length of @value on success, a negative value otherwise.
I'm sure you're already aware of this, but the above will need to be moved to security.c due to the changes in the lsm/next branch. That said, if you're basing on Linus' tree that's fine too, I'll fix it up during the merge; thankfully it's not a significant merge conflict.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/security.h b/include/linux/security.h index 8faed81fc3b4..329cd9d2be50 100644 --- a/include/linux/security.h +++ b/include/linux/security.h@@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry, struct inode *inode) { } +static inline int security_getselfattr(unsigned int __user attr, + struct lsm_ctx __user *ctx, + size_t __user *size, u32 __user flags) +{ + return -EINVAL; +} + +static inline int security_setselfattr(unsigned int __user attr, + struct lsm_ctx __user *ctx, + size_t __user size, u32 __user flags) +{ + return -EINVAL; +}
It seems like EOPNOTSUPP might be more appropriate than EINVAL for both of these dummy implementations.
quoted hunk ↗ jump to hunk
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h index 33a0ee3bcb2e..3feca00cb0c1 100644 --- a/include/linux/syscalls.h +++ b/include/linux/syscalls.h@@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags); asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len, unsigned long home_node, unsigned long flags); +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx, + size_t *size, __u64 flags); +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx, + __u64 flags);
As the kernel test robot already pointed out, the above needs to be updated.
quoted hunk ↗ jump to hunk
/* * Architecture-specific system callsdiff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h index aa3e01867739..adfb55dce2fd 100644 --- a/include/uapi/linux/lsm.h +++ b/include/uapi/linux/lsm.h@@ -9,6 +9,39 @@ #ifndef _UAPI_LINUX_LSM_H #define _UAPI_LINUX_LSM_H +#include <linux/types.h> +#include <linux/unistd.h> + +/** + * struct lsm_ctx - LSM context information + * @id: the LSM id number, see LSM_ID_XXX + * @flags: LSM specific flags + * @len: length of the lsm_ctx struct, @ctx and any other data or padding + * @ctx_len: the size of @ctx + * @ctx: the LSM context value + * + * The @len field MUST be equal to the size of the lsm_ctx struct + * plus any additional padding and/or data placed after @ctx. + * + * In all cases @ctx_len MUST be equal to the length of @ctx. + * If @ctx is a string value it should be nul terminated with + * @ctx_len equal to `strlen(@ctx) + 1`. Binary values are + * supported. + * + * The @flags and @ctx fields SHOULD only be interpreted by the + * LSM specified by @id; they MUST be set to zero/0 when not used. + */ +struct lsm_ctx { + __u64 id; + __u64 flags; + __u64 len; + __u64 ctx_len; + __u8 ctx[]; +}; + +#include <linux/types.h> +#include <linux/unistd.h>
I'm pretty sure the repeated #includes are a typo, right? Or is there some uapi trick I'm missing ...
quoted hunk ↗ jump to hunk
diff --git a/security/security.c b/security/security.c index 87c8796c3c46..2c57fe28c4f7 100644 --- a/security/security.c +++ b/security/security.c@@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode) } EXPORT_SYMBOL(security_d_instantiate); +/** + * security_getselfattr - Read an LSM attribute of the current process. + * @attr: which attribute to return + * @ctx: the user-space destination for the information, or NULL + * @size: the size of space available to receive the data + * @flags: reserved for future use, must be 0 + * + * 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. + */ +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx, + size_t __user *size, u32 __user flags) +{ + struct security_hook_list *hp; + void __user *base = (void *)ctx;
The casting seems wrong for a couple of reasons: I don't believe you need to cast the right side when the left side is a void pointer, and the right side cast drops the '__user' attribute when the left side is also a '__user' pointer value. That said, I think we may want @base to be 'u8 __user *base', more on that below ...
+ size_t total = 0; + size_t this;
Naming is hard, but 'this'? You can do better ...
+ size_t left; + bool istoobig = false;
Sorry, more naming nits and since it looks like you need to respin anyway ... please rename @istoobig to @toobig or something else. The phrases-as-variable-names has always grated on me.
+ int count = 0;
+ int rc;
+
+ if (attr == 0)
+ return -EINVAL;
+ if (flags != 0)
+ return -EINVAL;
+ if (size == NULL)
+ return -EINVAL;
+ if (get_user(left, size))
+ return -EFAULT;
+
+ hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
+ this = left;
+ if (base)
+ ctx = (struct lsm_ctx __user *)(base + total);Pointer math on void pointers always makes me nervous. Why not set @base's type to a 'u8' just to remove any concerns?
+ rc = hp->hook.getselfattr(attr, ctx, &this, flags);
+ switch (rc) {
+ case -EOPNOTSUPP:
+ rc = 0;
+ continue;
+ case -E2BIG:
+ istoobig = true;
+ left = 0;
+ break;
+ case 0:
+ left -= this;
+ break;
+ default:
+ return rc;I think the @getselfattr hook should behave similarly to the associated syscall, returning a non-negative number should indicate that @rc entries have been added to the @ctx array. Right now all the LSMs would just be adding one entry to the array, but we might as well code this up to be flexible.
+ } + total += this; + count++; + } + if (count == 0) + return LSM_RET_DEFAULT(getselfattr); + if (put_user(total, size)) + return -EFAULT; + if (rc) + return rc;
Is the 'if (rc)' check needed here? Shouldn't the switch-statement after the hook catch everything that this check would catch?
+ if (istoobig)
+ return -E2BIG;
+ return count;
+}
+
+/**
+ * security_setselfattr - Set an LSM attribute on the current process.
+ * @attr: which attribute to set
+ * @ctx: the user-space source for the information
+ * @size: the size of the data
+ * @flags: reserved for future use, must be 0
+ *
+ * Set an LSM attribute for the current process. The LSM, attribute
+ * and new value are included in @ctx.
+ *
+ * Returns 0 on success, an LSM specific value on failure.
+ */
+int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+ size_t __user size, u32 __user flags)
+{
+ struct security_hook_list *hp;
+ struct lsm_ctx lctx;Shouldn't we check @attr for valid values and return -EINVAL if bogus?
+ if (flags != 0) + return -EINVAL; + if (size < sizeof(*ctx)) + return -EINVAL;
If we're only going to support on 'lsm_ctx' entry in this function we should verify that the 'len' and 'ctx_len' fields are sane. Although more on this below ...
+ if (copy_from_user(&lctx, ctx, sizeof(*ctx))) + return -EFAULT; + + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list) + if ((hp->lsmid->id) == lctx.id) + return hp->hook.setselfattr(attr, ctx, size, flags);
Can anyone think of any good reason why we shouldn't support setting multiple LSMs in one call, similar to what we do with security_getselfattr()? It seems like it might be a nice thing to have ...
+ return LSM_RET_DEFAULT(setselfattr);
+}
+
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value)
{
--
2.39.2-- paul-moore.com