Re: [PATCH v7 07/11] LSM: Helpers for attribute names and filling an lsm_ctx

[Casey Schaufler]

On 3/30/2023 4:28 PM, Paul Moore wrote:
> On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 3/29/2023 6:13 PM, Paul Moore wrote:
>>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>>>> Add lsm_name_to_attr(), which translates a text string to a
>>>> LSM_ATTR value if one is available.
>>>>
>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>>> the trailing attribute value.
>>>>
>>>> All are used in module specific components of LSM system calls.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>>> ---
>>>>  include/linux/security.h | 13 ++++++++++
>>>>  security/lsm_syscalls.c  | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>  security/security.c      | 31 ++++++++++++++++++++++++
>>>>  3 files changed, 95 insertions(+)
>>> ..
>>>
>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>> index 6efbe244d304..55d849ad5d6e 100644
>>>> --- a/security/lsm_syscalls.c
>>>> +++ b/security/lsm_syscalls.c
>>>> @@ -17,6 +17,57 @@
>>>>  #include <linux/lsm_hooks.h>
>>>>  #include <uapi/linux/lsm.h>
>>>>
>>>> +struct attr_map {
>>>> +       char *name;
>>>> +       u64 attr;
>>>> +};
>>>> +
>>>> +static const struct attr_map lsm_attr_names[] = {
>>>> +       {
>>>> +               .name = "current",
>>>> +               .attr = LSM_ATTR_CURRENT,
>>>> +       },
>>>> +       {
>>>> +               .name = "exec",
>>>> +               .attr = LSM_ATTR_EXEC,
>>>> +       },
>>>> +       {
>>>> +               .name = "fscreate",
>>>> +               .attr = LSM_ATTR_FSCREATE,
>>>> +       },
>>>> +       {
>>>> +               .name = "keycreate",
>>>> +               .attr = LSM_ATTR_KEYCREATE,
>>>> +       },
>>>> +       {
>>>> +               .name = "prev",
>>>> +               .attr = LSM_ATTR_PREV,
>>>> +       },
>>>> +       {
>>>> +               .name = "sockcreate",
>>>> +               .attr = LSM_ATTR_SOCKCREATE,
>>>> +       },
>>>> +};
>>>> +
>>>> +/**
>>>> + * lsm_name_to_attr - map an LSM attribute name to its ID
>>>> + * @name: name of the attribute
>>>> + *
>>>> + * Look the given @name up in the table of know attribute names.
>>>> + *
>>>> + * Returns the LSM attribute value associated with @name, or 0 if
>>>> + * there is no mapping.
>>>> + */
>>>> +u64 lsm_name_to_attr(const char *name)
>>>> +{
>>>> +       int i;
>>>> +
>>>> +       for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
>>>> +               if (!strcmp(name, lsm_attr_names[i].name))
>>>> +                       return lsm_attr_names[i].attr;
>>> I'm pretty sure this is the only place where @lsm_attr_names is used,
>>> right?  If true, when coupled with the idea that these syscalls are
>>> going to close the door on new LSM attributes in procfs I think we can
>>> just put the mapping directly in this function via a series of
>>> if-statements.
>> Ick. You're not wrong, but the hard coded if-statement approach goes
>> against all sorts of coding principles. I'll do it, but I can't say I
>> like it.
> If it helps any, I understand and am sympathetic.  I guess I've gotten
> to that point where in addition to "code elegance", I'm also very
> concerned about defending against "code abuse", and something like an
> nicely defined mapping array is ripe for someone to come along and use
> that to justify further use of the attribute string names in some
> other function/API.
>
> If you want to stick with the array - I have no problem with that -
> make it local to lsm_name_to_attr().
>
>>>> +/**
>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>>> + * @ctx: an LSM context to be filled
>>>> + * @context: the new context value
>>>> + * @context_size: the size of the new context value
>>>> + * @id: LSM id
>>>> + * @flags: LSM defined flags
>>>> + *
>>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>>> + * Caller is assumed to have verified that @ctx has enough space
>>>> + * for @context.
>>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>>> + */
>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>>> +                     size_t context_size, u64 id, u64 flags)
>>>> +{
>>>> +       struct lsm_ctx local;
>>>> +       void __user *vc = ctx;
>>>> +
>>>> +       local.id = id;
>>>> +       local.flags = flags;
>>>> +       local.ctx_len = context_size;
>>>> +       local.len = context_size + sizeof(local);
>>>> +       vc += sizeof(local);
>>> See my prior comments about void pointer math.
>>>
>>>> +       if (copy_to_user(ctx, &local, sizeof(local)))
>>>> +               return -EFAULT;
>>>> +       if (context_size > 0 && copy_to_user(vc, context, context_size))
>>>> +               return -EFAULT;
>>> Should we handle the padding in this function?
>> This function fills in a lsm_ctx. The padding, if any, is in addition to
>> the lsm_ctx, not part of it.
> Okay, so where is the padding managed?  I may have missed it, but I
> don't recall seeing it anywhere in this patchset ...

Padding isn't being managed. There has been talk about using padding to
expand the API, but there is no use for it now. Or is there?