Date:   Thu, 10 Nov 2022 16:36:20 -0800
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     casey.schaufler@...el.com, linux-security-module@...r.kernel.org,
        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, mic@...ikod.net, casey@...aufler-ca.com
Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On 11/10/2022 3:36 PM, Paul Moore wrote:
> On Wed, Nov 9, 2022 at 6:34 PM Paul Moore <paul@...l-moore.com> wrote:
>> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <casey@...aufler-ca.com> 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@...aufler-ca.com>
>>> ---
>>>  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
> ..
>
>>> +/**
>>> + * 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)
>>
> I wanted to clarify how I originally envisioned this syscall/API, as
> it looks like it behaves a bit differently than I originally intended.

That's why we're having a review cycle.

> My thought was that this syscall would be used to fetch one LSM
> attribute context at a time, returning an array of lsm_ctx structs,
> with one, and only one, for each LSM that supports that particular
> attribute.

My thought with the interface I proposed is that we don't want a
separate syscall for each attribute: e.g. lsm_get_current(), lsm_get_prev(),
and we don't want a separate syscall for each LSM, e.g. lsm_get_selinux().
We can either specify which attribute is desired or which have been returned.
In either case we need an attribute identifier.

>   If the LSM does not support that attribute, it must not
> enter an entry to the array.  Requesting more than one attribute
> context per invocation is not allowed.

Why? That seems like an unnecessary and inconvenient restriction
for the program that wants to see more than one attribute. A service
that wants to check its fscreate, sockcreate and keycreate to see if
they're appropriate for the container it's running in, for example.

>   The idea was to closely
> resemble the familiar open("/proc/self/attr/current")/read()/close()
> result without relying on procfs and supporting multiple LSMs with an
> easy(ish) API.

	rc = lsm_get_self_attr(struct lsm_ctx *ctx, size, attr_id, flags);

This looks like what you're asking for. It's what I had proposed but with
the attr_id specified in the call rather than returned in the lsm_ctx.

>   The new, single syscall should also be faster,
> although none of this should be happening in a performance critical
> section anyway.

Yes.

> In addition, the lsm_ctx::flags field is intended to convey
> information specific to the given LSM, i.e. it should not repeat any
> of the flag information specified in the syscall parameters.  I don't
> believe any of the currently in-tree LSMs would require any special
> flags for their contexts, so this should always be zero/clear in this
> initial patchset, but it is something to keep in mind for the future.
>
> Thoughts?

I don't have any problem with *what I think* you're suggesting.
To make sure, I'll send a new patch. I'll also address lsm_set_self_attr().
Thank you for the feedback. Let's see if we can converge.

>
> --
> paul-moore.com

Powered by blists - more mailing lists