Message-ID: <b94069c5-7b52-42ac-5072-180a01879894@schaufler-ca.com>
Date:   Mon, 3 Apr 2023 11:28:22 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Mickaël Salaün <mic@...ikod.net>,
        paul@...l-moore.com, linux-security-module@...r.kernel.org
Cc:     jmorris@...ei.org, keescook@...omium.org,
        john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
        stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
        linux-api@...r.kernel.org, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v7 04/11] LSM: syscalls for current process attributes

On 4/3/2023 11:04 AM, Mickaël Salaün wrote:
>
> 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@...aufler-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@...aufler-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)?

The LSM ID is already in the lsm_attr being passed. An additional argument
would be redundant and introduce a potential error when the two values don't
match.

>
>
>>
>>> 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.

User space is going to have to deal with multiple values somehow,
either by fetching each possible value independently or by getting
them all at once in a set. Neither is pretty.

>
>>
>>>
>>>
>>>> + */
>>>> +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.

That does not meet the design requirement. Paul wants a lsm_attr structure.
I'm not going to deviate from that.

>
>>
>>>
>>>
>>>> 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.

I appreciate the viewpoint. It's not what I understand the maintainer wants.

>
>>
>>>
>>>
>>>> +        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)
>>>>    {

Powered by blists - more mailing lists