On 11/9/2022 3:34 PM, Paul Moore wrote:
> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> Create a system call lsm_self_attr() to provide the security
>> module maintained attributes of the current process. Historically
>> these attributes have been exposed to user space via entries in
>> procfs under /proc/self/attr.
>>
>> Attributes are provided as a collection of lsm_ctx structures
>> which are placed into a user supplied buffer. Each structure
>> identifys the security module providing the attribute, which
>> of the possible attributes is provided, the size of the
>> attribute, and finally the attribute value. The format of the
>> attribute value is defined by the security module, but will
>> always be \0 terminated. The ctx_len value will be larger than
>> strlen(ctx).
>>
>> ------------------------------
>> | unsigned int id |
>> ------------------------------
>> | unsigned int flags |
>> ------------------------------
>> | __kernel_size_t ctx_len |
>> ------------------------------
>> | unsigned char ctx[ctx_len] |
>> ------------------------------
>> | unsigned int id |
>> ------------------------------
>> | unsigned int flags |
>> ------------------------------
>> | __kernel_size_t ctx_len |
>> ------------------------------
>> | unsigned char ctx[ctx_len] |
>> ------------------------------
>>
>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> ---
>> include/linux/syscalls.h | 2 +
>> include/uapi/linux/lsm.h | 21 ++++++
>> kernel/sys_ni.c | 3 +
>> security/Makefile | 1 +
>> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 183 insertions(+)
>> create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index 61e13b1b9ece..1d27fb5b7746 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,27 @@
>> #ifndef _UAPI_LINUX_LSM_H
>> #define _UAPI_LINUX_LSM_H
>>
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
>> +
>> +/**
>> + * struct lsm_ctx - LSM context
>> + * @id: the LSM id number, see LSM_ID_XXX
>> + * @flags: context specifier and LSM specific flags
>> + * @ctx_len: the size of @ctx
>> + * @ctx: the LSM context, a nul terminated string
>> + *
>> + * @ctx in a nul terminated string.
>> + * (strlen(@ctx) < @ctx_len) is always true.
>> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
>> + */
> We can do better than this, or rather we *should* do better than this.
> One of the big advantages of creating a new API is we can fix some of
> the silly things we have had to do in the past, including the
> "sometimes" NUL terminator on strings. For this LSM syscalls let's
> make a promise that all human readable strings will be properly NUL
> terminated;
It requires effort and buffer management to ensure that ctx_len == strlen(ctx)+1,
but if you think it's important, sure.
> currently this includes all LSM contexts, and likely will
> remain that way forever for various reasons, but let's leave the door
> open for arbitrary blobs (see the "special" LSM ID discussion from
> earlier in the patchset). With that in mind I might suggest the
> following:
>
> /**
> * struct lsm_ctx - LSM context
> * @id: the LSM id number, see LSM_ID_XXX
> * @flags: context specifier and LSM specific flags
> * @ctx_len: the size of @ctx
> * @ctx: the LSM context, see description
> *
> * For LSMs which provide human readable contexts @ctx will be a nul
> * terminated string and @ctx_len will include the size of the string
> * and the nul terminator, e.g. 'ctx_len == strlen(ctx) + 1'. For LSMs
> * which provide binary-only contexts @cts will be a binary blob with
> * @ctx_len being the exact value of the blob. The type of the @ctx,
> * human readable string or binary, can be determined by inspecting
> * both @id and @flags.
I'd go a touch further, defining LSM_ATTR_BINARY as a flag and demanding
that any attribute that isn't a nul terminated string be thus identified,
even if it is always binary. Thus, LSM_ATTR_CURRENT and LSM_ATTR_BINARY
together would be an error.
> *
> * If a given LSM @id does not define a set of values for use in the
> * @flags field, @flags MUST be set to zero.
> */
> struct lsm_ctx {
> __u32 id;
> __U32 flags;
> __kernel_size_t ctx_len;
> __u8 ctx[];
> };
>
>> +struct lsm_ctx {
>> + unsigned int id;
>> + unsigned int flags;
>> + __kernel_size_t ctx_len;
>> + unsigned char ctx[];
>> +};
> I agree with Greg, we should be explicit about variable sizing, let's
> make sure everything in the UAPI header is defined in terms of
> __uXX/__sXX. This includes strings as __u8 arrays.
>
> Also, I sorta despite the 'let's line all the struct fields up
> horizontally!' approach in struct/variable definitions.
Kids. Got no respect for tradition.
> I personally
> think it looks horrible and it clutters up future patches. Please
> don't do this unless the individual file already does it, and since we
> are creating this new please don't :)
>
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> new file mode 100644
>> index 000000000000..da0fab7065e2
>> --- /dev/null
>> +++ b/security/lsm_syscalls.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * System calls implementing the Linux Security Module API.
>> + *
>> + * Copyright (C) 2022 Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> + * Copyright (C) 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>
>> +
>> +struct feature_map {
>> + char *name;
>> + int feature;
>> +};
>> +
>> +static const struct feature_map lsm_attr_names[] = {
>> + { .name = "current", .feature = LSM_ATTR_CURRENT, },
>> + { .name = "exec", .feature = LSM_ATTR_EXEC, },
>> + { .name = "fscreate", .feature = LSM_ATTR_FSCREATE, },
>> + { .name = "keycreate", .feature = LSM_ATTR_KEYCREATE, },
>> + { .name = "prev", .feature = LSM_ATTR_PREV, },
>> + { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
>> +};
>> +
>> +/**
>> + * lsm_self_attr - Return current task's security module attributes
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx, updated on return
>> + * @flags: reserved for future use, must be zero
>> + *
>> + * 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. In all other cases
>> + * a negative value indicating the error is returned.
>> + */
>> +SYSCALL_DEFINE3(lsm_self_attr,
>> + struct lsm_ctx __user *, ctx,
>> + size_t __user *, size,
>> + int, flags)
> See my comments above about UAPI types, let's change this to something
> like this:
>
> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
>
> SYSCALL_DEFINE3(lsm_self_attr,
> struct lsm_ctx __user *, ctx,
> __kernel_size_t __user *, size,
> __u32, flags)
>
>> +{
>> + struct lsm_ctx *final = NULL;
>> + struct lsm_ctx *interum;
>> + struct lsm_ctx *ip;
>> + void *curr;
>> + char **interum_ctx;
>> + char *cp;
>> + size_t total_size = 0;
>> + int count = 0;
>> + int attr;
>> + int len;
>> + int rc = 0;
>> + int i;
> Ungh, reverse christmas trees ... I kinda hate it from a style
> perspective, enough to mention it here, but I'm not going to be petty
> enough to say "change it".
I really don't care. Last I saw reverse christmas tree was the officially
recommended style. I don't care one way or the other.
>
> However, if you did want to flip it upside down (normal christmas
> tree?) during the respin I would be grateful ;)
>
> --
> paul-moore.com
