On Wed, Jan 11, 2023 at 7:05 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> 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
...
> >> 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
> >> @@ -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.
Ah, right. Okay, just leave it as-is then.
> >> 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
Hmm, I had to think a little on that to figure out the NALSMID bit.
Of the two I would prefer LSM_ID_UNDEF as UNDEF tends to be a bit more
common in the kernel and would be more likely to get people thinking
in the right direction.
> >> +#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.
Please.
> >> +#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.
No, that's okay. When I was reviewing this I forgot that changes
would be required here for stacking.
--
paul-moore.com
