Re: [PATCH v8 04/11] LSM: syscalls for current process attributes

From: Casey Schaufler
Date: Tue Apr 18 2023 - 18:34:21 EST

On 4/18/2023 2:49 PM, Paul Moore wrote:

> On Tue, Apr 11, 2023 at 12:01 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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
>> identifies 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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 | 30 +++++++++
>> kernel/sys_ni.c | 4 ++
>> security/Makefile | 1 +
>> security/lsm_syscalls.c | 55 ++++++++++++++++
>> security/security.c | 98 +++++++++++++++++++++++++++++
>> 10 files changed, 240 insertions(+)
>> create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 33a0ee3bcb2e..97487d66dca9 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -71,6 +71,7 @@ struct clone_args;
>> struct open_how;
>> struct mount_attr;
>> struct landlock_ruleset_attr;
>> +struct lsm_ctx;
>> enum landlock_rule_type;
>>
>> #include <linux/types.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, __u32 flags);
>> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> + __u32 flags);
> As pointed out by the kernel test robot, the above declaration is
> missing the @size parameter.

Yup.

>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index f27c9a9cc376..b10dfab8a4d9 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,36 @@
>> #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[];
>> +};
> Sorry, style nitpick since this needs to be respun anyway for the
> syscalls.h fix at the very least ... I *really* dislike when variable
> declarations, and field declarations in the case composite variables,
> are aligned with the vertically neighboring declarations; just use a
> single space please:

I'll do it, but the tab after type has been accepted for forever.
As an aside, I pulled out my 1978 K&R to prove my point and discovered
that it isn't consistent regarding this style.

> struct lsm_ctx {
> <tab>__u64 id;
> <tab>__u64 flags;
> <tab>__u64 len;
> <tab>__u64 ctx_len;
> <tab>__u8 ctx[];
> }
>
>> diff --git a/security/security.c b/security/security.c
>> index 38ca0e646cac..bfe9a1a426b2 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2167,6 +2167,104 @@ 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;
>> + u8 __user *base = (u8 __user *)ctx;
>> + size_t total = 0;
>> + size_t entrysize;
>> + size_t left;
>> + bool toobig = false;
>> + int count = 0;
>> + int rc;
>> +
>> + if (attr == 0)
>> + return -EINVAL;
>> + if (flags)
>> + return -EINVAL;
> I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which
> allows one to request a single LSM's attribute.

I don't, but I'll incorporate it.

> I don't think that
> support has to be part of this initial patchset, but I do think it
> would be good to have it in the same PR that goes up to Linus during
> the merge window.

As this patch set is intended to be what goes to Linus (isn't it?)
I'll put it in.

> If that's not something you want to do, let me know
> and I'll write up a quick patch on top of this patchset.
>
>> + if (size == NULL)
>> + return -EINVAL;
>> + if (get_user(left, size))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> + entrysize = left;
>> + if (base)
>> + ctx = (struct lsm_ctx __user *)(base + total);
>> + rc = hp->hook.getselfattr(attr, ctx, &entrysize, flags);
>> + if (rc == -EOPNOTSUPP) {
>> + rc = 0;
>> + continue;
>> + }
>> + if (rc == -E2BIG) {
>> + toobig = true;
>> + left = 0;
>> + break;
> It just occurred to me while reading this that we stop calculating the
> potential lsm_ctx size after we hit the first LSM where we go beyond
> the size given, `rc == -E2BIG`. I realize that the required size may
> change between calls to lsm_get_self_attr(2), but it seems like we
> should at least run through all the LSMs and total up the required
> buffer size, no?

The "break" should be a "continue". Artifact of an earlier version
that had a switch statement. Fix forthcoming.

>
> I may have missed something in the snippet below, but I think the code
> change should be pretty minor:
>
> if (rc == -EOPNOTSUPP) {
> rc = 0;
> continue;
> } else if (rc == -E2BIG) {
> toobig = true;
> base = NULL;
> } else if (rc < 0) {
> return rc;
> }
> left = (toobig ? 0 : left - entrysize);
> total += entrysize;
>
> count += rc;
>
>> + }
>> + if (rc < 0)
>> + return rc;
>> +
>> + left -= entrysize;
>> + total += entrysize;
>> + count += rc;
>> + }
>> + if (count == 0)
>> + return LSM_RET_DEFAULT(getselfattr);
>> + if (put_user(total, size))
>> + return -EFAULT;
>> + if (toobig)
>> + return -E2BIG;
>> + return count;
>> +}
> --
> paul-moore.com