[PATCH v7 04/11] LSM: syscalls for current process attributes
Mickaël Salaün
mic at digikod.net
Mon Apr 3 18:04:12 UTC 2023
On 03/04/2023 19:36, Casey Schaufler wrote:
> On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:46, Casey Schaufler 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
>>> 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 at 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
>>
>> [...]
>>
>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>> new file mode 100644
>>> index 000000000000..feee31600219
>>> --- /dev/null
>>> +++ b/security/lsm_syscalls.c
>>> @@ -0,0 +1,55 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * System calls implementing the Linux Security Module API.
>>> + *
>>> + * Copyright (C) 2022 Casey Schaufler <casey at schaufler-ca.com>
>>> + * Copyright (C) 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <asm/current.h>
>>> +#include <linux/compiler_types.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/security.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/syscalls.h>
>>> +#include <linux/types.h>
>>> +#include <linux/lsm_hooks.h>
>>> +#include <uapi/linux/lsm.h>
>>> +
>>> +/**
>>> + * sys_lsm_set_self_attr - Set current task's security module attribute
>>> + * @attr: which attribute to set
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx
>>> + * @flags: reserved for future use
>>> + *
>>> + * Sets the calling task's LSM context. On success this function
>>> + * returns 0. If the attribute specified cannot be set a negative
>>> + * value indicating the reason for the error is returned.
>>
>> Do you think it is really worth it to implement syscalls that can get
>> and set attributes to several LSMs at the same time, instead of one at
>> a time?
>
> Setting the values for more than one LSM is impractical due to the possibility
> that the Nth value may fail, and unwinding the N-1 values may not be possible.
Indeed, so unless I missed something, why not passing the LSM ID as a
syscall argument for lsm_set_self_attr() and lsm_get_self_attr(), and
avoid managing a set of contexts but instead only managing one context
at a time (to get or set)?
>
>> LSM-specific tools don't care about other LSMs.
>
> That's part of the problem. Are systemd, dbusd, ps and id LSM specific tools?
> They shouldn't be.
>
>> I still think it would be much simpler (for kernel and user space) to
>> pass an LSM ID to both syscalls. This would avoid dealing with
>> variable arrays of variable element lengths, to both get or set
>> attributes.
>
> ps and id should both work regardless of which and how many LSMs provide
> context attributes. They shouldn't need to know which LSMs are active in
> advance. If a new LSM is introduced, they shouldn't need to be updated to
> support it.
I agree, and making the syscalls simpler doesn't change that.
>
>>
>> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
>> previously talked about, getting an unknown number of file descriptor
>> doesn't look good neither.
>
> If you have multiple LSM_ATTR_MAGICFD values and can only get one at
> a time you have to do something convoluted with flags to get them all.
> I don't see that as a good thing.
Yes, that was another argument to *not* deal with a set of contexts.
>
>>
>>
>>> + */
>>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> + ctx, size_t __user, size, u32, flags)
>>> +{
>>> + return security_setselfattr(attr, ctx, size, flags);
>>> +}
>>> +
>>> +/**
>>> + * sys_lsm_get_self_attr - Return current task's security module
>>> attributes
>>> + * @attr: which attribute to set
>>
>> attribute to *get*
>>
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx, updated on return
>>
>> I suggest to use a dedicated argument to read the allocated size, and
>> another to write the actual/written size.
>>
>> This would not be required with an LSM ID passed to the syscall
>> because attribute sizes should be known by user space, and there is no
>> need to help them probe this information.
>>
>>
>>> + * @flags: reserved for future use
>>> + *
>>> + * Returns the calling task's LSM contexts. On success this
>>> + * function returns the number of @ctx array elements. This value
>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>> + * insufficient to contain the return data -E2BIG is returned and
>>> + * @size is set to the minimum required size.
>>
>> Doing something (updating a buffer) even when returning an error
>> doesn't look right. These sizes should be well-known to user space and
>> part of the ABI/UAPI.
>
> No. The size of attributes is not well known to user space.
> They are usually text strings. The maximum size will be known,
> but that's putting additional burden on user space to know
> about all possible LSMs. It's not always necessary.
Right, I forgot the strings stuff… The lsm_get_self_attr() syscall could
then return a ctx_actual_size (as one argument), and a ctx pointer (as
another argument). Similarly, the lsm_set_self_attr() syscall could use
a dedicated argument for ctx_size and another for the ctx pointer.
>
>>
>>
>>> In all other cases
>>> + * a negative value indicating the error is returned.
>>> + */
>>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> + ctx, size_t __user *, size, u32, flags)
>>> +{
>>> + return security_getselfattr(attr, ctx, size, flags);
>>> +}
>>> 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;
>>> + size_t total = 0;
>>> + size_t this;
>>> + size_t left;
>>> + bool istoobig = false;
>>> + 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);
>>> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>>> + switch (rc) {
>>> + case -EOPNOTSUPP:
>>> + rc = 0;
>>> + continue;
>>> + case -E2BIG:
>>> + istoobig = true;
>>> + left = 0;
>>> + break;
>>
>> These two error cases could be directly handled by
>> security_getselfattr() instead of relying on each LSM-specific
>> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
>
> Yes, they could. My understanding is that Paul wants the LSM layer
> to be "thin". Where possible and not insane, the logic should be in
> the LSM, not the infrastructure.
FWIW, since we are defining new syscalls to make user space's life
easier, I'm in favor of a well defined common behavior (e.g. returned
errno) and factoring common code to make each LSM-specific code thin.
>
>>
>>
>>> + case 0:
>>> + left -= this;
>>> + break;
>>> + default:
>>> + return rc;
>>> + }
>>> + total += this;
>>> + count++;
>>> + }
>>> + if (count == 0)
>>> + return LSM_RET_DEFAULT(getselfattr);
>>> + if (put_user(total, size))
>>> + return -EFAULT;
>>> + if (rc)
>>> + return rc;
>>> + 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;
>>> +
>>> + if (flags != 0)
>>> + return -EINVAL;
>>> + if (size < sizeof(*ctx))
>>> + return -EINVAL;
>>> + 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);
>>> +
>>> + return LSM_RET_DEFAULT(setselfattr);
>>> +}
>>> +
>>> int security_getprocattr(struct task_struct *p, int lsmid, const
>>> char *name,
>>> char **value)
>>> {
More information about the Linux-security-module-archive
mailing list