LinuxLists.cc - [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

2022-10-25 18:55:37

Subject: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

Create a system call lsm_self_attr() to provide the security
module maintained attributes of the current process. Historically
these attributes have been exposed to user space via entries in
procfs under /proc/self/attr.

Attributes are provided as a collection of lsm_ctx structures
which are placed into a user supplied buffer. Each structure
identifys the security module providing the attribute, which
of the possible attributes is provided, the size of the
attribute, and finally the attribute value. The format of the
attribute value is defined by the security module, but will
always be \0 terminated. The ctx_len value will be larger than
strlen(ctx).

------------------------------
| unsigned int id |
------------------------------
| unsigned int flags |
------------------------------
| __kernel_size_t ctx_len |
------------------------------
| unsigned char ctx[ctx_len] |
------------------------------
| unsigned int id |
------------------------------
| unsigned int flags |
------------------------------
| __kernel_size_t ctx_len |
------------------------------
| unsigned char ctx[ctx_len] |
------------------------------

Signed-off-by: Casey Schaufler <[email protected]>
---
include/linux/syscalls.h | 2 +
include/uapi/linux/lsm.h | 21 ++++++
kernel/sys_ni.c | 3 +
security/Makefile | 1 +
security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
5 files changed, 183 insertions(+)
create mode 100644 security/lsm_syscalls.c

diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index a34b0f9a9972..2d9033e9e5a0 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_cxt;
enum landlock_rule_type;

#include <linux/types.h>
@@ -1056,6 +1057,7 @@ 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_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);

/*
* Architecture-specific system calls
diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
index 61e13b1b9ece..1d27fb5b7746 100644
--- a/include/uapi/linux/lsm.h
+++ b/include/uapi/linux/lsm.h
@@ -9,6 +9,27 @@
#ifndef _UAPI_LINUX_LSM_H
#define _UAPI_LINUX_LSM_H

+#include <linux/types.h>
+#include <linux/unistd.h>
+
+/**
+ * struct lsm_ctx - LSM context
+ * @id: the LSM id number, see LSM_ID_XXX
+ * @flags: context specifier and LSM specific flags
+ * @ctx_len: the size of @ctx
+ * @ctx: the LSM context, a nul terminated string
+ *
+ * @ctx in a nul terminated string.
+ * (strlen(@ctx) < @ctx_len) is always true.
+ * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
+ */
+struct lsm_ctx {
+ unsigned int id;
+ unsigned int flags;
+ __kernel_size_t ctx_len;
+ unsigned char ctx[];
+};
+
/*
* ID values to identify security modules.
* A system may use more than one security module.
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 860b2dcf3ac4..0fdb0341251d 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -262,6 +262,9 @@ COND_SYSCALL_COMPAT(recvmsg);
/* mm/nommu.c, also with MMU */
COND_SYSCALL(mremap);

+/* security/lsm_syscalls.c */
+COND_SYSCALL(lsm_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..da0fab7065e2
--- /dev/null
+++ b/security/lsm_syscalls.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * System calls implementing the Linux Security Module API.
+ *
+ * Copyright (C) 2022 Casey Schaufler <[email protected]>
+ * Copyright (C) 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>
+
+struct feature_map {
+ char *name;
+ int feature;
+};
+
+static const struct feature_map lsm_attr_names[] = {
+ { .name = "current", .feature = LSM_ATTR_CURRENT, },
+ { .name = "exec", .feature = LSM_ATTR_EXEC, },
+ { .name = "fscreate", .feature = LSM_ATTR_FSCREATE, },
+ { .name = "keycreate", .feature = LSM_ATTR_KEYCREATE, },
+ { .name = "prev", .feature = LSM_ATTR_PREV, },
+ { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
+};
+
+/**
+ * lsm_self_attr - Return current task's security module attributes
+ * @ctx: the LSM contexts
+ * @size: size of @ctx, updated on return
+ * @flags: reserved for future use, must be zero
+ *
+ * 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_DEFINE3(lsm_self_attr,
+ struct lsm_ctx __user *, ctx,
+ size_t __user *, size,
+ int, flags)
+{
+ struct lsm_ctx *final = NULL;
+ struct lsm_ctx *interum;
+ struct lsm_ctx *ip;
+ void *curr;
+ char **interum_ctx;
+ char *cp;
+ size_t total_size = 0;
+ int count = 0;
+ int attr;
+ int len;
+ int rc = 0;
+ int i;
+
+ interum = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_id *
+ sizeof(*interum), GFP_KERNEL);
+ if (interum == NULL)
+ return -ENOMEM;
+ ip = interum;
+
+ interum_ctx = kzalloc(ARRAY_SIZE(lsm_attr_names) * lsm_id *
+ sizeof(*interum_ctx), GFP_KERNEL);
+ if (interum_ctx == NULL) {
+ kfree(interum);
+ return -ENOMEM;
+ }
+
+ for (attr = 0; attr < ARRAY_SIZE(lsm_attr_names); attr++) {
+ for (i = 0; i < lsm_id; i++) {
+ if ((lsm_idlist[i]->features &
+ lsm_attr_names[attr].feature) == 0)
+ continue;
+
+ len = security_getprocattr(current, lsm_idlist[i]->id,
+ lsm_attr_names[attr].name,
+ &cp);
+ if (len <= 0)
+ continue;
+
+ ip->id = lsm_idlist[i]->id;
+ ip->flags = lsm_attr_names[attr].feature;
+ /* space for terminating \0 is allocated below */
+ ip->ctx_len = len + 1;
+ interum_ctx[count] = cp;
+ /*
+ * Security modules have been inconsistent about
+ * including the \0 terminator in the size. The
+ * context len has been adjusted to ensure there
+ * is one.
+ * At least one security module adds a \n at the
+ * end of a context to make it look nicer. Change
+ * that to a \0 so that user space doesn't have to
+ * work around it. Because of this meddling it is
+ * safe to assume that lsm_ctx.name is terminated
+ * and that strlen(lsm_ctx.name) < lsm.ctx_len.
+ */
+ total_size += sizeof(*interum) + ip->ctx_len;
+ cp = strnchr(cp, len, '\n');
+ if (cp != NULL)
+ *cp = '\0';
+ ip++;
+ count++;
+ }
+ }
+
+ if (count == 0)
+ goto free_out;
+
+ final = kzalloc(total_size, GFP_KERNEL);
+ if (final == NULL) {
+ rc = -ENOMEM;
+ goto free_out;
+ }
+
+ curr = final;
+ ip = interum;
+ for (i = 0; i < count; i++) {
+ memcpy(curr, ip, sizeof(*interum));
+ curr += sizeof(*interum);
+ memcpy(curr, interum_ctx[i], ip->ctx_len);
+ curr += ip->ctx_len;
+ ip++;
+ }
+
+ if (get_user(len, size)) {
+ rc = -EFAULT;
+ goto free_out;
+ }
+ if (total_size > len) {
+ rc = -ERANGE;
+ goto free_out;
+ }
+ if (copy_to_user(ctx, final, total_size) != 0 ||
+ put_user(total_size, size) != 0)
+ rc = -EFAULT;
+ else
+ rc = count;
+
+free_out:
+ for (i = 0; i < count; i++)
+ kfree(interum_ctx[i]);
+ kfree(interum_ctx);
+ kfree(interum);
+ kfree(final);
+ return rc;
+}
--
2.37.3

2022-10-25 21:58:31

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

Hi Casey,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kees/for-next/hardening]
[also build test WARNING on pcmoore-selinux/next acme/perf/core linus/master v6.1-rc2 next-20221025]
[cannot apply to tip/perf/core]
[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-Identify-modules-by-more-than-name/20221026-034541
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
patch link: https://lore.kernel.org/r/20221025184519.13231-7-casey%40schaufler-ca.com
patch subject: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes
config: ia64-allyesconfig (attached as .config)
compiler: ia64-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/c9d17b230f202246a9451fbdefac8c1720eb68a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Casey-Schaufler/LSM-Identify-modules-by-more-than-name/20221026-034541
git checkout c9d17b230f202246a9451fbdefac8c1720eb68a6
# 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=ia64 SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from kernel/fork.c:55:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
kernel/fork.c:162:13: warning: no previous prototype for 'arch_release_task_struct' [-Wmissing-prototypes]
162 | void __weak arch_release_task_struct(struct task_struct *tsk)
| ^~~~~~~~~~~~~~~~~~~~~~~~
kernel/fork.c:849:20: warning: no previous prototype for 'arch_task_cache_init' [-Wmissing-prototypes]
849 | void __init __weak arch_task_cache_init(void) { }
| ^~~~~~~~~~~~~~~~~~~~
kernel/fork.c:944:12: warning: no previous prototype for 'arch_dup_task_struct' [-Wmissing-prototypes]
944 | int __weak arch_dup_task_struct(struct task_struct *dst,
| ^~~~~~~~~~~~~~~~~~~~
--
In file included from kernel/exec_domain.c:19:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
--
In file included from kernel/exit.c:42:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
kernel/exit.c:1839:13: warning: no previous prototype for 'abort' [-Wmissing-prototypes]
1839 | __weak void abort(void)
| ^~~~~
--
In file included from kernel/audit.c:44:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
kernel/audit.c: In function 'audit_log_vformat':
kernel/audit.c:1963:9: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
1963 | len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args);
| ^~~
kernel/audit.c:1972:17: warning: function 'audit_log_vformat' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
1972 | len = vsnprintf(skb_tail_pointer(skb), avail, fmt, args2);
| ^~~
--
In file included from include/linux/syscalls_api.h:1,
from kernel/sched/sched.h:61,
from kernel/sched/fair.c:55:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
kernel/sched/fair.c:11509:6: warning: no previous prototype for 'task_vruntime_update' [-Wmissing-prototypes]
11509 | void task_vruntime_update(struct rq *rq, struct task_struct *p, bool in_fi)
| ^~~~~~~~~~~~~~~~~~~~
--
In file included from kernel/time/hrtimer.c:30:
>> include/linux/syscalls.h:1060:42: warning: 'struct lsm_ctx' declared inside parameter list will not be visible outside of this definition or declaration
1060 | asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
| ^~~~~~~
kernel/time/hrtimer.c:120:35: warning: initialized field overwritten [-Woverride-init]
120 | [CLOCK_REALTIME] = HRTIMER_BASE_REALTIME,
| ^~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:120:35: note: (near initialization for 'hrtimer_clock_to_base_table[0]')
kernel/time/hrtimer.c:121:35: warning: initialized field overwritten [-Woverride-init]
121 | [CLOCK_MONOTONIC] = HRTIMER_BASE_MONOTONIC,
| ^~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:121:35: note: (near initialization for 'hrtimer_clock_to_base_table[1]')
kernel/time/hrtimer.c:122:35: warning: initialized field overwritten [-Woverride-init]
122 | [CLOCK_BOOTTIME] = HRTIMER_BASE_BOOTTIME,
| ^~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:122:35: note: (near initialization for 'hrtimer_clock_to_base_table[7]')
kernel/time/hrtimer.c:123:35: warning: initialized field overwritten [-Woverride-init]
123 | [CLOCK_TAI] = HRTIMER_BASE_TAI,
| ^~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:123:35: note: (near initialization for 'hrtimer_clock_to_base_table[11]')
kernel/time/hrtimer.c: In function '__run_hrtimer':
kernel/time/hrtimer.c:1648:14: warning: variable 'expires_in_hardirq' set but not used [-Wunused-but-set-variable]
1648 | bool expires_in_hardirq;
| ^~~~~~~~~~~~~~~~~~

vim +1060 include/linux/syscalls.h

904
905 /* mm/, CONFIG_MMU only */
906 asmlinkage long sys_swapon(const char __user *specialfile, int swap_flags);
907 asmlinkage long sys_swapoff(const char __user *specialfile);
908 asmlinkage long sys_mprotect(unsigned long start, size_t len,
909 unsigned long prot);
910 asmlinkage long sys_msync(unsigned long start, size_t len, int flags);
911 asmlinkage long sys_mlock(unsigned long start, size_t len);
912 asmlinkage long sys_munlock(unsigned long start, size_t len);
913 asmlinkage long sys_mlockall(int flags);
914 asmlinkage long sys_munlockall(void);
915 asmlinkage long sys_mincore(unsigned long start, size_t len,
916 unsigned char __user * vec);
917 asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
918 asmlinkage long sys_process_madvise(int pidfd, const struct iovec __user *vec,
919 size_t vlen, int behavior, unsigned int flags);
920 asmlinkage long sys_process_mrelease(int pidfd, unsigned int flags);
921 asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
922 unsigned long prot, unsigned long pgoff,
923 unsigned long flags);
924 asmlinkage long sys_mbind(unsigned long start, unsigned long len,
925 unsigned long mode,
926 const unsigned long __user *nmask,
927 unsigned long maxnode,
928 unsigned flags);
929 asmlinkage long sys_get_mempolicy(int __user *policy,
930 unsigned long __user *nmask,
931 unsigned long maxnode,
932 unsigned long addr, unsigned long flags);
933 asmlinkage long sys_set_mempolicy(int mode, const unsigned long __user *nmask,
934 unsigned long maxnode);
935 asmlinkage long sys_migrate_pages(pid_t pid, unsigned long maxnode,
936 const unsigned long __user *from,
937 const unsigned long __user *to);
938 asmlinkage long sys_move_pages(pid_t pid, unsigned long nr_pages,
939 const void __user * __user *pages,
940 const int __user *nodes,
941 int __user *status,
942 int flags);
943
944 asmlinkage long sys_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
945 siginfo_t __user *uinfo);
946 asmlinkage long sys_perf_event_open(
947 struct perf_event_attr __user *attr_uptr,
948 pid_t pid, int cpu, int group_fd, unsigned long flags);
949 asmlinkage long sys_accept4(int, struct sockaddr __user *, int __user *, int);
950 asmlinkage long sys_recvmmsg(int fd, struct mmsghdr __user *msg,
951 unsigned int vlen, unsigned flags,
952 struct __kernel_timespec __user *timeout);
953 asmlinkage long sys_recvmmsg_time32(int fd, struct mmsghdr __user *msg,
954 unsigned int vlen, unsigned flags,
955 struct old_timespec32 __user *timeout);
956
957 asmlinkage long sys_wait4(pid_t pid, int __user *stat_addr,
958 int options, struct rusage __user *ru);
959 asmlinkage long sys_prlimit64(pid_t pid, unsigned int resource,
960 const struct rlimit64 __user *new_rlim,
961 struct rlimit64 __user *old_rlim);
962 asmlinkage long sys_fanotify_init(unsigned int flags, unsigned int event_f_flags);
963 asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
964 u64 mask, int fd,
965 const char __user *pathname);
966 asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
967 struct file_handle __user *handle,
968 int __user *mnt_id, int flag);
969 asmlinkage long sys_open_by_handle_at(int mountdirfd,
970 struct file_handle __user *handle,
971 int flags);
972 asmlinkage long sys_clock_adjtime(clockid_t which_clock,
973 struct __kernel_timex __user *tx);
974 asmlinkage long sys_clock_adjtime32(clockid_t which_clock,
975 struct old_timex32 __user *tx);
976 asmlinkage long sys_syncfs(int fd);
977 asmlinkage long sys_setns(int fd, int nstype);
978 asmlinkage long sys_pidfd_open(pid_t pid, unsigned int flags);
979 asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
980 unsigned int vlen, unsigned flags);
981 asmlinkage long sys_process_vm_readv(pid_t pid,
982 const struct iovec __user *lvec,
983 unsigned long liovcnt,
984 const struct iovec __user *rvec,
985 unsigned long riovcnt,
986 unsigned long flags);
987 asmlinkage long sys_process_vm_writev(pid_t pid,
988 const struct iovec __user *lvec,
989 unsigned long liovcnt,
990 const struct iovec __user *rvec,
991 unsigned long riovcnt,
992 unsigned long flags);
993 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
994 unsigned long idx1, unsigned long idx2);
995 asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
996 asmlinkage long sys_sched_setattr(pid_t pid,
997 struct sched_attr __user *attr,
998 unsigned int flags);
999 asmlinkage long sys_sched_getattr(pid_t pid,
1000 struct sched_attr __user *attr,
1001 unsigned int size,
1002 unsigned int flags);
1003 asmlinkage long sys_renameat2(int olddfd, const char __user *oldname,
1004 int newdfd, const char __user *newname,
1005 unsigned int flags);
1006 asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
1007 void __user *uargs);
1008 asmlinkage long sys_getrandom(char __user *buf, size_t count,
1009 unsigned int flags);
1010 asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags);
1011 asmlinkage long sys_bpf(int cmd, union bpf_attr *attr, unsigned int size);
1012 asmlinkage long sys_execveat(int dfd, const char __user *filename,
1013 const char __user *const __user *argv,
1014 const char __user *const __user *envp, int flags);
1015 asmlinkage long sys_userfaultfd(int flags);
1016 asmlinkage long sys_membarrier(int cmd, unsigned int flags, int cpu_id);
1017 asmlinkage long sys_mlock2(unsigned long start, size_t len, int flags);
1018 asmlinkage long sys_copy_file_range(int fd_in, loff_t __user *off_in,
1019 int fd_out, loff_t __user *off_out,
1020 size_t len, unsigned int flags);
1021 asmlinkage long sys_preadv2(unsigned long fd, const struct iovec __user *vec,
1022 unsigned long vlen, unsigned long pos_l, unsigned long pos_h,
1023 rwf_t flags);
1024 asmlinkage long sys_pwritev2(unsigned long fd, const struct iovec __user *vec,
1025 unsigned long vlen, unsigned long pos_l, unsigned long pos_h,
1026 rwf_t flags);
1027 asmlinkage long sys_pkey_mprotect(unsigned long start, size_t len,
1028 unsigned long prot, int pkey);
1029 asmlinkage long sys_pkey_alloc(unsigned long flags, unsigned long init_val);
1030 asmlinkage long sys_pkey_free(int pkey);
1031 asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
1032 unsigned mask, struct statx __user *buffer);
1033 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
1034 int flags, uint32_t sig);
1035 asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
1036 asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path,
1037 int to_dfd, const char __user *to_path,
1038 unsigned int ms_flags);
1039 asmlinkage long sys_mount_setattr(int dfd, const char __user *path,
1040 unsigned int flags,
1041 struct mount_attr __user *uattr, size_t usize);
1042 asmlinkage long sys_fsopen(const char __user *fs_name, unsigned int flags);
1043 asmlinkage long sys_fsconfig(int fs_fd, unsigned int cmd, const char __user *key,
1044 const void __user *value, int aux);
1045 asmlinkage long sys_fsmount(int fs_fd, unsigned int flags, unsigned int ms_flags);
1046 asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags);
1047 asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
1048 siginfo_t __user *info,
1049 unsigned int flags);
1050 asmlinkage long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
1051 asmlinkage long sys_landlock_create_ruleset(const struct landlock_ruleset_attr __user *attr,
1052 size_t size, __u32 flags);
1053 asmlinkage long sys_landlock_add_rule(int ruleset_fd, enum landlock_rule_type rule_type,
1054 const void __user *rule_attr, __u32 flags);
1055 asmlinkage long sys_landlock_restrict_self(int ruleset_fd, __u32 flags);
1056 asmlinkage long sys_memfd_secret(unsigned int flags);
1057 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
1058 unsigned long home_node,
1059 unsigned long flags);
> 1060 asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
1061

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (16.18 kB)
.config.gz (73.30 kB)
Download all attachments

2022-10-26 06:31:14

by Greg KH

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Tue, Oct 25, 2022 at 11:45:17AM -0700, Casey Schaufler wrote:
> Create a system call lsm_self_attr() to provide the security
> module maintained attributes of the current process. Historically
> these attributes have been exposed to user space via entries in
> procfs under /proc/self/attr.
>
> Attributes are provided as a collection of lsm_ctx structures
> which are placed into a user supplied buffer. Each structure
> identifys the security module providing the attribute, which
> of the possible attributes is provided, the size of the
> attribute, and finally the attribute value. The format of the
> attribute value is defined by the security module, but will
> always be \0 terminated. The ctx_len value will be larger than
> strlen(ctx).
>
> ------------------------------
> | unsigned int id |
> ------------------------------
> | unsigned int flags |
> ------------------------------
> | __kernel_size_t ctx_len |
> ------------------------------
> | unsigned char ctx[ctx_len] |
> ------------------------------
> | unsigned int id |
> ------------------------------
> | unsigned int flags |
> ------------------------------
> | __kernel_size_t ctx_len |
> ------------------------------
> | unsigned char ctx[ctx_len] |
> ------------------------------
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> include/linux/syscalls.h | 2 +
> include/uapi/linux/lsm.h | 21 ++++++
> kernel/sys_ni.c | 3 +
> security/Makefile | 1 +
> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
> 5 files changed, 183 insertions(+)
> create mode 100644 security/lsm_syscalls.c
>
> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index a34b0f9a9972..2d9033e9e5a0 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_cxt;
> enum landlock_rule_type;
>
> #include <linux/types.h>
> @@ -1056,6 +1057,7 @@ 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_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
>
> /*
> * Architecture-specific system calls
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index 61e13b1b9ece..1d27fb5b7746 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -9,6 +9,27 @@
> #ifndef _UAPI_LINUX_LSM_H
> #define _UAPI_LINUX_LSM_H
>
> +#include <linux/types.h>
> +#include <linux/unistd.h>
> +
> +/**
> + * struct lsm_ctx - LSM context
> + * @id: the LSM id number, see LSM_ID_XXX
> + * @flags: context specifier and LSM specific flags
> + * @ctx_len: the size of @ctx
> + * @ctx: the LSM context, a nul terminated string
> + *
> + * @ctx in a nul terminated string.
> + * (strlen(@ctx) < @ctx_len) is always true.
> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
> + */
> +struct lsm_ctx {
> + unsigned int id;
> + unsigned int flags;
> + __kernel_size_t ctx_len;
> + unsigned char ctx[];

Please use data types that are allowed to cross the user/kernel boundry
in a safe way. That would mean this would use __u64 instead of unsigned
int, and __u8 instead of unsigned char.

thanks,

greg k-h

2022-10-26 07:08:46

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

Hi Casey,

I love your patch! Perhaps something to improve:

[auto build test WARNING on kees/for-next/hardening]
[also build test WARNING on pcmoore-selinux/next acme/perf/core linus/master v6.1-rc2 next-20221026]
[cannot apply to tip/perf/core]
[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-Identify-modules-by-more-than-name/20221026-034541
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
patch link: https://lore.kernel.org/r/20221025184519.13231-7-casey%40schaufler-ca.com
patch subject: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes
config: arm-randconfig-r031-20221025 (attached as .config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/c9d17b230f202246a9451fbdefac8c1720eb68a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Casey-Schaufler/LSM-Identify-modules-by-more-than-name/20221026-034541
git checkout c9d17b230f202246a9451fbdefac8c1720eb68a6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash kernel/time/ security/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from security/lsm_syscalls.c:15:
>> include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
security/lsm_syscalls.c:47:1: error: conflicting types for 'sys_lsm_self_attr'
SYSCALL_DEFINE3(lsm_self_attr,
^
include/linux/syscalls.h:220:36: note: expanded from macro 'SYSCALL_DEFINE3'
#define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
^
include/linux/syscalls.h:229:2: note: expanded from macro 'SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^
include/linux/syscalls.h:243:18: note: expanded from macro '__SYSCALL_DEFINEx'
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
^
<scratch space>:65:1: note: expanded from here
sys_lsm_self_attr
^
include/linux/syscalls.h:1060:17: note: previous declaration is here
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
1 warning and 1 error generated.
--
In file included from kernel/time/time.c:33:
>> include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
1 warning generated.
--
In file included from kernel/time/timer.c:35:
>> include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
kernel/time/timer.c:1365:20: warning: unused function 'del_timer_wait_running' [-Wunused-function]
static inline void del_timer_wait_running(struct timer_list *timer) { }
^
2 warnings generated.
--
In file included from kernel/time/hrtimer.c:30:
>> include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
kernel/time/hrtimer.c:120:21: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
[CLOCK_REALTIME] = HRTIMER_BASE_REALTIME,
^~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:118:27: note: previous initialization is here
[0 ... MAX_CLOCKS - 1] = HRTIMER_MAX_CLOCK_BASES,
^~~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:121:22: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
[CLOCK_MONOTONIC] = HRTIMER_BASE_MONOTONIC,
^~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:118:27: note: previous initialization is here
[0 ... MAX_CLOCKS - 1] = HRTIMER_MAX_CLOCK_BASES,
^~~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:122:21: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
[CLOCK_BOOTTIME] = HRTIMER_BASE_BOOTTIME,
^~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:118:27: note: previous initialization is here
[0 ... MAX_CLOCKS - 1] = HRTIMER_MAX_CLOCK_BASES,
^~~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:123:17: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
[CLOCK_TAI] = HRTIMER_BASE_TAI,
^~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:118:27: note: previous initialization is here
[0 ... MAX_CLOCKS - 1] = HRTIMER_MAX_CLOCK_BASES,
^~~~~~~~~~~~~~~~~~~~~~~
kernel/time/hrtimer.c:1648:7: warning: variable 'expires_in_hardirq' set but not used [-Wunused-but-set-variable]
bool expires_in_hardirq;
^
kernel/time/hrtimer.c:276:20: warning: unused function 'is_migration_base' [-Wunused-function]
static inline bool is_migration_base(struct hrtimer_clock_base *base)
^
kernel/time/hrtimer.c:1873:20: warning: unused function '__hrtimer_peek_ahead_timers' [-Wunused-function]
static inline void __hrtimer_peek_ahead_timers(void)
^
8 warnings generated.
--
In file included from kernel/time/posix-stubs.c:13:
>> include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
kernel/time/posix-stubs.c:25:17: warning: no previous prototype for function 'sys_ni_posix_timers' [-Wmissing-prototypes]
asmlinkage long sys_ni_posix_timers(void)
^
kernel/time/posix-stubs.c:25:12: note: declare 'static' if the function is not intended to be used outside of this translation unit
asmlinkage long sys_ni_posix_timers(void)
^
static
2 warnings generated.

vim +1060 include/linux/syscalls.h

904
905 /* mm/, CONFIG_MMU only */
906 asmlinkage long sys_swapon(const char __user *specialfile, int swap_flags);
907 asmlinkage long sys_swapoff(const char __user *specialfile);
908 asmlinkage long sys_mprotect(unsigned long start, size_t len,
909 unsigned long prot);
910 asmlinkage long sys_msync(unsigned long start, size_t len, int flags);
911 asmlinkage long sys_mlock(unsigned long start, size_t len);
912 asmlinkage long sys_munlock(unsigned long start, size_t len);
913 asmlinkage long sys_mlockall(int flags);
914 asmlinkage long sys_munlockall(void);
915 asmlinkage long sys_mincore(unsigned long start, size_t len,
916 unsigned char __user * vec);
917 asmlinkage long sys_madvise(unsigned long start, size_t len, int behavior);
918 asmlinkage long sys_process_madvise(int pidfd, const struct iovec __user *vec,
919 size_t vlen, int behavior, unsigned int flags);
920 asmlinkage long sys_process_mrelease(int pidfd, unsigned int flags);
921 asmlinkage long sys_remap_file_pages(unsigned long start, unsigned long size,
922 unsigned long prot, unsigned long pgoff,
923 unsigned long flags);
924 asmlinkage long sys_mbind(unsigned long start, unsigned long len,
925 unsigned long mode,
926 const unsigned long __user *nmask,
927 unsigned long maxnode,
928 unsigned flags);
929 asmlinkage long sys_get_mempolicy(int __user *policy,
930 unsigned long __user *nmask,
931 unsigned long maxnode,
932 unsigned long addr, unsigned long flags);
933 asmlinkage long sys_set_mempolicy(int mode, const unsigned long __user *nmask,
934 unsigned long maxnode);
935 asmlinkage long sys_migrate_pages(pid_t pid, unsigned long maxnode,
936 const unsigned long __user *from,
937 const unsigned long __user *to);
938 asmlinkage long sys_move_pages(pid_t pid, unsigned long nr_pages,
939 const void __user * __user *pages,
940 const int __user *nodes,
941 int __user *status,
942 int flags);
943
944 asmlinkage long sys_rt_tgsigqueueinfo(pid_t tgid, pid_t pid, int sig,
945 siginfo_t __user *uinfo);
946 asmlinkage long sys_perf_event_open(
947 struct perf_event_attr __user *attr_uptr,
948 pid_t pid, int cpu, int group_fd, unsigned long flags);
949 asmlinkage long sys_accept4(int, struct sockaddr __user *, int __user *, int);
950 asmlinkage long sys_recvmmsg(int fd, struct mmsghdr __user *msg,
951 unsigned int vlen, unsigned flags,
952 struct __kernel_timespec __user *timeout);
953 asmlinkage long sys_recvmmsg_time32(int fd, struct mmsghdr __user *msg,
954 unsigned int vlen, unsigned flags,
955 struct old_timespec32 __user *timeout);
956
957 asmlinkage long sys_wait4(pid_t pid, int __user *stat_addr,
958 int options, struct rusage __user *ru);
959 asmlinkage long sys_prlimit64(pid_t pid, unsigned int resource,
960 const struct rlimit64 __user *new_rlim,
961 struct rlimit64 __user *old_rlim);
962 asmlinkage long sys_fanotify_init(unsigned int flags, unsigned int event_f_flags);
963 asmlinkage long sys_fanotify_mark(int fanotify_fd, unsigned int flags,
964 u64 mask, int fd,
965 const char __user *pathname);
966 asmlinkage long sys_name_to_handle_at(int dfd, const char __user *name,
967 struct file_handle __user *handle,
968 int __user *mnt_id, int flag);
969 asmlinkage long sys_open_by_handle_at(int mountdirfd,
970 struct file_handle __user *handle,
971 int flags);
972 asmlinkage long sys_clock_adjtime(clockid_t which_clock,
973 struct __kernel_timex __user *tx);
974 asmlinkage long sys_clock_adjtime32(clockid_t which_clock,
975 struct old_timex32 __user *tx);
976 asmlinkage long sys_syncfs(int fd);
977 asmlinkage long sys_setns(int fd, int nstype);
978 asmlinkage long sys_pidfd_open(pid_t pid, unsigned int flags);
979 asmlinkage long sys_sendmmsg(int fd, struct mmsghdr __user *msg,
980 unsigned int vlen, unsigned flags);
981 asmlinkage long sys_process_vm_readv(pid_t pid,
982 const struct iovec __user *lvec,
983 unsigned long liovcnt,
984 const struct iovec __user *rvec,
985 unsigned long riovcnt,
986 unsigned long flags);
987 asmlinkage long sys_process_vm_writev(pid_t pid,
988 const struct iovec __user *lvec,
989 unsigned long liovcnt,
990 const struct iovec __user *rvec,
991 unsigned long riovcnt,
992 unsigned long flags);
993 asmlinkage long sys_kcmp(pid_t pid1, pid_t pid2, int type,
994 unsigned long idx1, unsigned long idx2);
995 asmlinkage long sys_finit_module(int fd, const char __user *uargs, int flags);
996 asmlinkage long sys_sched_setattr(pid_t pid,
997 struct sched_attr __user *attr,
998 unsigned int flags);
999 asmlinkage long sys_sched_getattr(pid_t pid,
1000 struct sched_attr __user *attr,
1001 unsigned int size,
1002 unsigned int flags);
1003 asmlinkage long sys_renameat2(int olddfd, const char __user *oldname,
1004 int newdfd, const char __user *newname,
1005 unsigned int flags);
1006 asmlinkage long sys_seccomp(unsigned int op, unsigned int flags,
1007 void __user *uargs);
1008 asmlinkage long sys_getrandom(char __user *buf, size_t count,
1009 unsigned int flags);
1010 asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags);
1011 asmlinkage long sys_bpf(int cmd, union bpf_attr *attr, unsigned int size);
1012 asmlinkage long sys_execveat(int dfd, const char __user *filename,
1013 const char __user *const __user *argv,
1014 const char __user *const __user *envp, int flags);
1015 asmlinkage long sys_userfaultfd(int flags);
1016 asmlinkage long sys_membarrier(int cmd, unsigned int flags, int cpu_id);
1017 asmlinkage long sys_mlock2(unsigned long start, size_t len, int flags);
1018 asmlinkage long sys_copy_file_range(int fd_in, loff_t __user *off_in,
1019 int fd_out, loff_t __user *off_out,
1020 size_t len, unsigned int flags);
1021 asmlinkage long sys_preadv2(unsigned long fd, const struct iovec __user *vec,
1022 unsigned long vlen, unsigned long pos_l, unsigned long pos_h,
1023 rwf_t flags);
1024 asmlinkage long sys_pwritev2(unsigned long fd, const struct iovec __user *vec,
1025 unsigned long vlen, unsigned long pos_l, unsigned long pos_h,
1026 rwf_t flags);
1027 asmlinkage long sys_pkey_mprotect(unsigned long start, size_t len,
1028 unsigned long prot, int pkey);
1029 asmlinkage long sys_pkey_alloc(unsigned long flags, unsigned long init_val);
1030 asmlinkage long sys_pkey_free(int pkey);
1031 asmlinkage long sys_statx(int dfd, const char __user *path, unsigned flags,
1032 unsigned mask, struct statx __user *buffer);
1033 asmlinkage long sys_rseq(struct rseq __user *rseq, uint32_t rseq_len,
1034 int flags, uint32_t sig);
1035 asmlinkage long sys_open_tree(int dfd, const char __user *path, unsigned flags);
1036 asmlinkage long sys_move_mount(int from_dfd, const char __user *from_path,
1037 int to_dfd, const char __user *to_path,
1038 unsigned int ms_flags);
1039 asmlinkage long sys_mount_setattr(int dfd, const char __user *path,
1040 unsigned int flags,
1041 struct mount_attr __user *uattr, size_t usize);
1042 asmlinkage long sys_fsopen(const char __user *fs_name, unsigned int flags);
1043 asmlinkage long sys_fsconfig(int fs_fd, unsigned int cmd, const char __user *key,
1044 const void __user *value, int aux);
1045 asmlinkage long sys_fsmount(int fs_fd, unsigned int flags, unsigned int ms_flags);
1046 asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags);
1047 asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
1048 siginfo_t __user *info,
1049 unsigned int flags);
1050 asmlinkage long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
1051 asmlinkage long sys_landlock_create_ruleset(const struct landlock_ruleset_attr __user *attr,
1052 size_t size, __u32 flags);
1053 asmlinkage long sys_landlock_add_rule(int ruleset_fd, enum landlock_rule_type rule_type,
1054 const void __user *rule_attr, __u32 flags);
1055 asmlinkage long sys_landlock_restrict_self(int ruleset_fd, __u32 flags);
1056 asmlinkage long sys_memfd_secret(unsigned int flags);
1057 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
1058 unsigned long home_node,
1059 unsigned long flags);
> 1060 asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
1061

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (16.55 kB)
.config.gz (35.34 kB)
Download all attachments

2022-10-26 08:23:22

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

Attachments:

(No filename) (2.84 kB)
.config.gz (29.62 kB)
Download all attachments

2022-10-26 09:38:13

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

Hi Casey,

I love your patch! Yet something to improve:

[auto build test ERROR on kees/for-next/hardening]
[also build test ERROR on pcmoore-selinux/next acme/perf/core linus/master v6.1-rc2 next-20221026]
[cannot apply to tip/perf/core]
[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-Identify-modules-by-more-than-name/20221026-034541
base: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/hardening
patch link: https://lore.kernel.org/r/20221025184519.13231-7-casey%40schaufler-ca.com
patch subject: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes
config: arm-randconfig-r031-20221025 (attached as .config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
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
# install arm cross compiling tool for clang build
# apt-get install binutils-arm-linux-gnueabi
# https://github.com/intel-lab-lkp/linux/commit/c9d17b230f202246a9451fbdefac8c1720eb68a6
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Casey-Schaufler/LSM-Identify-modules-by-more-than-name/20221026-034541
git checkout c9d17b230f202246a9451fbdefac8c1720eb68a6
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from security/lsm_syscalls.c:15:
include/linux/syscalls.h:1060:42: warning: declaration of 'struct lsm_ctx' will not be visible outside of this function [-Wvisibility]
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
>> security/lsm_syscalls.c:47:1: error: conflicting types for 'sys_lsm_self_attr'
SYSCALL_DEFINE3(lsm_self_attr,
^
include/linux/syscalls.h:220:36: note: expanded from macro 'SYSCALL_DEFINE3'
#define SYSCALL_DEFINE3(name, ...) SYSCALL_DEFINEx(3, _##name, __VA_ARGS__)
^
include/linux/syscalls.h:229:2: note: expanded from macro 'SYSCALL_DEFINEx'
__SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
^
include/linux/syscalls.h:243:18: note: expanded from macro '__SYSCALL_DEFINEx'
asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__)) \
^
<scratch space>:65:1: note: expanded from here
sys_lsm_self_attr
^
include/linux/syscalls.h:1060:17: note: previous declaration is here
asmlinkage long sys_lsm_self_attr(struct lsm_ctx *ctx, size_t *size, int flags);
^
1 warning and 1 error generated.

vim +/sys_lsm_self_attr +47 security/lsm_syscalls.c

33
34 /**
35 * lsm_self_attr - Return current task's security module attributes
36 * @ctx: the LSM contexts
37 * @size: size of @ctx, updated on return
38 * @flags: reserved for future use, must be zero
39 *
40 * Returns the calling task's LSM contexts. On success this
41 * function returns the number of @ctx array elements. This value
42 * may be zero if there are no LSM contexts assigned. If @size is
43 * insufficient to contain the return data -E2BIG is returned and
44 * @size is set to the minimum required size. In all other cases
45 * a negative value indicating the error is returned.
46 */
> 47 SYSCALL_DEFINE3(lsm_self_attr,

--
0-DAY CI Kernel Test Service
https://01.org/lkp

Attachments:

(No filename) (4.05 kB)
.config.gz (35.34 kB)
Download all attachments

2022-11-09 23:46:13

by Paul Moore

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
>
> Create a system call lsm_self_attr() to provide the security
> module maintained attributes of the current process. Historically
> these attributes have been exposed to user space via entries in
> procfs under /proc/self/attr.
>
> Attributes are provided as a collection of lsm_ctx structures
> which are placed into a user supplied buffer. Each structure
> identifys the security module providing the attribute, which
> of the possible attributes is provided, the size of the
> attribute, and finally the attribute value. The format of the
> attribute value is defined by the security module, but will
> always be \0 terminated. The ctx_len value will be larger than
> strlen(ctx).
>
> ------------------------------
> | unsigned int id |
> ------------------------------
> | unsigned int flags |
> ------------------------------
> | __kernel_size_t ctx_len |
> ------------------------------
> | unsigned char ctx[ctx_len] |
> ------------------------------
> | unsigned int id |
> ------------------------------
> | unsigned int flags |
> ------------------------------
> | __kernel_size_t ctx_len |
> ------------------------------
> | unsigned char ctx[ctx_len] |
> ------------------------------
>
> Signed-off-by: Casey Schaufler <[email protected]>
> ---
> include/linux/syscalls.h | 2 +
> include/uapi/linux/lsm.h | 21 ++++++
> kernel/sys_ni.c | 3 +
> security/Makefile | 1 +
> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
> 5 files changed, 183 insertions(+)
> create mode 100644 security/lsm_syscalls.c

...

> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index 61e13b1b9ece..1d27fb5b7746 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -9,6 +9,27 @@
> #ifndef _UAPI_LINUX_LSM_H
> #define _UAPI_LINUX_LSM_H
>
> +#include <linux/types.h>
> +#include <linux/unistd.h>
> +
> +/**
> + * struct lsm_ctx - LSM context
> + * @id: the LSM id number, see LSM_ID_XXX
> + * @flags: context specifier and LSM specific flags
> + * @ctx_len: the size of @ctx
> + * @ctx: the LSM context, a nul terminated string
> + *
> + * @ctx in a nul terminated string.
> + * (strlen(@ctx) < @ctx_len) is always true.
> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
> + */

We can do better than this, or rather we *should* do better than this.
One of the big advantages of creating a new API is we can fix some of
the silly things we have had to do in the past, including the
"sometimes" NUL terminator on strings. For this LSM syscalls let's
make a promise that all human readable strings will be properly NUL
terminated; currently this includes all LSM contexts, and likely will
remain that way forever for various reasons, but let's leave the door
open for arbitrary blobs (see the "special" LSM ID discussion from
earlier in the patchset). With that in mind I might suggest the
following:

/**
* struct lsm_ctx - LSM context
* @id: the LSM id number, see LSM_ID_XXX
* @flags: context specifier and LSM specific flags
* @ctx_len: the size of @ctx
* @ctx: the LSM context, see description
*
* For LSMs which provide human readable contexts @ctx will be a nul
* terminated string and @ctx_len will include the size of the string
* and the nul terminator, e.g. 'ctx_len == strlen(ctx) + 1'. For LSMs
* which provide binary-only contexts @cts will be a binary blob with
* @ctx_len being the exact value of the blob. The type of the @ctx,
* human readable string or binary, can be determined by inspecting
* both @id and @flags.
*
* If a given LSM @id does not define a set of values for use in the
* @flags field, @flags MUST be set to zero.
*/
struct lsm_ctx {
__u32 id;
__U32 flags;
__kernel_size_t ctx_len;
__u8 ctx[];
};

> +struct lsm_ctx {
> + unsigned int id;
> + unsigned int flags;
> + __kernel_size_t ctx_len;
> + unsigned char ctx[];
> +};

I agree with Greg, we should be explicit about variable sizing, let's
make sure everything in the UAPI header is defined in terms of
__uXX/__sXX. This includes strings as __u8 arrays.

Also, I sorta despite the 'let's line all the struct fields up
horizontally!' approach in struct/variable definitions. I personally
think it looks horrible and it clutters up future patches. Please
don't do this unless the individual file already does it, and since we
are creating this new please don't :)

> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> new file mode 100644
> index 000000000000..da0fab7065e2
> --- /dev/null
> +++ b/security/lsm_syscalls.c
> @@ -0,0 +1,156 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * System calls implementing the Linux Security Module API.
> + *
> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
> + * Copyright (C) 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>
> +
> +struct feature_map {
> + char *name;
> + int feature;
> +};
> +
> +static const struct feature_map lsm_attr_names[] = {
> + { .name = "current", .feature = LSM_ATTR_CURRENT, },
> + { .name = "exec", .feature = LSM_ATTR_EXEC, },
> + { .name = "fscreate", .feature = LSM_ATTR_FSCREATE, },
> + { .name = "keycreate", .feature = LSM_ATTR_KEYCREATE, },
> + { .name = "prev", .feature = LSM_ATTR_PREV, },
> + { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
> +};
> +
> +/**
> + * lsm_self_attr - Return current task's security module attributes
> + * @ctx: the LSM contexts
> + * @size: size of @ctx, updated on return
> + * @flags: reserved for future use, must be zero
> + *
> + * 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_DEFINE3(lsm_self_attr,
> + struct lsm_ctx __user *, ctx,
> + size_t __user *, size,
> + int, flags)

See my comments above about UAPI types, let's change this to something
like this:

[NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]

SYSCALL_DEFINE3(lsm_self_attr,
struct lsm_ctx __user *, ctx,
__kernel_size_t __user *, size,
__u32, flags)

> +{
> + struct lsm_ctx *final = NULL;
> + struct lsm_ctx *interum;
> + struct lsm_ctx *ip;
> + void *curr;
> + char **interum_ctx;
> + char *cp;
> + size_t total_size = 0;
> + int count = 0;
> + int attr;
> + int len;
> + int rc = 0;
> + int i;

Ungh, reverse christmas trees ... I kinda hate it from a style
perspective, enough to mention it here, but I'm not going to be petty
enough to say "change it".

However, if you did want to flip it upside down (normal christmas
tree?) during the respin I would be grateful ;)

--
paul-moore.com

2022-11-10 01:45:15

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On 11/9/2022 3:34 PM, Paul Moore wrote:
> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
>> Create a system call lsm_self_attr() to provide the security
>> module maintained attributes of the current process. Historically
>> these attributes have been exposed to user space via entries in
>> procfs under /proc/self/attr.
>>
>> Attributes are provided as a collection of lsm_ctx structures
>> which are placed into a user supplied buffer. Each structure
>> identifys the security module providing the attribute, which
>> of the possible attributes is provided, the size of the
>> attribute, and finally the attribute value. The format of the
>> attribute value is defined by the security module, but will
>> always be \0 terminated. The ctx_len value will be larger than
>> strlen(ctx).
>>
>> ------------------------------
>> | unsigned int id |
>> ------------------------------
>> | unsigned int flags |
>> ------------------------------
>> | __kernel_size_t ctx_len |
>> ------------------------------
>> | unsigned char ctx[ctx_len] |
>> ------------------------------
>> | unsigned int id |
>> ------------------------------
>> | unsigned int flags |
>> ------------------------------
>> | __kernel_size_t ctx_len |
>> ------------------------------
>> | unsigned char ctx[ctx_len] |
>> ------------------------------
>>
>> Signed-off-by: Casey Schaufler <[email protected]>
>> ---
>> include/linux/syscalls.h | 2 +
>> include/uapi/linux/lsm.h | 21 ++++++
>> kernel/sys_ni.c | 3 +
>> security/Makefile | 1 +
>> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 183 insertions(+)
>> create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index 61e13b1b9ece..1d27fb5b7746 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,27 @@
>> #ifndef _UAPI_LINUX_LSM_H
>> #define _UAPI_LINUX_LSM_H
>>
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
>> +
>> +/**
>> + * struct lsm_ctx - LSM context
>> + * @id: the LSM id number, see LSM_ID_XXX
>> + * @flags: context specifier and LSM specific flags
>> + * @ctx_len: the size of @ctx
>> + * @ctx: the LSM context, a nul terminated string
>> + *
>> + * @ctx in a nul terminated string.
>> + * (strlen(@ctx) < @ctx_len) is always true.
>> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
>> + */
> We can do better than this, or rather we *should* do better than this.
> One of the big advantages of creating a new API is we can fix some of
> the silly things we have had to do in the past, including the
> "sometimes" NUL terminator on strings. For this LSM syscalls let's
> make a promise that all human readable strings will be properly NUL
> terminated;

It requires effort and buffer management to ensure that ctx_len == strlen(ctx)+1,
but if you think it's important, sure.

> currently this includes all LSM contexts, and likely will
> remain that way forever for various reasons, but let's leave the door
> open for arbitrary blobs (see the "special" LSM ID discussion from
> earlier in the patchset). With that in mind I might suggest the
> following:
>
> /**
> * struct lsm_ctx - LSM context
> * @id: the LSM id number, see LSM_ID_XXX
> * @flags: context specifier and LSM specific flags
> * @ctx_len: the size of @ctx
> * @ctx: the LSM context, see description
> *
> * For LSMs which provide human readable contexts @ctx will be a nul
> * terminated string and @ctx_len will include the size of the string
> * and the nul terminator, e.g. 'ctx_len == strlen(ctx) + 1'. For LSMs
> * which provide binary-only contexts @cts will be a binary blob with
> * @ctx_len being the exact value of the blob. The type of the @ctx,
> * human readable string or binary, can be determined by inspecting
> * both @id and @flags.

I'd go a touch further, defining LSM_ATTR_BINARY as a flag and demanding
that any attribute that isn't a nul terminated string be thus identified,
even if it is always binary. Thus, LSM_ATTR_CURRENT and LSM_ATTR_BINARY
together would be an error.

> *
> * If a given LSM @id does not define a set of values for use in the
> * @flags field, @flags MUST be set to zero.
> */
> struct lsm_ctx {
> __u32 id;
> __U32 flags;
> __kernel_size_t ctx_len;
> __u8 ctx[];
> };
>
>> +struct lsm_ctx {
>> + unsigned int id;
>> + unsigned int flags;
>> + __kernel_size_t ctx_len;
>> + unsigned char ctx[];
>> +};
> I agree with Greg, we should be explicit about variable sizing, let's
> make sure everything in the UAPI header is defined in terms of
> __uXX/__sXX. This includes strings as __u8 arrays.
>
> Also, I sorta despite the 'let's line all the struct fields up
> horizontally!' approach in struct/variable definitions.

Kids. Got no respect for tradition.

> I personally
> think it looks horrible and it clutters up future patches. Please
> don't do this unless the individual file already does it, and since we
> are creating this new please don't :)
>
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> new file mode 100644
>> index 000000000000..da0fab7065e2
>> --- /dev/null
>> +++ b/security/lsm_syscalls.c
>> @@ -0,0 +1,156 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * System calls implementing the Linux Security Module API.
>> + *
>> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
>> + * Copyright (C) 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>
>> +
>> +struct feature_map {
>> + char *name;
>> + int feature;
>> +};
>> +
>> +static const struct feature_map lsm_attr_names[] = {
>> + { .name = "current", .feature = LSM_ATTR_CURRENT, },
>> + { .name = "exec", .feature = LSM_ATTR_EXEC, },
>> + { .name = "fscreate", .feature = LSM_ATTR_FSCREATE, },
>> + { .name = "keycreate", .feature = LSM_ATTR_KEYCREATE, },
>> + { .name = "prev", .feature = LSM_ATTR_PREV, },
>> + { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
>> +};
>> +
>> +/**
>> + * lsm_self_attr - Return current task's security module attributes
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx, updated on return
>> + * @flags: reserved for future use, must be zero
>> + *
>> + * 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_DEFINE3(lsm_self_attr,
>> + struct lsm_ctx __user *, ctx,
>> + size_t __user *, size,
>> + int, flags)
> See my comments above about UAPI types, let's change this to something
> like this:
>
> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
>
> SYSCALL_DEFINE3(lsm_self_attr,
> struct lsm_ctx __user *, ctx,
> __kernel_size_t __user *, size,
> __u32, flags)
>
>> +{
>> + struct lsm_ctx *final = NULL;
>> + struct lsm_ctx *interum;
>> + struct lsm_ctx *ip;
>> + void *curr;
>> + char **interum_ctx;
>> + char *cp;
>> + size_t total_size = 0;
>> + int count = 0;
>> + int attr;
>> + int len;
>> + int rc = 0;
>> + int i;
> Ungh, reverse christmas trees ... I kinda hate it from a style
> perspective, enough to mention it here, but I'm not going to be petty
> enough to say "change it".

I really don't care. Last I saw reverse christmas tree was the officially
recommended style. I don't care one way or the other.

>
> However, if you did want to flip it upside down (normal christmas
> tree?) during the respin I would be grateful ;)
>
> --
> paul-moore.com

2022-11-10 03:54:21

by Paul Moore

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Wed, Nov 9, 2022 at 8:32 PM Casey Schaufler <[email protected]> wrote:
> On 11/9/2022 3:34 PM, Paul Moore wrote:
> > On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
> >> Create a system call lsm_self_attr() to provide the security
> >> module maintained attributes of the current process. Historically
> >> these attributes have been exposed to user space via entries in
> >> procfs under /proc/self/attr.
> >>
> >> Attributes are provided as a collection of lsm_ctx structures
> >> which are placed into a user supplied buffer. Each structure
> >> identifys the security module providing the attribute, which
> >> of the possible attributes is provided, the size of the
> >> attribute, and finally the attribute value. The format of the
> >> attribute value is defined by the security module, but will
> >> always be \0 terminated. The ctx_len value will be larger than
> >> strlen(ctx).
> >>
> >> ------------------------------
> >> | unsigned int id |
> >> ------------------------------
> >> | unsigned int flags |
> >> ------------------------------
> >> | __kernel_size_t ctx_len |
> >> ------------------------------
> >> | unsigned char ctx[ctx_len] |
> >> ------------------------------
> >> | unsigned int id |
> >> ------------------------------
> >> | unsigned int flags |
> >> ------------------------------
> >> | __kernel_size_t ctx_len |
> >> ------------------------------
> >> | unsigned char ctx[ctx_len] |
> >> ------------------------------
> >>
> >> Signed-off-by: Casey Schaufler <[email protected]>
> >> ---
> >> include/linux/syscalls.h | 2 +
> >> include/uapi/linux/lsm.h | 21 ++++++
> >> kernel/sys_ni.c | 3 +
> >> security/Makefile | 1 +
> >> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 183 insertions(+)
> >> create mode 100644 security/lsm_syscalls.c
> > ..
> >
> >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> >> index 61e13b1b9ece..1d27fb5b7746 100644
> >> --- a/include/uapi/linux/lsm.h
> >> +++ b/include/uapi/linux/lsm.h
> >> @@ -9,6 +9,27 @@
> >> #ifndef _UAPI_LINUX_LSM_H
> >> #define _UAPI_LINUX_LSM_H
> >>
> >> +#include <linux/types.h>
> >> +#include <linux/unistd.h>
> >> +
> >> +/**
> >> + * struct lsm_ctx - LSM context
> >> + * @id: the LSM id number, see LSM_ID_XXX
> >> + * @flags: context specifier and LSM specific flags
> >> + * @ctx_len: the size of @ctx
> >> + * @ctx: the LSM context, a nul terminated string
> >> + *
> >> + * @ctx in a nul terminated string.
> >> + * (strlen(@ctx) < @ctx_len) is always true.
> >> + * (strlen(@ctx) == @ctx_len + 1) is not guaranteed.
> >> + */
> > We can do better than this, or rather we *should* do better than this.
> > One of the big advantages of creating a new API is we can fix some of
> > the silly things we have had to do in the past, including the
> > "sometimes" NUL terminator on strings. For this LSM syscalls let's
> > make a promise that all human readable strings will be properly NUL
> > terminated;
>
> It requires effort and buffer management to ensure that ctx_len == strlen(ctx)+1,
> but if you think it's important, sure.

I do, yes. A safe, familiar, and consistent API is worth a little
extra work. Ensuring the human readable strings are always nul
terminated is familiar to most everyone who has sat in from of a code
editor, and making sure that the @ctx_len variable indicates the full
length of the @ctx buffer (both for strings and binary blobs) provides
a consistent way to manage the context, even if the application isn't
aware of the exact LSM-specific format.

> > currently this includes all LSM contexts, and likely will
> > remain that way forever for various reasons, but let's leave the door
> > open for arbitrary blobs (see the "special" LSM ID discussion from
> > earlier in the patchset). With that in mind I might suggest the
> > following:
> >
> > /**
> > * struct lsm_ctx - LSM context
> > * @id: the LSM id number, see LSM_ID_XXX
> > * @flags: context specifier and LSM specific flags
> > * @ctx_len: the size of @ctx
> > * @ctx: the LSM context, see description
> > *
> > * For LSMs which provide human readable contexts @ctx will be a nul
> > * terminated string and @ctx_len will include the size of the string
> > * and the nul terminator, e.g. 'ctx_len == strlen(ctx) + 1'. For LSMs
> > * which provide binary-only contexts @cts will be a binary blob with
> > * @ctx_len being the exact value of the blob. The type of the @ctx,
> > * human readable string or binary, can be determined by inspecting
> > * both @id and @flags.
>
> I'd go a touch further, defining LSM_ATTR_BINARY as a flag and demanding
> that any attribute that isn't a nul terminated string be thus identified,
> even if it is always binary. Thus, LSM_ATTR_CURRENT and LSM_ATTR_BINARY
> together would be an error.

No, the class/format of the context (string or binary, and the LSM
specific formatting for each) can be deduced from the LSM ID, @id, and
if necessary the @flags field. I don't want this API to explicitly
prevent a binary LSM_ATTR_CURRENT if the rest of the system is
modified to support it in the future.

> > *
> > * If a given LSM @id does not define a set of values for use in the
> > * @flags field, @flags MUST be set to zero.
> > */
> > struct lsm_ctx {
> > __u32 id;
> > __U32 flags;
> > __kernel_size_t ctx_len;
> > __u8 ctx[];
> > };
> >
> >> +struct lsm_ctx {
> >> + unsigned int id;
> >> + unsigned int flags;
> >> + __kernel_size_t ctx_len;
> >> + unsigned char ctx[];
> >> +};
> > I agree with Greg, we should be explicit about variable sizing, let's
> > make sure everything in the UAPI header is defined in terms of
> > __uXX/__sXX. This includes strings as __u8 arrays.
> >
> > Also, I sorta despite the 'let's line all the struct fields up
> > horizontally!' approach in struct/variable definitions.
>
> Kids. Got no respect for tradition.

I think you meant to say, "Kids. Got no respect
for tradition."

> > I personally
> > think it looks horrible and it clutters up future patches. Please
> > don't do this unless the individual file already does it, and since we
> > are creating this new please don't :)
> >
> >> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> >> new file mode 100644
> >> index 000000000000..da0fab7065e2
> >> --- /dev/null
> >> +++ b/security/lsm_syscalls.c
> >> @@ -0,0 +1,156 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * System calls implementing the Linux Security Module API.
> >> + *
> >> + * Copyright (C) 2022 Casey Schaufler <[email protected]>
> >> + * Copyright (C) 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>
> >> +
> >> +struct feature_map {
> >> + char *name;
> >> + int feature;
> >> +};
> >> +
> >> +static const struct feature_map lsm_attr_names[] = {
> >> + { .name = "current", .feature = LSM_ATTR_CURRENT, },
> >> + { .name = "exec", .feature = LSM_ATTR_EXEC, },
> >> + { .name = "fscreate", .feature = LSM_ATTR_FSCREATE, },
> >> + { .name = "keycreate", .feature = LSM_ATTR_KEYCREATE, },
> >> + { .name = "prev", .feature = LSM_ATTR_PREV, },
> >> + { .name = "sockcreate", .feature = LSM_ATTR_SOCKCREATE, },
> >> +};
> >> +
> >> +/**
> >> + * lsm_self_attr - Return current task's security module attributes
> >> + * @ctx: the LSM contexts
> >> + * @size: size of @ctx, updated on return
> >> + * @flags: reserved for future use, must be zero
> >> + *
> >> + * 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_DEFINE3(lsm_self_attr,
> >> + struct lsm_ctx __user *, ctx,
> >> + size_t __user *, size,
> >> + int, flags)
> > See my comments above about UAPI types, let's change this to something
> > like this:
> >
> > [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
> >
> > SYSCALL_DEFINE3(lsm_self_attr,
> > struct lsm_ctx __user *, ctx,
> > __kernel_size_t __user *, size,
> > __u32, flags)
> >
> >> +{
> >> + struct lsm_ctx *final = NULL;
> >> + struct lsm_ctx *interum;
> >> + struct lsm_ctx *ip;
> >> + void *curr;
> >> + char **interum_ctx;
> >> + char *cp;
> >> + size_t total_size = 0;
> >> + int count = 0;
> >> + int attr;
> >> + int len;
> >> + int rc = 0;
> >> + int i;
> > Ungh, reverse christmas trees ... I kinda hate it from a style
> > perspective, enough to mention it here, but I'm not going to be petty
> > enough to say "change it".
>
> I really don't care. Last I saw reverse christmas tree was the officially
> recommended style. I don't care one way or the other.

I think it is one of those per-subsystem oddities like it or not.

> >
> > However, if you did want to flip it upside down (normal christmas
> > tree?) during the respin I would be grateful ;)
> >

--
paul-moore.com

2022-11-11 00:05:22

by Paul Moore

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Wed, Nov 9, 2022 at 6:34 PM Paul Moore <[email protected]> wrote:
> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
> >
> > Create a system call lsm_self_attr() to provide the security
> > module maintained attributes of the current process. Historically
> > these attributes have been exposed to user space via entries in
> > procfs under /proc/self/attr.
> >
> > Attributes are provided as a collection of lsm_ctx structures
> > which are placed into a user supplied buffer. Each structure
> > identifys the security module providing the attribute, which
> > of the possible attributes is provided, the size of the
> > attribute, and finally the attribute value. The format of the
> > attribute value is defined by the security module, but will
> > always be \0 terminated. The ctx_len value will be larger than
> > strlen(ctx).
> >
> > ------------------------------
> > | unsigned int id |
> > ------------------------------
> > | unsigned int flags |
> > ------------------------------
> > | __kernel_size_t ctx_len |
> > ------------------------------
> > | unsigned char ctx[ctx_len] |
> > ------------------------------
> > | unsigned int id |
> > ------------------------------
> > | unsigned int flags |
> > ------------------------------
> > | __kernel_size_t ctx_len |
> > ------------------------------
> > | unsigned char ctx[ctx_len] |
> > ------------------------------
> >
> > Signed-off-by: Casey Schaufler <[email protected]>
> > ---
> > include/linux/syscalls.h | 2 +
> > include/uapi/linux/lsm.h | 21 ++++++
> > kernel/sys_ni.c | 3 +
> > security/Makefile | 1 +
> > security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 183 insertions(+)
> > create mode 100644 security/lsm_syscalls.c

...

> > +/**
> > + * lsm_self_attr - Return current task's security module attributes
> > + * @ctx: the LSM contexts
> > + * @size: size of @ctx, updated on return
> > + * @flags: reserved for future use, must be zero
> > + *
> > + * 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_DEFINE3(lsm_self_attr,
> > + struct lsm_ctx __user *, ctx,
> > + size_t __user *, size,
> > + int, flags)
>
> See my comments above about UAPI types, let's change this to something
> like this:
>
> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
>
> SYSCALL_DEFINE3(lsm_self_attr,
> struct lsm_ctx __user *, ctx,
> __kernel_size_t __user *, size,
> __u32, flags)
>

I wanted to clarify how I originally envisioned this syscall/API, as
it looks like it behaves a bit differently than I originally intended.
My thought was that this syscall would be used to fetch one LSM
attribute context at a time, returning an array of lsm_ctx structs,
with one, and only one, for each LSM that supports that particular
attribute. If the LSM does not support that attribute, it must not
enter an entry to the array. Requesting more than one attribute
context per invocation is not allowed. The idea was to closely
resemble the familiar open("/proc/self/attr/current")/read()/close()
result without relying on procfs and supporting multiple LSMs with an
easy(ish) API. The new, single syscall should also be faster,
although none of this should be happening in a performance critical
section anyway.

In addition, the lsm_ctx::flags field is intended to convey
information specific to the given LSM, i.e. it should not repeat any
of the flag information specified in the syscall parameters. I don't
believe any of the currently in-tree LSMs would require any special
flags for their contexts, so this should always be zero/clear in this
initial patchset, but it is something to keep in mind for the future.

Thoughts?

--
paul-moore.com

2022-11-11 00:41:18

by Casey Schaufler

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On 11/10/2022 3:36 PM, Paul Moore wrote:
> On Wed, Nov 9, 2022 at 6:34 PM Paul Moore <[email protected]> wrote:
>> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
>>> Create a system call lsm_self_attr() to provide the security
>>> module maintained attributes of the current process. Historically
>>> these attributes have been exposed to user space via entries in
>>> procfs under /proc/self/attr.
>>>
>>> Attributes are provided as a collection of lsm_ctx structures
>>> which are placed into a user supplied buffer. Each structure
>>> identifys the security module providing the attribute, which
>>> of the possible attributes is provided, the size of the
>>> attribute, and finally the attribute value. The format of the
>>> attribute value is defined by the security module, but will
>>> always be \0 terminated. The ctx_len value will be larger than
>>> strlen(ctx).
>>>
>>> ------------------------------
>>> | unsigned int id |
>>> ------------------------------
>>> | unsigned int flags |
>>> ------------------------------
>>> | __kernel_size_t ctx_len |
>>> ------------------------------
>>> | unsigned char ctx[ctx_len] |
>>> ------------------------------
>>> | unsigned int id |
>>> ------------------------------
>>> | unsigned int flags |
>>> ------------------------------
>>> | __kernel_size_t ctx_len |
>>> ------------------------------
>>> | unsigned char ctx[ctx_len] |
>>> ------------------------------
>>>
>>> Signed-off-by: Casey Schaufler <[email protected]>
>>> ---
>>> include/linux/syscalls.h | 2 +
>>> include/uapi/linux/lsm.h | 21 ++++++
>>> kernel/sys_ni.c | 3 +
>>> security/Makefile | 1 +
>>> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
>>> 5 files changed, 183 insertions(+)
>>> create mode 100644 security/lsm_syscalls.c
> ..
>
>>> +/**
>>> + * lsm_self_attr - Return current task's security module attributes
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx, updated on return
>>> + * @flags: reserved for future use, must be zero
>>> + *
>>> + * 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_DEFINE3(lsm_self_attr,
>>> + struct lsm_ctx __user *, ctx,
>>> + size_t __user *, size,
>>> + int, flags)
>> See my comments above about UAPI types, let's change this to something
>> like this:
>>
>> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
>>
>> SYSCALL_DEFINE3(lsm_self_attr,
>> struct lsm_ctx __user *, ctx,
>> __kernel_size_t __user *, size,
>> __u32, flags)
>>
> I wanted to clarify how I originally envisioned this syscall/API, as
> it looks like it behaves a bit differently than I originally intended.

That's why we're having a review cycle.

> My thought was that this syscall would be used to fetch one LSM
> attribute context at a time, returning an array of lsm_ctx structs,
> with one, and only one, for each LSM that supports that particular
> attribute.

My thought with the interface I proposed is that we don't want a
separate syscall for each attribute: e.g. lsm_get_current(), lsm_get_prev(),
and we don't want a separate syscall for each LSM, e.g. lsm_get_selinux().
We can either specify which attribute is desired or which have been returned.
In either case we need an attribute identifier.

> If the LSM does not support that attribute, it must not
> enter an entry to the array. Requesting more than one attribute
> context per invocation is not allowed.

Why? That seems like an unnecessary and inconvenient restriction
for the program that wants to see more than one attribute. A service
that wants to check its fscreate, sockcreate and keycreate to see if
they're appropriate for the container it's running in, for example.

> The idea was to closely
> resemble the familiar open("/proc/self/attr/current")/read()/close()
> result without relying on procfs and supporting multiple LSMs with an
> easy(ish) API.

rc = lsm_get_self_attr(struct lsm_ctx *ctx, size, attr_id, flags);

This looks like what you're asking for. It's what I had proposed but with
the attr_id specified in the call rather than returned in the lsm_ctx.

> The new, single syscall should also be faster,
> although none of this should be happening in a performance critical
> section anyway.

Yes.

> In addition, the lsm_ctx::flags field is intended to convey
> information specific to the given LSM, i.e. it should not repeat any
> of the flag information specified in the syscall parameters. I don't
> believe any of the currently in-tree LSMs would require any special
> flags for their contexts, so this should always be zero/clear in this
> initial patchset, but it is something to keep in mind for the future.
>
> Thoughts?

I don't have any problem with *what I think* you're suggesting.
To make sure, I'll send a new patch. I'll also address lsm_set_self_attr().
Thank you for the feedback. Let's see if we can converge.

>
> --
> paul-moore.com

2022-11-11 03:39:42

by Paul Moore

[permalink] [raw]

Subject: Re: [PATCH v1 6/8] LSM: lsm_self_attr syscall for LSM self attributes

On Thu, Nov 10, 2022 at 7:36 PM Casey Schaufler <[email protected]> wrote:
> On 11/10/2022 3:36 PM, Paul Moore wrote:
> > On Wed, Nov 9, 2022 at 6:34 PM Paul Moore <[email protected]> wrote:
> >> On Tue, Oct 25, 2022 at 2:48 PM Casey Schaufler <[email protected]> wrote:
> >>> Create a system call lsm_self_attr() to provide the security
> >>> module maintained attributes of the current process. Historically
> >>> these attributes have been exposed to user space via entries in
> >>> procfs under /proc/self/attr.
> >>>
> >>> Attributes are provided as a collection of lsm_ctx structures
> >>> which are placed into a user supplied buffer. Each structure
> >>> identifys the security module providing the attribute, which
> >>> of the possible attributes is provided, the size of the
> >>> attribute, and finally the attribute value. The format of the
> >>> attribute value is defined by the security module, but will
> >>> always be \0 terminated. The ctx_len value will be larger than
> >>> strlen(ctx).
> >>>
> >>> ------------------------------
> >>> | unsigned int id |
> >>> ------------------------------
> >>> | unsigned int flags |
> >>> ------------------------------
> >>> | __kernel_size_t ctx_len |
> >>> ------------------------------
> >>> | unsigned char ctx[ctx_len] |
> >>> ------------------------------
> >>> | unsigned int id |
> >>> ------------------------------
> >>> | unsigned int flags |
> >>> ------------------------------
> >>> | __kernel_size_t ctx_len |
> >>> ------------------------------
> >>> | unsigned char ctx[ctx_len] |
> >>> ------------------------------
> >>>
> >>> Signed-off-by: Casey Schaufler <[email protected]>
> >>> ---
> >>> include/linux/syscalls.h | 2 +
> >>> include/uapi/linux/lsm.h | 21 ++++++
> >>> kernel/sys_ni.c | 3 +
> >>> security/Makefile | 1 +
> >>> security/lsm_syscalls.c | 156 +++++++++++++++++++++++++++++++++++++++
> >>> 5 files changed, 183 insertions(+)
> >>> create mode 100644 security/lsm_syscalls.c
> > ..
> >
> >>> +/**
> >>> + * lsm_self_attr - Return current task's security module attributes
> >>> + * @ctx: the LSM contexts
> >>> + * @size: size of @ctx, updated on return
> >>> + * @flags: reserved for future use, must be zero
> >>> + *
> >>> + * 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_DEFINE3(lsm_self_attr,
> >>> + struct lsm_ctx __user *, ctx,
> >>> + size_t __user *, size,
> >>> + int, flags)
> >> See my comments above about UAPI types, let's change this to something
> >> like this:
> >>
> >> [NOTE: I'm assuming it is safe to use __XXX types in syscall declarations?]
> >>
> >> SYSCALL_DEFINE3(lsm_self_attr,
> >> struct lsm_ctx __user *, ctx,
> >> __kernel_size_t __user *, size,
> >> __u32, flags)
> >>
> > I wanted to clarify how I originally envisioned this syscall/API, as
> > it looks like it behaves a bit differently than I originally intended.
>
> That's why we're having a review cycle.

:)

> > My thought was that this syscall would be used to fetch one LSM
> > attribute context at a time, returning an array of lsm_ctx structs,
> > with one, and only one, for each LSM that supports that particular
> > attribute.
>
> My thought with the interface I proposed is that we don't want a
> separate syscall for each attribute: e.g. lsm_get_current(), lsm_get_prev(),
> and we don't want a separate syscall for each LSM, e.g. lsm_get_selinux().

Agreed. I believe that proposed syscall prototype, leveraging the
@flags parameter, supports this.

> We can either specify which attribute is desired or which have been returned.
> In either case we need an attribute identifier.

I never intended the lsm_ctx::flags field to indicate the attribute
itself, e.g. LSM_ATTR_CURRENT, that is for future use by the
individual LSM which is providing the attribute information in that
lsm_ctx array slot. With that in mind, there is no way for the
syscall to return *what* attribute is returned, only the attribute
value. This is intentional as I wanted to avoid using this syscall to
fetch different attributes at the same time (one attribute, with
multiple LSMs providing values is the intent).

The "why" is explained below ...

> > If the LSM does not support that attribute, it must not
> > enter an entry to the array. Requesting more than one attribute
> > context per invocation is not allowed.
>
> Why? That seems like an unnecessary and inconvenient restriction
> for the program that wants to see more than one attribute. A service
> that wants to check its fscreate, sockcreate and keycreate to see if
> they're appropriate for the container it's running in, for example.

First off, the currently defined attributes are *very* different in
nature so it is unlikely that a chunk of application code would want
to manipulate more than one in a given function. That alone is a
fairly weak justification, but the idea that attributes aren't related
is important when one considers the access controls a LSM may place
around the management of these attributes. Combining multiple
attribute requests in a single syscall could increase confusion as
when one of the requests is blocked but others are allowed. What do
you do here, do you fail the entire syscall or supply just the
attribute that is allowed? If you do supply just the attribute that
is allowed, do you return an error code? How would you be able to
distinguish from a LSM which chose not return an attribute and one
that is actively blocking access to that attribute?

Supporting multiple attributes gets complicated very quickly. Given
the syscall prototype where we treat the flag parameter as a bitfield,
we do allow ourselves the possibility of supporting this in the
future, but let's keep it simple right now.

It's also important to note that currently applications cannot request
multiple attributes via one action. Even requesting one single
attribute requires a minimum of three syscalls, so we're definitely
improving things here with this syscall.

> > The idea was to closely
> > resemble the familiar open("/proc/self/attr/current")/read()/close()
> > result without relying on procfs and supporting multiple LSMs with an
> > easy(ish) API.
>
> rc = lsm_get_self_attr(struct lsm_ctx *ctx, size, attr_id, flags);
>
> This looks like what you're asking for. It's what I had proposed but with
> the attr_id specified in the call rather than returned in the lsm_ctx.

I was thinking that the attr_id would be conveyed as part of the flags
parameter, but I suppose keeping them separate makes life easier (no
worries about flag collisions with the much more generic attribute
ID). I would suggest that we still treat the attribute parameter as a
bitfield to allow for the possibility of multiple attributes in one
request; as well as a reordering of the parameters:

int lsm_get_self_attr(__u32 attr,
struct lsm_ctx *ctx,
__kernel_size_t *size,
__u32 flags);

> > Thoughts?
>
> I don't have any problem with *what I think* you're suggesting.
> To make sure, I'll send a new patch. I'll also address lsm_set_self_attr().
> Thank you for the feedback. Let's see if we can converge.

Sounds good. Thanks Casey.

--
paul-moore.com