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@xxxxxxxxxxxxxxxx>
>> ---
>> 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;
>> +}
