On 1/11/2023 1:00 PM, Paul Moore wrote:
> On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> Create a struct lsm_id to contain identifying information
>> about Linux Security Modules (LSMs). At inception this contains
>> the name of the module, an identifier associated with the security
>> module and an integer member "attrs_used" which identifies the API
>> related data associated with each security module. The initial set
>> of features maps to information that has traditionaly been available
>> in /proc/self/attr. They are documented in a new userspace-api file.
>> Change the security_add_hooks() interface to use this structure.
>> Change the individual modules to maintain their own struct lsm_id
>> and pass it to security_add_hooks().
>>
>> The values are for LSM identifiers are defined in a new UAPI
>> header file linux/lsm.h. Each existing LSM has been updated to
>> include it's LSMID in the lsm_id.
>>
>> The LSM ID values are sequential, with the oldest module
>> LSM_ID_CAPABILITY being the lowest value and the existing modules
>> numbered in the order they were included in the main line kernel.
>> This is an arbitrary convention for assigning the values, but
>> none better presents itself. The value 0 is defined as being invalid.
>> The values 1-99 are reserved for any special case uses which may
>> arise in the future. This may include attributes of the LSM
>> infrastructure itself, possibly related to namespacing or network
>> attribute management. A special range is identified for such attributes
>> to help reduce confusion for developers unfamiliar with LSMs.
>>
>> Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> ---
>> Documentation/userspace-api/index.rst | 1 +
>> Documentation/userspace-api/lsm.rst | 55 +++++++++++++++++++++++++++
>> include/linux/lsm_hooks.h | 18 ++++++++-
>> include/uapi/linux/lsm.h | 55 +++++++++++++++++++++++++++
>> security/apparmor/lsm.c | 9 ++++-
>> security/bpf/hooks.c | 13 ++++++-
>> security/commoncap.c | 8 +++-
>> security/landlock/cred.c | 2 +-
>> security/landlock/fs.c | 2 +-
>> security/landlock/ptrace.c | 2 +-
>> security/landlock/setup.c | 6 +++
>> security/landlock/setup.h | 1 +
>> security/loadpin/loadpin.c | 9 ++++-
>> security/lockdown/lockdown.c | 8 +++-
>> security/safesetid/lsm.c | 9 ++++-
>> security/security.c | 12 +++---
>> security/selinux/hooks.c | 11 +++++-
>> security/smack/smack_lsm.c | 9 ++++-
>> security/tomoyo/tomoyo.c | 9 ++++-
>> security/yama/yama_lsm.c | 8 +++-
>> 20 files changed, 226 insertions(+), 21 deletions(-)
>> create mode 100644 Documentation/userspace-api/lsm.rst
>> create mode 100644 include/uapi/linux/lsm.h
> Mostly just nitpicky stuff below ...
Nitpicky is fine.
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 0a5ba81f7367..6f2cabb79ec4 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -1665,6 +1665,20 @@ struct security_hook_heads {
>> #undef LSM_HOOK
>> } __randomize_layout;
>>
>> +/**
>> + * struct lsm_id - identify a Linux Security Module.
> According to the kernel-doc documentation it looks like "identify"
> should be capitalized.
>
> * https://docs.kernel.org/doc-guide/kernel-doc.html
I'll fix this, and the next as well. Thanks for pointing it out.
>> + * @lsm: Name of the LSM. Must be approved by the LSM maintainers.
>> + * @id: LSM ID number from uapi/linux/lsm.h
>> + * @attrs_used: Which attributes this LSM supports.
> In a bit of a reversal to the above comment, it appears that the
> parameter descriptions should not start with a capital and should not
> end with punctuation:
>
> * @lsm: name of the LSM, must be approved by the LSM maintainers
>
>> + * Contains the information that identifies the LSM.
>> + */
>> +struct lsm_id {
>> + const u8 *lsm;
>> + u32 id;
>> + u64 attrs_used;
>> +};
>> @@ -1708,7 +1722,7 @@ extern struct security_hook_heads security_hook_heads;
>> extern char *lsm_names;
>>
>> extern void security_add_hooks(struct security_hook_list *hooks, int count,
>> - const char *lsm);
>> + struct lsm_id *lsmid);
> We should be able to mark @lsmid as a const, right?
At this point, yes, but the const would have to come off when
the "slot" field is added to lsm_id in the upcoming stacking patches.
I can mark it const for now if it is important.
>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> new file mode 100644
>> index 000000000000..61a91b7d946f
>> --- /dev/null
>> +++ b/include/uapi/linux/lsm.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Linux Security Modules (LSM) - User space API
>> + *
>> + * Copyright (C) 2022 Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>> + * Copyright (C) 2022 Intel Corporation
>> + */
> This file should be added to the SECURITY SUBSYSTEM section in MAINTAINERS:
>
> F: include/uapi/linux/lsm.h
S'truth.
>> +#ifndef _UAPI_LINUX_LSM_H
>> +#define _UAPI_LINUX_LSM_H
>> +
>> +/*
>> + * ID values to identify security modules.
>> + * A system may use more than one security module.
>> + *
>> + * A value of 0 is considered invalid.
>> + * Values 1-99 are reserved for future use.
>> + * The interface is designed to extend to attributes beyond those which
>> + * are active today. Currently all the attributes are specific to the
>> + * individual modules. The LSM infrastructure itself has no variable state,
>> + * but that may change. One proposal would allow loadable modules, in which
>> + * case an attribute such as LSM_IS_LOADABLE might identify the dynamic
>> + * modules. Another potential attribute could be which security modules is
>> + * associated withnetwork labeling using netlabel. Another possible attribute
>> + * could be related to stacking behavior in a namespaced environment.
>> + * While it would be possible to intermingle the LSM infrastructure attribute
>> + * values with the security module provided values, keeping them separate
>> + * provides a clearer distinction.
>> + */
> As this is in a UAPI file, let's avoid speculation and stick to just
> the current facts. Anything we write here with respect to the future
> is likely to be wrong so let's not tempt fate.
Sure. I'll leave the rationale to the take message.
> Once I reached patch 3/8 I also realized that we may want to have more
> than just 0/invalid as a sentinel value, or at the very least we need
> to redefine 0 as something slightly different if for no other reason
> than we in fs/proc/base.c. I know it seems a little trivial, but
> since we're talking about values that will be used in the UAPI I think
> we need to give it some thought and discussion. The only think I can
> think of right now is to redefine 0 as "undefined", which isn't that
> far removed from "invalid" and will not look too terrible in patch 3/8
> - thoughts?
I originally had LSM_ID_INVALID for 0, but there was an objection.
It's not really invalid or undefined, it is reserved as not being an LSM id.
How about LSM_ID_NALSMID or LSM_ID_NOTALSMID for 0? sort of like NAN for
Not A Number.
> With all that in mind, I would suggest something like this:
>
> /*
> * ID tokens to identify Linux Security Modules (LSMs)
> *
> * These token values are used to uniquely identify specific LSMs
> * in the kernel as well in the kernel's LSM userspace API.
> *
> * A value of zero/0 is considered undefined and should not be used
> * outside of the kernel, values 1-99 are reserved for potential
> * future use.
> */
> #define LSM_ID_UNDEF 0
Fine, although I'd go with LSM_ID_NALSMID
>> +#define LSM_ID_CAPABILITY 100
>> +#define LSM_ID_SELINUX 101
>> +#define LSM_ID_SMACK 102
>> +#define LSM_ID_TOMOYO 103
>> +#define LSM_ID_IMA 104
>> +#define LSM_ID_APPARMOR 105
>> +#define LSM_ID_YAMA 106
>> +#define LSM_ID_LOADPIN 107
>> +#define LSM_ID_SAFESETID 108
>> +#define LSM_ID_LOCKDOWN 109
>> +#define LSM_ID_BPF 110
>> +#define LSM_ID_LANDLOCK 111
>> +
>> +/*
>> + * LSM_ATTR_XXX values identify the /proc/.../attr entry that the
>> + * context represents. Not all security modules provide all of these
>> + * values. Some security modules provide none of them.
>> + */
> I'd rather see text closer to this:
>
> /*
> * LSM attributes
> *
> * The LSM_ATTR_XXX definitions identify different LSM
> * attributes which are used in the kernel's LSM userspace API.
> * Support for these attributes vary across the different LSMs,
> * none are required.
> */
If you like that better I'm completely willing to adopt it.
>> +#define LSM_ATTR_CURRENT 0x0001
>> +#define LSM_ATTR_EXEC 0x0002
>> +#define LSM_ATTR_FSCREATE 0x0004
>> +#define LSM_ATTR_KEYCREATE 0x0008
>> +#define LSM_ATTR_PREV 0x0010
>> +#define LSM_ATTR_SOCKCREATE 0x0020
>> +
>> +#endif /* _UAPI_LINUX_LSM_H */
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index c6728a629437..63ea2a995987 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -24,6 +24,7 @@
>> #include <linux/zstd.h>
>> #include <net/sock.h>
>> #include <uapi/linux/mount.h>
>> +#include <uapi/linux/lsm.h>
>>
>> #include "include/apparmor.h"
>> #include "include/apparmorfs.h"
>> @@ -1217,6 +1218,12 @@ struct lsm_blob_sizes apparmor_blob_sizes __lsm_ro_after_init = {
>> .lbs_task = sizeof(struct aa_task_ctx),
>> };
>>
>> +static struct lsm_id apparmor_lsmid __lsm_ro_after_init = {
>> + .lsm = "apparmor",
>> + .id = LSM_ID_APPARMOR,
>> + .attrs_used = LSM_ATTR_CURRENT | LSM_ATTR_PREV | LSM_ATTR_EXEC,
>> +};
> Perhaps mark this as const in addition to ro_after_init? This applies
> to all the other @lsm_id defs in this patch too.
As I mentioned above, the lsm_id will eventually get changed during the
registration process. I can add the const for now, knowing full well that
it will be removed before long.
>> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
>> index e5971fa74fd7..20983ae8d31f 100644
>> --- a/security/bpf/hooks.c
>> +++ b/security/bpf/hooks.c
>> @@ -5,6 +5,7 @@
>> */
>> #include <linux/lsm_hooks.h>
>> #include <linux/bpf_lsm.h>
>> +#include <uapi/linux/lsm.h>
>>
>> static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>> #define LSM_HOOK(RET, DEFAULT, NAME, ...) \
>> @@ -15,9 +16,19 @@ static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {
>> LSM_HOOK_INIT(task_free, bpf_task_storage_free),
>> };
>>
>> +/*
>> + * slot has to be LSMBLOB_NEEDED because some of the hooks
>> + * supplied by this module require a slot.
>> + */
> I don't think we need the above comment here, right?
Whoops. I thought I'd gotten that one.
>
> --
> paul-moore.com
