| Message ID | 20230411155921.14716-5-casey@schaufler-ca.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | LSM: Three basic syscalls | expand |
Hi Casey,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/perf/core]
[also build test ERROR on acme/perf/core shuah-kselftest/next shuah-kselftest/fixes linus/master v6.3-rc6]
[cannot apply to next-20230412]
[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/20230412-005929
patch link: https://lore.kernel.org/r/20230411155921.14716-5-casey%40schaufler-ca.com
patch subject: [PATCH v8 04/11] LSM: syscalls for current process attributes
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20230412/202304121843.QWCxhPE9-lkp@intel.com/config)
compiler: m68k-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/0308dbcd345400b5ded1e1721045c6941979c7a6
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/20230412-005929
git checkout 0308dbcd345400b5ded1e1721045c6941979c7a6
# 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=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304121843.QWCxhPE9-lkp@intel.com/
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 *, __u32)' {aka 'long int(unsigned int, struct lsm_ctx *, unsigned int)'}
1064 | asmlinkage long sys_lsm_set_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
On Tue, Apr 11, 2023 at 12:01 PM Casey Schaufler <casey@schaufler-ca.com> 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 > identifies 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 <casey@schaufler-ca.com> > --- > 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 | 30 +++++++++ > kernel/sys_ni.c | 4 ++ > security/Makefile | 1 + > security/lsm_syscalls.c | 55 ++++++++++++++++ > security/security.c | 98 +++++++++++++++++++++++++++++ > 10 files changed, 240 insertions(+) > create mode 100644 security/lsm_syscalls.c ... > diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h > index 33a0ee3bcb2e..97487d66dca9 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, __u32 flags); > +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx, > + __u32 flags); As pointed out by the kernel test robot, the above declaration is missing the @size parameter. > diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h > index f27c9a9cc376..b10dfab8a4d9 100644 > --- a/include/uapi/linux/lsm.h > +++ b/include/uapi/linux/lsm.h > @@ -9,6 +9,36 @@ > #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[]; > +}; Sorry, style nitpick since this needs to be respun anyway for the syscalls.h fix at the very least ... I *really* dislike when variable declarations, and field declarations in the case composite variables, are aligned with the vertically neighboring declarations; just use a single space please: struct lsm_ctx { <tab>__u64 id; <tab>__u64 flags; <tab>__u64 len; <tab>__u64 ctx_len; <tab>__u8 ctx[]; } > diff --git a/security/security.c b/security/security.c > index 38ca0e646cac..bfe9a1a426b2 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2167,6 +2167,104 @@ 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; > + u8 __user *base = (u8 __user *)ctx; > + size_t total = 0; > + size_t entrysize; > + size_t left; > + bool toobig = false; > + int count = 0; > + int rc; > + > + if (attr == 0) > + return -EINVAL; > + if (flags) > + return -EINVAL; I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which allows one to request a single LSM's attribute. I don't think that support has to be part of this initial patchset, but I do think it would be good to have it in the same PR that goes up to Linus during the merge window. If that's not something you want to do, let me know and I'll write up a quick patch on top of this patchset. > + if (size == NULL) > + return -EINVAL; > + if (get_user(left, size)) > + return -EFAULT; > + > + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) { > + entrysize = left; > + if (base) > + ctx = (struct lsm_ctx __user *)(base + total); > + rc = hp->hook.getselfattr(attr, ctx, &entrysize, flags); > + if (rc == -EOPNOTSUPP) { > + rc = 0; > + continue; > + } > + if (rc == -E2BIG) { > + toobig = true; > + left = 0; > + break; It just occurred to me while reading this that we stop calculating the potential lsm_ctx size after we hit the first LSM where we go beyond the size given, `rc == -E2BIG`. I realize that the required size may change between calls to lsm_get_self_attr(2), but it seems like we should at least run through all the LSMs and total up the required buffer size, no? I may have missed something in the snippet below, but I think the code change should be pretty minor: if (rc == -EOPNOTSUPP) { rc = 0; continue; } else if (rc == -E2BIG) { toobig = true; base = NULL; } else if (rc < 0) { return rc; } left = (toobig ? 0 : left - entrysize); total += entrysize; count += rc; > + } > + if (rc < 0) > + return rc; > + > + left -= entrysize; > + total += entrysize; > + count += rc; > + } > + if (count == 0) > + return LSM_RET_DEFAULT(getselfattr); > + if (put_user(total, size)) > + return -EFAULT; > + if (toobig) > + return -E2BIG; > + return count; > +} -- paul-moore.com
On 4/18/2023 2:49 PM, Paul Moore wrote: > On Tue, Apr 11, 2023 at 12:01 PM Casey Schaufler <casey@schaufler-ca.com> 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 >> identifies 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 <casey@schaufler-ca.com> >> --- >> 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 | 30 +++++++++ >> kernel/sys_ni.c | 4 ++ >> security/Makefile | 1 + >> security/lsm_syscalls.c | 55 ++++++++++++++++ >> security/security.c | 98 +++++++++++++++++++++++++++++ >> 10 files changed, 240 insertions(+) >> create mode 100644 security/lsm_syscalls.c > .. > >> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h >> index 33a0ee3bcb2e..97487d66dca9 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, __u32 flags); >> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx, >> + __u32 flags); > As pointed out by the kernel test robot, the above declaration is > missing the @size parameter. Yup. >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h >> index f27c9a9cc376..b10dfab8a4d9 100644 >> --- a/include/uapi/linux/lsm.h >> +++ b/include/uapi/linux/lsm.h >> @@ -9,6 +9,36 @@ >> #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[]; >> +}; > Sorry, style nitpick since this needs to be respun anyway for the > syscalls.h fix at the very least ... I *really* dislike when variable > declarations, and field declarations in the case composite variables, > are aligned with the vertically neighboring declarations; just use a > single space please: I'll do it, but the tab after type has been accepted for forever. As an aside, I pulled out my 1978 K&R to prove my point and discovered that it isn't consistent regarding this style. > struct lsm_ctx { > <tab>__u64 id; > <tab>__u64 flags; > <tab>__u64 len; > <tab>__u64 ctx_len; > <tab>__u8 ctx[]; > } > >> diff --git a/security/security.c b/security/security.c >> index 38ca0e646cac..bfe9a1a426b2 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -2167,6 +2167,104 @@ 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; >> + u8 __user *base = (u8 __user *)ctx; >> + size_t total = 0; >> + size_t entrysize; >> + size_t left; >> + bool toobig = false; >> + int count = 0; >> + int rc; >> + >> + if (attr == 0) >> + return -EINVAL; >> + if (flags) >> + return -EINVAL; > I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which > allows one to request a single LSM's attribute. I don't, but I'll incorporate it. > I don't think that > support has to be part of this initial patchset, but I do think it > would be good to have it in the same PR that goes up to Linus during > the merge window. As this patch set is intended to be what goes to Linus (isn't it?) I'll put it in. > If that's not something you want to do, let me know > and I'll write up a quick patch on top of this patchset. > >> + if (size == NULL) >> + return -EINVAL; >> + if (get_user(left, size)) >> + return -EFAULT; >> + >> + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) { >> + entrysize = left; >> + if (base) >> + ctx = (struct lsm_ctx __user *)(base + total); >> + rc = hp->hook.getselfattr(attr, ctx, &entrysize, flags); >> + if (rc == -EOPNOTSUPP) { >> + rc = 0; >> + continue; >> + } >> + if (rc == -E2BIG) { >> + toobig = true; >> + left = 0; >> + break; > It just occurred to me while reading this that we stop calculating the > potential lsm_ctx size after we hit the first LSM where we go beyond > the size given, `rc == -E2BIG`. I realize that the required size may > change between calls to lsm_get_self_attr(2), but it seems like we > should at least run through all the LSMs and total up the required > buffer size, no? The "break" should be a "continue". Artifact of an earlier version that had a switch statement. Fix forthcoming. > > I may have missed something in the snippet below, but I think the code > change should be pretty minor: > > if (rc == -EOPNOTSUPP) { > rc = 0; > continue; > } else if (rc == -E2BIG) { > toobig = true; > base = NULL; > } else if (rc < 0) { > return rc; > } > left = (toobig ? 0 : left - entrysize); > total += entrysize; > > count += rc; > >> + } >> + if (rc < 0) >> + return rc; >> + >> + left -= entrysize; >> + total += entrysize; >> + count += rc; >> + } >> + if (count == 0) >> + return LSM_RET_DEFAULT(getselfattr); >> + if (put_user(total, size)) >> + return -EFAULT; >> + if (toobig) >> + return -E2BIG; >> + return count; >> +} > -- > paul-moore.com
On Tue, Apr 18, 2023 at 6:34 PM Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/18/2023 2:49 PM, Paul Moore wrote: > > On Tue, Apr 11, 2023 at 12:01 PM Casey Schaufler <casey@schaufler-ca.com> 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 > >> identifies 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 <casey@schaufler-ca.com> > >> --- > >> 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 | 30 +++++++++ > >> kernel/sys_ni.c | 4 ++ > >> security/Makefile | 1 + > >> security/lsm_syscalls.c | 55 ++++++++++++++++ > >> security/security.c | 98 +++++++++++++++++++++++++++++ > >> 10 files changed, 240 insertions(+) > >> create mode 100644 security/lsm_syscalls.c ... > >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h > >> index f27c9a9cc376..b10dfab8a4d9 100644 > >> --- a/include/uapi/linux/lsm.h > >> +++ b/include/uapi/linux/lsm.h > >> @@ -9,6 +9,36 @@ > >> #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[]; > >> +}; > > Sorry, style nitpick since this needs to be respun anyway for the > > syscalls.h fix at the very least ... I *really* dislike when variable > > declarations, and field declarations in the case composite variables, > > are aligned with the vertically neighboring declarations; just use a > > single space please: > > I'll do it, but the tab after type has been accepted for forever. > As an aside, I pulled out my 1978 K&R to prove my point and discovered > that it isn't consistent regarding this style. Yeah, it's not a question of acceptance or rejection, it's a matter of personal preference. I guess one of the few perks of this job is that I can occasionally ask people to write code in a style that makes me happy :) > > struct lsm_ctx { > > <tab>__u64 id; > > <tab>__u64 flags; > > <tab>__u64 len; > > <tab>__u64 ctx_len; > > <tab>__u8 ctx[]; > > } > > > >> diff --git a/security/security.c b/security/security.c > >> index 38ca0e646cac..bfe9a1a426b2 100644 > >> --- a/security/security.c > >> +++ b/security/security.c > >> @@ -2167,6 +2167,104 @@ 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; > >> + u8 __user *base = (u8 __user *)ctx; > >> + size_t total = 0; > >> + size_t entrysize; > >> + size_t left; > >> + bool toobig = false; > >> + int count = 0; > >> + int rc; > >> + > >> + if (attr == 0) > >> + return -EINVAL; > >> + if (flags) > >> + return -EINVAL; > > > > I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which > > allows one to request a single LSM's attribute. > > I don't, but I'll incorporate it. Thank you. > > I don't think that > > support has to be part of this initial patchset, but I do think it > > would be good to have it in the same PR that goes up to Linus during > > the merge window. > > As this patch set is intended to be what goes to Linus (isn't it?) > I'll put it in. Yes, but it goes to Linus via the lsm/next branch, which will likely contain other patches as well; one of these other patches could be a LSM_FLG_SINGLE patch if you wanted to focus on what you have here and leave that to someone else. Either way, it all goes up to Linus in one PR so I'm not too worried either way. What I do want to avoid is shipping the initial support in version N, and shipping LSM_FLG_SINGLE in release N+1.
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 8e6ba0a9896e..ed38ad5eb444 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..f7292890b6a2 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 -EOPNOTSUPP; +} + +static inline int security_setselfattr(unsigned int __user attr, + struct lsm_ctx __user *ctx, + size_t __user size, u32 __user flags) +{ + return -EOPNOTSUPP; +} + 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..97487d66dca9 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, __u32 flags); +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx, + __u32 flags); /* * Architecture-specific system calls diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h index f27c9a9cc376..b10dfab8a4d9 100644 --- a/include/uapi/linux/lsm.h +++ b/include/uapi/linux/lsm.h @@ -9,6 +9,36 @@ #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[]; +}; + /* * 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 <casey@schaufler-ca.com> + * 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 38ca0e646cac..bfe9a1a426b2 100644 --- a/security/security.c +++ b/security/security.c @@ -2167,6 +2167,104 @@ 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; + u8 __user *base = (u8 __user *)ctx; + size_t total = 0; + size_t entrysize; + size_t left; + bool toobig = false; + int count = 0; + int rc; + + if (attr == 0) + return -EINVAL; + if (flags) + return -EINVAL; + if (size == NULL) + return -EINVAL; + if (get_user(left, size)) + return -EFAULT; + + hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) { + entrysize = left; + if (base) + ctx = (struct lsm_ctx __user *)(base + total); + rc = hp->hook.getselfattr(attr, ctx, &entrysize, flags); + if (rc == -EOPNOTSUPP) { + rc = 0; + continue; + } + if (rc == -E2BIG) { + toobig = true; + left = 0; + break; + } + if (rc < 0) + return rc; + + left -= entrysize; + total += entrysize; + count += rc; + } + if (count == 0) + return LSM_RET_DEFAULT(getselfattr); + if (put_user(total, size)) + return -EFAULT; + if (toobig) + 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, -EINVAL if the input is inconsistent, -EFAULT + * if the user buffer is inaccessible or an LSM specific 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) + return -EINVAL; + if (size < sizeof(*ctx)) + return -EINVAL; + if (copy_from_user(&lctx, ctx, sizeof(*ctx))) + return -EFAULT; + if (size < lctx.len || size < lctx.ctx_len + sizeof(ctx) || + lctx.len < lctx.ctx_len + sizeof(ctx)) + return -EINVAL; + + 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) {
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 identifies 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 <casey@schaufler-ca.com> --- 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 | 30 +++++++++ kernel/sys_ni.c | 4 ++ security/Makefile | 1 + security/lsm_syscalls.c | 55 ++++++++++++++++ security/security.c | 98 +++++++++++++++++++++++++++++ 10 files changed, 240 insertions(+) create mode 100644 security/lsm_syscalls.c