Add three system calls for the Linux Security Module ABI.
lsm_get_self_attr() provides the security module specific attributes
that have previously been visible in the /proc/self/attr directory.
For each security module that uses the specified attribute on the
current process the system call will return an LSM identifier and
the value of the attribute. The LSM and attribute identifier values
are defined in include/uapi/linux/lsm.h
LSM identifiers are simple integers and reflect the order in which
the LSM was added to the mainline kernel. This is a convention, not
a promise of the API. LSM identifiers below the value of 100 are
reserved for unspecified future uses. That could include information
about the security infrastructure itself, or about how multiple LSMs
might interact with each other.
A new LSM hook security_getselfattr() is introduced to get the
required information from the security modules. This is similar
to the existing security_getprocattr() hook, but specifies the
format in which string data is returned and requires the module
to put the information into a userspace destination.
lsm_set_self_attr() changes the specified LSM attribute. Only one
attribute can be changed at a time, and then only if the specified
security module allows the change.
A new LSM hook security_setselfattr() is introduced to set the
required information in the security modules. This is similar
to the existing security_setprocattr() hook, but specifies the
format in which string data is presented and requires the module
to get the information from a userspace destination.
lsm_list_modules() provides the LSM identifiers, in order, of the
security modules that are active on the system. This has been
available in the securityfs file /sys/kernel/security/lsm.
Patch 0001 changes the LSM registration from passing the name
of the module to passing a lsm_id structure that contains the
name of the module, an LSM identifier number and an attribute
identifier.
Patch 0002 adds the registered lsm_ids to a table.
Patch 0003 changes security_[gs]etprocattr() to use LSM IDs instead
of LSM names.
Patch 0004 implements lsm_get_self_attr() and lsm_set_self_attr().
New LSM hooks security_getselfattr() and security_setselfattr() are
defined.
Patch 0005 implements lsm_list_modules().
Patch 0006 wires up the syscalls.
Patch 0007 implements helper functions to make it easier for
security modules to use lsm_ctx structures.
Patch 0008 provides the Smack implementation for [gs]etselfattr().
Patch 0009 provides the AppArmor implementation for [gs]etselfattr().
Patch 0010 provides the SELinux implementation for [gs]etselfattr().
Patch 0011 implements selftests for the three new syscalls.
https://github.com/cschaufler/lsm-stacking.git#lsm-syscalls-6.3-rc2-a
v7: Pass the attribute desired to lsm_[gs]et_self_attr in its own
parameter rather than encoding it in the flags.
Change the flags parameters to u32.
Don't shortcut out of calling LSM specific code in the
infrastructure, let the LSM report that doesn't support an
attribute instead. With that it is not necessary to maintain
a set of supported attributes in the lsm_id structure.
Fix a typing error.
v6: Switch from reusing security_[gs]procattr() to using new
security_[gs]selfattr() hooks. Use explicit sized data types
in the lsm_ctx structure.
v5: Correct syscall parameter data types.
v4: Restore "reserved" LSM ID values. Add explaination.
Squash patches that introduce fields in lsm_id.
Correct a wireup error.
v3: Add lsm_set_self_attr().
Rename lsm_self_attr() to lsm_get_self_attr().
Provide the values only for a specifed attribute in
lsm_get_self_attr().
Add selftests for the three new syscalls.
Correct some parameter checking.
v2: Use user-interface safe data types.
Remove "reserved" LSM ID values.
Improve kerneldoc comments
Include copyright dates
Use more descriptive name for LSM counter
Add documentation
Correct wireup errors
Casey Schaufler (11):
LSM: Identify modules by more than name
LSM: Maintain a table of LSM attribute data
proc: Use lsmids instead of lsm names for attrs
LSM: syscalls for current process attributes
LSM: Create lsm_list_modules system call
LSM: wireup Linux Security Module syscalls
LSM: Helpers for attribute names and filling an lsm_ctx
Smack: implement setselfattr and getselfattr hooks
AppArmor: Add selfattr hooks
SELinux: Add selfattr hooks
LSM: selftests for Linux Security Module syscalls
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/lsm.rst | 73 +++++
MAINTAINERS | 1 +
arch/alpha/kernel/syscalls/syscall.tbl | 3 +
arch/arm/tools/syscall.tbl | 3 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 6 +
arch/ia64/kernel/syscalls/syscall.tbl | 3 +
arch/m68k/kernel/syscalls/syscall.tbl | 3 +
arch/microblaze/kernel/syscalls/syscall.tbl | 3 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 3 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 3 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 3 +
arch/parisc/kernel/syscalls/syscall.tbl | 3 +
arch/powerpc/kernel/syscalls/syscall.tbl | 3 +
arch/s390/kernel/syscalls/syscall.tbl | 3 +
arch/sh/kernel/syscalls/syscall.tbl | 3 +
arch/sparc/kernel/syscalls/syscall.tbl | 3 +
arch/x86/entry/syscalls/syscall_32.tbl | 3 +
arch/x86/entry/syscalls/syscall_64.tbl | 3 +
arch/xtensa/kernel/syscalls/syscall.tbl | 3 +
fs/proc/base.c | 29 +-
fs/proc/internal.h | 2 +-
include/linux/lsm_hook_defs.h | 4 +
include/linux/lsm_hooks.h | 27 +-
include/linux/security.h | 45 ++-
include/linux/syscalls.h | 6 +
include/uapi/asm-generic/unistd.h | 11 +-
include/uapi/linux/lsm.h | 86 ++++++
kernel/sys_ni.c | 5 +
security/Makefile | 1 +
security/apparmor/include/procattr.h | 2 +-
security/apparmor/lsm.c | 104 ++++++-
security/apparmor/procattr.c | 11 +-
security/bpf/hooks.c | 9 +-
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/lsm_syscalls.c | 145 ++++++++++
security/safesetid/lsm.c | 9 +-
security/security.c | 191 +++++++++++--
security/selinux/hooks.c | 156 ++++++++--
security/smack/smack_lsm.c | 113 +++++++-
security/tomoyo/tomoyo.c | 9 +-
security/yama/yama_lsm.c | 8 +-
.../arch/mips/entry/syscalls/syscall_n64.tbl | 3 +
.../arch/powerpc/entry/syscalls/syscall.tbl | 3 +
.../perf/arch/s390/entry/syscalls/syscall.tbl | 3 +
.../arch/x86/entry/syscalls/syscall_64.tbl | 3 +
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/lsm/Makefile | 12 +
tools/testing/selftests/lsm/config | 2 +
.../selftests/lsm/lsm_get_self_attr_test.c | 268 ++++++++++++++++++
.../selftests/lsm/lsm_list_modules_test.c | 149 ++++++++++
.../selftests/lsm/lsm_set_self_attr_test.c | 70 +++++
60 files changed, 1555 insertions(+), 101 deletions(-)
create mode 100644 Documentation/userspace-api/lsm.rst
create mode 100644 include/uapi/linux/lsm.h
create mode 100644 security/lsm_syscalls.c
create mode 100644 tools/testing/selftests/lsm/Makefile
create mode 100644 tools/testing/selftests/lsm/config
create mode 100644 tools/testing/selftests/lsm/lsm_get_self_attr_test.c
create mode 100644 tools/testing/selftests/lsm/lsm_list_modules_test.c
create mode 100644 tools/testing/selftests/lsm/lsm_set_self_attr_test.c
--
2.39.2
Use the LSM ID number instead of the LSM name to identify which
security module's attibute data should be shown in /proc/self/attr.
The security_[gs]etprocattr() functions have been changed to expect
the LSM ID. The change from a string comparison to an integer comparison
in these functions will provide a minor performance improvement.
Signed-off-by: Casey Schaufler <[email protected]>
Cc: [email protected]
---
fs/proc/base.c | 29 +++++++++++++++--------------
fs/proc/internal.h | 2 +-
include/linux/security.h | 11 +++++------
security/security.c | 11 +++++------
4 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 5e0e0ccd47aa..cb6dec7473fe 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -96,6 +96,7 @@
#include <linux/time_namespace.h>
#include <linux/resctrl.h>
#include <linux/cn_proc.h>
+#include <uapi/linux/lsm.h>
#include <trace/events/oom.h>
#include "internal.h"
#include "fd.h"
@@ -145,10 +146,10 @@ struct pid_entry {
NOD(NAME, (S_IFREG|(MODE)), \
NULL, &proc_single_file_operations, \
{ .proc_show = show } )
-#define ATTR(LSM, NAME, MODE) \
+#define ATTR(LSMID, NAME, MODE) \
NOD(NAME, (S_IFREG|(MODE)), \
NULL, &proc_pid_attr_operations, \
- { .lsm = LSM })
+ { .lsmid = LSMID })
/*
* Count the number of hardlinks for the pid_entry table, excluding the .
@@ -2730,7 +2731,7 @@ static ssize_t proc_pid_attr_read(struct file * file, char __user * buf,
if (!task)
return -ESRCH;
- length = security_getprocattr(task, PROC_I(inode)->op.lsm,
+ length = security_getprocattr(task, PROC_I(inode)->op.lsmid,
file->f_path.dentry->d_name.name,
&p);
put_task_struct(task);
@@ -2788,7 +2789,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
if (rv < 0)
goto out_free;
- rv = security_setprocattr(PROC_I(inode)->op.lsm,
+ rv = security_setprocattr(PROC_I(inode)->op.lsmid,
file->f_path.dentry->d_name.name, page,
count);
mutex_unlock(¤t->signal->cred_guard_mutex);
@@ -2837,27 +2838,27 @@ static const struct inode_operations proc_##LSM##_attr_dir_inode_ops = { \
#ifdef CONFIG_SECURITY_SMACK
static const struct pid_entry smack_attr_dir_stuff[] = {
- ATTR("smack", "current", 0666),
+ ATTR(LSM_ID_SMACK, "current", 0666),
};
LSM_DIR_OPS(smack);
#endif
#ifdef CONFIG_SECURITY_APPARMOR
static const struct pid_entry apparmor_attr_dir_stuff[] = {
- ATTR("apparmor", "current", 0666),
- ATTR("apparmor", "prev", 0444),
- ATTR("apparmor", "exec", 0666),
+ ATTR(LSM_ID_APPARMOR, "current", 0666),
+ ATTR(LSM_ID_APPARMOR, "prev", 0444),
+ ATTR(LSM_ID_APPARMOR, "exec", 0666),
};
LSM_DIR_OPS(apparmor);
#endif
static const struct pid_entry attr_dir_stuff[] = {
- ATTR(NULL, "current", 0666),
- ATTR(NULL, "prev", 0444),
- ATTR(NULL, "exec", 0666),
- ATTR(NULL, "fscreate", 0666),
- ATTR(NULL, "keycreate", 0666),
- ATTR(NULL, "sockcreate", 0666),
+ ATTR(LSM_ID_UNDEF, "current", 0666),
+ ATTR(LSM_ID_UNDEF, "prev", 0444),
+ ATTR(LSM_ID_UNDEF, "exec", 0666),
+ ATTR(LSM_ID_UNDEF, "fscreate", 0666),
+ ATTR(LSM_ID_UNDEF, "keycreate", 0666),
+ ATTR(LSM_ID_UNDEF, "sockcreate", 0666),
#ifdef CONFIG_SECURITY_SMACK
DIR("smack", 0555,
proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index 9dda7e54b2d0..a889d9ef9584 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -92,7 +92,7 @@ union proc_op {
int (*proc_show)(struct seq_file *m,
struct pid_namespace *ns, struct pid *pid,
struct task_struct *task);
- const char *lsm;
+ int lsmid;
};
struct proc_inode {
diff --git a/include/linux/security.h b/include/linux/security.h
index e70fc863b04a..8faed81fc3b4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -473,10 +473,9 @@ int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
-int security_getprocattr(struct task_struct *p, const char *lsm, const char *name,
+int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value);
-int security_setprocattr(const char *lsm, const char *name, void *value,
- size_t size);
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
int security_netlink_send(struct sock *sk, struct sk_buff *skb);
int security_ismaclabel(const char *name);
int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
@@ -1344,14 +1343,14 @@ static inline void security_d_instantiate(struct dentry *dentry,
struct inode *inode)
{ }
-static inline int security_getprocattr(struct task_struct *p, const char *lsm,
+static inline int security_getprocattr(struct task_struct *p, int lsmid,
const char *name, char **value)
{
return -EINVAL;
}
-static inline int security_setprocattr(const char *lsm, char *name,
- void *value, size_t size)
+static inline int security_setprocattr(int lsmid, char *name, void *value,
+ size_t size)
{
return -EINVAL;
}
diff --git a/security/security.c b/security/security.c
index aa84b1cf4253..87c8796c3c46 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2168,26 +2168,25 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
}
EXPORT_SYMBOL(security_d_instantiate);
-int security_getprocattr(struct task_struct *p, const char *lsm,
- const char *name, char **value)
+int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
+ char **value)
{
struct security_hook_list *hp;
hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
+ if (lsmid != 0 && lsmid != hp->lsmid->id)
continue;
return hp->hook.getprocattr(p, name, value);
}
return LSM_RET_DEFAULT(getprocattr);
}
-int security_setprocattr(const char *lsm, const char *name, void *value,
- size_t size)
+int security_setprocattr(int lsmid, const char *name, void *value, size_t size)
{
struct security_hook_list *hp;
hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) {
- if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
+ if (lsmid != 0 && lsmid != hp->lsmid->id)
continue;
return hp->hook.setprocattr(name, value, size);
}
--
2.39.2
Create a system call lsm_get_self_attr() to provide the security
module maintained attributes of the current process.
Create a system call lsm_set_self_attr() to set a security
module maintained attribute of the current process.
Historically these attributes have been exposed to user space via
entries in procfs under /proc/self/attr.
The attribute value is provided in a lsm_ctx structure. The structure
identifys the size of the attribute, and the attribute value. The format
of the attribute value is defined by the security module. A flags field
is included for LSM specific information. It is currently unused and must
be 0. The total size of the data, including the lsm_ctx structure and any
padding, is maintained as well.
struct lsm_ctx {
__u64 id;
__u64 flags;
__u64 len;
__u64 ctx_len;
__u8 ctx[];
};
Two new LSM hooks are used to interface with the LSMs.
security_getselfattr() collects the lsm_ctx values from the
LSMs that support the hook, accounting for space requirements.
security_setselfattr() identifies which LSM the attribute is
intended for and passes it along.
Signed-off-by: Casey Schaufler <[email protected]>
---
Documentation/userspace-api/lsm.rst | 15 +++++
include/linux/lsm_hook_defs.h | 4 ++
include/linux/lsm_hooks.h | 9 +++
include/linux/security.h | 19 ++++++
include/linux/syscalls.h | 5 ++
include/uapi/linux/lsm.h | 33 ++++++++++
kernel/sys_ni.c | 4 ++
security/Makefile | 1 +
security/lsm_syscalls.c | 55 ++++++++++++++++
security/security.c | 97 +++++++++++++++++++++++++++++
10 files changed, 242 insertions(+)
create mode 100644 security/lsm_syscalls.c
diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
index 6ddf5506110b..b45e402302b3 100644
--- a/Documentation/userspace-api/lsm.rst
+++ b/Documentation/userspace-api/lsm.rst
@@ -48,6 +48,21 @@ creating socket objects.
The proc filesystem provides this value in ``/proc/self/attr/sockcreate``.
This is supported by the SELinux security module.
+Kernel interface
+================
+
+Set a security attribute of the current process
+--------------------------------------------------
+
+.. kernel-doc:: security/lsm_syscalls.c
+ :identifiers: sys_lsm_set_self_attr
+
+Get the specified security attributes of the current process
+--------------------------------------------------
+
+.. kernel-doc:: security/lsm_syscalls.c
+ :identifiers: sys_lsm_get_self_attr
+
Additional documentation
========================
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 094b76dc7164..7177d9554f4a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -261,6 +261,10 @@ LSM_HOOK(int, 0, sem_semop, struct kern_ipc_perm *perm, struct sembuf *sops,
LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
struct inode *inode)
+LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t *size, u32 __user flags)
+LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t size, u32 __user flags)
LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
char **value)
LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 32285ce65419..3c2c4916bd53 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -25,6 +25,7 @@
#ifndef __LINUX_LSM_HOOKS_H
#define __LINUX_LSM_HOOKS_H
+#include <uapi/linux/lsm.h>
#include <linux/security.h>
#include <linux/init.h>
#include <linux/rculist.h>
@@ -503,6 +504,14 @@
* and writing the xattrs as this hook is merely a filter.
* @d_instantiate:
* Fill in @inode security information for a @dentry if allowed.
+ * @getselfattr:
+ * Read attribute @attr for the current process and store it into @ctx.
+ * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
+ * or another negative value otherwise.
+ * @setselfattr:
+ * Set attribute @attr for the current process.
+ * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
+ * or another negative value otherwise.
* @getprocattr:
* Read attribute @name for process @p and store it into @value if allowed.
* Return the length of @value on success, a negative value otherwise.
diff --git a/include/linux/security.h b/include/linux/security.h
index 8faed81fc3b4..329cd9d2be50 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -60,6 +60,7 @@ struct fs_parameter;
enum fs_value_type;
struct watch;
struct watch_notification;
+struct lsm_ctx;
/* Default (no) options for the capable function */
#define CAP_OPT_NONE 0x0
@@ -473,6 +474,10 @@ int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
unsigned nsops, int alter);
void security_d_instantiate(struct dentry *dentry, struct inode *inode);
+int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+ size_t __user *size, u32 __user flags);
+int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+ size_t __user size, u32 __user flags);
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value);
int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
@@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry,
struct inode *inode)
{ }
+static inline int security_getselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx,
+ size_t __user *size, u32 __user flags)
+{
+ return -EINVAL;
+}
+
+static inline int security_setselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx,
+ size_t __user size, u32 __user flags)
+{
+ return -EINVAL;
+}
+
static inline int security_getprocattr(struct task_struct *p, int lsmid,
const char *name, char **value)
{
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 33a0ee3bcb2e..3feca00cb0c1 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -71,6 +71,7 @@ struct clone_args;
struct open_how;
struct mount_attr;
struct landlock_ruleset_attr;
+struct lsm_ctx;
enum landlock_rule_type;
#include <linux/types.h>
@@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
unsigned long home_node,
unsigned long flags);
+asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
+ size_t *size, __u64 flags);
+asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
+ __u64 flags);
/*
* Architecture-specific system calls
diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
index aa3e01867739..adfb55dce2fd 100644
--- a/include/uapi/linux/lsm.h
+++ b/include/uapi/linux/lsm.h
@@ -9,6 +9,39 @@
#ifndef _UAPI_LINUX_LSM_H
#define _UAPI_LINUX_LSM_H
+#include <linux/types.h>
+#include <linux/unistd.h>
+
+/**
+ * struct lsm_ctx - LSM context information
+ * @id: the LSM id number, see LSM_ID_XXX
+ * @flags: LSM specific flags
+ * @len: length of the lsm_ctx struct, @ctx and any other data or padding
+ * @ctx_len: the size of @ctx
+ * @ctx: the LSM context value
+ *
+ * The @len field MUST be equal to the size of the lsm_ctx struct
+ * plus any additional padding and/or data placed after @ctx.
+ *
+ * In all cases @ctx_len MUST be equal to the length of @ctx.
+ * If @ctx is a string value it should be nul terminated with
+ * @ctx_len equal to `strlen(@ctx) + 1`. Binary values are
+ * supported.
+ *
+ * The @flags and @ctx fields SHOULD only be interpreted by the
+ * LSM specified by @id; they MUST be set to zero/0 when not used.
+ */
+struct lsm_ctx {
+ __u64 id;
+ __u64 flags;
+ __u64 len;
+ __u64 ctx_len;
+ __u8 ctx[];
+};
+
+#include <linux/types.h>
+#include <linux/unistd.h>
+
/*
* ID tokens to identify Linux Security Modules (LSMs)
*
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 860b2dcf3ac4..d03c78ef1562 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -262,6 +262,10 @@ COND_SYSCALL_COMPAT(recvmsg);
/* mm/nommu.c, also with MMU */
COND_SYSCALL(mremap);
+/* security/lsm_syscalls.c */
+COND_SYSCALL(lsm_get_self_attr);
+COND_SYSCALL(lsm_set_self_attr);
+
/* security/keys/keyctl.c */
COND_SYSCALL(add_key);
COND_SYSCALL(request_key);
diff --git a/security/Makefile b/security/Makefile
index 18121f8f85cd..59f238490665 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_KEYS) += keys/
# always enable default capabilities
obj-y += commoncap.o
+obj-$(CONFIG_SECURITY) += lsm_syscalls.o
obj-$(CONFIG_MMU) += min_addr.o
# Object file lists
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
new file mode 100644
index 000000000000..feee31600219
--- /dev/null
+++ b/security/lsm_syscalls.c
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * System calls implementing the Linux Security Module API.
+ *
+ * Copyright (C) 2022 Casey Schaufler <[email protected]>
+ * Copyright (C) 2022 Intel Corporation
+ */
+
+#include <asm/current.h>
+#include <linux/compiler_types.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/security.h>
+#include <linux/stddef.h>
+#include <linux/syscalls.h>
+#include <linux/types.h>
+#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>
+
+/**
+ * sys_lsm_set_self_attr - Set current task's security module attribute
+ * @attr: which attribute to set
+ * @ctx: the LSM contexts
+ * @size: size of @ctx
+ * @flags: reserved for future use
+ *
+ * Sets the calling task's LSM context. On success this function
+ * returns 0. If the attribute specified cannot be set a negative
+ * value indicating the reason for the error is returned.
+ */
+SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
+ ctx, size_t __user, size, u32, flags)
+{
+ return security_setselfattr(attr, ctx, size, flags);
+}
+
+/**
+ * sys_lsm_get_self_attr - Return current task's security module attributes
+ * @attr: which attribute to set
+ * @ctx: the LSM contexts
+ * @size: size of @ctx, updated on return
+ * @flags: reserved for future use
+ *
+ * Returns the calling task's LSM contexts. On success this
+ * function returns the number of @ctx array elements. This value
+ * may be zero if there are no LSM contexts assigned. 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_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
+ ctx, size_t __user *, size, u32, flags)
+{
+ return security_getselfattr(attr, ctx, size, flags);
+}
diff --git a/security/security.c b/security/security.c
index 87c8796c3c46..2c57fe28c4f7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
}
EXPORT_SYMBOL(security_d_instantiate);
+/**
+ * security_getselfattr - Read an LSM attribute of the current process.
+ * @attr: which attribute to return
+ * @ctx: the user-space destination for the information, or NULL
+ * @size: the size of space available to receive the data
+ * @flags: reserved for future use, must be 0
+ *
+ * Returns the number of attributes found on success, negative value
+ * on error. @size is reset to the total size of the data.
+ * If @size is insufficient to contain the data -E2BIG is returned.
+ */
+int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+ size_t __user *size, u32 __user flags)
+{
+ struct security_hook_list *hp;
+ void __user *base = (void *)ctx;
+ size_t total = 0;
+ size_t this;
+ size_t left;
+ bool istoobig = false;
+ int count = 0;
+ int rc;
+
+ if (attr == 0)
+ return -EINVAL;
+ if (flags != 0)
+ return -EINVAL;
+ if (size == NULL)
+ return -EINVAL;
+ if (get_user(left, size))
+ return -EFAULT;
+
+ hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
+ this = left;
+ if (base)
+ ctx = (struct lsm_ctx __user *)(base + total);
+ rc = hp->hook.getselfattr(attr, ctx, &this, flags);
+ switch (rc) {
+ case -EOPNOTSUPP:
+ rc = 0;
+ continue;
+ case -E2BIG:
+ istoobig = true;
+ left = 0;
+ break;
+ case 0:
+ left -= this;
+ break;
+ default:
+ return rc;
+ }
+ total += this;
+ count++;
+ }
+ if (count == 0)
+ return LSM_RET_DEFAULT(getselfattr);
+ if (put_user(total, size))
+ return -EFAULT;
+ if (rc)
+ return rc;
+ if (istoobig)
+ return -E2BIG;
+ return count;
+}
+
+/**
+ * security_setselfattr - Set an LSM attribute on the current process.
+ * @attr: which attribute to set
+ * @ctx: the user-space source for the information
+ * @size: the size of the data
+ * @flags: reserved for future use, must be 0
+ *
+ * Set an LSM attribute for the current process. The LSM, attribute
+ * and new value are included in @ctx.
+ *
+ * Returns 0 on success, an LSM specific value on failure.
+ */
+int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+ size_t __user size, u32 __user flags)
+{
+ struct security_hook_list *hp;
+ struct lsm_ctx lctx;
+
+ if (flags != 0)
+ return -EINVAL;
+ if (size < sizeof(*ctx))
+ return -EINVAL;
+ if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
+ return -EFAULT;
+
+ hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
+ if ((hp->lsmid->id) == lctx.id)
+ return hp->hook.setselfattr(attr, ctx, size, flags);
+
+ return LSM_RET_DEFAULT(setselfattr);
+}
+
int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
char **value)
{
--
2.39.2
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
output format, determining required privilege or bypassing
security module specific behavior.
Signed-off-by: Casey Schaufler <[email protected]>
---
Documentation/userspace-api/lsm.rst | 3 +++
include/linux/syscalls.h | 1 +
kernel/sys_ni.c | 1 +
security/lsm_syscalls.c | 39 +++++++++++++++++++++++++++++
4 files changed, 44 insertions(+)
diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
index b45e402302b3..a86e3817f062 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_list_modules
+
Additional documentation
========================
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 3feca00cb0c1..f755c583f949 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1063,6 +1063,7 @@ asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
size_t *size, __u64 flags);
asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
__u64 flags);
+asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
/*
* Architecture-specific system calls
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index d03c78ef1562..ceb3d21a62d0 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_list_modules);
/* security/keys/keyctl.c */
COND_SYSCALL(add_key);
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index feee31600219..6efbe244d304 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -53,3 +53,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
{
return security_getselfattr(attr, ctx, size, flags);
}
+
+/**
+ * sys_lsm_list_modules - 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_list_modules, u64 __user *, ids, size_t __user *, size,
+ u32, flags)
+{
+ 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;
+
+ 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++))
+ return -EFAULT;
+
+ return lsm_active_cnt;
+}
--
2.39.2
Wireup lsm_get_self_attr, lsm_set_self_attr and lsm_list_modules
system calls.
Signed-off-by: Casey Schaufler <[email protected]>
Acked-by: Geert Uytterhoeven <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Cc: [email protected]
---
arch/alpha/kernel/syscalls/syscall.tbl | 3 +++
arch/arm/tools/syscall.tbl | 3 +++
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 6 ++++++
arch/ia64/kernel/syscalls/syscall.tbl | 3 +++
arch/m68k/kernel/syscalls/syscall.tbl | 3 +++
arch/microblaze/kernel/syscalls/syscall.tbl | 3 +++
arch/mips/kernel/syscalls/syscall_n32.tbl | 3 +++
arch/mips/kernel/syscalls/syscall_n64.tbl | 3 +++
arch/mips/kernel/syscalls/syscall_o32.tbl | 3 +++
arch/parisc/kernel/syscalls/syscall.tbl | 3 +++
arch/powerpc/kernel/syscalls/syscall.tbl | 3 +++
arch/s390/kernel/syscalls/syscall.tbl | 3 +++
arch/sh/kernel/syscalls/syscall.tbl | 3 +++
arch/sparc/kernel/syscalls/syscall.tbl | 3 +++
arch/x86/entry/syscalls/syscall_32.tbl | 3 +++
arch/x86/entry/syscalls/syscall_64.tbl | 3 +++
arch/xtensa/kernel/syscalls/syscall.tbl | 3 +++
include/uapi/asm-generic/unistd.h | 11 ++++++++++-
tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl | 3 +++
tools/perf/arch/powerpc/entry/syscalls/syscall.tbl | 3 +++
tools/perf/arch/s390/entry/syscalls/syscall.tbl | 3 +++
tools/perf/arch/x86/entry/syscalls/syscall_64.tbl | 3 +++
23 files changed, 77 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 8ebacf37a8cf..178e2792c251 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -490,3 +490,6 @@
558 common process_mrelease sys_process_mrelease
559 common futex_waitv sys_futex_waitv
560 common set_mempolicy_home_node sys_ni_syscall
+561 common lsm_get_self_attr sys_lsm_get_self_attr
+562 common lsm_list_modules sys_lsm_list_modules
+563 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index ac964612d8b0..9cda144f9631 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -464,3 +464,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 037feba03a51..6a28fb91b85d 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -39,7 +39,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 451
+#define __NR_compat_syscalls 454
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index 604a2053d006..72022ffd5faa 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -907,6 +907,12 @@ __SYSCALL(__NR_process_mrelease, sys_process_mrelease)
__SYSCALL(__NR_futex_waitv, sys_futex_waitv)
#define __NR_set_mempolicy_home_node 450
__SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)
+#define __NR_lsm_get_self_attr 451
+__SYSCALL(__NR_lsm_get_self_attr, sys_lsm_get_self_attr)
+#define __NR_lsm_list_modules 452
+__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+#define __NR_lsm_set_self_attr 453
+__SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 72c929d9902b..c52e9d87f47d 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -371,3 +371,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index b1f3940bc298..31eac3c99d84 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -450,3 +450,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 820145e47350..5037fa1f74b8 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -456,3 +456,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index 253ff994ed2e..29545b3ec587 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -389,3 +389,6 @@
448 n32 process_mrelease sys_process_mrelease
449 n32 futex_waitv sys_futex_waitv
450 n32 set_mempolicy_home_node sys_set_mempolicy_home_node
+451 n32 lsm_get_self_attr sys_lsm_get_self_attr
+452 n32 lsm_list_modules sys_lsm_list_modules
+453 n32 lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 3f1886ad9d80..8492aa4a771f 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -365,3 +365,6 @@
448 n64 process_mrelease sys_process_mrelease
449 n64 futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 n64 lsm_get_self_attr sys_lsm_get_self_attr
+452 n64 lsm_list_modules sys_lsm_list_modules
+453 n64 lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 8f243e35a7b2..d74fd86de2a2 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -438,3 +438,6 @@
448 o32 process_mrelease sys_process_mrelease
449 o32 futex_waitv sys_futex_waitv
450 o32 set_mempolicy_home_node sys_set_mempolicy_home_node
+451 o32 lsm_get_self_attr sys_lsm_get_self_attr
+452 o32 lsm_list_modules sys_lsm_list_modules
+453 032 lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 0e42fceb2d5e..d1a5f3120d6c 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -448,3 +448,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index a0be127475b1..a414fe8c069b 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -537,3 +537,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index 799147658dee..96b7e6b72747 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -453,3 +453,6 @@
448 common process_mrelease sys_process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index 2de85c977f54..1a75a599bb55 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -453,3 +453,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 4398cc6fb68d..80b165091f6f 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -496,3 +496,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 320480a8db4f..130f9feb9eb9 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -455,3 +455,6 @@
448 i386 process_mrelease sys_process_mrelease
449 i386 futex_waitv sys_futex_waitv
450 i386 set_mempolicy_home_node sys_set_mempolicy_home_node
+451 i386 lsm_get_self_attr sys_lsm_get_self_attr
+452 i386 lsm_list_modules sys_lsm_list_modules
+453 i386 lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index c84d12608cd2..96dd45bc5988 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -372,6 +372,9 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
#
# Due to a historical design error, certain syscalls are numbered differently
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 52c94ab5c205..2610aba19802 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -421,3 +421,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 45fa180cc56a..93f89fb06ef5 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -886,8 +886,17 @@ __SYSCALL(__NR_futex_waitv, sys_futex_waitv)
#define __NR_set_mempolicy_home_node 450
__SYSCALL(__NR_set_mempolicy_home_node, sys_set_mempolicy_home_node)
+#define __NR_lsm_get_self_attr 451
+__SYSCALL(__NR_lsm_get_self_attr, sys_lsm_get_self_attr)
+
+#define __NR_lsm_list_modules 452
+__SYSCALL(__NR_lsm_list_modules, sys_lsm_list_modules)
+
+#define __NR_lsm_set_self_attr 453
+__SYSCALL(__NR_lsm_set_self_attr, sys_lsm_set_self_attr)
+
#undef __NR_syscalls
-#define __NR_syscalls 451
+#define __NR_syscalls 454
/*
* 32 bit systems traditionally used different
diff --git a/tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl b/tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl
index 3f1886ad9d80..8492aa4a771f 100644
--- a/tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl
+++ b/tools/perf/arch/mips/entry/syscalls/syscall_n64.tbl
@@ -365,3 +365,6 @@
448 n64 process_mrelease sys_process_mrelease
449 n64 futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 n64 lsm_get_self_attr sys_lsm_get_self_attr
+452 n64 lsm_list_modules sys_lsm_list_modules
+453 n64 lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
index a0be127475b1..a414fe8c069b 100644
--- a/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
+++ b/tools/perf/arch/powerpc/entry/syscalls/syscall.tbl
@@ -537,3 +537,6 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 nospu set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/tools/perf/arch/s390/entry/syscalls/syscall.tbl b/tools/perf/arch/s390/entry/syscalls/syscall.tbl
index 799147658dee..f9257e040109 100644
--- a/tools/perf/arch/s390/entry/syscalls/syscall.tbl
+++ b/tools/perf/arch/s390/entry/syscalls/syscall.tbl
@@ -453,3 +453,6 @@
448 common process_mrelease sys_process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr sys_lsm_set_self_attr
diff --git a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
index c84d12608cd2..96dd45bc5988 100644
--- a/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/tools/perf/arch/x86/entry/syscalls/syscall_64.tbl
@@ -372,6 +372,9 @@
448 common process_mrelease sys_process_mrelease
449 common futex_waitv sys_futex_waitv
450 common set_mempolicy_home_node sys_set_mempolicy_home_node
+451 common lsm_get_self_attr sys_lsm_get_self_attr
+452 common lsm_list_modules sys_lsm_list_modules
+453 common lsm_set_self_attr sys_lsm_set_self_attr
#
# Due to a historical design error, certain syscalls are numbered differently
--
2.39.2
Add lsm_name_to_attr(), which translates a text string to a
LSM_ATTR value if one is available.
Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
the trailing attribute value.
All are used in module specific components of LSM system calls.
Signed-off-by: Casey Schaufler <[email protected]>
---
include/linux/security.h | 13 ++++++++++
security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
security/security.c | 31 ++++++++++++++++++++++++
3 files changed, 95 insertions(+)
diff --git a/include/linux/security.h b/include/linux/security.h
index 329cd9d2be50..a5e860d332b5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -263,6 +263,7 @@ int unregister_blocking_lsm_notifier(struct notifier_block *nb);
/* prototypes */
extern int security_init(void);
extern int early_security_init(void);
+extern u64 lsm_name_to_attr(const char *name);
/* Security operations */
int security_binder_set_context_mgr(const struct cred *mgr);
@@ -491,6 +492,8 @@ int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
int security_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen);
int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
int security_locked_down(enum lockdown_reason what);
+int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
+ size_t context_size, u64 id, u64 flags);
#else /* CONFIG_SECURITY */
static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -508,6 +511,11 @@ static inline int unregister_blocking_lsm_notifier(struct notifier_block *nb)
return 0;
}
+static inline u64 lsm_name_to_attr(const char *name)
+{
+ return 0;
+}
+
static inline void security_free_mnt_opts(void **mnt_opts)
{
}
@@ -1420,6 +1428,11 @@ static inline int security_locked_down(enum lockdown_reason what)
{
return 0;
}
+static inline int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
+ size_t context_size, u64 id, u64 flags)
+{
+ return 0;
+}
#endif /* CONFIG_SECURITY */
#if defined(CONFIG_SECURITY) && defined(CONFIG_WATCH_QUEUE)
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 6efbe244d304..55d849ad5d6e 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c
@@ -17,6 +17,57 @@
#include <linux/lsm_hooks.h>
#include <uapi/linux/lsm.h>
+struct attr_map {
+ char *name;
+ u64 attr;
+};
+
+static const struct attr_map lsm_attr_names[] = {
+ {
+ .name = "current",
+ .attr = LSM_ATTR_CURRENT,
+ },
+ {
+ .name = "exec",
+ .attr = LSM_ATTR_EXEC,
+ },
+ {
+ .name = "fscreate",
+ .attr = LSM_ATTR_FSCREATE,
+ },
+ {
+ .name = "keycreate",
+ .attr = LSM_ATTR_KEYCREATE,
+ },
+ {
+ .name = "prev",
+ .attr = LSM_ATTR_PREV,
+ },
+ {
+ .name = "sockcreate",
+ .attr = LSM_ATTR_SOCKCREATE,
+ },
+};
+
+/**
+ * lsm_name_to_attr - map an LSM attribute name to its ID
+ * @name: name of the attribute
+ *
+ * Look the given @name up in the table of know attribute names.
+ *
+ * Returns the LSM attribute value associated with @name, or 0 if
+ * there is no mapping.
+ */
+u64 lsm_name_to_attr(const char *name)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
+ if (!strcmp(name, lsm_attr_names[i].name))
+ return lsm_attr_names[i].attr;
+ return 0;
+}
+
/**
* sys_lsm_set_self_attr - Set current task's security module attribute
* @attr: which attribute to set
diff --git a/security/security.c b/security/security.c
index 2c57fe28c4f7..f7b814a3940c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
return 0;
}
+/**
+ * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
+ * @ctx: an LSM context to be filled
+ * @context: the new context value
+ * @context_size: the size of the new context value
+ * @id: LSM id
+ * @flags: LSM defined flags
+ *
+ * Fill all of the fields in a user space lsm_ctx structure.
+ * Caller is assumed to have verified that @ctx has enough space
+ * for @context.
+ * Returns 0 on success, -EFAULT on a copyout error.
+ */
+int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
+ size_t context_size, u64 id, u64 flags)
+{
+ struct lsm_ctx local;
+ void __user *vc = ctx;
+
+ local.id = id;
+ local.flags = flags;
+ local.ctx_len = context_size;
+ local.len = context_size + sizeof(local);
+ vc += sizeof(local);
+ if (copy_to_user(ctx, &local, sizeof(local)))
+ return -EFAULT;
+ if (context_size > 0 && copy_to_user(vc, context, context_size))
+ return -EFAULT;
+ return 0;
+}
+
/*
* The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
* can be accessed with:
--
2.39.2
Implement Smack support for security_[gs]etselfattr.
Refactor the setprocattr hook to avoid code duplication.
Signed-off-by: Casey Schaufler <[email protected]>
---
security/smack/smack_lsm.c | 105 +++++++++++++++++++++++++++++++++++--
1 file changed, 100 insertions(+), 5 deletions(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 3cf862fcbe08..b3e72b82ced9 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3552,6 +3552,41 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode)
return;
}
+/**
+ * smack_getselfattr - Smack current process attribute
+ * @attr: which attribute to fetch
+ * @ctx: buffer to receive the result
+ * @size: available size in, actual size out
+ * @flags: unused
+ *
+ * Fill the passed user space @ctx with the details of the requested
+ * attribute.
+ *
+ * Returns 0 on success, an error code otherwise.
+ */
+static int smack_getselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t *size,
+ u32 __user flags)
+{
+ struct smack_known *skp = smk_of_current();
+ int total;
+ int slen;
+ int rc = 0;
+
+ if (attr != LSM_ATTR_CURRENT)
+ return -EOPNOTSUPP;
+
+ slen = strlen(skp->smk_known) + 1;
+ total = slen + sizeof(*ctx);
+ if (total > *size)
+ rc = -E2BIG;
+ else
+ lsm_fill_user_ctx(ctx, skp->smk_known, slen, LSM_ID_SMACK, 0);
+
+ *size = total;
+ return rc;
+}
+
/**
* smack_getprocattr - Smack process attribute access
* @p: the object task
@@ -3581,8 +3616,8 @@ static int smack_getprocattr(struct task_struct *p, const char *name, char **val
}
/**
- * smack_setprocattr - Smack process attribute setting
- * @name: the name of the attribute in /proc/.../attr
+ * do_setattr - Smack process attribute setting
+ * @attr: the ID of the attribute
* @value: the value to set
* @size: the size of the value
*
@@ -3591,7 +3626,7 @@ static int smack_getprocattr(struct task_struct *p, const char *name, char **val
*
* Returns the length of the smack label or an error code
*/
-static int smack_setprocattr(const char *name, void *value, size_t size)
+static int do_setattr(u64 attr, void *value, size_t size)
{
struct task_smack *tsp = smack_cred(current_cred());
struct cred *new;
@@ -3605,8 +3640,8 @@ static int smack_setprocattr(const char *name, void *value, size_t size)
if (value == NULL || size == 0 || size >= SMK_LONGLABEL)
return -EINVAL;
- if (strcmp(name, "current") != 0)
- return -EINVAL;
+ if (attr != LSM_ATTR_CURRENT)
+ return -EOPNOTSUPP;
skp = smk_import_entry(value, size);
if (IS_ERR(skp))
@@ -3645,6 +3680,64 @@ static int smack_setprocattr(const char *name, void *value, size_t size)
return size;
}
+/**
+ * smack_setselfattr - Set a Smack process attribute
+ * @attr: which attribute to set
+ * @ctx: buffer containing the data
+ * @size: size of @ctx
+ * @flags: unused
+ *
+ * Fill the passed user space @ctx with the details of the requested
+ * attribute.
+ *
+ * Returns 0 on success, an error code otherwise.
+ */
+static int smack_setselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t __user size,
+ u32 __user flags)
+{
+ struct lsm_ctx *lctx;
+ void *context;
+ int rc;
+
+ context = kmalloc(size, GFP_KERNEL);
+ if (context == NULL)
+ return -ENOMEM;
+
+ lctx = (struct lsm_ctx *)context;
+ if (copy_from_user(context, ctx, size))
+ rc = -EFAULT;
+ else if (lctx->ctx_len > size)
+ rc = -EINVAL;
+ else
+ rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
+
+ kfree(context);
+ if (rc > 0)
+ return 0;
+ return rc;
+}
+
+/**
+ * smack_setprocattr - Smack process attribute setting
+ * @name: the name of the attribute in /proc/.../attr
+ * @value: the value to set
+ * @size: the size of the value
+ *
+ * Sets the Smack value of the task. Only setting self
+ * is permitted and only with privilege
+ *
+ * Returns the length of the smack label or an error code
+ */
+static int smack_setprocattr(const char *name, void *value, size_t size)
+{
+ int attr = lsm_name_to_attr(name);
+
+ if (attr)
+ return do_setattr(attr, value, size);
+ return -EINVAL;
+}
+
/**
* smack_unix_stream_connect - Smack access on UDS
* @sock: one sock
@@ -4955,6 +5048,8 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(d_instantiate, smack_d_instantiate),
+ LSM_HOOK_INIT(getselfattr, smack_getselfattr),
+ LSM_HOOK_INIT(setselfattr, smack_setselfattr),
LSM_HOOK_INIT(getprocattr, smack_getprocattr),
LSM_HOOK_INIT(setprocattr, smack_setprocattr),
--
2.39.2
Add hooks for setselfattr and getselfattr. These hooks are not very
different from their setprocattr and getprocattr equivalents, and
much of the code is shared.
Signed-off-by: Casey Schaufler <[email protected]>
Cc: John Johansen <[email protected]>
---
security/apparmor/include/procattr.h | 2 +-
security/apparmor/lsm.c | 96 ++++++++++++++++++++++++++--
security/apparmor/procattr.c | 11 +++-
3 files changed, 99 insertions(+), 10 deletions(-)
diff --git a/security/apparmor/include/procattr.h b/security/apparmor/include/procattr.h
index 31689437e0e1..03dbfdb2f2c0 100644
--- a/security/apparmor/include/procattr.h
+++ b/security/apparmor/include/procattr.h
@@ -11,7 +11,7 @@
#ifndef __AA_PROCATTR_H
#define __AA_PROCATTR_H
-int aa_getprocattr(struct aa_label *label, char **string);
+int aa_getprocattr(struct aa_label *label, char **string, bool newline);
int aa_setprocattr_changehat(char *args, size_t size, int flags);
#endif /* __AA_PROCATTR_H */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ce6ccb7e06ec..89ee9d71791c 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -630,6 +630,45 @@ static int apparmor_sb_pivotroot(const struct path *old_path,
return error;
}
+static int apparmor_getselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *lx, size_t *size,
+ u32 __user flags)
+{
+ int error = -ENOENT;
+ struct aa_task_ctx *ctx = task_ctx(current);
+ struct aa_label *label = NULL;
+ size_t total_len;
+ char *value;
+
+ if (attr == LSM_ATTR_CURRENT)
+ label = aa_get_newest_label(cred_label(current_cred()));
+ else if (attr == LSM_ATTR_PREV && ctx->previous)
+ label = aa_get_newest_label(ctx->previous);
+ else if (attr == LSM_ATTR_EXEC && ctx->onexec)
+ label = aa_get_newest_label(ctx->onexec);
+ else
+ error = -EOPNOTSUPP;
+
+ if (label) {
+ error = aa_getprocattr(label, &value, false);
+ if (error > 0) {
+ total_len = error + sizeof(*ctx);
+ if (total_len > *size)
+ error = -E2BIG;
+ else
+ lsm_fill_user_ctx(lx, value, error,
+ LSM_ID_APPARMOR, 0);
+ }
+ }
+
+ aa_put_label(label);
+
+ *size = total_len;
+ if (error > 0)
+ return 0;
+ return error;
+}
+
static int apparmor_getprocattr(struct task_struct *task, const char *name,
char **value)
{
@@ -649,7 +688,7 @@ static int apparmor_getprocattr(struct task_struct *task, const char *name,
error = -EINVAL;
if (label)
- error = aa_getprocattr(label, value);
+ error = aa_getprocattr(label, value, true);
aa_put_label(label);
put_cred(cred);
@@ -657,8 +696,7 @@ static int apparmor_getprocattr(struct task_struct *task, const char *name,
return error;
}
-static int apparmor_setprocattr(const char *name, void *value,
- size_t size)
+static int do_setattr(u64 attr, void *value, size_t size)
{
char *command, *largs = NULL, *args = value;
size_t arg_size;
@@ -689,7 +727,7 @@ static int apparmor_setprocattr(const char *name, void *value,
goto out;
arg_size = size - (args - (largs ? largs : (char *) value));
- if (strcmp(name, "current") == 0) {
+ if (attr == LSM_ATTR_CURRENT) {
if (strcmp(command, "changehat") == 0) {
error = aa_setprocattr_changehat(args, arg_size,
AA_CHANGE_NOFLAGS);
@@ -704,7 +742,7 @@ static int apparmor_setprocattr(const char *name, void *value,
error = aa_change_profile(args, AA_CHANGE_STACK);
} else
goto fail;
- } else if (strcmp(name, "exec") == 0) {
+ } else if (attr == LSM_ATTR_EXEC) {
if (strcmp(command, "exec") == 0)
error = aa_change_profile(args, AA_CHANGE_ONEXEC);
else if (strcmp(command, "stack") == 0)
@@ -724,13 +762,57 @@ static int apparmor_setprocattr(const char *name, void *value,
fail:
aad(&sa)->label = begin_current_label_crit_section();
- aad(&sa)->info = name;
+ if (attr == LSM_ATTR_CURRENT)
+ aad(&sa)->info = "current";
+ else if (attr == LSM_ATTR_EXEC)
+ aad(&sa)->info = "exec";
+ else
+ aad(&sa)->info = "invalid";
aad(&sa)->error = error = -EINVAL;
aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL);
end_current_label_crit_section(aad(&sa)->label);
goto out;
}
+static int apparmor_setselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t __user size,
+ u32 __user flags)
+{
+ struct lsm_ctx *lctx;
+ void *context;
+ int rc;
+
+ if (attr != LSM_ATTR_CURRENT && attr != LSM_ATTR_EXEC)
+ return -EOPNOTSUPP;
+
+ context = kmalloc(size, GFP_KERNEL);
+ if (context == NULL)
+ return -ENOMEM;
+
+ lctx = (struct lsm_ctx *)context;
+ if (copy_from_user(context, ctx, size))
+ rc = -EFAULT;
+ else if (lctx->ctx_len > size)
+ rc = -EINVAL;
+ else
+ rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
+
+ kfree(context);
+ if (rc > 0)
+ return 0;
+ return rc;
+}
+
+static int apparmor_setprocattr(const char *name, void *value,
+ size_t size)
+{
+ int attr = lsm_name_to_attr(name);
+
+ if (attr)
+ return do_setattr(attr, value, size);
+ return -EINVAL;
+}
+
/**
* apparmor_bprm_committing_creds - do task cleanup on committing new creds
* @bprm: binprm for the exec (NOT NULL)
@@ -1253,6 +1335,8 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(file_lock, apparmor_file_lock),
LSM_HOOK_INIT(file_truncate, apparmor_file_truncate),
+ LSM_HOOK_INIT(getselfattr, apparmor_getselfattr),
+ LSM_HOOK_INIT(setselfattr, apparmor_setselfattr),
LSM_HOOK_INIT(getprocattr, apparmor_getprocattr),
LSM_HOOK_INIT(setprocattr, apparmor_setprocattr),
diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index 197d41f9c32b..196f319aa3b2 100644
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -20,6 +20,7 @@
* aa_getprocattr - Return the label information for @label
* @label: the label to print label info about (NOT NULL)
* @string: Returns - string containing the label info (NOT NULL)
+ * @newline: indicates that a newline should be added
*
* Requires: label != NULL && string != NULL
*
@@ -27,7 +28,7 @@
*
* Returns: size of string placed in @string else error code on failure
*/
-int aa_getprocattr(struct aa_label *label, char **string)
+int aa_getprocattr(struct aa_label *label, char **string, bool newline)
{
struct aa_ns *ns = labels_ns(label);
struct aa_ns *current_ns = aa_get_current_ns();
@@ -57,10 +58,14 @@ int aa_getprocattr(struct aa_label *label, char **string)
return len;
}
- (*string)[len] = '\n';
- (*string)[len + 1] = 0;
+ if (newline)
+ (*string)[len++] = '\n';
+ (*string)[len] = 0;
aa_put_ns(current_ns);
+
+ if (newline)
+ return len;
return len + 1;
}
--
2.39.2
Add hooks for setselfattr and getselfattr. These hooks are not very
different from their setprocattr and getprocattr equivalents, and
much of the code is shared.
Signed-off-by: Casey Schaufler <[email protected]>
Cc: [email protected]
Cc: Paul Moore <[email protected]>
---
security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
1 file changed, 117 insertions(+), 30 deletions(-)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9403aee75981..8896edf80aa9 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
inode_doinit_with_dentry(inode, dentry);
}
-static int selinux_getprocattr(struct task_struct *p,
- const char *name, char **value)
+static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
{
const struct task_security_struct *__tsec;
u32 sid;
@@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
goto bad;
}
- if (!strcmp(name, "current"))
+ switch (attr) {
+ case LSM_ATTR_CURRENT:
sid = __tsec->sid;
- else if (!strcmp(name, "prev"))
+ break;
+ case LSM_ATTR_PREV:
sid = __tsec->osid;
- else if (!strcmp(name, "exec"))
+ break;
+ case LSM_ATTR_EXEC:
sid = __tsec->exec_sid;
- else if (!strcmp(name, "fscreate"))
+ break;
+ case LSM_ATTR_FSCREATE:
sid = __tsec->create_sid;
- else if (!strcmp(name, "keycreate"))
+ break;
+ case LSM_ATTR_KEYCREATE:
sid = __tsec->keycreate_sid;
- else if (!strcmp(name, "sockcreate"))
+ break;
+ case LSM_ATTR_SOCKCREATE:
sid = __tsec->sockcreate_sid;
- else {
- error = -EINVAL;
+ break;
+ default:
+ error = -EOPNOTSUPP;
goto bad;
}
rcu_read_unlock();
@@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
return error;
}
-static int selinux_setprocattr(const char *name, void *value, size_t size)
+static int do_setattr(u64 attr, void *value, size_t size)
{
struct task_security_struct *tsec;
struct cred *new;
@@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
/*
* Basic control over ability to set these attributes at all.
*/
- if (!strcmp(name, "exec"))
+ switch (attr) {
+ case LSM_ATTR_CURRENT:
+ error = avc_has_perm(&selinux_state,
+ mysid, mysid, SECCLASS_PROCESS,
+ PROCESS__SETCURRENT, NULL);
+ break;
+ case LSM_ATTR_EXEC:
error = avc_has_perm(&selinux_state,
mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETEXEC, NULL);
- else if (!strcmp(name, "fscreate"))
+ break;
+ case LSM_ATTR_FSCREATE:
error = avc_has_perm(&selinux_state,
mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETFSCREATE, NULL);
- else if (!strcmp(name, "keycreate"))
+ break;
+ case LSM_ATTR_KEYCREATE:
error = avc_has_perm(&selinux_state,
mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETKEYCREATE, NULL);
- else if (!strcmp(name, "sockcreate"))
+ break;
+ case LSM_ATTR_SOCKCREATE:
error = avc_has_perm(&selinux_state,
mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETSOCKCREATE, NULL);
- else if (!strcmp(name, "current"))
- error = avc_has_perm(&selinux_state,
- mysid, mysid, SECCLASS_PROCESS,
- PROCESS__SETCURRENT, NULL);
- else
- error = -EINVAL;
+ break;
+ default:
+ error = -EOPNOTSUPP;
+ break;
+ }
if (error)
return error;
@@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
}
error = security_context_to_sid(&selinux_state, value, size,
&sid, GFP_KERNEL);
- if (error == -EINVAL && !strcmp(name, "fscreate")) {
+ if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
if (!has_cap_mac_admin(true)) {
struct audit_buffer *ab;
size_t audit_size;
- /* We strip a nul only if it is at the end, otherwise the
- * context contains a nul and we should audit that */
+ /* We strip a nul only if it is at the end,
+ * otherwise the context contains a nul and
+ * we should audit that */
if (str[size - 1] == '\0')
audit_size = size - 1;
else
@@ -6459,7 +6474,8 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
if (!ab)
return error;
audit_log_format(ab, "op=fscreate invalid_context=");
- audit_log_n_untrustedstring(ab, value, audit_size);
+ audit_log_n_untrustedstring(ab, value,
+ audit_size);
audit_log_end(ab);
return error;
@@ -6483,11 +6499,11 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
checks and may_create for the file creation checks. The
operation will then fail if the context is not permitted. */
tsec = selinux_cred(new);
- if (!strcmp(name, "exec")) {
+ if (attr == LSM_ATTR_EXEC) {
tsec->exec_sid = sid;
- } else if (!strcmp(name, "fscreate")) {
+ } else if (attr == LSM_ATTR_FSCREATE) {
tsec->create_sid = sid;
- } else if (!strcmp(name, "keycreate")) {
+ } else if (attr == LSM_ATTR_KEYCREATE) {
if (sid) {
error = avc_has_perm(&selinux_state, mysid, sid,
SECCLASS_KEY, KEY__CREATE, NULL);
@@ -6495,9 +6511,9 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
goto abort_change;
}
tsec->keycreate_sid = sid;
- } else if (!strcmp(name, "sockcreate")) {
+ } else if (attr == LSM_ATTR_SOCKCREATE) {
tsec->sockcreate_sid = sid;
- } else if (!strcmp(name, "current")) {
+ } else if (attr == LSM_ATTR_CURRENT) {
error = -EINVAL;
if (sid == 0)
goto abort_change;
@@ -6542,6 +6558,75 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
return error;
}
+static int selinux_getselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t *size,
+ u32 __user flags)
+{
+ char *value;
+ size_t total_len;
+ int len;
+ int rc = 0;
+
+ len = do_getattr(attr, current, &value);
+ if (len < 0)
+ return len;
+
+ total_len = len + sizeof(*ctx);
+
+ if (total_len > *size)
+ rc = -E2BIG;
+ else
+ lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
+
+ *size = total_len;
+ return rc;
+}
+
+static int selinux_setselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t __user size,
+ u32 __user flags)
+{
+ struct lsm_ctx *lctx;
+ void *context;
+ int rc;
+
+ context = kmalloc(size, GFP_KERNEL);
+ if (context == NULL)
+ return -ENOMEM;
+
+ lctx = (struct lsm_ctx *)context;
+ if (copy_from_user(context, ctx, size))
+ rc = -EFAULT;
+ else if (lctx->ctx_len > size)
+ rc = -EINVAL;
+ else
+ rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
+
+ kfree(context);
+ if (rc > 0)
+ return 0;
+ return rc;
+}
+
+static int selinux_getprocattr(struct task_struct *p,
+ const char *name, char **value)
+{
+ unsigned int attr = lsm_name_to_attr(name);
+
+ if (attr)
+ return do_getattr(attr, p, value);
+ return -EINVAL;
+}
+
+static int selinux_setprocattr(const char *name, void *value, size_t size)
+{
+ int attr = lsm_name_to_attr(name);
+
+ if (attr)
+ return do_setattr(attr, value, size);
+ return -EINVAL;
+}
+
static int selinux_ismaclabel(const char *name)
{
return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
@@ -7183,6 +7268,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
LSM_HOOK_INIT(d_instantiate, selinux_d_instantiate),
+ LSM_HOOK_INIT(getselfattr, selinux_getselfattr),
+ LSM_HOOK_INIT(setselfattr, selinux_setselfattr),
LSM_HOOK_INIT(getprocattr, selinux_getprocattr),
LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
--
2.39.2
Add selftests for the three system calls supporting the LSM
infrastructure.
Signed-off-by: Casey Schaufler <[email protected]>
---
tools/testing/selftests/Makefile | 1 +
tools/testing/selftests/lsm/Makefile | 12 +
tools/testing/selftests/lsm/config | 2 +
.../selftests/lsm/lsm_get_self_attr_test.c | 268 ++++++++++++++++++
.../selftests/lsm/lsm_list_modules_test.c | 149 ++++++++++
.../selftests/lsm/lsm_set_self_attr_test.c | 70 +++++
6 files changed, 502 insertions(+)
create mode 100644 tools/testing/selftests/lsm/Makefile
create mode 100644 tools/testing/selftests/lsm/config
create mode 100644 tools/testing/selftests/lsm/lsm_get_self_attr_test.c
create mode 100644 tools/testing/selftests/lsm/lsm_list_modules_test.c
create mode 100644 tools/testing/selftests/lsm/lsm_set_self_attr_test.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 13a6837a0c6b..b18d133a1141 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -38,6 +38,7 @@ TARGETS += landlock
TARGETS += lib
TARGETS += livepatch
TARGETS += lkdtm
+TARGETS += lsm
TARGETS += membarrier
TARGETS += memfd
TARGETS += memory-hotplug
diff --git a/tools/testing/selftests/lsm/Makefile b/tools/testing/selftests/lsm/Makefile
new file mode 100644
index 000000000000..f39a75212b78
--- /dev/null
+++ b/tools/testing/selftests/lsm/Makefile
@@ -0,0 +1,12 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# First run: make -C ../../../.. headers_install
+
+CFLAGS += -Wall -O2 $(KHDR_INCLUDES)
+
+TEST_GEN_PROGS := lsm_get_self_attr_test lsm_list_modules_test \
+ lsm_set_self_attr_test
+
+include ../lib.mk
+
+$(TEST_GEN_PROGS):
diff --git a/tools/testing/selftests/lsm/config b/tools/testing/selftests/lsm/config
new file mode 100644
index 000000000000..afb887715f64
--- /dev/null
+++ b/tools/testing/selftests/lsm/config
@@ -0,0 +1,2 @@
+CONFIG_SYSFS=y
+CONFIG_SECURITY=y
diff --git a/tools/testing/selftests/lsm/lsm_get_self_attr_test.c b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
new file mode 100644
index 000000000000..2c61a1411c54
--- /dev/null
+++ b/tools/testing/selftests/lsm/lsm_get_self_attr_test.c
@@ -0,0 +1,268 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Linux Security Module infrastructure tests
+ * Tests for the lsm_get_self_attr system call
+ *
+ * Copyright © 2022 Casey Schaufler <[email protected]>
+ * Copyright © 2022 Intel Corporation
+ */
+
+#define _GNU_SOURCE
+#include <linux/lsm.h>
+#include <fcntl.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include "../kselftest_harness.h"
+
+#define PROCATTR "/proc/self/attr/"
+
+static int read_proc_attr(const char *attr, char *value, __kernel_size_t size)
+{
+ int fd;
+ int len;
+ char *path;
+
+ len = strlen(PROCATTR) + strlen(attr) + 1;
+ path = calloc(len, 1);
+ if (path == NULL)
+ return -1;
+ sprintf(path, "%s%s", PROCATTR, attr);
+
+ fd = open(path, O_RDONLY);
+ free(path);
+
+ if (fd < 0)
+ return -1;
+ len = read(fd, value, size);
+ if (len <= 0)
+ return -1;
+fprintf(stderr, "len=%d\n", len);
+ close(fd);
+
+ path = strchr(value, '\n');
+ if (path)
+ *path = '\0';
+
+ return 0;
+}
+
+static struct lsm_ctx *next_ctx(struct lsm_ctx *ctxp)
+{
+ void *vp;
+
+ vp = (void *)ctxp + sizeof(*ctxp) + ctxp->ctx_len;
+ return (struct lsm_ctx *)vp;
+}
+
+TEST(size_null_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_EQ(-1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ NULL, 0));
+ ASSERT_EQ(EINVAL, errno);
+
+ free(ctx);
+}
+
+TEST(ctx_null_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(-1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, NULL,
+ &size, 0));
+ ASSERT_NE(1, size);
+}
+
+TEST(size_too_small_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+ __kernel_size_t size = 1;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_EQ(-1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ &size, 0));
+ ASSERT_EQ(E2BIG, errno);
+ ASSERT_NE(1, size);
+
+ free(ctx);
+}
+
+TEST(flags_zero_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_EQ(-1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ &size, 1));
+ ASSERT_EQ(EINVAL, errno);
+ ASSERT_EQ(page_size, size);
+
+ free(ctx);
+}
+
+TEST(flags_overset_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_EQ(-1, syscall(__NR_lsm_get_self_attr,
+ LSM_ATTR_CURRENT | LSM_ATTR_PREV, ctx, &size, 0));
+ ASSERT_EQ(EOPNOTSUPP, errno);
+ ASSERT_EQ(page_size, size);
+
+ free(ctx);
+}
+
+TEST(basic_lsm_get_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ __kernel_size_t size = page_size;
+ struct lsm_ctx *ctx = calloc(page_size, 1);
+ struct lsm_ctx *tctx = NULL;
+ __u64 *syscall_lsms = calloc(page_size, 1);
+ char *attr = calloc(page_size, 1);
+ int cnt_current = 0;
+ int cnt_exec = 0;
+ int cnt_fscreate = 0;
+ int cnt_keycreate = 0;
+ int cnt_prev = 0;
+ int cnt_sockcreate = 0;
+ int lsmcount;
+ int count;
+ int i;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_NE(NULL, syscall_lsms);
+
+ lsmcount = syscall(__NR_lsm_list_modules, syscall_lsms, &size, 0);
+ ASSERT_LE(1, lsmcount);
+
+ for (i = 0; i < lsmcount; i++) {
+ switch (syscall_lsms[i]) {
+ case LSM_ID_SELINUX:
+ cnt_current++;
+ cnt_exec++;
+ cnt_fscreate++;
+ cnt_keycreate++;
+ cnt_prev++;
+ cnt_sockcreate++;
+ break;
+ case LSM_ID_SMACK:
+ cnt_current++;
+ break;
+ case LSM_ID_APPARMOR:
+ cnt_current++;
+ cnt_exec++;
+ cnt_prev++;
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (cnt_current) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ &size, 0);
+ ASSERT_EQ(cnt_current, count);
+ tctx = ctx;
+ ASSERT_EQ(0, read_proc_attr("current", attr, page_size));
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+ if (cnt_exec) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_EXEC, ctx,
+ &size, 0);
+ ASSERT_GE(cnt_exec, count);
+ if (count > 0) {
+ tctx = ctx;
+ if (read_proc_attr("exec", attr, page_size) == 0)
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ }
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+ if (cnt_fscreate) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_FSCREATE, ctx,
+ &size, 0);
+ ASSERT_GE(cnt_fscreate, count);
+ if (count > 0) {
+ tctx = ctx;
+ if (read_proc_attr("fscreate", attr, page_size) == 0)
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ }
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+ if (cnt_keycreate) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_KEYCREATE, ctx,
+ &size, 0);
+ ASSERT_GE(cnt_keycreate, count);
+ if (count > 0) {
+ tctx = ctx;
+ if (read_proc_attr("keycreate", attr, page_size) == 0)
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ }
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+ if (cnt_prev) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_PREV, ctx,
+ &size, 0);
+ ASSERT_GE(cnt_prev, count);
+ if (count > 0) {
+ tctx = ctx;
+ ASSERT_EQ(0, read_proc_attr("prev", attr, page_size));
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+ }
+ if (cnt_sockcreate) {
+ size = page_size;
+ count = syscall(__NR_lsm_get_self_attr, LSM_ATTR_SOCKCREATE,
+ ctx, &size, 0);
+ ASSERT_GE(cnt_sockcreate, count);
+ if (count > 0) {
+ tctx = ctx;
+ if (read_proc_attr("sockcreate", attr, page_size) == 0)
+ ASSERT_EQ(0, strcmp((char *)tctx->ctx, attr));
+ }
+ for (i = 1; i < count; i++) {
+ tctx = next_ctx(tctx);
+ ASSERT_NE(0, strcmp((char *)tctx->ctx, attr));
+ }
+ }
+
+ free(ctx);
+ free(attr);
+ free(syscall_lsms);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/lsm/lsm_list_modules_test.c b/tools/testing/selftests/lsm/lsm_list_modules_test.c
new file mode 100644
index 000000000000..3ec814002710
--- /dev/null
+++ b/tools/testing/selftests/lsm/lsm_list_modules_test.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Linux Security Module infrastructure tests
+ * Tests for the lsm_list_modules system call
+ *
+ * Copyright © 2022 Casey Schaufler <[email protected]>
+ * Copyright © 2022 Intel Corporation
+ */
+
+#define _GNU_SOURCE
+#include <linux/lsm.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include "../kselftest_harness.h"
+
+static int read_sysfs_lsms(char *lsms, __kernel_size_t size)
+{
+ FILE *fp;
+
+ fp = fopen("/sys/kernel/security/lsm", "r");
+ if (fp == NULL)
+ return -1;
+ if (fread(lsms, 1, size, fp) <= 0)
+ return -1;
+ fclose(fp);
+ return 0;
+}
+
+TEST(size_null_lsm_list_modules)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *syscall_lsms = calloc(page_size, 1);
+
+ ASSERT_NE(NULL, syscall_lsms);
+ ASSERT_EQ(-1, syscall(__NR_lsm_list_modules, syscall_lsms, NULL, 0));
+ ASSERT_EQ(EFAULT, errno);
+
+ free(syscall_lsms);
+}
+
+TEST(ids_null_lsm_list_modules)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ __kernel_size_t size = page_size;
+
+ ASSERT_EQ(-1, syscall(__NR_lsm_list_modules, NULL, &size, 0));
+ ASSERT_EQ(EFAULT, errno);
+ ASSERT_NE(1, size);
+}
+
+TEST(size_too_small_lsm_list_modules)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *syscall_lsms = calloc(page_size, 1);
+ __kernel_size_t size = 1;
+
+ ASSERT_NE(NULL, syscall_lsms);
+ ASSERT_EQ(-1, syscall(__NR_lsm_list_modules, syscall_lsms, &size, 0));
+ ASSERT_EQ(E2BIG, errno);
+ ASSERT_NE(1, size);
+
+ free(syscall_lsms);
+}
+
+TEST(flags_set_lsm_list_modules)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *syscall_lsms = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(NULL, syscall_lsms);
+ ASSERT_EQ(-1, syscall(__NR_lsm_list_modules, syscall_lsms, &size, 7));
+ ASSERT_EQ(EINVAL, errno);
+ ASSERT_EQ(page_size, size);
+
+ free(syscall_lsms);
+}
+
+TEST(correct_lsm_list_modules)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ __kernel_size_t size = page_size;
+ __u64 *syscall_lsms = calloc(page_size, 1);
+ char *sysfs_lsms = calloc(page_size, 1);
+ char *name;
+ char *cp;
+ int count;
+ int i;
+
+ ASSERT_NE(NULL, sysfs_lsms);
+ ASSERT_NE(NULL, syscall_lsms);
+ ASSERT_EQ(0, read_sysfs_lsms(sysfs_lsms, page_size));
+
+ count = syscall(__NR_lsm_list_modules, syscall_lsms, &size, 0);
+ ASSERT_LE(1, count);
+ cp = sysfs_lsms;
+ for (i = 0; i < count; i++) {
+ switch (syscall_lsms[i]) {
+ case LSM_ID_CAPABILITY:
+ name = "capability";
+ break;
+ case LSM_ID_SELINUX:
+ name = "selinux";
+ break;
+ case LSM_ID_SMACK:
+ name = "smack";
+ break;
+ case LSM_ID_TOMOYO:
+ name = "tomoyo";
+ break;
+ case LSM_ID_IMA:
+ name = "ima";
+ break;
+ case LSM_ID_APPARMOR:
+ name = "apparmor";
+ break;
+ case LSM_ID_YAMA:
+ name = "yama";
+ break;
+ case LSM_ID_LOADPIN:
+ name = "loadpin";
+ break;
+ case LSM_ID_SAFESETID:
+ name = "safesetid";
+ break;
+ case LSM_ID_LOCKDOWN:
+ name = "lockdown";
+ break;
+ case LSM_ID_BPF:
+ name = "bpf";
+ break;
+ case LSM_ID_LANDLOCK:
+ name = "landlock";
+ break;
+ default:
+ name = "INVALID";
+ break;
+ }
+ ASSERT_EQ(0, strncmp(cp, name, strlen(name)));
+ cp += strlen(name) + 1;
+ }
+
+ free(sysfs_lsms);
+ free(syscall_lsms);
+}
+
+TEST_HARNESS_MAIN
diff --git a/tools/testing/selftests/lsm/lsm_set_self_attr_test.c b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
new file mode 100644
index 000000000000..ca538a703168
--- /dev/null
+++ b/tools/testing/selftests/lsm/lsm_set_self_attr_test.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Linux Security Module infrastructure tests
+ * Tests for the lsm_set_self_attr system call
+ *
+ * Copyright © 2022 Casey Schaufler <[email protected]>
+ * Copyright © 2022 Intel Corporation
+ */
+
+#define _GNU_SOURCE
+#include <linux/lsm.h>
+#include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include "../kselftest_harness.h"
+
+TEST(ctx_null_lsm_set_self_attr)
+{
+ ASSERT_EQ(-1, syscall(__NR_lsm_set_self_attr, LSM_ATTR_CURRENT, NULL,
+ sizeof(struct lsm_ctx), 0));
+}
+
+TEST(size_too_small_lsm_set_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ struct lsm_ctx *ctx = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_GE(1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ &size, 0));
+ ASSERT_EQ(-1, syscall(__NR_lsm_set_self_attr, LSM_ATTR_CURRENT, ctx, 1,
+ 0));
+
+ free(ctx);
+}
+
+TEST(flags_zero_lsm_set_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_GE(1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, ctx,
+ &size, 0));
+ ASSERT_EQ(-1, syscall(__NR_lsm_set_self_attr, LSM_ATTR_CURRENT, ctx,
+ size, 1));
+
+ free(ctx);
+}
+
+TEST(flags_overset_lsm_set_self_attr)
+{
+ const long page_size = sysconf(_SC_PAGESIZE);
+ char *ctx = calloc(page_size, 1);
+ __kernel_size_t size = page_size;
+ struct lsm_ctx *tctx = (struct lsm_ctx *)ctx;
+
+ ASSERT_NE(NULL, ctx);
+ ASSERT_GE(1, syscall(__NR_lsm_get_self_attr, LSM_ATTR_CURRENT, tctx,
+ &size, 0));
+ ASSERT_EQ(-1, syscall(__NR_lsm_set_self_attr,
+ LSM_ATTR_CURRENT | LSM_ATTR_PREV, tctx, size, 0));
+
+ free(ctx);
+}
+
+TEST_HARNESS_MAIN
--
2.39.2
Hi Casey,
I love your patch! Yet something to improve:
[auto build test ERROR on shuah-kselftest/next]
[also build test ERROR on shuah-kselftest/fixes linus/master v6.3-rc2]
[cannot apply to tip/perf/core acme/perf/core next-20230316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Casey-Schaufler/LSM-Maintain-a-table-of-LSM-attribute-data/20230316-074751
base: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
patch link: https://lore.kernel.org/r/20230315224704.2672-5-casey%40schaufler-ca.com
patch subject: [PATCH v7 04/11] LSM: syscalls for current process attributes
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230316/[email protected]/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/0883a93af669a6fcb80a9cc74737d5285a1c46ae
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Casey-Schaufler/LSM-Maintain-a-table-of-LSM-attribute-data/20230316-074751
git checkout 0883a93af669a6fcb80a9cc74737d5285a1c46ae
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
In file included from security/lsm_syscalls.c:15:
>> include/linux/syscalls.h:243:25: error: conflicting types for 'sys_lsm_set_self_attr'; have 'long int(unsigned int, struct lsm_ctx *, size_t, u32)' {aka 'long int(unsigned int, struct lsm_ctx *, unsigned int, unsigned int)'}
243 | asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
| ^~~
include/linux/syscalls.h:229:9: note: in expansion of macro '__SYSCALL_DEFINEx'
229 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
| ^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:221:36: note: in expansion of macro 'SYSCALL_DEFINEx'
221 | #define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
security/lsm_syscalls.c:31:1: note: in expansion of macro 'SYSCALL_DEFINE4'
31 | SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
| ^~~~~~~~~~~~~~~
include/linux/syscalls.h:1064:17: note: previous declaration of 'sys_lsm_set_self_attr' with type 'long int(unsigned int, struct lsm_ctx *, __u64)' {aka 'long int(unsigned int, struct lsm_ctx *, long long unsigned int)'}
1064 | asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
| ^~~~~~~~~~~~~~~~~~~~~
>> include/linux/syscalls.h:243:25: error: conflicting types for 'sys_lsm_get_self_attr'; have 'long int(unsigned int, struct lsm_ctx *, size_t *, u32)' {aka 'long int(unsigned int, struct lsm_ctx *, unsigned int *, unsigned int)'}
243 | asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
| ^~~
include/linux/syscalls.h:229:9: note: in expansion of macro '__SYSCALL_DEFINEx'
229 | __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
| ^~~~~~~~~~~~~~~~~
include/linux/syscalls.h:221:36: note: in expansion of macro 'SYSCALL_DEFINEx'
221 | #define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
| ^~~~~~~~~~~~~~~
security/lsm_syscalls.c:51:1: note: in expansion of macro 'SYSCALL_DEFINE4'
51 | SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
| ^~~~~~~~~~~~~~~
include/linux/syscalls.h:1062:17: note: previous declaration of 'sys_lsm_get_self_attr' with type 'long int(unsigned int, struct lsm_ctx *, size_t *, __u64)' {aka 'long int(unsigned int, struct lsm_ctx *, unsigned int *, long long unsigned int)'}
1062 | asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
| ^~~~~~~~~~~~~~~~~~~~~
vim +243 include/linux/syscalls.h
1bd21c6c21e848 Dominik Brodowski 2018-04-05 232
e145242ea0df6b Dominik Brodowski 2018-04-09 233 /*
e145242ea0df6b Dominik Brodowski 2018-04-09 234 * The asmlinkage stub is aliased to a function named __se_sys_*() which
e145242ea0df6b Dominik Brodowski 2018-04-09 235 * sign-extends 32-bit ints to longs whenever needed. The actual work is
e145242ea0df6b Dominik Brodowski 2018-04-09 236 * done within __do_sys_*().
e145242ea0df6b Dominik Brodowski 2018-04-09 237 */
1bd21c6c21e848 Dominik Brodowski 2018-04-05 238 #ifndef __SYSCALL_DEFINEx
bed1ffca022cc8 Frederic Weisbecker 2009-03-13 239 #define __SYSCALL_DEFINEx(x, name, ...) \
bee20031772af3 Arnd Bergmann 2018-06-19 240 __diag_push(); \
bee20031772af3 Arnd Bergmann 2018-06-19 241 __diag_ignore(GCC, 8, "-Wattribute-alias", \
bee20031772af3 Arnd Bergmann 2018-06-19 242 "Type aliasing is used to sanitize syscall arguments");\
83460ec8dcac14 Andi Kleen 2013-11-12 @243 asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
e145242ea0df6b Dominik Brodowski 2018-04-09 244 __attribute__((alias(__stringify(__se_sys##name)))); \
c9a211951c7c79 Howard McLauchlan 2018-03-21 245 ALLOW_ERROR_INJECTION(sys##name, ERRNO); \
e145242ea0df6b Dominik Brodowski 2018-04-09 246 static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\
e145242ea0df6b Dominik Brodowski 2018-04-09 247 asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)); \
e145242ea0df6b Dominik Brodowski 2018-04-09 248 asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__)) \
1a94bc34768e46 Heiko Carstens 2009-01-14 249 { \
e145242ea0df6b Dominik Brodowski 2018-04-09 250 long ret = __do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\
07fe6e00f6cca6 Al Viro 2013-01-21 251 __MAP(x,__SC_TEST,__VA_ARGS__); \
2cf0966683430b Al Viro 2013-01-21 252 __PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__)); \
2cf0966683430b Al Viro 2013-01-21 253 return ret; \
1a94bc34768e46 Heiko Carstens 2009-01-14 254 } \
bee20031772af3 Arnd Bergmann 2018-06-19 255 __diag_pop(); \
e145242ea0df6b Dominik Brodowski 2018-04-09 256 static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
1bd21c6c21e848 Dominik Brodowski 2018-04-05 257 #endif /* __SYSCALL_DEFINEx */
1a94bc34768e46 Heiko Carstens 2009-01-14 258
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[email protected]> wrote:
>
> Create a system call lsm_get_self_attr() to provide the security
> module maintained attributes of the current process.
> Create a system call lsm_set_self_attr() to set a security
> module maintained attribute of the current process.
> Historically these attributes have been exposed to user space via
> entries in procfs under /proc/self/attr.
>
> The attribute value is provided in a lsm_ctx structure. The structure
> identifys the size of the attribute, and the attribute value. The format
"identifies"
> of the attribute value is defined by the security module. A flags field
> is included for LSM specific information. It is currently unused and must
> be 0. The total size of the data, including the lsm_ctx structure and any
> padding, is maintained as well.
>
> struct lsm_ctx {
> __u64 id;
> __u64 flags;
> __u64 len;
> __u64 ctx_len;
> __u8 ctx[];
> };
>
> Two new LSM hooks are used to interface with the LSMs.
> security_getselfattr() collects the lsm_ctx values from the
> LSMs that support the hook, accounting for space requirements.
> security_setselfattr() identifies which LSM the attribute is
> intended for and passes it along.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> Documentation/userspace-api/lsm.rst | 15 +++++
> include/linux/lsm_hook_defs.h | 4 ++
> include/linux/lsm_hooks.h | 9 +++
> include/linux/security.h | 19 ++++++
> include/linux/syscalls.h | 5 ++
> include/uapi/linux/lsm.h | 33 ++++++++++
> kernel/sys_ni.c | 4 ++
> security/Makefile | 1 +
> security/lsm_syscalls.c | 55 ++++++++++++++++
> security/security.c | 97 +++++++++++++++++++++++++++++
> 10 files changed, 242 insertions(+)
> create mode 100644 security/lsm_syscalls.c
...
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 32285ce65419..3c2c4916bd53 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -503,6 +504,14 @@
> * and writing the xattrs as this hook is merely a filter.
> * @d_instantiate:
> * Fill in @inode security information for a @dentry if allowed.
> + * @getselfattr:
> + * Read attribute @attr for the current process and store it into @ctx.
> + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
> + * or another negative value otherwise.
> + * @setselfattr:
> + * Set attribute @attr for the current process.
> + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
> + * or another negative value otherwise.
> * @getprocattr:
> * Read attribute @name for process @p and store it into @value if allowed.
> * Return the length of @value on success, a negative value otherwise.
I'm sure you're already aware of this, but the above will need to be
moved to security.c due to the changes in the lsm/next branch. That
said, if you're basing on Linus' tree that's fine too, I'll fix it up
during the merge; thankfully it's not a significant merge conflict.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8faed81fc3b4..329cd9d2be50 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry,
> struct inode *inode)
> { }
>
> +static inline int security_getselfattr(unsigned int __user attr,
> + struct lsm_ctx __user *ctx,
> + size_t __user *size, u32 __user flags)
> +{
> + return -EINVAL;
> +}
> +
> +static inline int security_setselfattr(unsigned int __user attr,
> + struct lsm_ctx __user *ctx,
> + size_t __user size, u32 __user flags)
> +{
> + return -EINVAL;
> +}
It seems like EOPNOTSUPP might be more appropriate than EINVAL for
both of these dummy implementations.
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 33a0ee3bcb2e..3feca00cb0c1 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
> asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
> unsigned long home_node,
> unsigned long flags);
> +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> + size_t *size, __u64 flags);
> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> + __u64 flags);
As the kernel test robot already pointed out, the above needs to be updated.
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index aa3e01867739..adfb55dce2fd 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -9,6 +9,39 @@
> #ifndef _UAPI_LINUX_LSM_H
> #define _UAPI_LINUX_LSM_H
>
> +#include <linux/types.h>
> +#include <linux/unistd.h>
> +
> +/**
> + * struct lsm_ctx - LSM context information
> + * @id: the LSM id number, see LSM_ID_XXX
> + * @flags: LSM specific flags
> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
> + * @ctx_len: the size of @ctx
> + * @ctx: the LSM context value
> + *
> + * The @len field MUST be equal to the size of the lsm_ctx struct
> + * plus any additional padding and/or data placed after @ctx.
> + *
> + * In all cases @ctx_len MUST be equal to the length of @ctx.
> + * If @ctx is a string value it should be nul terminated with
> + * @ctx_len equal to `strlen(@ctx) + 1`. Binary values are
> + * supported.
> + *
> + * The @flags and @ctx fields SHOULD only be interpreted by the
> + * LSM specified by @id; they MUST be set to zero/0 when not used.
> + */
> +struct lsm_ctx {
> + __u64 id;
> + __u64 flags;
> + __u64 len;
> + __u64 ctx_len;
> + __u8 ctx[];
> +};
> +
> +#include <linux/types.h>
> +#include <linux/unistd.h>
I'm pretty sure the repeated #includes are a typo, right? Or is there
some uapi trick I'm missing ...
> diff --git a/security/security.c b/security/security.c
> index 87c8796c3c46..2c57fe28c4f7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
> }
> EXPORT_SYMBOL(security_d_instantiate);
>
> +/**
> + * security_getselfattr - Read an LSM attribute of the current process.
> + * @attr: which attribute to return
> + * @ctx: the user-space destination for the information, or NULL
> + * @size: the size of space available to receive the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Returns the number of attributes found on success, negative value
> + * on error. @size is reset to the total size of the data.
> + * If @size is insufficient to contain the data -E2BIG is returned.
> + */
> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> + size_t __user *size, u32 __user flags)
> +{
> + struct security_hook_list *hp;
> + void __user *base = (void *)ctx;
The casting seems wrong for a couple of reasons: I don't believe you
need to cast the right side when the left side is a void pointer, and
the right side cast drops the '__user' attribute when the left side is
also a '__user' pointer value.
That said, I think we may want @base to be 'u8 __user *base', more on
that below ...
> + size_t total = 0;
> + size_t this;
Naming is hard, but 'this'? You can do better ...
> + size_t left;
> + bool istoobig = false;
Sorry, more naming nits and since it looks like you need to respin
anyway ... please rename @istoobig to @toobig or something else. The
phrases-as-variable-names has always grated on me.
> + int count = 0;
> + int rc;
> +
> + if (attr == 0)
> + return -EINVAL;
> + if (flags != 0)
> + return -EINVAL;
> + if (size == NULL)
> + return -EINVAL;
> + if (get_user(left, size))
> + return -EFAULT;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> + this = left;
> + if (base)
> + ctx = (struct lsm_ctx __user *)(base + total);
Pointer math on void pointers always makes me nervous. Why not set
@base's type to a 'u8' just to remove any concerns?
> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> + switch (rc) {
> + case -EOPNOTSUPP:
> + rc = 0;
> + continue;
> + case -E2BIG:
> + istoobig = true;
> + left = 0;
> + break;
> + case 0:
> + left -= this;
> + break;
> + default:
> + return rc;
I think the @getselfattr hook should behave similarly to the
associated syscall, returning a non-negative number should indicate
that @rc entries have been added to the @ctx array. Right now all the
LSMs would just be adding one entry to the array, but we might as well
code this up to be flexible.
> + }
> + total += this;
> + count++;
> + }
> + if (count == 0)
> + return LSM_RET_DEFAULT(getselfattr);
> + if (put_user(total, size))
> + return -EFAULT;
> + if (rc)
> + return rc;
Is the 'if (rc)' check needed here? Shouldn't the switch-statement
after the hook catch everything that this check would catch?
> + if (istoobig)
> + return -E2BIG;
> + return count;
> +}
> +
> +/**
> + * security_setselfattr - Set an LSM attribute on the current process.
> + * @attr: which attribute to set
> + * @ctx: the user-space source for the information
> + * @size: the size of the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Set an LSM attribute for the current process. The LSM, attribute
> + * and new value are included in @ctx.
> + *
> + * Returns 0 on success, an LSM specific value on failure.
> + */
> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> + size_t __user size, u32 __user flags)
> +{
> + struct security_hook_list *hp;
> + struct lsm_ctx lctx;
Shouldn't we check @attr for valid values and return -EINVAL if bogus?
> + if (flags != 0)
> + return -EINVAL;
> + if (size < sizeof(*ctx))
> + return -EINVAL;
If we're only going to support on 'lsm_ctx' entry in this function we
should verify that the 'len' and 'ctx_len' fields are sane. Although
more on this below ...
> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> + return -EFAULT;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> + if ((hp->lsmid->id) == lctx.id)
> + return hp->hook.setselfattr(attr, ctx, size, flags);
Can anyone think of any good reason why we shouldn't support setting
multiple LSMs in one call, similar to what we do with
security_getselfattr()? It seems like it might be a nice thing to
have ...
> + return LSM_RET_DEFAULT(setselfattr);
> +}
> +
> int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
> char **value)
> {
> --
> 2.39.2
--
paul-moore.com
On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
>
> Add lsm_name_to_attr(), which translates a text string to a
> LSM_ATTR value if one is available.
>
> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
> the trailing attribute value.
>
> All are used in module specific components of LSM system calls.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> include/linux/security.h | 13 ++++++++++
> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
> security/security.c | 31 ++++++++++++++++++++++++
> 3 files changed, 95 insertions(+)
...
> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> index 6efbe244d304..55d849ad5d6e 100644
> --- a/security/lsm_syscalls.c
> +++ b/security/lsm_syscalls.c
> @@ -17,6 +17,57 @@
> #include <linux/lsm_hooks.h>
> #include <uapi/linux/lsm.h>
>
> +struct attr_map {
> + char *name;
> + u64 attr;
> +};
> +
> +static const struct attr_map lsm_attr_names[] = {
> + {
> + .name = "current",
> + .attr = LSM_ATTR_CURRENT,
> + },
> + {
> + .name = "exec",
> + .attr = LSM_ATTR_EXEC,
> + },
> + {
> + .name = "fscreate",
> + .attr = LSM_ATTR_FSCREATE,
> + },
> + {
> + .name = "keycreate",
> + .attr = LSM_ATTR_KEYCREATE,
> + },
> + {
> + .name = "prev",
> + .attr = LSM_ATTR_PREV,
> + },
> + {
> + .name = "sockcreate",
> + .attr = LSM_ATTR_SOCKCREATE,
> + },
> +};
> +
> +/**
> + * lsm_name_to_attr - map an LSM attribute name to its ID
> + * @name: name of the attribute
> + *
> + * Look the given @name up in the table of know attribute names.
> + *
> + * Returns the LSM attribute value associated with @name, or 0 if
> + * there is no mapping.
> + */
> +u64 lsm_name_to_attr(const char *name)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
> + if (!strcmp(name, lsm_attr_names[i].name))
> + return lsm_attr_names[i].attr;
I'm pretty sure this is the only place where @lsm_attr_names is used,
right? If true, when coupled with the idea that these syscalls are
going to close the door on new LSM attributes in procfs I think we can
just put the mapping directly in this function via a series of
if-statements.
> + return 0;
> +}
> +
> /**
> * sys_lsm_set_self_attr - Set current task's security module attribute
> * @attr: which attribute to set
> diff --git a/security/security.c b/security/security.c
> index 2c57fe28c4f7..f7b814a3940c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
> return 0;
> }
>
> +/**
> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
> + * @ctx: an LSM context to be filled
> + * @context: the new context value
> + * @context_size: the size of the new context value
> + * @id: LSM id
> + * @flags: LSM defined flags
> + *
> + * Fill all of the fields in a user space lsm_ctx structure.
> + * Caller is assumed to have verified that @ctx has enough space
> + * for @context.
> + * Returns 0 on success, -EFAULT on a copyout error.
> + */
> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
> + size_t context_size, u64 id, u64 flags)
> +{
> + struct lsm_ctx local;
> + void __user *vc = ctx;
> +
> + local.id = id;
> + local.flags = flags;
> + local.ctx_len = context_size;
> + local.len = context_size + sizeof(local);
> + vc += sizeof(local);
See my prior comments about void pointer math.
> + if (copy_to_user(ctx, &local, sizeof(local)))
> + return -EFAULT;
> + if (context_size > 0 && copy_to_user(vc, context, context_size))
> + return -EFAULT;
Should we handle the padding in this function?
> + return 0;
> +}
--
paul-moore.com
On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[email protected]> 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
> output format, determining required privilege or bypassing
> security module specific behavior.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> Documentation/userspace-api/lsm.rst | 3 +++
> include/linux/syscalls.h | 1 +
> kernel/sys_ni.c | 1 +
> security/lsm_syscalls.c | 39 +++++++++++++++++++++++++++++
> 4 files changed, 44 insertions(+)
...
> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> index feee31600219..6efbe244d304 100644
> --- a/security/lsm_syscalls.c
> +++ b/security/lsm_syscalls.c
> @@ -53,3 +53,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> {
> return security_getselfattr(attr, ctx, size, flags);
> }
> +
> +/**
> + * sys_lsm_list_modules - 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_list_modules, u64 __user *, ids, size_t __user *, size,
> + u32, flags)
> +{
> + size_t total_size = lsm_active_cnt * sizeof(*ids);
> + size_t usize;
> + int i;
> +
> + if (flags)
> + return -EINVAL;
In other patches in this patchset you use 'if (flags != 0)'; I don't
care too much which approach you take, but please be consistent.
Actually, I guess you might as well just go with 'if (flags)' since
I'm pretty sure someone later down the line will end up wasting
reviewer time by changing '(flags != 0)' into '(flags)' ...
> + if (get_user(usize, size))
> + return -EFAULT;
> +
> + 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++))
> + return -EFAULT;
> +
> + return lsm_active_cnt;
> +}
> --
> 2.39.2
--
paul-moore.com
On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <[email protected]> wrote:
>
> Add hooks for setselfattr and getselfattr. These hooks are not very
> different from their setprocattr and getprocattr equivalents, and
> much of the code is shared.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> Cc: [email protected]
> Cc: Paul Moore <[email protected]>
> ---
> security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
> 1 file changed, 117 insertions(+), 30 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9403aee75981..8896edf80aa9 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
> inode_doinit_with_dentry(inode, dentry);
> }
>
> -static int selinux_getprocattr(struct task_struct *p,
> - const char *name, char **value)
> +static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
Are you ready for more naming nitpicks? ;)
Let's call this 'selinux_lsm_getattr()', and the matching setter
should be 'selinux_lsm_setattr()'.
> {
> const struct task_security_struct *__tsec;
> u32 sid;
> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
> goto bad;
> }
>
> - if (!strcmp(name, "current"))
> + switch (attr) {
> + case LSM_ATTR_CURRENT:
> sid = __tsec->sid;
> - else if (!strcmp(name, "prev"))
> + break;
> + case LSM_ATTR_PREV:
> sid = __tsec->osid;
> - else if (!strcmp(name, "exec"))
> + break;
> + case LSM_ATTR_EXEC:
> sid = __tsec->exec_sid;
> - else if (!strcmp(name, "fscreate"))
> + break;
> + case LSM_ATTR_FSCREATE:
> sid = __tsec->create_sid;
> - else if (!strcmp(name, "keycreate"))
> + break;
> + case LSM_ATTR_KEYCREATE:
> sid = __tsec->keycreate_sid;
> - else if (!strcmp(name, "sockcreate"))
> + break;
> + case LSM_ATTR_SOCKCREATE:
> sid = __tsec->sockcreate_sid;
> - else {
> - error = -EINVAL;
> + break;
> + default:
> + error = -EOPNOTSUPP;
The error should probably be -EINVAL.
> goto bad;
> }
> rcu_read_unlock();
> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
> return error;
> }
>
> -static int selinux_setprocattr(const char *name, void *value, size_t size)
> +static int do_setattr(u64 attr, void *value, size_t size)
> {
> struct task_security_struct *tsec;
> struct cred *new;
> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> /*
> * Basic control over ability to set these attributes at all.
> */
> - if (!strcmp(name, "exec"))
> + switch (attr) {
> + case LSM_ATTR_CURRENT:
> + error = avc_has_perm(&selinux_state,
> + mysid, mysid, SECCLASS_PROCESS,
> + PROCESS__SETCURRENT, NULL);
> + break;
> + case LSM_ATTR_EXEC:
> error = avc_has_perm(&selinux_state,
> mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETEXEC, NULL);
> - else if (!strcmp(name, "fscreate"))
> + break;
> + case LSM_ATTR_FSCREATE:
> error = avc_has_perm(&selinux_state,
> mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETFSCREATE, NULL);
> - else if (!strcmp(name, "keycreate"))
> + break;
> + case LSM_ATTR_KEYCREATE:
> error = avc_has_perm(&selinux_state,
> mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETKEYCREATE, NULL);
> - else if (!strcmp(name, "sockcreate"))
> + break;
> + case LSM_ATTR_SOCKCREATE:
> error = avc_has_perm(&selinux_state,
> mysid, mysid, SECCLASS_PROCESS,
> PROCESS__SETSOCKCREATE, NULL);
> - else if (!strcmp(name, "current"))
> - error = avc_has_perm(&selinux_state,
> - mysid, mysid, SECCLASS_PROCESS,
> - PROCESS__SETCURRENT, NULL);
> - else
> - error = -EINVAL;
> + break;
> + default:
> + error = -EOPNOTSUPP;
Same as above, should be -EINVAL.
> + break;
> + }
> if (error)
> return error;
>
> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> }
> error = security_context_to_sid(&selinux_state, value, size,
> &sid, GFP_KERNEL);
> - if (error == -EINVAL && !strcmp(name, "fscreate")) {
> + if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
> if (!has_cap_mac_admin(true)) {
> struct audit_buffer *ab;
> size_t audit_size;
>
> - /* We strip a nul only if it is at the end, otherwise the
> - * context contains a nul and we should audit that */
> + /* We strip a nul only if it is at the end,
> + * otherwise the context contains a nul and
> + * we should audit that */
You *do* get gold stars for fixing line lengths in close proximity ;)
> if (str[size - 1] == '\0')
> audit_size = size - 1;
> else
> @@ -6459,7 +6474,8 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> if (!ab)
> return error;
> audit_log_format(ab, "op=fscreate invalid_context=");
> - audit_log_n_untrustedstring(ab, value, audit_size);
> + audit_log_n_untrustedstring(ab, value,
> + audit_size);
> audit_log_end(ab);
>
> return error;
> @@ -6483,11 +6499,11 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> checks and may_create for the file creation checks. The
> operation will then fail if the context is not permitted. */
> tsec = selinux_cred(new);
> - if (!strcmp(name, "exec")) {
> + if (attr == LSM_ATTR_EXEC) {
> tsec->exec_sid = sid;
> - } else if (!strcmp(name, "fscreate")) {
> + } else if (attr == LSM_ATTR_FSCREATE) {
> tsec->create_sid = sid;
> - } else if (!strcmp(name, "keycreate")) {
> + } else if (attr == LSM_ATTR_KEYCREATE) {
> if (sid) {
> error = avc_has_perm(&selinux_state, mysid, sid,
> SECCLASS_KEY, KEY__CREATE, NULL);
> @@ -6495,9 +6511,9 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> goto abort_change;
> }
> tsec->keycreate_sid = sid;
> - } else if (!strcmp(name, "sockcreate")) {
> + } else if (attr == LSM_ATTR_SOCKCREATE) {
> tsec->sockcreate_sid = sid;
> - } else if (!strcmp(name, "current")) {
> + } else if (attr == LSM_ATTR_CURRENT) {
> error = -EINVAL;
> if (sid == 0)
> goto abort_change;
> @@ -6542,6 +6558,75 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> return error;
> }
>
> +static int selinux_getselfattr(unsigned int __user attr,
> + struct lsm_ctx __user *ctx, size_t *size,
> + u32 __user flags)
> +{
> + char *value;
> + size_t total_len;
> + int len;
> + int rc = 0;
> +
> + len = do_getattr(attr, current, &value);
> + if (len < 0)
> + return len;
> +
> + total_len = len + sizeof(*ctx);
> +
> + if (total_len > *size)
> + rc = -E2BIG;
> + else
> + lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
> +
> + *size = total_len;
> + return rc;
> +}
> +
> +static int selinux_setselfattr(unsigned int __user attr,
> + struct lsm_ctx __user *ctx, size_t __user size,
> + u32 __user flags)
> +{
> + struct lsm_ctx *lctx;
> + void *context;
> + int rc;
> +
> + context = kmalloc(size, GFP_KERNEL);
> + if (context == NULL)
> + return -ENOMEM;
> +
> + lctx = (struct lsm_ctx *)context;
> + if (copy_from_user(context, ctx, size))
> + rc = -EFAULT;
> + else if (lctx->ctx_len > size)
> + rc = -EINVAL;
> + else
> + rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
> +
> + kfree(context);
> + if (rc > 0)
> + return 0;
> + return rc;
> +}
> +
> +static int selinux_getprocattr(struct task_struct *p,
> + const char *name, char **value)
> +{
> + unsigned int attr = lsm_name_to_attr(name);
> +
> + if (attr)
> + return do_getattr(attr, p, value);
> + return -EINVAL;
> +}
> +
> +static int selinux_setprocattr(const char *name, void *value, size_t size)
> +{
> + int attr = lsm_name_to_attr(name);
> +
> + if (attr)
> + return do_setattr(attr, value, size);
> + return -EINVAL;
> +}
> +
> static int selinux_ismaclabel(const char *name)
> {
> return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
> @@ -7183,6 +7268,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>
> LSM_HOOK_INIT(d_instantiate, selinux_d_instantiate),
>
> + LSM_HOOK_INIT(getselfattr, selinux_getselfattr),
> + LSM_HOOK_INIT(setselfattr, selinux_setselfattr),
> LSM_HOOK_INIT(getprocattr, selinux_getprocattr),
> LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
>
> --
> 2.39.2
--
paul-moore.com
On March 29, 2023 9:12:19 PM Paul Moore <[email protected]> wrote:
> On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[email protected]> wrote:
...
>
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> + size_t __user size, u32 __user flags)
>> +{
>> + struct security_hook_list *hp;
>> + struct lsm_ctx lctx;
>
> Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
>> + if (flags != 0)
>> + return -EINVAL;
>> + if (size < sizeof(*ctx))
>> + return -EINVAL;
>
> If we're only going to support on 'lsm_ctx' entry in this function we
> should verify that the 'len' and 'ctx_len' fields are sane. Although
> more on this below ...
>
>> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> + if ((hp->lsmid->id) == lctx.id)
>> + return hp->hook.setselfattr(attr, ctx, size, flags);
>
> Can anyone think of any good reason why we shouldn't support setting
> multiple LSMs in one call, similar to what we do with
> security_getselfattr()? It seems like it might be a nice thing to
> have ...
Scratch that, I just reminded myself why attempting to set an attribute on
multiple LSMs in one operation would be a nightmare. Sorry about the confusion.
--
paul-moore.com
On 3/29/2023 6:12 PM, Paul Moore wrote:
> On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[email protected]> wrote:
>> Create a system call lsm_get_self_attr() to provide the security
>> module maintained attributes of the current process.
>> Create a system call lsm_set_self_attr() to set a security
>> module maintained attribute of the current process.
>> Historically these attributes have been exposed to user space via
>> entries in procfs under /proc/self/attr.
>>
>> The attribute value is provided in a lsm_ctx structure. The structure
>> identifys the size of the attribute, and the attribute value. The format
> "identifies"
>
>> of the attribute value is defined by the security module. A flags field
>> is included for LSM specific information. It is currently unused and must
>> be 0. The total size of the data, including the lsm_ctx structure and any
>> padding, is maintained as well.
>>
>> struct lsm_ctx {
>> __u64 id;
>> __u64 flags;
>> __u64 len;
>> __u64 ctx_len;
>> __u8 ctx[];
>> };
>>
>> Two new LSM hooks are used to interface with the LSMs.
>> security_getselfattr() collects the lsm_ctx values from the
>> LSMs that support the hook, accounting for space requirements.
>> security_setselfattr() identifies which LSM the attribute is
>> intended for and passes it along.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> Documentation/userspace-api/lsm.rst | 15 +++++
>> include/linux/lsm_hook_defs.h | 4 ++
>> include/linux/lsm_hooks.h | 9 +++
>> include/linux/security.h | 19 ++++++
>> include/linux/syscalls.h | 5 ++
>> include/uapi/linux/lsm.h | 33 ++++++++++
>> kernel/sys_ni.c | 4 ++
>> security/Makefile | 1 +
>> security/lsm_syscalls.c | 55 ++++++++++++++++
>> security/security.c | 97 +++++++++++++++++++++++++++++
>> 10 files changed, 242 insertions(+)
>> create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 32285ce65419..3c2c4916bd53 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -503,6 +504,14 @@
>> * and writing the xattrs as this hook is merely a filter.
>> * @d_instantiate:
>> * Fill in @inode security information for a @dentry if allowed.
>> + * @getselfattr:
>> + * Read attribute @attr for the current process and store it into @ctx.
>> + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
>> + * or another negative value otherwise.
>> + * @setselfattr:
>> + * Set attribute @attr for the current process.
>> + * Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
>> + * or another negative value otherwise.
>> * @getprocattr:
>> * Read attribute @name for process @p and store it into @value if allowed.
>> * Return the length of @value on success, a negative value otherwise.
> I'm sure you're already aware of this, but the above will need to be
> moved to security.c due to the changes in the lsm/next branch. That
> said, if you're basing on Linus' tree that's fine too, I'll fix it up
> during the merge; thankfully it's not a significant merge conflict.
I'm based on Linus' tree.
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 8faed81fc3b4..329cd9d2be50 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry,
>> struct inode *inode)
>> { }
>>
>> +static inline int security_getselfattr(unsigned int __user attr,
>> + struct lsm_ctx __user *ctx,
>> + size_t __user *size, u32 __user flags)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static inline int security_setselfattr(unsigned int __user attr,
>> + struct lsm_ctx __user *ctx,
>> + size_t __user size, u32 __user flags)
>> +{
>> + return -EINVAL;
>> +}
> It seems like EOPNOTSUPP might be more appropriate than EINVAL for
> both of these dummy implementations.
Sure.
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 33a0ee3bcb2e..3feca00cb0c1 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>> asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>> unsigned long home_node,
>> unsigned long flags);
>> +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> + size_t *size, __u64 flags);
>> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> + __u64 flags);
> As the kernel test robot already pointed out, the above needs to be updated.
>
>> /*
>> * Architecture-specific system calls
>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index aa3e01867739..adfb55dce2fd 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,39 @@
>> #ifndef _UAPI_LINUX_LSM_H
>> #define _UAPI_LINUX_LSM_H
>>
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
>> +
>> +/**
>> + * struct lsm_ctx - LSM context information
>> + * @id: the LSM id number, see LSM_ID_XXX
>> + * @flags: LSM specific flags
>> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
>> + * @ctx_len: the size of @ctx
>> + * @ctx: the LSM context value
>> + *
>> + * The @len field MUST be equal to the size of the lsm_ctx struct
>> + * plus any additional padding and/or data placed after @ctx.
>> + *
>> + * In all cases @ctx_len MUST be equal to the length of @ctx.
>> + * If @ctx is a string value it should be nul terminated with
>> + * @ctx_len equal to `strlen(@ctx) + 1`. Binary values are
>> + * supported.
>> + *
>> + * The @flags and @ctx fields SHOULD only be interpreted by the
>> + * LSM specified by @id; they MUST be set to zero/0 when not used.
>> + */
>> +struct lsm_ctx {
>> + __u64 id;
>> + __u64 flags;
>> + __u64 len;
>> + __u64 ctx_len;
>> + __u8 ctx[];
>> +};
>> +
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
> I'm pretty sure the repeated #includes are a typo, right? Or is there
> some uapi trick I'm missing ...
An artifact of patch (mis)management. Thanks for noticing.
>> diff --git a/security/security.c b/security/security.c
>> index 87c8796c3c46..2c57fe28c4f7 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>> }
>> EXPORT_SYMBOL(security_d_instantiate);
>>
>> +/**
>> + * security_getselfattr - Read an LSM attribute of the current process.
>> + * @attr: which attribute to return
>> + * @ctx: the user-space destination for the information, or NULL
>> + * @size: the size of space available to receive the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Returns the number of attributes found on success, negative value
>> + * on error. @size is reset to the total size of the data.
>> + * If @size is insufficient to contain the data -E2BIG is returned.
>> + */
>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> + size_t __user *size, u32 __user flags)
>> +{
>> + struct security_hook_list *hp;
>> + void __user *base = (void *)ctx;
> The casting seems wrong for a couple of reasons: I don't believe you
> need to cast the right side when the left side is a void pointer, and
> the right side cast drops the '__user' attribute when the left side is
> also a '__user' pointer value.
>
> That said, I think we may want @base to be 'u8 __user *base', more on
> that below ...
>
>> + size_t total = 0;
>> + size_t this;
> Naming is hard, but 'this'? You can do better ...
It seemed like a good idea at the time, but a rose by any other
name still has thorns. I'll come up with something "better".
>> + size_t left;
>> + bool istoobig = false;
> Sorry, more naming nits and since it looks like you need to respin
> anyway ... please rename @istoobig to @toobig or something else. The
> phrases-as-variable-names has always grated on me.
Sure.
>> + int count = 0;
>> + int rc;
>> +
>> + if (attr == 0)
>> + return -EINVAL;
>> + if (flags != 0)
>> + return -EINVAL;
>> + if (size == NULL)
>> + return -EINVAL;
>> + if (get_user(left, size))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> + this = left;
>> + if (base)
>> + ctx = (struct lsm_ctx __user *)(base + total);
> Pointer math on void pointers always makes me nervous. Why not set
> @base's type to a 'u8' just to remove any concerns?
I can do that. I made it a void pointer to reflect the notion that
the attributes aren't necessarily strings. Making it a u8 may suggest that
the data is a string to some developers.
>> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>> + switch (rc) {
>> + case -EOPNOTSUPP:
>> + rc = 0;
>> + continue;
>> + case -E2BIG:
>> + istoobig = true;
>> + left = 0;
>> + break;
>> + case 0:
>> + left -= this;
>> + break;
>> + default:
>> + return rc;
> I think the @getselfattr hook should behave similarly to the
> associated syscall, returning a non-negative number should indicate
> that @rc entries have been added to the @ctx array. Right now all the
> LSMs would just be adding one entry to the array, but we might as well
> code this up to be flexible.
Yes, some LSM may decide to have multiple "contexts".
>> + }
>> + total += this;
>> + count++;
>> + }
>> + if (count == 0)
>> + return LSM_RET_DEFAULT(getselfattr);
>> + if (put_user(total, size))
>> + return -EFAULT;
>> + if (rc)
>> + return rc;
> Is the 'if (rc)' check needed here? Shouldn't the switch-statement
> after the hook catch everything that this check would catch?
It's necessary because of BPF, which doesn't follow the LSM rules.
>> + if (istoobig)
>> + return -E2BIG;
>> + return count;
>> +}
>> +
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> + size_t __user size, u32 __user flags)
>> +{
>> + struct security_hook_list *hp;
>> + struct lsm_ctx lctx;
> Shouldn't we check @attr for valid values and return -EINVAL if bogus?
Sure.
>> + if (flags != 0)
>> + return -EINVAL;
>> + if (size < sizeof(*ctx))
>> + return -EINVAL;
> If we're only going to support on 'lsm_ctx' entry in this function we
> should verify that the 'len' and 'ctx_len' fields are sane. Although
> more on this below ...
The LSM is going to have to do its own version of sanity checking. Having
sanity checking here as well seems excessive.
>> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> + if ((hp->lsmid->id) == lctx.id)
>> + return hp->hook.setselfattr(attr, ctx, size, flags);
> Can anyone think of any good reason why we shouldn't support setting
> multiple LSMs in one call, similar to what we do with
> security_getselfattr()? It seems like it might be a nice thing to
> have ...
If you're setting the context for multiple LSMs and one fails the recovery
process is horrendous. Putting values you've already changed back to their
previous state may not even be possible. We could have a two pass scheme, one
to verify that the request would succeed and a second to do the work. That
doesn't address all the issues, including how to report which attribute failed.
I had planned to do multiple settings, but the weight of the mechanism to
deal with the failure case is considerable for a "nice to have".
>> + return LSM_RET_DEFAULT(setselfattr);
>> +}
>> +
>> int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>> char **value)
>> {
>> --
>> 2.39.2
> --
> paul-moore.com
On 3/29/2023 6:13 PM, Paul Moore wrote:
> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
>> Add lsm_name_to_attr(), which translates a text string to a
>> LSM_ATTR value if one is available.
>>
>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>> the trailing attribute value.
>>
>> All are used in module specific components of LSM system calls.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> include/linux/security.h | 13 ++++++++++
>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>> security/security.c | 31 ++++++++++++++++++++++++
>> 3 files changed, 95 insertions(+)
> ..
>
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> index 6efbe244d304..55d849ad5d6e 100644
>> --- a/security/lsm_syscalls.c
>> +++ b/security/lsm_syscalls.c
>> @@ -17,6 +17,57 @@
>> #include <linux/lsm_hooks.h>
>> #include <uapi/linux/lsm.h>
>>
>> +struct attr_map {
>> + char *name;
>> + u64 attr;
>> +};
>> +
>> +static const struct attr_map lsm_attr_names[] = {
>> + {
>> + .name = "current",
>> + .attr = LSM_ATTR_CURRENT,
>> + },
>> + {
>> + .name = "exec",
>> + .attr = LSM_ATTR_EXEC,
>> + },
>> + {
>> + .name = "fscreate",
>> + .attr = LSM_ATTR_FSCREATE,
>> + },
>> + {
>> + .name = "keycreate",
>> + .attr = LSM_ATTR_KEYCREATE,
>> + },
>> + {
>> + .name = "prev",
>> + .attr = LSM_ATTR_PREV,
>> + },
>> + {
>> + .name = "sockcreate",
>> + .attr = LSM_ATTR_SOCKCREATE,
>> + },
>> +};
>> +
>> +/**
>> + * lsm_name_to_attr - map an LSM attribute name to its ID
>> + * @name: name of the attribute
>> + *
>> + * Look the given @name up in the table of know attribute names.
>> + *
>> + * Returns the LSM attribute value associated with @name, or 0 if
>> + * there is no mapping.
>> + */
>> +u64 lsm_name_to_attr(const char *name)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
>> + if (!strcmp(name, lsm_attr_names[i].name))
>> + return lsm_attr_names[i].attr;
> I'm pretty sure this is the only place where @lsm_attr_names is used,
> right? If true, when coupled with the idea that these syscalls are
> going to close the door on new LSM attributes in procfs I think we can
> just put the mapping directly in this function via a series of
> if-statements.
Ick. You're not wrong, but the hard coded if-statement approach goes
against all sorts of coding principles. I'll do it, but I can't say I
like it.
>> + return 0;
>> +}
>> +
>> /**
>> * sys_lsm_set_self_attr - Set current task's security module attribute
>> * @attr: which attribute to set
>> diff --git a/security/security.c b/security/security.c
>> index 2c57fe28c4f7..f7b814a3940c 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
>> return 0;
>> }
>>
>> +/**
>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>> + * @ctx: an LSM context to be filled
>> + * @context: the new context value
>> + * @context_size: the size of the new context value
>> + * @id: LSM id
>> + * @flags: LSM defined flags
>> + *
>> + * Fill all of the fields in a user space lsm_ctx structure.
>> + * Caller is assumed to have verified that @ctx has enough space
>> + * for @context.
>> + * Returns 0 on success, -EFAULT on a copyout error.
>> + */
>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>> + size_t context_size, u64 id, u64 flags)
>> +{
>> + struct lsm_ctx local;
>> + void __user *vc = ctx;
>> +
>> + local.id = id;
>> + local.flags = flags;
>> + local.ctx_len = context_size;
>> + local.len = context_size + sizeof(local);
>> + vc += sizeof(local);
> See my prior comments about void pointer math.
>
>> + if (copy_to_user(ctx, &local, sizeof(local)))
>> + return -EFAULT;
>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>> + return -EFAULT;
> Should we handle the padding in this function?
This function fills in a lsm_ctx. The padding, if any, is in addition to
the lsm_ctx, not part of it.
>> + return 0;
>> +}
> --
> paul-moore.com
On 3/29/2023 6:13 PM, Paul Moore wrote:
> On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <[email protected]> wrote:
>> Add hooks for setselfattr and getselfattr. These hooks are not very
>> different from their setprocattr and getprocattr equivalents, and
>> much of the code is shared.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> Cc: [email protected]
>> Cc: Paul Moore <[email protected]>
>> ---
>> security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
>> 1 file changed, 117 insertions(+), 30 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 9403aee75981..8896edf80aa9 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
>> inode_doinit_with_dentry(inode, dentry);
>> }
>>
>> -static int selinux_getprocattr(struct task_struct *p,
>> - const char *name, char **value)
>> +static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
> Are you ready for more naming nitpicks? ;)
I would expect nothing less. :)
> Let's call this 'selinux_lsm_getattr()', and the matching setter
> should be 'selinux_lsm_setattr()'.
As you wish. It's your LSM.
>> {
>> const struct task_security_struct *__tsec;
>> u32 sid;
>> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
>> goto bad;
>> }
>>
>> - if (!strcmp(name, "current"))
>> + switch (attr) {
>> + case LSM_ATTR_CURRENT:
>> sid = __tsec->sid;
>> - else if (!strcmp(name, "prev"))
>> + break;
>> + case LSM_ATTR_PREV:
>> sid = __tsec->osid;
>> - else if (!strcmp(name, "exec"))
>> + break;
>> + case LSM_ATTR_EXEC:
>> sid = __tsec->exec_sid;
>> - else if (!strcmp(name, "fscreate"))
>> + break;
>> + case LSM_ATTR_FSCREATE:
>> sid = __tsec->create_sid;
>> - else if (!strcmp(name, "keycreate"))
>> + break;
>> + case LSM_ATTR_KEYCREATE:
>> sid = __tsec->keycreate_sid;
>> - else if (!strcmp(name, "sockcreate"))
>> + break;
>> + case LSM_ATTR_SOCKCREATE:
>> sid = __tsec->sockcreate_sid;
>> - else {
>> - error = -EINVAL;
>> + break;
>> + default:
>> + error = -EOPNOTSUPP;
> The error should probably be -EINVAL.
It's possible that we may add an attribute that SELinux doesn't
support, say LSM_ATTR_CRYPTO_KEY, that another LSM does. This is
the same behavior the other LSMs exhibit in the face of a request
for attributes such as LSM_ATTR_KEYCREATE that they don't support.
>> goto bad;
>> }
>> rcu_read_unlock();
>> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
>> return error;
>> }
>>
>> -static int selinux_setprocattr(const char *name, void *value, size_t size)
>> +static int do_setattr(u64 attr, void *value, size_t size)
>> {
>> struct task_security_struct *tsec;
>> struct cred *new;
>> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> /*
>> * Basic control over ability to set these attributes at all.
>> */
>> - if (!strcmp(name, "exec"))
>> + switch (attr) {
>> + case LSM_ATTR_CURRENT:
>> + error = avc_has_perm(&selinux_state,
>> + mysid, mysid, SECCLASS_PROCESS,
>> + PROCESS__SETCURRENT, NULL);
>> + break;
>> + case LSM_ATTR_EXEC:
>> error = avc_has_perm(&selinux_state,
>> mysid, mysid, SECCLASS_PROCESS,
>> PROCESS__SETEXEC, NULL);
>> - else if (!strcmp(name, "fscreate"))
>> + break;
>> + case LSM_ATTR_FSCREATE:
>> error = avc_has_perm(&selinux_state,
>> mysid, mysid, SECCLASS_PROCESS,
>> PROCESS__SETFSCREATE, NULL);
>> - else if (!strcmp(name, "keycreate"))
>> + break;
>> + case LSM_ATTR_KEYCREATE:
>> error = avc_has_perm(&selinux_state,
>> mysid, mysid, SECCLASS_PROCESS,
>> PROCESS__SETKEYCREATE, NULL);
>> - else if (!strcmp(name, "sockcreate"))
>> + break;
>> + case LSM_ATTR_SOCKCREATE:
>> error = avc_has_perm(&selinux_state,
>> mysid, mysid, SECCLASS_PROCESS,
>> PROCESS__SETSOCKCREATE, NULL);
>> - else if (!strcmp(name, "current"))
>> - error = avc_has_perm(&selinux_state,
>> - mysid, mysid, SECCLASS_PROCESS,
>> - PROCESS__SETCURRENT, NULL);
>> - else
>> - error = -EINVAL;
>> + break;
>> + default:
>> + error = -EOPNOTSUPP;
> Same as above, should be -EINVAL.
Same as above, there may be attributes SELinux doesn't support.
>> + break;
>> + }
>> if (error)
>> return error;
>>
>> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> }
>> error = security_context_to_sid(&selinux_state, value, size,
>> &sid, GFP_KERNEL);
>> - if (error == -EINVAL && !strcmp(name, "fscreate")) {
>> + if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
>> if (!has_cap_mac_admin(true)) {
>> struct audit_buffer *ab;
>> size_t audit_size;
>>
>> - /* We strip a nul only if it is at the end, otherwise the
>> - * context contains a nul and we should audit that */
>> + /* We strip a nul only if it is at the end,
>> + * otherwise the context contains a nul and
>> + * we should audit that */
> You *do* get gold stars for fixing line lengths in close proximity ;)
I guess I'm the Last User of the 80 character terminal.
>> if (str[size - 1] == '\0')
>> audit_size = size - 1;
>> else
>> @@ -6459,7 +6474,8 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> if (!ab)
>> return error;
>> audit_log_format(ab, "op=fscreate invalid_context=");
>> - audit_log_n_untrustedstring(ab, value, audit_size);
>> + audit_log_n_untrustedstring(ab, value,
>> + audit_size);
>> audit_log_end(ab);
>>
>> return error;
>> @@ -6483,11 +6499,11 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> checks and may_create for the file creation checks. The
>> operation will then fail if the context is not permitted. */
>> tsec = selinux_cred(new);
>> - if (!strcmp(name, "exec")) {
>> + if (attr == LSM_ATTR_EXEC) {
>> tsec->exec_sid = sid;
>> - } else if (!strcmp(name, "fscreate")) {
>> + } else if (attr == LSM_ATTR_FSCREATE) {
>> tsec->create_sid = sid;
>> - } else if (!strcmp(name, "keycreate")) {
>> + } else if (attr == LSM_ATTR_KEYCREATE) {
>> if (sid) {
>> error = avc_has_perm(&selinux_state, mysid, sid,
>> SECCLASS_KEY, KEY__CREATE, NULL);
>> @@ -6495,9 +6511,9 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> goto abort_change;
>> }
>> tsec->keycreate_sid = sid;
>> - } else if (!strcmp(name, "sockcreate")) {
>> + } else if (attr == LSM_ATTR_SOCKCREATE) {
>> tsec->sockcreate_sid = sid;
>> - } else if (!strcmp(name, "current")) {
>> + } else if (attr == LSM_ATTR_CURRENT) {
>> error = -EINVAL;
>> if (sid == 0)
>> goto abort_change;
>> @@ -6542,6 +6558,75 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
>> return error;
>> }
>>
>> +static int selinux_getselfattr(unsigned int __user attr,
>> + struct lsm_ctx __user *ctx, size_t *size,
>> + u32 __user flags)
>> +{
>> + char *value;
>> + size_t total_len;
>> + int len;
>> + int rc = 0;
>> +
>> + len = do_getattr(attr, current, &value);
>> + if (len < 0)
>> + return len;
>> +
>> + total_len = len + sizeof(*ctx);
>> +
>> + if (total_len > *size)
>> + rc = -E2BIG;
>> + else
>> + lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
>> +
>> + *size = total_len;
>> + return rc;
>> +}
>> +
>> +static int selinux_setselfattr(unsigned int __user attr,
>> + struct lsm_ctx __user *ctx, size_t __user size,
>> + u32 __user flags)
>> +{
>> + struct lsm_ctx *lctx;
>> + void *context;
>> + int rc;
>> +
>> + context = kmalloc(size, GFP_KERNEL);
>> + if (context == NULL)
>> + return -ENOMEM;
>> +
>> + lctx = (struct lsm_ctx *)context;
>> + if (copy_from_user(context, ctx, size))
>> + rc = -EFAULT;
>> + else if (lctx->ctx_len > size)
>> + rc = -EINVAL;
>> + else
>> + rc = do_setattr(attr, lctx + 1, lctx->ctx_len);
>> +
>> + kfree(context);
>> + if (rc > 0)
>> + return 0;
>> + return rc;
>> +}
>> +
>> +static int selinux_getprocattr(struct task_struct *p,
>> + const char *name, char **value)
>> +{
>> + unsigned int attr = lsm_name_to_attr(name);
>> +
>> + if (attr)
>> + return do_getattr(attr, p, value);
>> + return -EINVAL;
>> +}
>> +
>> +static int selinux_setprocattr(const char *name, void *value, size_t size)
>> +{
>> + int attr = lsm_name_to_attr(name);
>> +
>> + if (attr)
>> + return do_setattr(attr, value, size);
>> + return -EINVAL;
>> +}
>> +
>> static int selinux_ismaclabel(const char *name)
>> {
>> return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
>> @@ -7183,6 +7268,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>
>> LSM_HOOK_INIT(d_instantiate, selinux_d_instantiate),
>>
>> + LSM_HOOK_INIT(getselfattr, selinux_getselfattr),
>> + LSM_HOOK_INIT(setselfattr, selinux_setselfattr),
>> LSM_HOOK_INIT(getprocattr, selinux_getprocattr),
>> LSM_HOOK_INIT(setprocattr, selinux_setprocattr),
>>
>> --
>> 2.39.2
> --
> paul-moore.com
On Thu, Mar 30, 2023 at 4:00 PM Casey Schaufler <[email protected]> wrote:
> On 3/29/2023 6:12 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[email protected]> wrote:
> >> Create a system call lsm_get_self_attr() to provide the security
> >> module maintained attributes of the current process.
> >> Create a system call lsm_set_self_attr() to set a security
> >> module maintained attribute of the current process.
> >> Historically these attributes have been exposed to user space via
> >> entries in procfs under /proc/self/attr.
> >>
> >> The attribute value is provided in a lsm_ctx structure. The structure
> >> identifys the size of the attribute, and the attribute value. The format
> > "identifies"
> >
> >> of the attribute value is defined by the security module. A flags field
> >> is included for LSM specific information. It is currently unused and must
> >> be 0. The total size of the data, including the lsm_ctx structure and any
> >> padding, is maintained as well.
> >>
> >> struct lsm_ctx {
> >> __u64 id;
> >> __u64 flags;
> >> __u64 len;
> >> __u64 ctx_len;
> >> __u8 ctx[];
> >> };
> >>
> >> Two new LSM hooks are used to interface with the LSMs.
> >> security_getselfattr() collects the lsm_ctx values from the
> >> LSMs that support the hook, accounting for space requirements.
> >> security_setselfattr() identifies which LSM the attribute is
> >> intended for and passes it along.
> >>
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> ---
> >> Documentation/userspace-api/lsm.rst | 15 +++++
> >> include/linux/lsm_hook_defs.h | 4 ++
> >> include/linux/lsm_hooks.h | 9 +++
> >> include/linux/security.h | 19 ++++++
> >> include/linux/syscalls.h | 5 ++
> >> include/uapi/linux/lsm.h | 33 ++++++++++
> >> kernel/sys_ni.c | 4 ++
> >> security/Makefile | 1 +
> >> security/lsm_syscalls.c | 55 ++++++++++++++++
> >> security/security.c | 97 +++++++++++++++++++++++++++++
> >> 10 files changed, 242 insertions(+)
> >> create mode 100644 security/lsm_syscalls.c
...
> >> + int count = 0;
> >> + int rc;
> >> +
> >> + if (attr == 0)
> >> + return -EINVAL;
> >> + if (flags != 0)
> >> + return -EINVAL;
> >> + if (size == NULL)
> >> + return -EINVAL;
> >> + if (get_user(left, size))
> >> + return -EFAULT;
> >> +
> >> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> >> + this = left;
> >> + if (base)
> >> + ctx = (struct lsm_ctx __user *)(base + total);
> > Pointer math on void pointers always makes me nervous. Why not set
> > @base's type to a 'u8' just to remove any concerns?
>
> I can do that. I made it a void pointer to reflect the notion that
> the attributes aren't necessarily strings. Making it a u8 may suggest that
> the data is a string to some developers.
That's a fair concern, but there is plenty of precedence of binary
blobs being stored in 'unsigned char' arrays to make it easier to
pluck data out at random byte offsets.
> >> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> >> + switch (rc) {
> >> + case -EOPNOTSUPP:
> >> + rc = 0;
> >> + continue;
> >> + case -E2BIG:
> >> + istoobig = true;
> >> + left = 0;
> >> + break;
> >> + case 0:
> >> + left -= this;
> >> + break;
> >> + default:
> >> + return rc;
> > I think the @getselfattr hook should behave similarly to the
> > associated syscall, returning a non-negative number should indicate
> > that @rc entries have been added to the @ctx array. Right now all the
> > LSMs would just be adding one entry to the array, but we might as well
> > code this up to be flexible.
>
> Yes, some LSM may decide to have multiple "contexts".
>
> >> + }
> >> + total += this;
> >> + count++;
> >> + }
> >> + if (count == 0)
> >> + return LSM_RET_DEFAULT(getselfattr);
> >> + if (put_user(total, size))
> >> + return -EFAULT;
> >> + if (rc)
> >> + return rc;
> > Is the 'if (rc)' check needed here? Shouldn't the switch-statement
> > after the hook catch everything that this check would catch?
>
> It's necessary because of BPF, which doesn't follow the LSM rules.
I thought if it made it this far in the function the LSM, BPF or not,
would still have gone through the switch statement above which would
have returned early if the the value was something other than one of
the accepted return codes ... right?
> >> + if (istoobig)
> >> + return -E2BIG;
> >> + return count;
> >> +}
> >> +
> >> +/**
> >> + * security_setselfattr - Set an LSM attribute on the current process.
> >> + * @attr: which attribute to set
> >> + * @ctx: the user-space source for the information
> >> + * @size: the size of the data
> >> + * @flags: reserved for future use, must be 0
> >> + *
> >> + * Set an LSM attribute for the current process. The LSM, attribute
> >> + * and new value are included in @ctx.
> >> + *
> >> + * Returns 0 on success, an LSM specific value on failure.
> >> + */
> >> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> >> + size_t __user size, u32 __user flags)
> >> +{
> >> + struct security_hook_list *hp;
> >> + struct lsm_ctx lctx;
> > Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
> Sure.
>
> >> + if (flags != 0)
> >> + return -EINVAL;
> >> + if (size < sizeof(*ctx))
> >> + return -EINVAL;
> > If we're only going to support on 'lsm_ctx' entry in this function we
> > should verify that the 'len' and 'ctx_len' fields are sane. Although
> > more on this below ...
>
> The LSM is going to have to do its own version of sanity checking. Having
> sanity checking here as well seems excessive.
Yes, the LSM will probably need to do some checks, but we can safely
do the length checking here so we might as well do it simply so every
LSM doesn't have to duplicate the length checks.
> >> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> >> + return -EFAULT;
> >> +
> >> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> >> + if ((hp->lsmid->id) == lctx.id)
> >> + return hp->hook.setselfattr(attr, ctx, size, flags);
> > Can anyone think of any good reason why we shouldn't support setting
> > multiple LSMs in one call, similar to what we do with
> > security_getselfattr()? It seems like it might be a nice thing to
> > have ...
>
> If you're setting the context for multiple LSMs ...
See my follow-up to my original reply sent earlier today.
--
paul-moore.com
On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <[email protected]> wrote:
> On 3/29/2023 6:13 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
> >> Add lsm_name_to_attr(), which translates a text string to a
> >> LSM_ATTR value if one is available.
> >>
> >> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
> >> the trailing attribute value.
> >>
> >> All are used in module specific components of LSM system calls.
> >>
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> ---
> >> include/linux/security.h | 13 ++++++++++
> >> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
> >> security/security.c | 31 ++++++++++++++++++++++++
> >> 3 files changed, 95 insertions(+)
> > ..
> >
> >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> >> index 6efbe244d304..55d849ad5d6e 100644
> >> --- a/security/lsm_syscalls.c
> >> +++ b/security/lsm_syscalls.c
> >> @@ -17,6 +17,57 @@
> >> #include <linux/lsm_hooks.h>
> >> #include <uapi/linux/lsm.h>
> >>
> >> +struct attr_map {
> >> + char *name;
> >> + u64 attr;
> >> +};
> >> +
> >> +static const struct attr_map lsm_attr_names[] = {
> >> + {
> >> + .name = "current",
> >> + .attr = LSM_ATTR_CURRENT,
> >> + },
> >> + {
> >> + .name = "exec",
> >> + .attr = LSM_ATTR_EXEC,
> >> + },
> >> + {
> >> + .name = "fscreate",
> >> + .attr = LSM_ATTR_FSCREATE,
> >> + },
> >> + {
> >> + .name = "keycreate",
> >> + .attr = LSM_ATTR_KEYCREATE,
> >> + },
> >> + {
> >> + .name = "prev",
> >> + .attr = LSM_ATTR_PREV,
> >> + },
> >> + {
> >> + .name = "sockcreate",
> >> + .attr = LSM_ATTR_SOCKCREATE,
> >> + },
> >> +};
> >> +
> >> +/**
> >> + * lsm_name_to_attr - map an LSM attribute name to its ID
> >> + * @name: name of the attribute
> >> + *
> >> + * Look the given @name up in the table of know attribute names.
> >> + *
> >> + * Returns the LSM attribute value associated with @name, or 0 if
> >> + * there is no mapping.
> >> + */
> >> +u64 lsm_name_to_attr(const char *name)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
> >> + if (!strcmp(name, lsm_attr_names[i].name))
> >> + return lsm_attr_names[i].attr;
> > I'm pretty sure this is the only place where @lsm_attr_names is used,
> > right? If true, when coupled with the idea that these syscalls are
> > going to close the door on new LSM attributes in procfs I think we can
> > just put the mapping directly in this function via a series of
> > if-statements.
>
> Ick. You're not wrong, but the hard coded if-statement approach goes
> against all sorts of coding principles. I'll do it, but I can't say I
> like it.
If it helps any, I understand and am sympathetic. I guess I've gotten
to that point where in addition to "code elegance", I'm also very
concerned about defending against "code abuse", and something like an
nicely defined mapping array is ripe for someone to come along and use
that to justify further use of the attribute string names in some
other function/API.
If you want to stick with the array - I have no problem with that -
make it local to lsm_name_to_attr().
> >> +/**
> >> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
> >> + * @ctx: an LSM context to be filled
> >> + * @context: the new context value
> >> + * @context_size: the size of the new context value
> >> + * @id: LSM id
> >> + * @flags: LSM defined flags
> >> + *
> >> + * Fill all of the fields in a user space lsm_ctx structure.
> >> + * Caller is assumed to have verified that @ctx has enough space
> >> + * for @context.
> >> + * Returns 0 on success, -EFAULT on a copyout error.
> >> + */
> >> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
> >> + size_t context_size, u64 id, u64 flags)
> >> +{
> >> + struct lsm_ctx local;
> >> + void __user *vc = ctx;
> >> +
> >> + local.id = id;
> >> + local.flags = flags;
> >> + local.ctx_len = context_size;
> >> + local.len = context_size + sizeof(local);
> >> + vc += sizeof(local);
> > See my prior comments about void pointer math.
> >
> >> + if (copy_to_user(ctx, &local, sizeof(local)))
> >> + return -EFAULT;
> >> + if (context_size > 0 && copy_to_user(vc, context, context_size))
> >> + return -EFAULT;
> > Should we handle the padding in this function?
>
> This function fills in a lsm_ctx. The padding, if any, is in addition to
> the lsm_ctx, not part of it.
Okay, so where is the padding managed? I may have missed it, but I
don't recall seeing it anywhere in this patchset ...
--
paul-moore.com
On Thu, Mar 30, 2023 at 4:55 PM Casey Schaufler <[email protected]> wrote:
> On 3/29/2023 6:13 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <[email protected]> wrote:
> >> Add hooks for setselfattr and getselfattr. These hooks are not very
> >> different from their setprocattr and getprocattr equivalents, and
> >> much of the code is shared.
> >>
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> Cc: [email protected]
> >> Cc: Paul Moore <[email protected]>
> >> ---
> >> security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
> >> 1 file changed, 117 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 9403aee75981..8896edf80aa9 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
> >> inode_doinit_with_dentry(inode, dentry);
> >> }
> >>
> >> -static int selinux_getprocattr(struct task_struct *p,
> >> - const char *name, char **value)
> >> +static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
> > Are you ready for more naming nitpicks? ;)
>
> I would expect nothing less. :)
>
> > Let's call this 'selinux_lsm_getattr()', and the matching setter
> > should be 'selinux_lsm_setattr()'.
>
> As you wish. It's your LSM.
>
>
> >> {
> >> const struct task_security_struct *__tsec;
> >> u32 sid;
> >> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
> >> goto bad;
> >> }
> >>
> >> - if (!strcmp(name, "current"))
> >> + switch (attr) {
> >> + case LSM_ATTR_CURRENT:
> >> sid = __tsec->sid;
> >> - else if (!strcmp(name, "prev"))
> >> + break;
> >> + case LSM_ATTR_PREV:
> >> sid = __tsec->osid;
> >> - else if (!strcmp(name, "exec"))
> >> + break;
> >> + case LSM_ATTR_EXEC:
> >> sid = __tsec->exec_sid;
> >> - else if (!strcmp(name, "fscreate"))
> >> + break;
> >> + case LSM_ATTR_FSCREATE:
> >> sid = __tsec->create_sid;
> >> - else if (!strcmp(name, "keycreate"))
> >> + break;
> >> + case LSM_ATTR_KEYCREATE:
> >> sid = __tsec->keycreate_sid;
> >> - else if (!strcmp(name, "sockcreate"))
> >> + break;
> >> + case LSM_ATTR_SOCKCREATE:
> >> sid = __tsec->sockcreate_sid;
> >> - else {
> >> - error = -EINVAL;
> >> + break;
> >> + default:
> >> + error = -EOPNOTSUPP;
> > The error should probably be -EINVAL.
>
> It's possible that we may add an attribute that SELinux doesn't
> support, say LSM_ATTR_CRYPTO_KEY, that another LSM does. This is
> the same behavior the other LSMs exhibit in the face of a request
> for attributes such as LSM_ATTR_KEYCREATE that they don't support.
Okay, I'll accept that argument, but I would ask that add some
additional handling in selinux_getprocattr() so that it returns
-EINVAL in this case just as it does today.
> >> goto bad;
> >> }
> >> rcu_read_unlock();
> >> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
> >> return error;
> >> }
> >>
> >> -static int selinux_setprocattr(const char *name, void *value, size_t size)
> >> +static int do_setattr(u64 attr, void *value, size_t size)
> >> {
> >> struct task_security_struct *tsec;
> >> struct cred *new;
> >> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> >> /*
> >> * Basic control over ability to set these attributes at all.
> >> */
> >> - if (!strcmp(name, "exec"))
> >> + switch (attr) {
> >> + case LSM_ATTR_CURRENT:
> >> + error = avc_has_perm(&selinux_state,
> >> + mysid, mysid, SECCLASS_PROCESS,
> >> + PROCESS__SETCURRENT, NULL);
> >> + break;
> >> + case LSM_ATTR_EXEC:
> >> error = avc_has_perm(&selinux_state,
> >> mysid, mysid, SECCLASS_PROCESS,
> >> PROCESS__SETEXEC, NULL);
> >> - else if (!strcmp(name, "fscreate"))
> >> + break;
> >> + case LSM_ATTR_FSCREATE:
> >> error = avc_has_perm(&selinux_state,
> >> mysid, mysid, SECCLASS_PROCESS,
> >> PROCESS__SETFSCREATE, NULL);
> >> - else if (!strcmp(name, "keycreate"))
> >> + break;
> >> + case LSM_ATTR_KEYCREATE:
> >> error = avc_has_perm(&selinux_state,
> >> mysid, mysid, SECCLASS_PROCESS,
> >> PROCESS__SETKEYCREATE, NULL);
> >> - else if (!strcmp(name, "sockcreate"))
> >> + break;
> >> + case LSM_ATTR_SOCKCREATE:
> >> error = avc_has_perm(&selinux_state,
> >> mysid, mysid, SECCLASS_PROCESS,
> >> PROCESS__SETSOCKCREATE, NULL);
> >> - else if (!strcmp(name, "current"))
> >> - error = avc_has_perm(&selinux_state,
> >> - mysid, mysid, SECCLASS_PROCESS,
> >> - PROCESS__SETCURRENT, NULL);
> >> - else
> >> - error = -EINVAL;
> >> + break;
> >> + default:
> >> + error = -EOPNOTSUPP;
> > Same as above, should be -EINVAL.
>
> Same as above, there may be attributes SELinux doesn't support.
Also, same as above.
> >> + break;
> >> + }
> >> if (error)
> >> return error;
> >>
> >> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> >> }
> >> error = security_context_to_sid(&selinux_state, value, size,
> >> &sid, GFP_KERNEL);
> >> - if (error == -EINVAL && !strcmp(name, "fscreate")) {
> >> + if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
> >> if (!has_cap_mac_admin(true)) {
> >> struct audit_buffer *ab;
> >> size_t audit_size;
> >>
> >> - /* We strip a nul only if it is at the end, otherwise the
> >> - * context contains a nul and we should audit that */
> >> + /* We strip a nul only if it is at the end,
> >> + * otherwise the context contains a nul and
> >> + * we should audit that */
> > You *do* get gold stars for fixing line lengths in close proximity ;)
>
> I guess I'm the Last User of the 80 character terminal.
I'm still a big fan and I'm sticking to the 80 char limit for the LSM
layer as well as the SELinux, audit, and labeled networking
subsystems. Longer lines either predate me or I simply didn't catch
them during review/merge.
--
paul-moore.com
On 3/30/2023 4:28 PM, Paul Moore wrote:
> On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <[email protected]> wrote:
>> On 3/29/2023 6:13 PM, Paul Moore wrote:
>>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
>>>> Add lsm_name_to_attr(), which translates a text string to a
>>>> LSM_ATTR value if one is available.
>>>>
>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>>> the trailing attribute value.
>>>>
>>>> All are used in module specific components of LSM system calls.
>>>>
>>>> Signed-off-by: Casey Schaufler <[email protected]>
>>>> ---
>>>> include/linux/security.h | 13 ++++++++++
>>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>> security/security.c | 31 ++++++++++++++++++++++++
>>>> 3 files changed, 95 insertions(+)
>>> ..
>>>
>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>> index 6efbe244d304..55d849ad5d6e 100644
>>>> --- a/security/lsm_syscalls.c
>>>> +++ b/security/lsm_syscalls.c
>>>> @@ -17,6 +17,57 @@
>>>> #include <linux/lsm_hooks.h>
>>>> #include <uapi/linux/lsm.h>
>>>>
>>>> +struct attr_map {
>>>> + char *name;
>>>> + u64 attr;
>>>> +};
>>>> +
>>>> +static const struct attr_map lsm_attr_names[] = {
>>>> + {
>>>> + .name = "current",
>>>> + .attr = LSM_ATTR_CURRENT,
>>>> + },
>>>> + {
>>>> + .name = "exec",
>>>> + .attr = LSM_ATTR_EXEC,
>>>> + },
>>>> + {
>>>> + .name = "fscreate",
>>>> + .attr = LSM_ATTR_FSCREATE,
>>>> + },
>>>> + {
>>>> + .name = "keycreate",
>>>> + .attr = LSM_ATTR_KEYCREATE,
>>>> + },
>>>> + {
>>>> + .name = "prev",
>>>> + .attr = LSM_ATTR_PREV,
>>>> + },
>>>> + {
>>>> + .name = "sockcreate",
>>>> + .attr = LSM_ATTR_SOCKCREATE,
>>>> + },
>>>> +};
>>>> +
>>>> +/**
>>>> + * lsm_name_to_attr - map an LSM attribute name to its ID
>>>> + * @name: name of the attribute
>>>> + *
>>>> + * Look the given @name up in the table of know attribute names.
>>>> + *
>>>> + * Returns the LSM attribute value associated with @name, or 0 if
>>>> + * there is no mapping.
>>>> + */
>>>> +u64 lsm_name_to_attr(const char *name)
>>>> +{
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
>>>> + if (!strcmp(name, lsm_attr_names[i].name))
>>>> + return lsm_attr_names[i].attr;
>>> I'm pretty sure this is the only place where @lsm_attr_names is used,
>>> right? If true, when coupled with the idea that these syscalls are
>>> going to close the door on new LSM attributes in procfs I think we can
>>> just put the mapping directly in this function via a series of
>>> if-statements.
>> Ick. You're not wrong, but the hard coded if-statement approach goes
>> against all sorts of coding principles. I'll do it, but I can't say I
>> like it.
> If it helps any, I understand and am sympathetic. I guess I've gotten
> to that point where in addition to "code elegance", I'm also very
> concerned about defending against "code abuse", and something like an
> nicely defined mapping array is ripe for someone to come along and use
> that to justify further use of the attribute string names in some
> other function/API.
>
> If you want to stick with the array - I have no problem with that -
> make it local to lsm_name_to_attr().
>
>>>> +/**
>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>>> + * @ctx: an LSM context to be filled
>>>> + * @context: the new context value
>>>> + * @context_size: the size of the new context value
>>>> + * @id: LSM id
>>>> + * @flags: LSM defined flags
>>>> + *
>>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>>> + * Caller is assumed to have verified that @ctx has enough space
>>>> + * for @context.
>>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>>> + */
>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>>> + size_t context_size, u64 id, u64 flags)
>>>> +{
>>>> + struct lsm_ctx local;
>>>> + void __user *vc = ctx;
>>>> +
>>>> + local.id = id;
>>>> + local.flags = flags;
>>>> + local.ctx_len = context_size;
>>>> + local.len = context_size + sizeof(local);
>>>> + vc += sizeof(local);
>>> See my prior comments about void pointer math.
>>>
>>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>>> + return -EFAULT;
>>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>>> + return -EFAULT;
>>> Should we handle the padding in this function?
>> This function fills in a lsm_ctx. The padding, if any, is in addition to
>> the lsm_ctx, not part of it.
> Okay, so where is the padding managed? I may have missed it, but I
> don't recall seeing it anywhere in this patchset ...
Padding isn't being managed. There has been talk about using padding to
expand the API, but there is no use for it now. Or is there?
On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <[email protected]> wrote:
> On 3/30/2023 4:28 PM, Paul Moore wrote:
> > On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <[email protected]> wrote:
> >> On 3/29/2023 6:13 PM, Paul Moore wrote:
> >>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
> >>>> Add lsm_name_to_attr(), which translates a text string to a
> >>>> LSM_ATTR value if one is available.
> >>>>
> >>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
> >>>> the trailing attribute value.
> >>>>
> >>>> All are used in module specific components of LSM system calls.
> >>>>
> >>>> Signed-off-by: Casey Schaufler <[email protected]>
> >>>> ---
> >>>> include/linux/security.h | 13 ++++++++++
> >>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
> >>>> security/security.c | 31 ++++++++++++++++++++++++
> >>>> 3 files changed, 95 insertions(+)
> >>> ..
> >>>
> >>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> >>>> index 6efbe244d304..55d849ad5d6e 100644
> >>>> --- a/security/lsm_syscalls.c
> >>>> +++ b/security/lsm_syscalls.c
> >>>> @@ -17,6 +17,57 @@
> >>>> #include <linux/lsm_hooks.h>
> >>>> #include <uapi/linux/lsm.h>
> >>>>
> >>>> +struct attr_map {
> >>>> + char *name;
> >>>> + u64 attr;
> >>>> +};
> >>>> +
> >>>> +static const struct attr_map lsm_attr_names[] = {
> >>>> + {
> >>>> + .name = "current",
> >>>> + .attr = LSM_ATTR_CURRENT,
> >>>> + },
> >>>> + {
> >>>> + .name = "exec",
> >>>> + .attr = LSM_ATTR_EXEC,
> >>>> + },
> >>>> + {
> >>>> + .name = "fscreate",
> >>>> + .attr = LSM_ATTR_FSCREATE,
> >>>> + },
> >>>> + {
> >>>> + .name = "keycreate",
> >>>> + .attr = LSM_ATTR_KEYCREATE,
> >>>> + },
> >>>> + {
> >>>> + .name = "prev",
> >>>> + .attr = LSM_ATTR_PREV,
> >>>> + },
> >>>> + {
> >>>> + .name = "sockcreate",
> >>>> + .attr = LSM_ATTR_SOCKCREATE,
> >>>> + },
> >>>> +};
> >>>> +
> >>>> +/**
> >>>> + * lsm_name_to_attr - map an LSM attribute name to its ID
> >>>> + * @name: name of the attribute
> >>>> + *
> >>>> + * Look the given @name up in the table of know attribute names.
> >>>> + *
> >>>> + * Returns the LSM attribute value associated with @name, or 0 if
> >>>> + * there is no mapping.
> >>>> + */
> >>>> +u64 lsm_name_to_attr(const char *name)
> >>>> +{
> >>>> + int i;
> >>>> +
> >>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
> >>>> + if (!strcmp(name, lsm_attr_names[i].name))
> >>>> + return lsm_attr_names[i].attr;
> >>> I'm pretty sure this is the only place where @lsm_attr_names is used,
> >>> right? If true, when coupled with the idea that these syscalls are
> >>> going to close the door on new LSM attributes in procfs I think we can
> >>> just put the mapping directly in this function via a series of
> >>> if-statements.
> >> Ick. You're not wrong, but the hard coded if-statement approach goes
> >> against all sorts of coding principles. I'll do it, but I can't say I
> >> like it.
> > If it helps any, I understand and am sympathetic. I guess I've gotten
> > to that point where in addition to "code elegance", I'm also very
> > concerned about defending against "code abuse", and something like an
> > nicely defined mapping array is ripe for someone to come along and use
> > that to justify further use of the attribute string names in some
> > other function/API.
> >
> > If you want to stick with the array - I have no problem with that -
> > make it local to lsm_name_to_attr().
> >
> >>>> +/**
> >>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
> >>>> + * @ctx: an LSM context to be filled
> >>>> + * @context: the new context value
> >>>> + * @context_size: the size of the new context value
> >>>> + * @id: LSM id
> >>>> + * @flags: LSM defined flags
> >>>> + *
> >>>> + * Fill all of the fields in a user space lsm_ctx structure.
> >>>> + * Caller is assumed to have verified that @ctx has enough space
> >>>> + * for @context.
> >>>> + * Returns 0 on success, -EFAULT on a copyout error.
> >>>> + */
> >>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
> >>>> + size_t context_size, u64 id, u64 flags)
> >>>> +{
> >>>> + struct lsm_ctx local;
> >>>> + void __user *vc = ctx;
> >>>> +
> >>>> + local.id = id;
> >>>> + local.flags = flags;
> >>>> + local.ctx_len = context_size;
> >>>> + local.len = context_size + sizeof(local);
> >>>> + vc += sizeof(local);
> >>> See my prior comments about void pointer math.
> >>>
> >>>> + if (copy_to_user(ctx, &local, sizeof(local)))
> >>>> + return -EFAULT;
> >>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
> >>>> + return -EFAULT;
> >>> Should we handle the padding in this function?
> >> This function fills in a lsm_ctx. The padding, if any, is in addition to
> >> the lsm_ctx, not part of it.
> > Okay, so where is the padding managed? I may have missed it, but I
> > don't recall seeing it anywhere in this patchset ...
>
> Padding isn't being managed. There has been talk about using padding to
> expand the API, but there is no use for it now. Or is there?
I think two separate ideas are getting conflated, likely because the
'len' field is involved in both.
THe first issue is padding at the end of the lsm_ctx struct to ensure
that the next array element is aligned. The second issue is the
potential for extending the lsm_ctx struct on a per-LSM basis through
creative use of the 'flags' and 'len' fields; in this case additional
information could be stashed at the end of the lsm_ctx struct after
the 'ctx' field. The latter issue (extending the lsm_ctx) isn't
something we want to jump into, but it is something the syscall/struct
API would allow, and I don't want to exclude it as a possible future
solution to a yet unknown future problem. The former issue (padding
array elements) isn't a strict requirement as the syscall/struct API
works either way, but it seems like a good thing to do.
--
paul-moore.com
On 3/31/2023 12:24 PM, Paul Moore wrote:
> On Fri, Mar 31, 2023 at 12:56 PM Casey Schaufler <[email protected]> wrote:
>> On 3/30/2023 4:28 PM, Paul Moore wrote:
>>> On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler <[email protected]> wrote:
>>>> On 3/29/2023 6:13 PM, Paul Moore wrote:
>>>>> On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler <[email protected]> wrote:
>>>>>> Add lsm_name_to_attr(), which translates a text string to a
>>>>>> LSM_ATTR value if one is available.
>>>>>>
>>>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>>>>> the trailing attribute value.
>>>>>>
>>>>>> All are used in module specific components of LSM system calls.
>>>>>>
>>>>>> Signed-off-by: Casey Schaufler <[email protected]>
>>>>>> ---
>>>>>> include/linux/security.h | 13 ++++++++++
>>>>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>>>>> security/security.c | 31 ++++++++++++++++++++++++
>>>>>> 3 files changed, 95 insertions(+)
>>>>> ..
>>>>>
>>>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>>>> index 6efbe244d304..55d849ad5d6e 100644
>>>>>> --- a/security/lsm_syscalls.c
>>>>>> +++ b/security/lsm_syscalls.c
>>>>>> @@ -17,6 +17,57 @@
>>>>>> #include <linux/lsm_hooks.h>
>>>>>> #include <uapi/linux/lsm.h>
>>>>>>
>>>>>> +struct attr_map {
>>>>>> + char *name;
>>>>>> + u64 attr;
>>>>>> +};
>>>>>> +
>>>>>> +static const struct attr_map lsm_attr_names[] = {
>>>>>> + {
>>>>>> + .name = "current",
>>>>>> + .attr = LSM_ATTR_CURRENT,
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "exec",
>>>>>> + .attr = LSM_ATTR_EXEC,
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "fscreate",
>>>>>> + .attr = LSM_ATTR_FSCREATE,
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "keycreate",
>>>>>> + .attr = LSM_ATTR_KEYCREATE,
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "prev",
>>>>>> + .attr = LSM_ATTR_PREV,
>>>>>> + },
>>>>>> + {
>>>>>> + .name = "sockcreate",
>>>>>> + .attr = LSM_ATTR_SOCKCREATE,
>>>>>> + },
>>>>>> +};
>>>>>> +
>>>>>> +/**
>>>>>> + * lsm_name_to_attr - map an LSM attribute name to its ID
>>>>>> + * @name: name of the attribute
>>>>>> + *
>>>>>> + * Look the given @name up in the table of know attribute names.
>>>>>> + *
>>>>>> + * Returns the LSM attribute value associated with @name, or 0 if
>>>>>> + * there is no mapping.
>>>>>> + */
>>>>>> +u64 lsm_name_to_attr(const char *name)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
>>>>>> + if (!strcmp(name, lsm_attr_names[i].name))
>>>>>> + return lsm_attr_names[i].attr;
>>>>> I'm pretty sure this is the only place where @lsm_attr_names is used,
>>>>> right? If true, when coupled with the idea that these syscalls are
>>>>> going to close the door on new LSM attributes in procfs I think we can
>>>>> just put the mapping directly in this function via a series of
>>>>> if-statements.
>>>> Ick. You're not wrong, but the hard coded if-statement approach goes
>>>> against all sorts of coding principles. I'll do it, but I can't say I
>>>> like it.
>>> If it helps any, I understand and am sympathetic. I guess I've gotten
>>> to that point where in addition to "code elegance", I'm also very
>>> concerned about defending against "code abuse", and something like an
>>> nicely defined mapping array is ripe for someone to come along and use
>>> that to justify further use of the attribute string names in some
>>> other function/API.
>>>
>>> If you want to stick with the array - I have no problem with that -
>>> make it local to lsm_name_to_attr().
>>>
>>>>>> +/**
>>>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>>>>> + * @ctx: an LSM context to be filled
>>>>>> + * @context: the new context value
>>>>>> + * @context_size: the size of the new context value
>>>>>> + * @id: LSM id
>>>>>> + * @flags: LSM defined flags
>>>>>> + *
>>>>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>>>>> + * Caller is assumed to have verified that @ctx has enough space
>>>>>> + * for @context.
>>>>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>>>>> + */
>>>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>>>>> + size_t context_size, u64 id, u64 flags)
>>>>>> +{
>>>>>> + struct lsm_ctx local;
>>>>>> + void __user *vc = ctx;
>>>>>> +
>>>>>> + local.id = id;
>>>>>> + local.flags = flags;
>>>>>> + local.ctx_len = context_size;
>>>>>> + local.len = context_size + sizeof(local);
>>>>>> + vc += sizeof(local);
>>>>> See my prior comments about void pointer math.
>>>>>
>>>>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>>>>> + return -EFAULT;
>>>>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>>>>> + return -EFAULT;
>>>>> Should we handle the padding in this function?
>>>> This function fills in a lsm_ctx. The padding, if any, is in addition to
>>>> the lsm_ctx, not part of it.
>>> Okay, so where is the padding managed? I may have missed it, but I
>>> don't recall seeing it anywhere in this patchset ...
>> Padding isn't being managed. There has been talk about using padding to
>> expand the API, but there is no use for it now. Or is there?
> I think two separate ideas are getting conflated, likely because the
> 'len' field is involved in both.
>
> THe first issue is padding at the end of the lsm_ctx struct to ensure
> that the next array element is aligned. The second issue is the
> potential for extending the lsm_ctx struct on a per-LSM basis through
> creative use of the 'flags' and 'len' fields; in this case additional
> information could be stashed at the end of the lsm_ctx struct after
> the 'ctx' field. The latter issue (extending the lsm_ctx) isn't
> something we want to jump into, but it is something the syscall/struct
> API would allow, and I don't want to exclude it as a possible future
> solution to a yet unknown future problem. The former issue (padding
> array elements) isn't a strict requirement as the syscall/struct API
> works either way, but it seems like a good thing to do.
Reasonable. Thanks for the clarification.
On 15/03/2023 23:47, Casey Schaufler wrote:
> Add lsm_name_to_attr(), which translates a text string to a
> LSM_ATTR value if one is available.
>
> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
> the trailing attribute value.
>
> All are used in module specific components of LSM system calls.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> include/linux/security.h | 13 ++++++++++
> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
> security/security.c | 31 ++++++++++++++++++++++++
> 3 files changed, 95 insertions(+)
[...]
> diff --git a/security/security.c b/security/security.c
> index 2c57fe28c4f7..f7b814a3940c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
> return 0;
> }
>
> +/**
> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
> + * @ctx: an LSM context to be filled
> + * @context: the new context value
> + * @context_size: the size of the new context value
> + * @id: LSM id
> + * @flags: LSM defined flags
> + *
> + * Fill all of the fields in a user space lsm_ctx structure.
> + * Caller is assumed to have verified that @ctx has enough space
> + * for @context.
> + * Returns 0 on success, -EFAULT on a copyout error.
> + */
> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
> + size_t context_size, u64 id, u64 flags)
> +{
> + struct lsm_ctx local;
> + void __user *vc = ctx;
> +
> + local.id = id;
> + local.flags = flags;
> + local.ctx_len = context_size;
> + local.len = context_size + sizeof(local);
> + vc += sizeof(local);
> + if (copy_to_user(ctx, &local, sizeof(local)))
> + return -EFAULT;
> + if (context_size > 0 && copy_to_user(vc, context, context_size))
> + return -EFAULT;
Can we do a single copy_to_user() call? That would avoid inconsistent
user space data, could speed up a bit the operation, and make the code
easier to understand. To use the stack, we need to know the maximum size
of context_size for all use cases, which seems reasonable and can be
checked at build time (on each LSM side, and potentially with specific
context type passed as enum instead of context_size) and run time (for
this generic helper).
> + return 0;
> +}
> +
> /*
> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
> * can be accessed with:
On 03/04/2023 11:47, Mickaël Salaün wrote:
>
> On 15/03/2023 23:47, Casey Schaufler wrote:
>> Add lsm_name_to_attr(), which translates a text string to a
>> LSM_ATTR value if one is available.
>>
>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>> the trailing attribute value.
>>
>> All are used in module specific components of LSM system calls.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> include/linux/security.h | 13 ++++++++++
>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>> security/security.c | 31 ++++++++++++++++++++++++
>> 3 files changed, 95 insertions(+)
>
> [...]
>
>> diff --git a/security/security.c b/security/security.c
>> index 2c57fe28c4f7..f7b814a3940c 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
>> return 0;
>> }
>>
>> +/**
>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>> + * @ctx: an LSM context to be filled
>> + * @context: the new context value
>> + * @context_size: the size of the new context value
>> + * @id: LSM id
>> + * @flags: LSM defined flags
>> + *
>> + * Fill all of the fields in a user space lsm_ctx structure.
>> + * Caller is assumed to have verified that @ctx has enough space
>> + * for @context.
>> + * Returns 0 on success, -EFAULT on a copyout error.
>> + */
>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>> + size_t context_size, u64 id, u64 flags)
>> +{
>> + struct lsm_ctx local;
>> + void __user *vc = ctx;
>> +
>> + local.id = id;
>> + local.flags = flags;
>> + local.ctx_len = context_size;
>> + local.len = context_size + sizeof(local);
>> + vc += sizeof(local);
>> + if (copy_to_user(ctx, &local, sizeof(local)))
>> + return -EFAULT;
>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>> + return -EFAULT;
>
> Can we do a single copy_to_user() call? That would avoid inconsistent
> user space data, could speed up a bit the operation, and make the code
> easier to understand. To use the stack, we need to know the maximum size
> of context_size for all use cases, which seems reasonable and can be
> checked at build time (on each LSM side, and potentially with specific
> context type passed as enum instead of context_size) and run time (for
> this generic helper).
Well, actually the context_size should be inferred from id, and the
"local" size should be defined and check at build time against all
context ID sizes.
>
>
>> + return 0;
>> +}
>> +
>> /*
>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>> * can be accessed with:
On 03/04/2023 11:54, Mickaël Salaün wrote:
>
> On 03/04/2023 11:47, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:47, Casey Schaufler wrote:
>>> Add lsm_name_to_attr(), which translates a text string to a
>>> LSM_ATTR value if one is available.
>>>
>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>> the trailing attribute value.
>>>
>>> All are used in module specific components of LSM system calls.
>>>
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>> ---
>>> include/linux/security.h | 13 ++++++++++
>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>> security/security.c | 31 ++++++++++++++++++++++++
>>> 3 files changed, 95 insertions(+)
>>
>> [...]
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index 2c57fe28c4f7..f7b814a3940c 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct super_block *sb)
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>> + * @ctx: an LSM context to be filled
>>> + * @context: the new context value
>>> + * @context_size: the size of the new context value
>>> + * @id: LSM id
>>> + * @flags: LSM defined flags
>>> + *
>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>> + * Caller is assumed to have verified that @ctx has enough space
>>> + * for @context.
>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>> + */
>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>> + size_t context_size, u64 id, u64 flags)
>>> +{
>>> + struct lsm_ctx local;
>>> + void __user *vc = ctx;
>>> +
>>> + local.id = id;
>>> + local.flags = flags;
>>> + local.ctx_len = context_size;
>>> + local.len = context_size + sizeof(local);
>>> + vc += sizeof(local);
>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>> + return -EFAULT;
>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>> + return -EFAULT;
>>
>> Can we do a single copy_to_user() call? That would avoid inconsistent
>> user space data, could speed up a bit the operation, and make the code
>> easier to understand. To use the stack, we need to know the maximum size
>> of context_size for all use cases, which seems reasonable and can be
>> checked at build time (on each LSM side, and potentially with specific
>> context type passed as enum instead of context_size) and run time (for
>> this generic helper).
>
> Well, actually the context_size should be inferred from id, and the
> "local" size should be defined and check at build time against all
> context ID sizes.
@ctx_len should already be known by user space according to the LSM ID
and the requested attribute. @len should already be known by user space
because lsm_ctx is part of the ABI.
The only reason I can think of the rationale for @len and @ctx_len is
that struct lsm_ctx could gain more fields. If this happen, they would
then need to be inserted before @ctx. This would make this struct
lsm_ctx too flexible and complex for user space to parse correctly (e.g.
for strace, gdb).
I don't see where we could use @flags instead of relying on a new
attribute type.
I think security_getselfattr() and lsm_fill_user_ctx() could be changed
to avoid each LSM to pass their own ID to lsm_fill_user_ctx(). We could
have a lsm_get_attr_size(lsm_id, attr) helper (called by
security_getselfattr) to group these relations, based on fixed values,
exposed in the UAPI, and checked at build time with the size of the
related LSM-specific attribute type. This would also allow to factor out
the total size calculation needed before calling the getselfattr()
implementers, and then rely on a common consistent behavior. That could
also be used to not call getselfattr() implementers if they don't handle
a specific attribute, and then remove their related error handling for
this case.
For now, the getselfattr() hook (not the related syscall) doesn't need
to pass a "flags" argument to each LSM because there is no use of it.
>
>>
>>
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * The default value of the LSM hook is defined in linux/lsm_hook_defs.h and
>>> * can be accessed with:
On 15/03/2023 23:46, Casey Schaufler wrote:
> Create a system call lsm_get_self_attr() to provide the security
> module maintained attributes of the current process.
> Create a system call lsm_set_self_attr() to set a security
> module maintained attribute of the current process.
> Historically these attributes have been exposed to user space via
> entries in procfs under /proc/self/attr.
>
> The attribute value is provided in a lsm_ctx structure. The structure
> identifys the size of the attribute, and the attribute value. The format
> of the attribute value is defined by the security module. A flags field
> is included for LSM specific information. It is currently unused and must
> be 0. The total size of the data, including the lsm_ctx structure and any
> padding, is maintained as well.
>
> struct lsm_ctx {
> __u64 id;
> __u64 flags;
> __u64 len;
> __u64 ctx_len;
> __u8 ctx[];
> };
>
> Two new LSM hooks are used to interface with the LSMs.
> security_getselfattr() collects the lsm_ctx values from the
> LSMs that support the hook, accounting for space requirements.
> security_setselfattr() identifies which LSM the attribute is
> intended for and passes it along.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> Documentation/userspace-api/lsm.rst | 15 +++++
> include/linux/lsm_hook_defs.h | 4 ++
> include/linux/lsm_hooks.h | 9 +++
> include/linux/security.h | 19 ++++++
> include/linux/syscalls.h | 5 ++
> include/uapi/linux/lsm.h | 33 ++++++++++
> kernel/sys_ni.c | 4 ++
> security/Makefile | 1 +
> security/lsm_syscalls.c | 55 ++++++++++++++++
> security/security.c | 97 +++++++++++++++++++++++++++++
> 10 files changed, 242 insertions(+)
> create mode 100644 security/lsm_syscalls.c
[...]
> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> new file mode 100644
> index 000000000000..feee31600219
> --- /dev/null
> +++ b/security/lsm_syscalls.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * System calls implementing the Linux Security Module API.
> + *
> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
> + * Copyright (C) 2022 Intel Corporation
> + */
> +
> +#include <asm/current.h>
> +#include <linux/compiler_types.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/security.h>
> +#include <linux/stddef.h>
> +#include <linux/syscalls.h>
> +#include <linux/types.h>
> +#include <linux/lsm_hooks.h>
> +#include <uapi/linux/lsm.h>
> +
> +/**
> + * sys_lsm_set_self_attr - Set current task's security module attribute
> + * @attr: which attribute to set
> + * @ctx: the LSM contexts
> + * @size: size of @ctx
> + * @flags: reserved for future use
> + *
> + * Sets the calling task's LSM context. On success this function
> + * returns 0. If the attribute specified cannot be set a negative
> + * value indicating the reason for the error is returned.
Do you think it is really worth it to implement syscalls that can get
and set attributes to several LSMs at the same time, instead of one at a
time? LSM-specific tools don't care about other LSMs. I still think it
would be much simpler (for kernel and user space) to pass an LSM ID to
both syscalls. This would avoid dealing with variable arrays of variable
element lengths, to both get or set attributes.
Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
previously talked about, getting an unknown number of file descriptor
doesn't look good neither.
> + */
> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> + ctx, size_t __user, size, u32, flags)
> +{
> + return security_setselfattr(attr, ctx, size, flags);
> +}
> +
> +/**
> + * sys_lsm_get_self_attr - Return current task's security module attributes
> + * @attr: which attribute to set
attribute to *get*
> + * @ctx: the LSM contexts
> + * @size: size of @ctx, updated on return
I suggest to use a dedicated argument to read the allocated size, and
another to write the actual/written size.
This would not be required with an LSM ID passed to the syscall because
attribute sizes should be known by user space, and there is no need to
help them probe this information.
> + * @flags: reserved for future use
> + *
> + * Returns the calling task's LSM contexts. On success this
> + * function returns the number of @ctx array elements. This value
> + * may be zero if there are no LSM contexts assigned. If @size is
> + * insufficient to contain the return data -E2BIG is returned and
> + * @size is set to the minimum required size.
Doing something (updating a buffer) even when returning an error doesn't
look right. These sizes should be well-known to user space and part of
the ABI/UAPI.
> In all other cases
> + * a negative value indicating the error is returned.
> + */
> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> + ctx, size_t __user *, size, u32, flags)
> +{
> + return security_getselfattr(attr, ctx, size, flags);
> +}
> diff --git a/security/security.c b/security/security.c
> index 87c8796c3c46..2c57fe28c4f7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
> }
> EXPORT_SYMBOL(security_d_instantiate);
>
> +/**
> + * security_getselfattr - Read an LSM attribute of the current process.
> + * @attr: which attribute to return
> + * @ctx: the user-space destination for the information, or NULL
> + * @size: the size of space available to receive the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Returns the number of attributes found on success, negative value
> + * on error. @size is reset to the total size of the data.
> + * If @size is insufficient to contain the data -E2BIG is returned.
> + */
> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> + size_t __user *size, u32 __user flags)
> +{
> + struct security_hook_list *hp;
> + void __user *base = (void *)ctx;
> + size_t total = 0;
> + size_t this;
> + size_t left;
> + bool istoobig = false;
> + int count = 0;
> + int rc;
> +
> + if (attr == 0)
> + return -EINVAL;
> + if (flags != 0)
> + return -EINVAL;
> + if (size == NULL)
> + return -EINVAL;
> + if (get_user(left, size))
> + return -EFAULT;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> + this = left;
> + if (base)
> + ctx = (struct lsm_ctx __user *)(base + total);
> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> + switch (rc) {
> + case -EOPNOTSUPP:
> + rc = 0;
> + continue;
> + case -E2BIG:
> + istoobig = true;
> + left = 0;
> + break;
These two error cases could be directly handled by
security_getselfattr() instead of relying on each LSM-specific
implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
> + case 0:
> + left -= this;
> + break;
> + default:
> + return rc;
> + }
> + total += this;
> + count++;
> + }
> + if (count == 0)
> + return LSM_RET_DEFAULT(getselfattr);
> + if (put_user(total, size))
> + return -EFAULT;
> + if (rc)
> + return rc;
> + if (istoobig)
> + return -E2BIG;
> + return count;
> +}
> +
> +/**
> + * security_setselfattr - Set an LSM attribute on the current process.
> + * @attr: which attribute to set
> + * @ctx: the user-space source for the information
> + * @size: the size of the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Set an LSM attribute for the current process. The LSM, attribute
> + * and new value are included in @ctx.
> + *
> + * Returns 0 on success, an LSM specific value on failure.
> + */
> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> + size_t __user size, u32 __user flags)
> +{
> + struct security_hook_list *hp;
> + struct lsm_ctx lctx;
> +
> + if (flags != 0)
> + return -EINVAL;
> + if (size < sizeof(*ctx))
> + return -EINVAL;
> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> + return -EFAULT;
> +
> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> + if ((hp->lsmid->id) == lctx.id)
> + return hp->hook.setselfattr(attr, ctx, size, flags);
> +
> + return LSM_RET_DEFAULT(setselfattr);
> +}
> +
> int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
> char **value)
> {
It looks like you missed my preview reviews on these patches.
On 15/03/2023 23:46, 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
> output format, determining required privilege or bypassing
> security module specific behavior.
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> Documentation/userspace-api/lsm.rst | 3 +++
> include/linux/syscalls.h | 1 +
> kernel/sys_ni.c | 1 +
> security/lsm_syscalls.c | 39 +++++++++++++++++++++++++++++
> 4 files changed, 44 insertions(+)
>
> diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
> index b45e402302b3..a86e3817f062 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_list_modules
> +
> Additional documentation
> ========================
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 3feca00cb0c1..f755c583f949 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1063,6 +1063,7 @@ asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> size_t *size, __u64 flags);
> asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> __u64 flags);
> +asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> index d03c78ef1562..ceb3d21a62d0 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_list_modules);
>
> /* security/keys/keyctl.c */
> COND_SYSCALL(add_key);
> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> index feee31600219..6efbe244d304 100644
> --- a/security/lsm_syscalls.c
> +++ b/security/lsm_syscalls.c
> @@ -53,3 +53,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> {
> return security_getselfattr(attr, ctx, size, flags);
> }
> +
> +/**
> + * sys_lsm_list_modules - 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_list_modules, u64 __user *, ids, size_t __user *, size,
> + u32, flags)
> +{
> + 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;
> +
> + 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++))
> + return -EFAULT;
> +
> + return lsm_active_cnt;
> +}
On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>
> On 15/03/2023 23:46, Casey Schaufler wrote:
>> Create a system call lsm_get_self_attr() to provide the security
>> module maintained attributes of the current process.
>> Create a system call lsm_set_self_attr() to set a security
>> module maintained attribute of the current process.
>> Historically these attributes have been exposed to user space via
>> entries in procfs under /proc/self/attr.
>>
>> The attribute value is provided in a lsm_ctx structure. The structure
>> identifys the size of the attribute, and the attribute value. The format
>> of the attribute value is defined by the security module. A flags field
>> is included for LSM specific information. It is currently unused and
>> must
>> be 0. The total size of the data, including the lsm_ctx structure and
>> any
>> padding, is maintained as well.
>>
>> struct lsm_ctx {
>> __u64 id;
>> __u64 flags;
>> __u64 len;
>> __u64 ctx_len;
>> __u8 ctx[];
>> };
>>
>> Two new LSM hooks are used to interface with the LSMs.
>> security_getselfattr() collects the lsm_ctx values from the
>> LSMs that support the hook, accounting for space requirements.
>> security_setselfattr() identifies which LSM the attribute is
>> intended for and passes it along.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> Documentation/userspace-api/lsm.rst | 15 +++++
>> include/linux/lsm_hook_defs.h | 4 ++
>> include/linux/lsm_hooks.h | 9 +++
>> include/linux/security.h | 19 ++++++
>> include/linux/syscalls.h | 5 ++
>> include/uapi/linux/lsm.h | 33 ++++++++++
>> kernel/sys_ni.c | 4 ++
>> security/Makefile | 1 +
>> security/lsm_syscalls.c | 55 ++++++++++++++++
>> security/security.c | 97 +++++++++++++++++++++++++++++
>> 10 files changed, 242 insertions(+)
>> create mode 100644 security/lsm_syscalls.c
>
> [...]
>
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> new file mode 100644
>> index 000000000000..feee31600219
>> --- /dev/null
>> +++ b/security/lsm_syscalls.c
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * System calls implementing the Linux Security Module API.
>> + *
>> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
>> + * Copyright (C) 2022 Intel Corporation
>> + */
>> +
>> +#include <asm/current.h>
>> +#include <linux/compiler_types.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/security.h>
>> +#include <linux/stddef.h>
>> +#include <linux/syscalls.h>
>> +#include <linux/types.h>
>> +#include <linux/lsm_hooks.h>
>> +#include <uapi/linux/lsm.h>
>> +
>> +/**
>> + * sys_lsm_set_self_attr - Set current task's security module attribute
>> + * @attr: which attribute to set
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx
>> + * @flags: reserved for future use
>> + *
>> + * Sets the calling task's LSM context. On success this function
>> + * returns 0. If the attribute specified cannot be set a negative
>> + * value indicating the reason for the error is returned.
>
> Do you think it is really worth it to implement syscalls that can get
> and set attributes to several LSMs at the same time, instead of one at
> a time?
Setting the values for more than one LSM is impractical due to the possibility
that the Nth value may fail, and unwinding the N-1 values may not be possible.
> LSM-specific tools don't care about other LSMs.
That's part of the problem. Are systemd, dbusd, ps and id LSM specific tools?
They shouldn't be.
> I still think it would be much simpler (for kernel and user space) to
> pass an LSM ID to both syscalls. This would avoid dealing with
> variable arrays of variable element lengths, to both get or set
> attributes.
ps and id should both work regardless of which and how many LSMs provide
context attributes. They shouldn't need to know which LSMs are active in
advance. If a new LSM is introduced, they shouldn't need to be updated to
support it.
>
> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
> previously talked about, getting an unknown number of file descriptor
> doesn't look good neither.
If you have multiple LSM_ATTR_MAGICFD values and can only get one at
a time you have to do something convoluted with flags to get them all.
I don't see that as a good thing.
>
>
>> + */
>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>> lsm_ctx __user *,
>> + ctx, size_t __user, size, u32, flags)
>> +{
>> + return security_setselfattr(attr, ctx, size, flags);
>> +}
>> +
>> +/**
>> + * sys_lsm_get_self_attr - Return current task's security module
>> attributes
>> + * @attr: which attribute to set
>
> attribute to *get*
>
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx, updated on return
>
> I suggest to use a dedicated argument to read the allocated size, and
> another to write the actual/written size.
>
> This would not be required with an LSM ID passed to the syscall
> because attribute sizes should be known by user space, and there is no
> need to help them probe this information.
>
>
>> + * @flags: reserved for future use
>> + *
>> + * Returns the calling task's LSM contexts. On success this
>> + * function returns the number of @ctx array elements. This value
>> + * may be zero if there are no LSM contexts assigned. If @size is
>> + * insufficient to contain the return data -E2BIG is returned and
>> + * @size is set to the minimum required size.
>
> Doing something (updating a buffer) even when returning an error
> doesn't look right. These sizes should be well-known to user space and
> part of the ABI/UAPI.
No. The size of attributes is not well known to user space.
They are usually text strings. The maximum size will be known,
but that's putting additional burden on user space to know
about all possible LSMs. It's not always necessary.
>
>
>> In all other cases
>> + * a negative value indicating the error is returned.
>> + */
>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>> lsm_ctx __user *,
>> + ctx, size_t __user *, size, u32, flags)
>> +{
>> + return security_getselfattr(attr, ctx, size, flags);
>> +}
>> diff --git a/security/security.c b/security/security.c
>> index 87c8796c3c46..2c57fe28c4f7 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>> *dentry, struct inode *inode)
>> }
>> EXPORT_SYMBOL(security_d_instantiate);
>> +/**
>> + * security_getselfattr - Read an LSM attribute of the current process.
>> + * @attr: which attribute to return
>> + * @ctx: the user-space destination for the information, or NULL
>> + * @size: the size of space available to receive the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Returns the number of attributes found on success, negative value
>> + * on error. @size is reset to the total size of the data.
>> + * If @size is insufficient to contain the data -E2BIG is returned.
>> + */
>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>> __user *ctx,
>> + size_t __user *size, u32 __user flags)
>> +{
>> + struct security_hook_list *hp;
>> + void __user *base = (void *)ctx;
>> + size_t total = 0;
>> + size_t this;
>> + size_t left;
>> + bool istoobig = false;
>> + int count = 0;
>> + int rc;
>> +
>> + if (attr == 0)
>> + return -EINVAL;
>> + if (flags != 0)
>> + return -EINVAL;
>> + if (size == NULL)
>> + return -EINVAL;
>> + if (get_user(left, size))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> + this = left;
>> + if (base)
>> + ctx = (struct lsm_ctx __user *)(base + total);
>> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>> + switch (rc) {
>> + case -EOPNOTSUPP:
>> + rc = 0;
>> + continue;
>> + case -E2BIG:
>> + istoobig = true;
>> + left = 0;
>> + break;
>
> These two error cases could be directly handled by
> security_getselfattr() instead of relying on each LSM-specific
> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
Yes, they could. My understanding is that Paul wants the LSM layer
to be "thin". Where possible and not insane, the logic should be in
the LSM, not the infrastructure.
>
>
>> + case 0:
>> + left -= this;
>> + break;
>> + default:
>> + return rc;
>> + }
>> + total += this;
>> + count++;
>> + }
>> + if (count == 0)
>> + return LSM_RET_DEFAULT(getselfattr);
>> + if (put_user(total, size))
>> + return -EFAULT;
>> + if (rc)
>> + return rc;
>> + if (istoobig)
>> + return -E2BIG;
>> + return count;
>> +}
>> +
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>> __user *ctx,
>> + size_t __user size, u32 __user flags)
>> +{
>> + struct security_hook_list *hp;
>> + struct lsm_ctx lctx;
>> +
>> + if (flags != 0)
>> + return -EINVAL;
>> + if (size < sizeof(*ctx))
>> + return -EINVAL;
>> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> + return -EFAULT;
>> +
>> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> + if ((hp->lsmid->id) == lctx.id)
>> + return hp->hook.setselfattr(attr, ctx, size, flags);
>> +
>> + return LSM_RET_DEFAULT(setselfattr);
>> +}
>> +
>> int security_getprocattr(struct task_struct *p, int lsmid, const
>> char *name,
>> char **value)
>> {
On 4/3/2023 2:47 AM, Mickaël Salaün wrote:
>
> On 15/03/2023 23:47, Casey Schaufler wrote:
>> Add lsm_name_to_attr(), which translates a text string to a
>> LSM_ATTR value if one is available.
>>
>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>> the trailing attribute value.
>>
>> All are used in module specific components of LSM system calls.
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> include/linux/security.h | 13 ++++++++++
>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>> security/security.c | 31 ++++++++++++++++++++++++
>> 3 files changed, 95 insertions(+)
>
> [...]
>
>> diff --git a/security/security.c b/security/security.c
>> index 2c57fe28c4f7..f7b814a3940c 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct
>> super_block *sb)
>> return 0;
>> }
>> +/**
>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>> + * @ctx: an LSM context to be filled
>> + * @context: the new context value
>> + * @context_size: the size of the new context value
>> + * @id: LSM id
>> + * @flags: LSM defined flags
>> + *
>> + * Fill all of the fields in a user space lsm_ctx structure.
>> + * Caller is assumed to have verified that @ctx has enough space
>> + * for @context.
>> + * Returns 0 on success, -EFAULT on a copyout error.
>> + */
>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>> + size_t context_size, u64 id, u64 flags)
>> +{
>> + struct lsm_ctx local;
>> + void __user *vc = ctx;
>> +
>> + local.id = id;
>> + local.flags = flags;
>> + local.ctx_len = context_size;
>> + local.len = context_size + sizeof(local);
>> + vc += sizeof(local);
>> + if (copy_to_user(ctx, &local, sizeof(local)))
>> + return -EFAULT;
>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>> + return -EFAULT;
>
> Can we do a single copy_to_user() call?
It would be possible, but would require allocating memory and copying
the context. I don't see that as an improvement.
> That would avoid inconsistent user space data, could speed up a bit
> the operation, and make the code easier to understand. To use the
> stack, we need to know the maximum size of context_size for all use
> cases, which seems reasonable and can be checked at build time (on
> each LSM side, and potentially with specific context type passed as
> enum instead of context_size) and run time (for this generic helper).
Knowning the maximum size of attributes for all LSMs and hard coding
that here would make maintaining this code really painful.
>
>
>> + return 0;
>> +}
>> +
>> /*
>> * The default value of the LSM hook is defined in
>> linux/lsm_hook_defs.h and
>> * can be accessed with:
On 03/04/2023 19:36, Casey Schaufler wrote:
> On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:46, Casey Schaufler wrote:
>>> Create a system call lsm_get_self_attr() to provide the security
>>> module maintained attributes of the current process.
>>> Create a system call lsm_set_self_attr() to set a security
>>> module maintained attribute of the current process.
>>> Historically these attributes have been exposed to user space via
>>> entries in procfs under /proc/self/attr.
>>>
>>> The attribute value is provided in a lsm_ctx structure. The structure
>>> identifys the size of the attribute, and the attribute value. The format
>>> of the attribute value is defined by the security module. A flags field
>>> is included for LSM specific information. It is currently unused and
>>> must
>>> be 0. The total size of the data, including the lsm_ctx structure and
>>> any
>>> padding, is maintained as well.
>>>
>>> struct lsm_ctx {
>>> __u64 id;
>>> __u64 flags;
>>> __u64 len;
>>> __u64 ctx_len;
>>> __u8 ctx[];
>>> };
>>>
>>> Two new LSM hooks are used to interface with the LSMs.
>>> security_getselfattr() collects the lsm_ctx values from the
>>> LSMs that support the hook, accounting for space requirements.
>>> security_setselfattr() identifies which LSM the attribute is
>>> intended for and passes it along.
>>>
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>> ---
>>> Documentation/userspace-api/lsm.rst | 15 +++++
>>> include/linux/lsm_hook_defs.h | 4 ++
>>> include/linux/lsm_hooks.h | 9 +++
>>> include/linux/security.h | 19 ++++++
>>> include/linux/syscalls.h | 5 ++
>>> include/uapi/linux/lsm.h | 33 ++++++++++
>>> kernel/sys_ni.c | 4 ++
>>> security/Makefile | 1 +
>>> security/lsm_syscalls.c | 55 ++++++++++++++++
>>> security/security.c | 97 +++++++++++++++++++++++++++++
>>> 10 files changed, 242 insertions(+)
>>> create mode 100644 security/lsm_syscalls.c
>>
>> [...]
>>
>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>> new file mode 100644
>>> index 000000000000..feee31600219
>>> --- /dev/null
>>> +++ b/security/lsm_syscalls.c
>>> @@ -0,0 +1,55 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * System calls implementing the Linux Security Module API.
>>> + *
>>> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
>>> + * Copyright (C) 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <asm/current.h>
>>> +#include <linux/compiler_types.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/security.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/syscalls.h>
>>> +#include <linux/types.h>
>>> +#include <linux/lsm_hooks.h>
>>> +#include <uapi/linux/lsm.h>
>>> +
>>> +/**
>>> + * sys_lsm_set_self_attr - Set current task's security module attribute
>>> + * @attr: which attribute to set
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx
>>> + * @flags: reserved for future use
>>> + *
>>> + * Sets the calling task's LSM context. On success this function
>>> + * returns 0. If the attribute specified cannot be set a negative
>>> + * value indicating the reason for the error is returned.
>>
>> Do you think it is really worth it to implement syscalls that can get
>> and set attributes to several LSMs at the same time, instead of one at
>> a time?
>
> Setting the values for more than one LSM is impractical due to the possibility
> that the Nth value may fail, and unwinding the N-1 values may not be possible.
Indeed, so unless I missed something, why not passing the LSM ID as a
syscall argument for lsm_set_self_attr() and lsm_get_self_attr(), and
avoid managing a set of contexts but instead only managing one context
at a time (to get or set)?
>
>> LSM-specific tools don't care about other LSMs.
>
> That's part of the problem. Are systemd, dbusd, ps and id LSM specific tools?
> They shouldn't be.
>
>> I still think it would be much simpler (for kernel and user space) to
>> pass an LSM ID to both syscalls. This would avoid dealing with
>> variable arrays of variable element lengths, to both get or set
>> attributes.
>
> ps and id should both work regardless of which and how many LSMs provide
> context attributes. They shouldn't need to know which LSMs are active in
> advance. If a new LSM is introduced, they shouldn't need to be updated to
> support it.
I agree, and making the syscalls simpler doesn't change that.
>
>>
>> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
>> previously talked about, getting an unknown number of file descriptor
>> doesn't look good neither.
>
> If you have multiple LSM_ATTR_MAGICFD values and can only get one at
> a time you have to do something convoluted with flags to get them all.
> I don't see that as a good thing.
Yes, that was another argument to *not* deal with a set of contexts.
>
>>
>>
>>> + */
>>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> + ctx, size_t __user, size, u32, flags)
>>> +{
>>> + return security_setselfattr(attr, ctx, size, flags);
>>> +}
>>> +
>>> +/**
>>> + * sys_lsm_get_self_attr - Return current task's security module
>>> attributes
>>> + * @attr: which attribute to set
>>
>> attribute to *get*
>>
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx, updated on return
>>
>> I suggest to use a dedicated argument to read the allocated size, and
>> another to write the actual/written size.
>>
>> This would not be required with an LSM ID passed to the syscall
>> because attribute sizes should be known by user space, and there is no
>> need to help them probe this information.
>>
>>
>>> + * @flags: reserved for future use
>>> + *
>>> + * Returns the calling task's LSM contexts. On success this
>>> + * function returns the number of @ctx array elements. This value
>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>> + * insufficient to contain the return data -E2BIG is returned and
>>> + * @size is set to the minimum required size.
>>
>> Doing something (updating a buffer) even when returning an error
>> doesn't look right. These sizes should be well-known to user space and
>> part of the ABI/UAPI.
>
> No. The size of attributes is not well known to user space.
> They are usually text strings. The maximum size will be known,
> but that's putting additional burden on user space to know
> about all possible LSMs. It's not always necessary.
Right, I forgot the strings stuff… The lsm_get_self_attr() syscall could
then return a ctx_actual_size (as one argument), and a ctx pointer (as
another argument). Similarly, the lsm_set_self_attr() syscall could use
a dedicated argument for ctx_size and another for the ctx pointer.
>
>>
>>
>>> In all other cases
>>> + * a negative value indicating the error is returned.
>>> + */
>>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> + ctx, size_t __user *, size, u32, flags)
>>> +{
>>> + return security_getselfattr(attr, ctx, size, flags);
>>> +}
>>> diff --git a/security/security.c b/security/security.c
>>> index 87c8796c3c46..2c57fe28c4f7 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>>> *dentry, struct inode *inode)
>>> }
>>> EXPORT_SYMBOL(security_d_instantiate);
>>> +/**
>>> + * security_getselfattr - Read an LSM attribute of the current process.
>>> + * @attr: which attribute to return
>>> + * @ctx: the user-space destination for the information, or NULL
>>> + * @size: the size of space available to receive the data
>>> + * @flags: reserved for future use, must be 0
>>> + *
>>> + * Returns the number of attributes found on success, negative value
>>> + * on error. @size is reset to the total size of the data.
>>> + * If @size is insufficient to contain the data -E2BIG is returned.
>>> + */
>>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>>> __user *ctx,
>>> + size_t __user *size, u32 __user flags)
>>> +{
>>> + struct security_hook_list *hp;
>>> + void __user *base = (void *)ctx;
>>> + size_t total = 0;
>>> + size_t this;
>>> + size_t left;
>>> + bool istoobig = false;
>>> + int count = 0;
>>> + int rc;
>>> +
>>> + if (attr == 0)
>>> + return -EINVAL;
>>> + if (flags != 0)
>>> + return -EINVAL;
>>> + if (size == NULL)
>>> + return -EINVAL;
>>> + if (get_user(left, size))
>>> + return -EFAULT;
>>> +
>>> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>>> + this = left;
>>> + if (base)
>>> + ctx = (struct lsm_ctx __user *)(base + total);
>>> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>>> + switch (rc) {
>>> + case -EOPNOTSUPP:
>>> + rc = 0;
>>> + continue;
>>> + case -E2BIG:
>>> + istoobig = true;
>>> + left = 0;
>>> + break;
>>
>> These two error cases could be directly handled by
>> security_getselfattr() instead of relying on each LSM-specific
>> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
>
> Yes, they could. My understanding is that Paul wants the LSM layer
> to be "thin". Where possible and not insane, the logic should be in
> the LSM, not the infrastructure.
FWIW, since we are defining new syscalls to make user space's life
easier, I'm in favor of a well defined common behavior (e.g. returned
errno) and factoring common code to make each LSM-specific code thin.
>
>>
>>
>>> + case 0:
>>> + left -= this;
>>> + break;
>>> + default:
>>> + return rc;
>>> + }
>>> + total += this;
>>> + count++;
>>> + }
>>> + if (count == 0)
>>> + return LSM_RET_DEFAULT(getselfattr);
>>> + if (put_user(total, size))
>>> + return -EFAULT;
>>> + if (rc)
>>> + return rc;
>>> + if (istoobig)
>>> + return -E2BIG;
>>> + return count;
>>> +}
>>> +
>>> +/**
>>> + * security_setselfattr - Set an LSM attribute on the current process.
>>> + * @attr: which attribute to set
>>> + * @ctx: the user-space source for the information
>>> + * @size: the size of the data
>>> + * @flags: reserved for future use, must be 0
>>> + *
>>> + * Set an LSM attribute for the current process. The LSM, attribute
>>> + * and new value are included in @ctx.
>>> + *
>>> + * Returns 0 on success, an LSM specific value on failure.
>>> + */
>>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>>> __user *ctx,
>>> + size_t __user size, u32 __user flags)
>>> +{
>>> + struct security_hook_list *hp;
>>> + struct lsm_ctx lctx;
>>> +
>>> + if (flags != 0)
>>> + return -EINVAL;
>>> + if (size < sizeof(*ctx))
>>> + return -EINVAL;
>>> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>>> + return -EFAULT;
>>> +
>>> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>>> + if ((hp->lsmid->id) == lctx.id)
>>> + return hp->hook.setselfattr(attr, ctx, size, flags);
>>> +
>>> + return LSM_RET_DEFAULT(setselfattr);
>>> +}
>>> +
>>> int security_getprocattr(struct task_struct *p, int lsmid, const
>>> char *name,
>>> char **value)
>>> {
On 4/3/2023 2:54 AM, Mickaël Salaün wrote:
>
> On 03/04/2023 11:47, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:47, Casey Schaufler wrote:
>>> Add lsm_name_to_attr(), which translates a text string to a
>>> LSM_ATTR value if one is available.
>>>
>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>> the trailing attribute value.
>>>
>>> All are used in module specific components of LSM system calls.
>>>
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>> ---
>>> include/linux/security.h | 13 ++++++++++
>>> security/lsm_syscalls.c | 51
>>> ++++++++++++++++++++++++++++++++++++++++
>>> security/security.c | 31 ++++++++++++++++++++++++
>>> 3 files changed, 95 insertions(+)
>>
>> [...]
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index 2c57fe28c4f7..f7b814a3940c 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct
>>> super_block *sb)
>>> return 0;
>>> }
>>> +/**
>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>> + * @ctx: an LSM context to be filled
>>> + * @context: the new context value
>>> + * @context_size: the size of the new context value
>>> + * @id: LSM id
>>> + * @flags: LSM defined flags
>>> + *
>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>> + * Caller is assumed to have verified that @ctx has enough space
>>> + * for @context.
>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>> + */
>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>> + size_t context_size, u64 id, u64 flags)
>>> +{
>>> + struct lsm_ctx local;
>>> + void __user *vc = ctx;
>>> +
>>> + local.id = id;
>>> + local.flags = flags;
>>> + local.ctx_len = context_size;
>>> + local.len = context_size + sizeof(local);
>>> + vc += sizeof(local);
>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>> + return -EFAULT;
>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>> + return -EFAULT;
>>
>> Can we do a single copy_to_user() call? That would avoid inconsistent
>> user space data, could speed up a bit the operation, and make the code
>> easier to understand. To use the stack, we need to know the maximum size
>> of context_size for all use cases, which seems reasonable and can be
>> checked at build time (on each LSM side, and potentially with specific
>> context type passed as enum instead of context_size) and run time (for
>> this generic helper).
>
> Well, actually the context_size should be inferred from id, and the
> "local" size should be defined and check at build time against all
> context ID sizes.
Again, no, I don't see this as an improvement.
>
>>
>>
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * The default value of the LSM hook is defined in
>>> linux/lsm_hook_defs.h and
>>> * can be accessed with:
On 03/04/2023 20:03, Casey Schaufler wrote:
> On 4/3/2023 2:47 AM, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:47, Casey Schaufler wrote:
>>> Add lsm_name_to_attr(), which translates a text string to a
>>> LSM_ATTR value if one is available.
>>>
>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>> the trailing attribute value.
>>>
>>> All are used in module specific components of LSM system calls.
>>>
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>> ---
>>> include/linux/security.h | 13 ++++++++++
>>> security/lsm_syscalls.c | 51 ++++++++++++++++++++++++++++++++++++++++
>>> security/security.c | 31 ++++++++++++++++++++++++
>>> 3 files changed, 95 insertions(+)
>>
>> [...]
>>
>>> diff --git a/security/security.c b/security/security.c
>>> index 2c57fe28c4f7..f7b814a3940c 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct
>>> super_block *sb)
>>> return 0;
>>> }
>>> +/**
>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>> + * @ctx: an LSM context to be filled
>>> + * @context: the new context value
>>> + * @context_size: the size of the new context value
>>> + * @id: LSM id
>>> + * @flags: LSM defined flags
>>> + *
>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>> + * Caller is assumed to have verified that @ctx has enough space
>>> + * for @context.
>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>> + */
>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>> + size_t context_size, u64 id, u64 flags)
>>> +{
>>> + struct lsm_ctx local;
>>> + void __user *vc = ctx;
>>> +
>>> + local.id = id;
>>> + local.flags = flags;
>>> + local.ctx_len = context_size;
>>> + local.len = context_size + sizeof(local);
>>> + vc += sizeof(local);
>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>> + return -EFAULT;
>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>> + return -EFAULT;
>>
>> Can we do a single copy_to_user() call?
>
> It would be possible, but would require allocating memory and copying
> the context. I don't see that as an improvement.
>
>> That would avoid inconsistent user space data, could speed up a bit
>> the operation, and make the code easier to understand. To use the
>> stack, we need to know the maximum size of context_size for all use
>> cases, which seems reasonable and can be checked at build time (on
>> each LSM side, and potentially with specific context type passed as
>> enum instead of context_size) and run time (for this generic helper).
>
> Knowning the maximum size of attributes for all LSMs and hard coding
> that here would make maintaining this code really painful.
Hmm, I forgot about variable-length strings, but maybe a reasonable
common maximum size (that could fit on the stack) could be found?
>
>>
>>
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * The default value of the LSM hook is defined in
>>> linux/lsm_hook_defs.h and
>>> * can be accessed with:
On 4/3/2023 11:04 AM, Mickaël Salaün wrote:
>
> On 03/04/2023 19:36, Casey Schaufler wrote:
>> On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>>>
>>> On 15/03/2023 23:46, Casey Schaufler wrote:
>>>> Create a system call lsm_get_self_attr() to provide the security
>>>> module maintained attributes of the current process.
>>>> Create a system call lsm_set_self_attr() to set a security
>>>> module maintained attribute of the current process.
>>>> Historically these attributes have been exposed to user space via
>>>> entries in procfs under /proc/self/attr.
>>>>
>>>> The attribute value is provided in a lsm_ctx structure. The structure
>>>> identifys the size of the attribute, and the attribute value. The
>>>> format
>>>> of the attribute value is defined by the security module. A flags
>>>> field
>>>> is included for LSM specific information. It is currently unused and
>>>> must
>>>> be 0. The total size of the data, including the lsm_ctx structure and
>>>> any
>>>> padding, is maintained as well.
>>>>
>>>> struct lsm_ctx {
>>>> __u64 id;
>>>> __u64 flags;
>>>> __u64 len;
>>>> __u64 ctx_len;
>>>> __u8 ctx[];
>>>> };
>>>>
>>>> Two new LSM hooks are used to interface with the LSMs.
>>>> security_getselfattr() collects the lsm_ctx values from the
>>>> LSMs that support the hook, accounting for space requirements.
>>>> security_setselfattr() identifies which LSM the attribute is
>>>> intended for and passes it along.
>>>>
>>>> Signed-off-by: Casey Schaufler <[email protected]>
>>>> ---
>>>> Documentation/userspace-api/lsm.rst | 15 +++++
>>>> include/linux/lsm_hook_defs.h | 4 ++
>>>> include/linux/lsm_hooks.h | 9 +++
>>>> include/linux/security.h | 19 ++++++
>>>> include/linux/syscalls.h | 5 ++
>>>> include/uapi/linux/lsm.h | 33 ++++++++++
>>>> kernel/sys_ni.c | 4 ++
>>>> security/Makefile | 1 +
>>>> security/lsm_syscalls.c | 55 ++++++++++++++++
>>>> security/security.c | 97
>>>> +++++++++++++++++++++++++++++
>>>> 10 files changed, 242 insertions(+)
>>>> create mode 100644 security/lsm_syscalls.c
>>>
>>> [...]
>>>
>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>> new file mode 100644
>>>> index 000000000000..feee31600219
>>>> --- /dev/null
>>>> +++ b/security/lsm_syscalls.c
>>>> @@ -0,0 +1,55 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * System calls implementing the Linux Security Module API.
>>>> + *
>>>> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
>>>> + * Copyright (C) 2022 Intel Corporation
>>>> + */
>>>> +
>>>> +#include <asm/current.h>
>>>> +#include <linux/compiler_types.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/security.h>
>>>> +#include <linux/stddef.h>
>>>> +#include <linux/syscalls.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/lsm_hooks.h>
>>>> +#include <uapi/linux/lsm.h>
>>>> +
>>>> +/**
>>>> + * sys_lsm_set_self_attr - Set current task's security module
>>>> attribute
>>>> + * @attr: which attribute to set
>>>> + * @ctx: the LSM contexts
>>>> + * @size: size of @ctx
>>>> + * @flags: reserved for future use
>>>> + *
>>>> + * Sets the calling task's LSM context. On success this function
>>>> + * returns 0. If the attribute specified cannot be set a negative
>>>> + * value indicating the reason for the error is returned.
>>>
>>> Do you think it is really worth it to implement syscalls that can get
>>> and set attributes to several LSMs at the same time, instead of one at
>>> a time?
>>
>> Setting the values for more than one LSM is impractical due to the
>> possibility
>> that the Nth value may fail, and unwinding the N-1 values may not be
>> possible.
>
> Indeed, so unless I missed something, why not passing the LSM ID as a
> syscall argument for lsm_set_self_attr() and lsm_get_self_attr(), and
> avoid managing a set of contexts but instead only managing one context
> at a time (to get or set)?
The LSM ID is already in the lsm_attr being passed. An additional argument
would be redundant and introduce a potential error when the two values don't
match.
>
>
>>
>>> LSM-specific tools don't care about other LSMs.
>>
>> That's part of the problem. Are systemd, dbusd, ps and id LSM
>> specific tools?
>> They shouldn't be.
>>
>>> I still think it would be much simpler (for kernel and user space) to
>>> pass an LSM ID to both syscalls. This would avoid dealing with
>>> variable arrays of variable element lengths, to both get or set
>>> attributes.
>>
>> ps and id should both work regardless of which and how many LSMs provide
>> context attributes. They shouldn't need to know which LSMs are active in
>> advance. If a new LSM is introduced, they shouldn't need to be
>> updated to
>> support it.
>
> I agree, and making the syscalls simpler doesn't change that.
>
>>
>>>
>>> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
>>> previously talked about, getting an unknown number of file descriptor
>>> doesn't look good neither.
>>
>> If you have multiple LSM_ATTR_MAGICFD values and can only get one at
>> a time you have to do something convoluted with flags to get them all.
>> I don't see that as a good thing.
>
> Yes, that was another argument to *not* deal with a set of contexts.
User space is going to have to deal with multiple values somehow,
either by fetching each possible value independently or by getting
them all at once in a set. Neither is pretty.
>
>>
>>>
>>>
>>>> + */
>>>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>>>> lsm_ctx __user *,
>>>> + ctx, size_t __user, size, u32, flags)
>>>> +{
>>>> + return security_setselfattr(attr, ctx, size, flags);
>>>> +}
>>>> +
>>>> +/**
>>>> + * sys_lsm_get_self_attr - Return current task's security module
>>>> attributes
>>>> + * @attr: which attribute to set
>>>
>>> attribute to *get*
>>>
>>>> + * @ctx: the LSM contexts
>>>> + * @size: size of @ctx, updated on return
>>>
>>> I suggest to use a dedicated argument to read the allocated size, and
>>> another to write the actual/written size.
>>>
>>> This would not be required with an LSM ID passed to the syscall
>>> because attribute sizes should be known by user space, and there is no
>>> need to help them probe this information.
>>>
>>>
>>>> + * @flags: reserved for future use
>>>> + *
>>>> + * Returns the calling task's LSM contexts. On success this
>>>> + * function returns the number of @ctx array elements. This value
>>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>>> + * insufficient to contain the return data -E2BIG is returned and
>>>> + * @size is set to the minimum required size.
>>>
>>> Doing something (updating a buffer) even when returning an error
>>> doesn't look right. These sizes should be well-known to user space and
>>> part of the ABI/UAPI.
>>
>> No. The size of attributes is not well known to user space.
>> They are usually text strings. The maximum size will be known,
>> but that's putting additional burden on user space to know
>> about all possible LSMs. It's not always necessary.
>
> Right, I forgot the strings stuff… The lsm_get_self_attr() syscall
> could then return a ctx_actual_size (as one argument), and a ctx
> pointer (as another argument). Similarly, the lsm_set_self_attr()
> syscall could use a dedicated argument for ctx_size and another for
> the ctx pointer.
That does not meet the design requirement. Paul wants a lsm_attr structure.
I'm not going to deviate from that.
>
>>
>>>
>>>
>>>> In all other cases
>>>> + * a negative value indicating the error is returned.
>>>> + */
>>>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>>>> lsm_ctx __user *,
>>>> + ctx, size_t __user *, size, u32, flags)
>>>> +{
>>>> + return security_getselfattr(attr, ctx, size, flags);
>>>> +}
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 87c8796c3c46..2c57fe28c4f7 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>>>> *dentry, struct inode *inode)
>>>> }
>>>> EXPORT_SYMBOL(security_d_instantiate);
>>>> +/**
>>>> + * security_getselfattr - Read an LSM attribute of the current
>>>> process.
>>>> + * @attr: which attribute to return
>>>> + * @ctx: the user-space destination for the information, or NULL
>>>> + * @size: the size of space available to receive the data
>>>> + * @flags: reserved for future use, must be 0
>>>> + *
>>>> + * Returns the number of attributes found on success, negative value
>>>> + * on error. @size is reset to the total size of the data.
>>>> + * If @size is insufficient to contain the data -E2BIG is returned.
>>>> + */
>>>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>>>> __user *ctx,
>>>> + size_t __user *size, u32 __user flags)
>>>> +{
>>>> + struct security_hook_list *hp;
>>>> + void __user *base = (void *)ctx;
>>>> + size_t total = 0;
>>>> + size_t this;
>>>> + size_t left;
>>>> + bool istoobig = false;
>>>> + int count = 0;
>>>> + int rc;
>>>> +
>>>> + if (attr == 0)
>>>> + return -EINVAL;
>>>> + if (flags != 0)
>>>> + return -EINVAL;
>>>> + if (size == NULL)
>>>> + return -EINVAL;
>>>> + if (get_user(left, size))
>>>> + return -EFAULT;
>>>> +
>>>> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr,
>>>> list) {
>>>> + this = left;
>>>> + if (base)
>>>> + ctx = (struct lsm_ctx __user *)(base + total);
>>>> + rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>>>> + switch (rc) {
>>>> + case -EOPNOTSUPP:
>>>> + rc = 0;
>>>> + continue;
>>>> + case -E2BIG:
>>>> + istoobig = true;
>>>> + left = 0;
>>>> + break;
>>>
>>> These two error cases could be directly handled by
>>> security_getselfattr() instead of relying on each LSM-specific
>>> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
>>
>> Yes, they could. My understanding is that Paul wants the LSM layer
>> to be "thin". Where possible and not insane, the logic should be in
>> the LSM, not the infrastructure.
>
> FWIW, since we are defining new syscalls to make user space's life
> easier, I'm in favor of a well defined common behavior (e.g. returned
> errno) and factoring common code to make each LSM-specific code thin.
I appreciate the viewpoint. It's not what I understand the maintainer wants.
>
>>
>>>
>>>
>>>> + case 0:
>>>> + left -= this;
>>>> + break;
>>>> + default:
>>>> + return rc;
>>>> + }
>>>> + total += this;
>>>> + count++;
>>>> + }
>>>> + if (count == 0)
>>>> + return LSM_RET_DEFAULT(getselfattr);
>>>> + if (put_user(total, size))
>>>> + return -EFAULT;
>>>> + if (rc)
>>>> + return rc;
>>>> + if (istoobig)
>>>> + return -E2BIG;
>>>> + return count;
>>>> +}
>>>> +
>>>> +/**
>>>> + * security_setselfattr - Set an LSM attribute on the current
>>>> process.
>>>> + * @attr: which attribute to set
>>>> + * @ctx: the user-space source for the information
>>>> + * @size: the size of the data
>>>> + * @flags: reserved for future use, must be 0
>>>> + *
>>>> + * Set an LSM attribute for the current process. The LSM, attribute
>>>> + * and new value are included in @ctx.
>>>> + *
>>>> + * Returns 0 on success, an LSM specific value on failure.
>>>> + */
>>>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>>>> __user *ctx,
>>>> + size_t __user size, u32 __user flags)
>>>> +{
>>>> + struct security_hook_list *hp;
>>>> + struct lsm_ctx lctx;
>>>> +
>>>> + if (flags != 0)
>>>> + return -EINVAL;
>>>> + if (size < sizeof(*ctx))
>>>> + return -EINVAL;
>>>> + if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>>>> + return -EFAULT;
>>>> +
>>>> + hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>>>> + if ((hp->lsmid->id) == lctx.id)
>>>> + return hp->hook.setselfattr(attr, ctx, size, flags);
>>>> +
>>>> + return LSM_RET_DEFAULT(setselfattr);
>>>> +}
>>>> +
>>>> int security_getprocattr(struct task_struct *p, int lsmid, const
>>>> char *name,
>>>> char **value)
>>>> {
On 4/3/2023 11:06 AM, Mickaël Salaün wrote:
>
> On 03/04/2023 20:03, Casey Schaufler wrote:
>> On 4/3/2023 2:47 AM, Mickaël Salaün wrote:
>>>
>>> On 15/03/2023 23:47, Casey Schaufler wrote:
>>>> Add lsm_name_to_attr(), which translates a text string to a
>>>> LSM_ATTR value if one is available.
>>>>
>>>> Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
>>>> the trailing attribute value.
>>>>
>>>> All are used in module specific components of LSM system calls.
>>>>
>>>> Signed-off-by: Casey Schaufler <[email protected]>
>>>> ---
>>>> include/linux/security.h | 13 ++++++++++
>>>> security/lsm_syscalls.c | 51
>>>> ++++++++++++++++++++++++++++++++++++++++
>>>> security/security.c | 31 ++++++++++++++++++++++++
>>>> 3 files changed, 95 insertions(+)
>>>
>>> [...]
>>>
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 2c57fe28c4f7..f7b814a3940c 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -753,6 +753,37 @@ static int lsm_superblock_alloc(struct
>>>> super_block *sb)
>>>> return 0;
>>>> }
>>>> +/**
>>>> + * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
>>>> + * @ctx: an LSM context to be filled
>>>> + * @context: the new context value
>>>> + * @context_size: the size of the new context value
>>>> + * @id: LSM id
>>>> + * @flags: LSM defined flags
>>>> + *
>>>> + * Fill all of the fields in a user space lsm_ctx structure.
>>>> + * Caller is assumed to have verified that @ctx has enough space
>>>> + * for @context.
>>>> + * Returns 0 on success, -EFAULT on a copyout error.
>>>> + */
>>>> +int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
>>>> + size_t context_size, u64 id, u64 flags)
>>>> +{
>>>> + struct lsm_ctx local;
>>>> + void __user *vc = ctx;
>>>> +
>>>> + local.id = id;
>>>> + local.flags = flags;
>>>> + local.ctx_len = context_size;
>>>> + local.len = context_size + sizeof(local);
>>>> + vc += sizeof(local);
>>>> + if (copy_to_user(ctx, &local, sizeof(local)))
>>>> + return -EFAULT;
>>>> + if (context_size > 0 && copy_to_user(vc, context, context_size))
>>>> + return -EFAULT;
>>>
>>> Can we do a single copy_to_user() call?
>>
>> It would be possible, but would require allocating memory and copying
>> the context. I don't see that as an improvement.
>>
>>> That would avoid inconsistent user space data, could speed up a bit
>>> the operation, and make the code easier to understand. To use the
>>> stack, we need to know the maximum size of context_size for all use
>>> cases, which seems reasonable and can be checked at build time (on
>>> each LSM side, and potentially with specific context type passed as
>>> enum instead of context_size) and run time (for this generic helper).
>>
>> Knowning the maximum size of attributes for all LSMs and hard coding
>> that here would make maintaining this code really painful.
>
> Hmm, I forgot about variable-length strings, but maybe a reasonable
> common maximum size (that could fit on the stack) could be found?
Putting a maximum size limit on LSM attributes just to reduce the
number of copy_to_user() calls in this helper function does not make
a whole lot of sense to me.
>
>>
>>>
>>>
>>>> + return 0;
>>>> +}
>>>> +
>>>> /*
>>>> * The default value of the LSM hook is defined in
>>>> linux/lsm_hook_defs.h and
>>>> * can be accessed with:
On Mon, Apr 3, 2023 at 8:04 AM Mickaël Salaün <[email protected]> wrote:
>
> It looks like you missed my preview reviews on these patches.
For reference, I believe this is Mickaël's review of the associated v6 patch:
https://lore.kernel.org/linux-security-module/[email protected]/
> On 15/03/2023 23:46, 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
> > output format, determining required privilege or bypassing
> > security module specific behavior.
> >
> > Signed-off-by: Casey Schaufler <[email protected]>
> > ---
> > Documentation/userspace-api/lsm.rst | 3 +++
> > include/linux/syscalls.h | 1 +
> > kernel/sys_ni.c | 1 +
> > security/lsm_syscalls.c | 39 +++++++++++++++++++++++++++++
> > 4 files changed, 44 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
> > index b45e402302b3..a86e3817f062 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_list_modules
> > +
> > Additional documentation
> > ========================
> >
> > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > index 3feca00cb0c1..f755c583f949 100644
> > --- a/include/linux/syscalls.h
> > +++ b/include/linux/syscalls.h
> > @@ -1063,6 +1063,7 @@ asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > size_t *size, __u64 flags);
> > asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > __u64 flags);
> > +asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
> >
> > /*
> > * Architecture-specific system calls
> > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > index d03c78ef1562..ceb3d21a62d0 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_list_modules);
> >
> > /* security/keys/keyctl.c */
> > COND_SYSCALL(add_key);
> > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> > index feee31600219..6efbe244d304 100644
> > --- a/security/lsm_syscalls.c
> > +++ b/security/lsm_syscalls.c
> > @@ -53,3 +53,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > {
> > return security_getselfattr(attr, ctx, size, flags);
> > }
> > +
> > +/**
> > + * sys_lsm_list_modules - 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_list_modules, u64 __user *, ids, size_t __user *, size,
> > + u32, flags)
> > +{
> > + 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;
> > +
> > + 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++))
> > + return -EFAULT;
> > +
> > + return lsm_active_cnt;
> > +}
--
paul-moore.com
On Mon, Apr 10, 2023 at 7:37 PM Paul Moore <[email protected]> wrote:
>
> On Mon, Apr 3, 2023 at 8:04 AM Mickaël Salaün <[email protected]> wrote:
> >
> > It looks like you missed my preview reviews on these patches.
>
> For reference, I believe this is Mickaël's review of the associated v6 patch:
>
> https://lore.kernel.org/linux-security-module/[email protected]/
My apologies, I hit send too soon ... Mickaël, if there are a specific
points you feel have not been addressed, but should be, it would be
helpful if you could list them in this thread.
> > On 15/03/2023 23:46, 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
> > > output format, determining required privilege or bypassing
> > > security module specific behavior.
> > >
> > > Signed-off-by: Casey Schaufler <[email protected]>
> > > ---
> > > Documentation/userspace-api/lsm.rst | 3 +++
> > > include/linux/syscalls.h | 1 +
> > > kernel/sys_ni.c | 1 +
> > > security/lsm_syscalls.c | 39 +++++++++++++++++++++++++++++
> > > 4 files changed, 44 insertions(+)
> > >
> > > diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
> > > index b45e402302b3..a86e3817f062 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_list_modules
> > > +
> > > Additional documentation
> > > ========================
> > >
> > > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> > > index 3feca00cb0c1..f755c583f949 100644
> > > --- a/include/linux/syscalls.h
> > > +++ b/include/linux/syscalls.h
> > > @@ -1063,6 +1063,7 @@ asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > > size_t *size, __u64 flags);
> > > asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> > > __u64 flags);
> > > +asmlinkage long sys_lsm_list_modules(u64 *ids, size_t *size, u32 flags);
> > >
> > > /*
> > > * Architecture-specific system calls
> > > diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
> > > index d03c78ef1562..ceb3d21a62d0 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_list_modules);
> > >
> > > /* security/keys/keyctl.c */
> > > COND_SYSCALL(add_key);
> > > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> > > index feee31600219..6efbe244d304 100644
> > > --- a/security/lsm_syscalls.c
> > > +++ b/security/lsm_syscalls.c
> > > @@ -53,3 +53,42 @@ SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > > {
> > > return security_getselfattr(attr, ctx, size, flags);
> > > }
> > > +
> > > +/**
> > > + * sys_lsm_list_modules - 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_list_modules, u64 __user *, ids, size_t __user *, size,
> > > + u32, flags)
> > > +{
> > > + 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;
> > > +
> > > + 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++))
> > > + return -EFAULT;
> > > +
> > > + return lsm_active_cnt;
> > > +}
>
> --
> paul-moore.com
--
paul-moore.com
On Mon, Apr 3, 2023 at 8:04 AM Mickaël Salaün <[email protected]> wrote:
> On 15/03/2023 23:46, Casey Schaufler wrote:
> > Create a system call lsm_get_self_attr() to provide the security
> > module maintained attributes of the current process.
> > Create a system call lsm_set_self_attr() to set a security
> > module maintained attribute of the current process.
> > Historically these attributes have been exposed to user space via
> > entries in procfs under /proc/self/attr.
> >
> > The attribute value is provided in a lsm_ctx structure. The structure
> > identifys the size of the attribute, and the attribute value. The format
> > of the attribute value is defined by the security module. A flags field
> > is included for LSM specific information. It is currently unused and must
> > be 0. The total size of the data, including the lsm_ctx structure and any
> > padding, is maintained as well.
> >
> > struct lsm_ctx {
> > __u64 id;
> > __u64 flags;
> > __u64 len;
> > __u64 ctx_len;
> > __u8 ctx[];
> > };
> >
> > Two new LSM hooks are used to interface with the LSMs.
> > security_getselfattr() collects the lsm_ctx values from the
> > LSMs that support the hook, accounting for space requirements.
> > security_setselfattr() identifies which LSM the attribute is
> > intended for and passes it along.
> >
> > Signed-off-by: Casey Schaufler <[email protected]>
> > ---
> > Documentation/userspace-api/lsm.rst | 15 +++++
> > include/linux/lsm_hook_defs.h | 4 ++
> > include/linux/lsm_hooks.h | 9 +++
> > include/linux/security.h | 19 ++++++
> > include/linux/syscalls.h | 5 ++
> > include/uapi/linux/lsm.h | 33 ++++++++++
> > kernel/sys_ni.c | 4 ++
> > security/Makefile | 1 +
> > security/lsm_syscalls.c | 55 ++++++++++++++++
> > security/security.c | 97 +++++++++++++++++++++++++++++
> > 10 files changed, 242 insertions(+)
> > create mode 100644 security/lsm_syscalls.c
>
> [...]
>
> > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> > new file mode 100644
> > index 000000000000..feee31600219
> > --- /dev/null
> > +++ b/security/lsm_syscalls.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * System calls implementing the Linux Security Module API.
> > + *
> > + * Copyright (C) 2022 Casey Schaufler <[email protected]>
> > + * Copyright (C) 2022 Intel Corporation
> > + */
> > +
> > +#include <asm/current.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/security.h>
> > +#include <linux/stddef.h>
> > +#include <linux/syscalls.h>
> > +#include <linux/types.h>
> > +#include <linux/lsm_hooks.h>
> > +#include <uapi/linux/lsm.h>
> > +
> > +/**
> > + * sys_lsm_set_self_attr - Set current task's security module attribute
> > + * @attr: which attribute to set
> > + * @ctx: the LSM contexts
> > + * @size: size of @ctx
> > + * @flags: reserved for future use
> > + *
> > + * Sets the calling task's LSM context. On success this function
> > + * returns 0. If the attribute specified cannot be set a negative
> > + * value indicating the reason for the error is returned.
>
> Do you think it is really worth it to implement syscalls that can get
> and set attributes to several LSMs at the same time, instead of one at a
> time?
As mentioned elsewhere, the "set" operations pretty much have to be
one LSM at a time; the error handling is almost impossible otherwise.
However, it would be possible to have a single LSM "get" operation.
We could do this with the proposed lsm_get_self_attr() syscall and a
flag to indicate that only a single LSM's attribute information is
being requested, and that the desired LSM is indicated by the
lsm_ctx::id field (populated by the userspace caller). I'm imagining
something like this:
lsm_ctx->id = LSM_ID_MYFAVORITELSM;
lsm_get_self_attr(LSM_ATTR_CURRENT,
lsm_ctx, &lsm_ctx_size, LSM_FLG_SINGLE);
> LSM-specific tools don't care about other LSMs.
That's why they are called "LSM-specific tools" ;) I think it is a
reasonable request to provide optimizations for that, the
discussion/example above, but I think we also want to support tools
which are LSM "aware" but don't need to be made specific to any one
particular LSM. Thankfully, I think we can do both.
> I still think it
> would be much simpler (for kernel and user space) to pass an LSM ID to
> both syscalls. This would avoid dealing with variable arrays of variable
> element lengths, to both get or set attributes.
I think we should support "get" operations that support getting an
attribute from multiple LSMs, but I'm perfectly fine with also
supporting a single LSM "get" operation as described in the example
above.
> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
> previously talked about, getting an unknown number of file descriptor
> doesn't look good neither.
We are already in a place where not all LSMs support all of the
attributes, and we handle that. If you are concerned about a specific
LSM returning some additional, or "richer", attribute data, the
syscall does support that; it is just a matter of the userspace caller
being able to understand the LSM-specific data ... which is true for
even the simple/standard case.
> > + */
> > +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > + ctx, size_t __user, size, u32, flags)
> > +{
> > + return security_setselfattr(attr, ctx, size, flags);
> > +}
> > +
> > +/**
> > + * sys_lsm_get_self_attr - Return current task's security module attributes
> > + * @attr: which attribute to set
>
> attribute to *get*
>
> > + * @ctx: the LSM contexts
> > + * @size: size of @ctx, updated on return
>
> I suggest to use a dedicated argument to read the allocated size, and
> another to write the actual/written size.
Can you elaborate on this? There is plenty of precedence for this approach.
> This would not be required with an LSM ID passed to the syscall because
> attribute sizes should be known by user space, and there is no need to
> help them probe this information.
No. As we move forward, and LSMs potentially introduce additional
attribute information/types/etc., there will be cases where the kernel
could need more buffer space than userspace would realize. Keeping
the length flexible allows this, with the extra information ignored by
"legacy" userspace, and utilized by "new" userspace.
> > + * @flags: reserved for future use
> > + *
> > + * Returns the calling task's LSM contexts. On success this
> > + * function returns the number of @ctx array elements. This value
> > + * may be zero if there are no LSM contexts assigned. If @size is
> > + * insufficient to contain the return data -E2BIG is returned and
> > + * @size is set to the minimum required size.
>
> Doing something (updating a buffer) even when returning an error doesn't
> look right.
We could zero the buffer on error/E2BIG if that is a concern, but
unfortunately due the nature of the LSM it is not possible to safely
(no races) determine the size of the buffer before populating it.
> These sizes should be well-known to user space and part of
> the ABI/UAPI.
That may be true for specific LSMs at this point in time, but I
believe it would be a serious mistake to impose that constraint on
these syscalls.
--
paul-moore.com
On 11/04/2023 01:38, Paul Moore wrote:
> On Mon, Apr 10, 2023 at 7:37 PM Paul Moore <[email protected]> wrote:
>>
>> On Mon, Apr 3, 2023 at 8:04 AM Mickaël Salaün <[email protected]> wrote:
>>>
>>> It looks like you missed my preview reviews on these patches.
>>
>> For reference, I believe this is Mickaël's review of the associated v6 patch:
>>
>> https://lore.kernel.org/linux-security-module/[email protected]/
>
> My apologies, I hit send too soon ... Mickaël, if there are a specific
> points you feel have not been addressed, but should be, it would be
> helpful if you could list them in this thread.
No worries, Casey replied to the original thread:
https://lore.kernel.org/linux-security-module/[email protected]/