Re: [PATCH v6 05/11] LSM: Create lsm_module_list system call

[Casey Schaufler <casey@schaufler-ca.com>]

On 3/7/2023 3:33 AM, Mickaël Salaün wrote:
>
> On 22/02/2023 21:08, Casey Schaufler wrote:
>> Create a system call to report the list of Linux Security Modules
>> that are active on the system. The list is provided as an array
>> of LSM ID numbers.
>>
>> The calling application can use this list determine what LSM
>> specific actions it might take. That might include chosing an
>
> "choosing"

Oops. Thank you.

>
>> output format, determining required privilege or bypassing
>> security module specific behavior.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>   Documentation/userspace-api/lsm.rst |  3 ++
>>   include/linux/syscalls.h            |  1 +
>>   kernel/sys_ni.c                     |  1 +
>>   security/lsm_syscalls.c             | 43 ++++++++++++++++++++++++++++-
>>   4 files changed, 47 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/lsm.rst
>> b/Documentation/userspace-api/lsm.rst
>> index b45e402302b3..ecdf1acd15b1 100644
>> --- a/Documentation/userspace-api/lsm.rst
>> +++ b/Documentation/userspace-api/lsm.rst
>> @@ -63,6 +63,9 @@ Get the specified security attributes of the
>> current process
>>   .. kernel-doc:: security/lsm_syscalls.c
>>       :identifiers: sys_lsm_get_self_attr
>>   +.. kernel-doc:: security/lsm_syscalls.c
>> +    :identifiers: sys_lsm_module_list
>> +
>>   Additional documentation
>>   ========================
>>   diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 1ef2a3de8ae0..9c947022a411 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -1062,6 +1062,7 @@ asmlinkage long
>> sys_set_mempolicy_home_node(unsigned long start, unsigned long l
>>   asmlinkage long sys_lsm_get_self_attr(struct lsm_ctx *ctx, size_t
>> *size,
>>                         __u64 flags);
>>   asmlinkage long sys_lsm_set_self_attr(struct lsm_ctx *ctx, __u64
>> flags);
>> +asmlinkage long sys_lsm_module_list(u64 *ids, size_t *size, int flags);
>>     /*
>>    * Architecture-specific system calls
>> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
>> index d03c78ef1562..32784e271fa5 100644
>> --- a/kernel/sys_ni.c
>> +++ b/kernel/sys_ni.c
>> @@ -265,6 +265,7 @@ COND_SYSCALL(mremap);
>>   /* security/lsm_syscalls.c */
>>   COND_SYSCALL(lsm_get_self_attr);
>>   COND_SYSCALL(lsm_set_self_attr);
>> +COND_SYSCALL(lsm_module_list);
>>     /* security/keys/keyctl.c */
>>   COND_SYSCALL(add_key);
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> index b89c4e7d009e..ccd3b236670b 100644
>> --- a/security/lsm_syscalls.c
>> +++ b/security/lsm_syscalls.c
>> @@ -19,7 +19,7 @@
>>     struct attrs_map {
>>       char *name;
>> -    int attrs;
>> +    u64 attrs;
>
> Why do we need this change in this patch?

We don't. It's gone in subsequent versions.

>
>>   };
>>     static const struct attrs_map lsm_attr_names[] = {
>> @@ -102,3 +102,44 @@ SYSCALL_DEFINE3(lsm_get_self_attr, struct
>> lsm_ctx __user *, ctx,
>>   {
>>       return security_getselfattr(flags, ctx, size);
>>   }
>> +
>> +/**
>> + * sys_lsm_module_list - Return a list of the active security modules
>> + * @ids: the LSM module ids
>> + * @size: size of @ids, updated on return
>> + * @flags: reserved for future use, must be zero
>> + *
>> + * Returns a list of the active LSM ids. On success this function
>> + * returns the number of @ids array elements. This value may be zero
>> + * if there are no LSMs active. 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_module_list,
>
> The name of this syscall differ from the two others: there is not
> "get" verb. What about "lsm_get_modules" or "lsm_list_modules"?

lsm_list_modules() it is henceforth.

>
>> +        u64 __user *, ids,
>> +        size_t __user *, size,
>> +        u64, flags)
>
> As Arnd said, flags should be a u32.

Agreed.

>
>> +{
>> +    size_t total_size = lsm_active_cnt * sizeof(*ids);
>> +    size_t usize;
>> +    int i;
>> +
>> +    if (flags)
>> +        return -EINVAL;
>> +
>> +    if (get_user(usize, size))
>> +        return -EFAULT;
>
> I'm not a fan of using the same pointer to read and write. This avoid
> using const pointers and differentiate between input and output
> values. I suggest using a dedicated argument for each.

This is pretty standard practice.

>
>> +
>> +    if (put_user(total_size, size) != 0)
>> +        return -EFAULT;
>> +
>> +    if (usize < total_size)
>> +        return -E2BIG;
>> +
>> +    for (i = 0; i < lsm_active_cnt; i++)
>> +        if (put_user(lsm_idlist[i]->id, ids++))
>
> I'm not sure about it, but it may be better to put the complete list
> of IDs at once. Is it better to set the size before or after?

You may be right. I'll consider it.

>
>
>> +            return -EFAULT;
>> +
>> +    return lsm_active_cnt;
>> +}