2023-01-09 18:11:38

by Casey Schaufler

[permalink] [raw]
Subject: [PATCH v5 1/8] LSM: Identify modules by more than name

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 <[email protected]>
---
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/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index f16337bdb852..54c0f54cde89 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -31,6 +31,7 @@ place where this information is gathered.
sysfs-platform_profile
vduse
futex2
+ lsm

.. only:: subproject and html

diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
new file mode 100644
index 000000000000..6ddf5506110b
--- /dev/null
+++ b/Documentation/userspace-api/lsm.rst
@@ -0,0 +1,55 @@
+.. SPDX-License-Identifier: GPL-2.0
+.. Copyright (C) 2022 Casey Schaufler <[email protected]>
+.. Copyright (C) 2022 Intel Corporation
+
+=====================================
+Linux Security Modules
+=====================================
+
+:Author: Casey Schaufler
+:Date: November 2022
+
+Linux security modules (LSM) provide a mechanism to implement
+additional access controls to the Linux security policies.
+
+The various security modules may support any of these attributes:
+
+``LSM_ATTR_CURRENT`` is the current, active security context of the
+process.
+The proc filesystem provides this value in ``/proc/self/attr/current``.
+This is supported by the SELinux, Smack and AppArmor security modules.
+Smack also provides this value in ``/proc/self/attr/smack/current``.
+AppArmor also provides this value in ``/proc/self/attr/apparmor/current``.
+
+``LSM_ATTR_EXEC`` is the security context of the process at the time the
+current image was executed.
+The proc filesystem provides this value in ``/proc/self/attr/exec``.
+This is supported by the SELinux and AppArmor security modules.
+AppArmor also provides this value in ``/proc/self/attr/apparmor/exec``.
+
+``LSM_ATTR_FSCREATE`` is the security context of the process used when
+creating file system objects.
+The proc filesystem provides this value in ``/proc/self/attr/fscreate``.
+This is supported by the SELinux security module.
+
+``LSM_ATTR_KEYCREATE`` is the security context of the process used when
+creating key objects.
+The proc filesystem provides this value in ``/proc/self/attr/keycreate``.
+This is supported by the SELinux security module.
+
+``LSM_ATTR_PREV`` is the security context of the process at the time the
+current security context was set.
+The proc filesystem provides this value in ``/proc/self/attr/prev``.
+This is supported by the SELinux and AppArmor security modules.
+AppArmor also provides this value in ``/proc/self/attr/apparmor/prev``.
+
+``LSM_ATTR_SOCKCREATE`` is the security context of the process used when
+creating socket objects.
+The proc filesystem provides this value in ``/proc/self/attr/sockcreate``.
+This is supported by the SELinux security module.
+
+Additional documentation
+========================
+
+* Documentation/security/lsm.rst
+* Documentation/security/lsm-development.rst
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.
+ * @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.
+ *
+ * Contains the information that identifies the LSM.
+ */
+struct lsm_id {
+ const u8 *lsm;
+ u32 id;
+ u64 attrs_used;
+};
+
/*
* Security module hook list structure.
* For use with generic list macros for common operations.
@@ -1673,7 +1687,7 @@ struct security_hook_list {
struct hlist_node list;
struct hlist_head *head;
union security_list_options hook;
- const char *lsm;
+ struct lsm_id *lsmid;
} __randomize_layout;

/*
@@ -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);

#define LSM_FLAG_LEGACY_MAJOR BIT(0)
#define LSM_FLAG_EXCLUSIVE BIT(1)
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 <[email protected]>
+ * Copyright (C) 2022 Intel Corporation
+ */
+
+#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.
+ */
+#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.
+ */
+#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,
+};
+
static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
@@ -1912,7 +1919,7 @@ static int __init apparmor_init(void)
goto buffers_out;
}
security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
- "apparmor");
+ &apparmor_lsmid);

/* Report that AppArmor successfully initialized */
apparmor_initialized = 1;
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.
+ */
+struct lsm_id bpf_lsmid __lsm_ro_after_init = {
+ .lsm = "bpf",
+ .id = LSM_ID_BPF,
+};
+
static int __init bpf_lsm_init(void)
{
- security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
+ security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks),
+ &bpf_lsmid);
pr_info("LSM support for eBPF active\n");
return 0;
}
diff --git a/security/commoncap.c b/security/commoncap.c
index 1164278b97fd..76c5a0af95d6 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -25,6 +25,7 @@
#include <linux/binfmts.h>
#include <linux/personality.h>
#include <linux/mnt_idmapping.h>
+#include <uapi/linux/lsm.h>

/*
* If a non-root user executes a setuid-root binary in
@@ -1445,6 +1446,11 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,

#ifdef CONFIG_SECURITY

+static struct lsm_id capability_lsmid __lsm_ro_after_init = {
+ .lsm = "capability",
+ .id = LSM_ID_CAPABILITY,
+};
+
static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(capable, cap_capable),
LSM_HOOK_INIT(settime, cap_settime),
@@ -1469,7 +1475,7 @@ static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
static int __init capability_init(void)
{
security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
- "capability");
+ &capability_lsmid);
return 0;
}

diff --git a/security/landlock/cred.c b/security/landlock/cred.c
index ec6c37f04a19..2eb1d65f10d6 100644
--- a/security/landlock/cred.c
+++ b/security/landlock/cred.c
@@ -42,5 +42,5 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
__init void landlock_add_cred_hooks(void)
{
security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
- LANDLOCK_NAME);
+ &landlock_lsmid);
}
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index adcea0fe7e68..fa0e6e76991c 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1307,5 +1307,5 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
__init void landlock_add_fs_hooks(void)
{
security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
- LANDLOCK_NAME);
+ &landlock_lsmid);
}
diff --git a/security/landlock/ptrace.c b/security/landlock/ptrace.c
index 4c5b9cd71286..eab35808f395 100644
--- a/security/landlock/ptrace.c
+++ b/security/landlock/ptrace.c
@@ -116,5 +116,5 @@ static struct security_hook_list landlock_hooks[] __lsm_ro_after_init = {
__init void landlock_add_ptrace_hooks(void)
{
security_add_hooks(landlock_hooks, ARRAY_SIZE(landlock_hooks),
- LANDLOCK_NAME);
+ &landlock_lsmid);
}
diff --git a/security/landlock/setup.c b/security/landlock/setup.c
index 3f196d2ce4f9..9104133d04ca 100644
--- a/security/landlock/setup.c
+++ b/security/landlock/setup.c
@@ -8,6 +8,7 @@

#include <linux/init.h>
#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>

#include "common.h"
#include "cred.h"
@@ -24,6 +25,11 @@ struct lsm_blob_sizes landlock_blob_sizes __lsm_ro_after_init = {
.lbs_superblock = sizeof(struct landlock_superblock_security),
};

+struct lsm_id landlock_lsmid __lsm_ro_after_init = {
+ .lsm = LANDLOCK_NAME,
+ .id = LSM_ID_LANDLOCK,
+};
+
static int __init landlock_init(void)
{
landlock_add_cred_hooks();
diff --git a/security/landlock/setup.h b/security/landlock/setup.h
index 1daffab1ab4b..38bce5b172dc 100644
--- a/security/landlock/setup.h
+++ b/security/landlock/setup.h
@@ -14,5 +14,6 @@
extern bool landlock_initialized;

extern struct lsm_blob_sizes landlock_blob_sizes;
+extern struct lsm_id landlock_lsmid;

#endif /* _SECURITY_LANDLOCK_SETUP_H */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 110a5ab2b46b..d5c1373a096d 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -20,6 +20,7 @@
#include <linux/string_helpers.h>
#include <linux/dm-verity-loadpin.h>
#include <uapi/linux/loadpin.h>
+#include <uapi/linux/lsm.h>

#define VERITY_DIGEST_FILE_HEADER "# LOADPIN_TRUSTED_VERITY_ROOT_DIGESTS"

@@ -203,6 +204,11 @@ static int loadpin_load_data(enum kernel_load_data_id id, bool contents)
return loadpin_check(NULL, (enum kernel_read_file_id) id);
}

+static struct lsm_id loadpin_lsmid __lsm_ro_after_init = {
+ .lsm = "loadpin",
+ .id = LSM_ID_LOADPIN,
+};
+
static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
@@ -250,7 +256,8 @@ static int __init loadpin_init(void)
pr_info("ready to pin (currently %senforcing)\n",
enforce ? "" : "not ");
parse_exclude();
- security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+ security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks),
+ &loadpin_lsmid);

return 0;
}
diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
index a79b985e917e..e8c41a0caf7d 100644
--- a/security/lockdown/lockdown.c
+++ b/security/lockdown/lockdown.c
@@ -13,6 +13,7 @@
#include <linux/security.h>
#include <linux/export.h>
#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>

static enum lockdown_reason kernel_locked_down;

@@ -75,6 +76,11 @@ static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
};

+static struct lsm_id lockdown_lsmid __lsm_ro_after_init = {
+ .lsm = "lockdown",
+ .id = LSM_ID_LOCKDOWN,
+};
+
static int __init lockdown_lsm_init(void)
{
#if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
@@ -83,7 +89,7 @@ static int __init lockdown_lsm_init(void)
lock_kernel_down("Kernel configuration", LOCKDOWN_CONFIDENTIALITY_MAX);
#endif
security_add_hooks(lockdown_hooks, ARRAY_SIZE(lockdown_hooks),
- "lockdown");
+ &lockdown_lsmid);
return 0;
}

diff --git a/security/safesetid/lsm.c b/security/safesetid/lsm.c
index e806739f7868..8d0742ba045d 100644
--- a/security/safesetid/lsm.c
+++ b/security/safesetid/lsm.c
@@ -19,6 +19,7 @@
#include <linux/ptrace.h>
#include <linux/sched/task_stack.h>
#include <linux/security.h>
+#include <uapi/linux/lsm.h>
#include "lsm.h"

/* Flag indicating whether initialization completed */
@@ -261,6 +262,11 @@ static int safesetid_task_fix_setgroups(struct cred *new, const struct cred *old
return 0;
}

+static struct lsm_id safesetid_lsmid __lsm_ro_after_init = {
+ .lsm = "safesetid",
+ .id = LSM_ID_SAFESETID,
+};
+
static struct security_hook_list safesetid_security_hooks[] = {
LSM_HOOK_INIT(task_fix_setuid, safesetid_task_fix_setuid),
LSM_HOOK_INIT(task_fix_setgid, safesetid_task_fix_setgid),
@@ -271,7 +277,8 @@ static struct security_hook_list safesetid_security_hooks[] = {
static int __init safesetid_security_init(void)
{
security_add_hooks(safesetid_security_hooks,
- ARRAY_SIZE(safesetid_security_hooks), "safesetid");
+ ARRAY_SIZE(safesetid_security_hooks),
+ &safesetid_lsmid);

/* Report that SafeSetID successfully initialized */
safesetid_initialized = 1;
diff --git a/security/security.c b/security/security.c
index d1571900a8c7..07a8fe7f92bf 100644
--- a/security/security.c
+++ b/security/security.c
@@ -504,17 +504,17 @@ static int lsm_append(const char *new, char **result)
* security_add_hooks - Add a modules hooks to the hook lists.
* @hooks: the hooks to add
* @count: the number of hooks to add
- * @lsm: the name of the security module
+ * @lsmid: the identification information for the security module
*
* Each LSM has to register its hooks with the infrastructure.
*/
void __init security_add_hooks(struct security_hook_list *hooks, int count,
- const char *lsm)
+ struct lsm_id *lsmid)
{
int i;

for (i = 0; i < count; i++) {
- hooks[i].lsm = lsm;
+ hooks[i].lsmid = lsmid;
hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
}

@@ -523,7 +523,7 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
* and fix this up afterwards.
*/
if (slab_is_available()) {
- if (lsm_append(lsm, &lsm_names) < 0)
+ if (lsm_append(lsmid->lsm, &lsm_names) < 0)
panic("%s - Cannot get early memory.\n", __func__);
}
}
@@ -2145,7 +2145,7 @@ int security_getprocattr(struct task_struct *p, const char *lsm,
struct security_hook_list *hp;

hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsm))
+ if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
continue;
return hp->hook.getprocattr(p, name, value);
}
@@ -2158,7 +2158,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
struct security_hook_list *hp;

hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsm))
+ if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
continue;
return hp->hook.setprocattr(name, value, size);
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3c5be76a9199..7398819a0036 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -92,6 +92,7 @@
#include <linux/fsnotify.h>
#include <linux/fanotify.h>
#include <linux/io_uring.h>
+#include <uapi/linux/lsm.h>

#include "avc.h"
#include "objsec.h"
@@ -7032,6 +7033,13 @@ static int selinux_uring_cmd(struct io_uring_cmd *ioucmd)
}
#endif /* CONFIG_IO_URING */

+static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
+ .lsm = "selinux",
+ .id = LSM_ID_SELINUX,
+ .attrs_used = LSM_ATTR_CURRENT | LSM_ATTR_EXEC | LSM_ATTR_FSCREATE |
+ LSM_ATTR_KEYCREATE | LSM_ATTR_PREV | LSM_ATTR_SOCKCREATE,
+};
+
/*
* IMPORTANT NOTE: When adding new hooks, please be careful to keep this order:
* 1. any hooks that don't belong to (2.) or (3.) below,
@@ -7355,7 +7363,8 @@ static __init int selinux_init(void)

hashtab_cache_init();

- security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+ security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
+ &selinux_lsmid);

if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
panic("SELinux: Unable to register AVC netcache callback\n");
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 9a82a15685d1..8918b52cff43 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -43,6 +43,7 @@
#include <linux/fs_parser.h>
#include <linux/watch_queue.h>
#include <linux/io_uring.h>
+#include <uapi/linux/lsm.h>
#include "smack.h"

#define TRANS_TRUE "TRUE"
@@ -4856,6 +4857,12 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
.lbs_superblock = sizeof(struct superblock_smack),
};

+static struct lsm_id smack_lsmid __lsm_ro_after_init = {
+ .lsm = "smack",
+ .id = LSM_ID_SMACK,
+ .attrs_used = LSM_ATTR_CURRENT,
+};
+
static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
@@ -5062,7 +5069,7 @@ static __init int smack_init(void)
/*
* Register with LSM
*/
- security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+ security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), &smack_lsmid);
smack_enabled = 1;

pr_info("Smack: Initializing.\n");
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index af04a7b7eb28..a4658fb5ef0e 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -6,6 +6,7 @@
*/

#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>
#include "common.h"

/**
@@ -542,6 +543,11 @@ static void tomoyo_task_free(struct task_struct *task)
}
}

+static struct lsm_id tomoyo_lsmid __lsm_ro_after_init = {
+ .lsm = "tomoyo",
+ .id = LSM_ID_TOMOYO,
+};
+
/*
* tomoyo_security_ops is a "struct security_operations" which is used for
* registering TOMOYO.
@@ -595,7 +601,8 @@ static int __init tomoyo_init(void)
struct tomoyo_task *s = tomoyo_task(current);

/* register ourselves with the security framework */
- security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
+ security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks),
+ &tomoyo_lsmid);
pr_info("TOMOYO Linux initialized\n");
s->domain_info = &tomoyo_kernel_domain;
atomic_inc(&tomoyo_kernel_domain.users);
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 06e226166aab..2487b8f847f3 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -18,6 +18,7 @@
#include <linux/task_work.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
+#include <uapi/linux/lsm.h>

#define YAMA_SCOPE_DISABLED 0
#define YAMA_SCOPE_RELATIONAL 1
@@ -421,6 +422,11 @@ static int yama_ptrace_traceme(struct task_struct *parent)
return rc;
}

+static struct lsm_id yama_lsmid __lsm_ro_after_init = {
+ .lsm = "yama",
+ .id = LSM_ID_YAMA,
+};
+
static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
@@ -477,7 +483,7 @@ static inline void yama_init_sysctl(void) { }
static int __init yama_init(void)
{
pr_info("Yama: becoming mindful.\n");
- security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
+ security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), &yama_lsmid);
yama_init_sysctl();
return 0;
}
--
2.39.0


2023-01-11 21:14:43

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v5 1/8] LSM: Identify modules by more than name

On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
> ---
> 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 ...

> 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

> + * @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?

> 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 <[email protected]>
> + * Copyright (C) 2022 Intel Corporation
> + */

This file should be added to the SECURITY SUBSYSTEM section in MAINTAINERS:

F: include/uapi/linux/lsm.h

> +#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.

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?

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

> +#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.
*/

> +#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.

> 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?

--
paul-moore.com

2023-01-12 00:33:16

by Casey Schaufler

[permalink] [raw]
Subject: Re: [PATCH v5 1/8] LSM: Identify modules by more than name

On 1/11/2023 1:00 PM, Paul Moore wrote:
> On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> + * 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

2023-01-12 21:33:54

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v5 1/8] LSM: Identify modules by more than name

On Wed, Jan 11, 2023 at 7:05 PM Casey Schaufler <[email protected]> wrote:
> On 1/11/2023 1:00 PM, Paul Moore wrote:
> > On Mon, Jan 9, 2023 at 1:07 PM Casey Schaufler <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >> + * 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