[v7,04/11] LSM: syscalls for current process attributes

Commit Message

Casey Schaufler March 15, 2023, 10:46 p.m. UTC

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

The attribute value is provided in a lsm_ctx structure. The structure
identifys the size of the attribute, and the attribute value. The format
of the attribute value is defined by the security module. A flags field
is included for LSM specific information. It is currently unused and must
be 0. The total size of the data, including the lsm_ctx structure and any
padding, is maintained as well.

struct lsm_ctx {
        __u64   id;
        __u64   flags;
        __u64   len;
        __u64   ctx_len;
        __u8    ctx[];
};

Two new LSM hooks are used to interface with the LSMs.
security_getselfattr() collects the lsm_ctx values from the
LSMs that support the hook, accounting for space requirements.
security_setselfattr() identifies which LSM the attribute is
intended for and passes it along.

Signed-off-by: Casey Schaufler <[e-mail]>
---
 Documentation/userspace-api/lsm.rst | 15 +++++
 include/linux/lsm_hook_defs.h       |  4 ++
 include/linux/lsm_hooks.h           |  9 +++
 include/linux/security.h            | 19 ++++++
 include/linux/syscalls.h            |  5 ++
 include/uapi/linux/lsm.h            | 33 ++++++++++
 kernel/sys_ni.c                     |  4 ++
 security/Makefile                   |  1 +
 security/lsm_syscalls.c             | 55 ++++++++++++++++
 security/security.c                 | 97 +++++++++++++++++++++++++++++
 10 files changed, 242 insertions(+)
 create mode 100644 security/lsm_syscalls.c

Comments

kernel test robot March 16, 2023, 12:35 p.m. UTC | #1

Hi Casey,

I love your patch! Yet something to improve:

[auto build test ERROR on shuah-kselftest/next]
[also build test ERROR on shuah-kselftest/fixes linus/master v6.3-rc2]
[cannot apply to tip/perf/core acme/perf/core next-20230316]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Casey-Schaufler/LSM-Maintain-a-table-of-LSM-attribute-data/20230316-074751
base:   https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git next
patch link:    https://lore.kernel.org/r/20230315224704.2672-5-casey%40schaufler-ca.com
patch subject: [PATCH v7 04/11] LSM: syscalls for current process attributes
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20230316/[e-mail]/config)
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/0883a93af669a6fcb80a9cc74737d5285a1c46ae
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Casey-Schaufler/LSM-Maintain-a-table-of-LSM-attribute-data/20230316-074751
        git checkout 0883a93af669a6fcb80a9cc74737d5285a1c46ae
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[e-mail]>
| Link: https://lore.kernel.org/oe-kbuild-all/[e-mail]/

All errors (new ones prefixed by >>):

   In file included from security/lsm_syscalls.c:15:
>> include/linux/syscalls.h:243:25: error: conflicting types for 'sys_lsm_set_self_attr'; have 'long int(unsigned int,  struct lsm_ctx *, size_t,  u32)' {aka 'long int(unsigned int,  struct lsm_ctx *, unsigned int,  unsigned int)'}
     243 |         asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))       \
         |                         ^~~
   include/linux/syscalls.h:229:9: note: in expansion of macro '__SYSCALL_DEFINEx'
     229 |         __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~
   include/linux/syscalls.h:221:36: note: in expansion of macro 'SYSCALL_DEFINEx'
     221 | #define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
         |                                    ^~~~~~~~~~~~~~~
   security/lsm_syscalls.c:31:1: note: in expansion of macro 'SYSCALL_DEFINE4'
      31 | SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
         | ^~~~~~~~~~~~~~~
   include/linux/syscalls.h:1064:17: note: previous declaration of 'sys_lsm_set_self_attr' with type 'long int(unsigned int,  struct lsm_ctx *, __u64)' {aka 'long int(unsigned int,  struct lsm_ctx *, long long unsigned int)'}
    1064 | asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
         |                 ^~~~~~~~~~~~~~~~~~~~~
>> include/linux/syscalls.h:243:25: error: conflicting types for 'sys_lsm_get_self_attr'; have 'long int(unsigned int,  struct lsm_ctx *, size_t *, u32)' {aka 'long int(unsigned int,  struct lsm_ctx *, unsigned int *, unsigned int)'}
     243 |         asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))       \
         |                         ^~~
   include/linux/syscalls.h:229:9: note: in expansion of macro '__SYSCALL_DEFINEx'
     229 |         __SYSCALL_DEFINEx(x, sname, __VA_ARGS__)
         |         ^~~~~~~~~~~~~~~~~
   include/linux/syscalls.h:221:36: note: in expansion of macro 'SYSCALL_DEFINEx'
     221 | #define SYSCALL_DEFINE4(name, ...) SYSCALL_DEFINEx(4, _##name, __VA_ARGS__)
         |                                    ^~~~~~~~~~~~~~~
   security/lsm_syscalls.c:51:1: note: in expansion of macro 'SYSCALL_DEFINE4'
      51 | SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
         | ^~~~~~~~~~~~~~~
   include/linux/syscalls.h:1062:17: note: previous declaration of 'sys_lsm_get_self_attr' with type 'long int(unsigned int,  struct lsm_ctx *, size_t *, __u64)' {aka 'long int(unsigned int,  struct lsm_ctx *, unsigned int *, long long unsigned int)'}
    1062 | asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
         |                 ^~~~~~~~~~~~~~~~~~~~~


vim +243 include/linux/syscalls.h

1bd21c6c21e848 Dominik Brodowski   2018-04-05  232  
e145242ea0df6b Dominik Brodowski   2018-04-09  233  /*
e145242ea0df6b Dominik Brodowski   2018-04-09  234   * The asmlinkage stub is aliased to a function named __se_sys_*() which
e145242ea0df6b Dominik Brodowski   2018-04-09  235   * sign-extends 32-bit ints to longs whenever needed. The actual work is
e145242ea0df6b Dominik Brodowski   2018-04-09  236   * done within __do_sys_*().
e145242ea0df6b Dominik Brodowski   2018-04-09  237   */
1bd21c6c21e848 Dominik Brodowski   2018-04-05  238  #ifndef __SYSCALL_DEFINEx
bed1ffca022cc8 Frederic Weisbecker 2009-03-13  239  #define __SYSCALL_DEFINEx(x, name, ...)					\
bee20031772af3 Arnd Bergmann       2018-06-19  240  	__diag_push();							\
bee20031772af3 Arnd Bergmann       2018-06-19  241  	__diag_ignore(GCC, 8, "-Wattribute-alias",			\
bee20031772af3 Arnd Bergmann       2018-06-19  242  		      "Type aliasing is used to sanitize syscall arguments");\
83460ec8dcac14 Andi Kleen          2013-11-12 @243  	asmlinkage long sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))	\
e145242ea0df6b Dominik Brodowski   2018-04-09  244  		__attribute__((alias(__stringify(__se_sys##name))));	\
c9a211951c7c79 Howard McLauchlan   2018-03-21  245  	ALLOW_ERROR_INJECTION(sys##name, ERRNO);			\
e145242ea0df6b Dominik Brodowski   2018-04-09  246  	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__));\
e145242ea0df6b Dominik Brodowski   2018-04-09  247  	asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__));	\
e145242ea0df6b Dominik Brodowski   2018-04-09  248  	asmlinkage long __se_sys##name(__MAP(x,__SC_LONG,__VA_ARGS__))	\
1a94bc34768e46 Heiko Carstens      2009-01-14  249  	{								\
e145242ea0df6b Dominik Brodowski   2018-04-09  250  		long ret = __do_sys##name(__MAP(x,__SC_CAST,__VA_ARGS__));\
07fe6e00f6cca6 Al Viro             2013-01-21  251  		__MAP(x,__SC_TEST,__VA_ARGS__);				\
2cf0966683430b Al Viro             2013-01-21  252  		__PROTECT(x, ret,__MAP(x,__SC_ARGS,__VA_ARGS__));	\
2cf0966683430b Al Viro             2013-01-21  253  		return ret;						\
1a94bc34768e46 Heiko Carstens      2009-01-14  254  	}								\
bee20031772af3 Arnd Bergmann       2018-06-19  255  	__diag_pop();							\
e145242ea0df6b Dominik Brodowski   2018-04-09  256  	static inline long __do_sys##name(__MAP(x,__SC_DECL,__VA_ARGS__))
1bd21c6c21e848 Dominik Brodowski   2018-04-05  257  #endif /* __SYSCALL_DEFINEx */
1a94bc34768e46 Heiko Carstens      2009-01-14  258

Paul Moore March 30, 2023, 1:12 a.m. UTC | #2

On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[e-mail]> wrote:
>
> Create a system call lsm_get_self_attr() to provide the security
> module maintained attributes of the current process.
> Create a system call lsm_set_self_attr() to set a security
> module maintained attribute of the current process.
> Historically these attributes have been exposed to user space via
> entries in procfs under /proc/self/attr.
>
> The attribute value is provided in a lsm_ctx structure. The structure
> identifys the size of the attribute, and the attribute value. The format

"identifies"

> of the attribute value is defined by the security module. A flags field
> is included for LSM specific information. It is currently unused and must
> be 0. The total size of the data, including the lsm_ctx structure and any
> padding, is maintained as well.
>
> struct lsm_ctx {
>         __u64   id;
>         __u64   flags;
>         __u64   len;
>         __u64   ctx_len;
>         __u8    ctx[];
> };
>
> Two new LSM hooks are used to interface with the LSMs.
> security_getselfattr() collects the lsm_ctx values from the
> LSMs that support the hook, accounting for space requirements.
> security_setselfattr() identifies which LSM the attribute is
> intended for and passes it along.
>
> Signed-off-by: Casey Schaufler <[e-mail]>
> ---
>  Documentation/userspace-api/lsm.rst | 15 +++++
>  include/linux/lsm_hook_defs.h       |  4 ++
>  include/linux/lsm_hooks.h           |  9 +++
>  include/linux/security.h            | 19 ++++++
>  include/linux/syscalls.h            |  5 ++
>  include/uapi/linux/lsm.h            | 33 ++++++++++
>  kernel/sys_ni.c                     |  4 ++
>  security/Makefile                   |  1 +
>  security/lsm_syscalls.c             | 55 ++++++++++++++++
>  security/security.c                 | 97 +++++++++++++++++++++++++++++
>  10 files changed, 242 insertions(+)
>  create mode 100644 security/lsm_syscalls.c

...

> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 32285ce65419..3c2c4916bd53 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -503,6 +504,14 @@
>   *     and writing the xattrs as this hook is merely a filter.
>   * @d_instantiate:
>   *     Fill in @inode security information for a @dentry if allowed.
> + * @getselfattr:
> + *     Read attribute @attr for the current process and store it into @ctx.
> + *     Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
> + *     or another negative value otherwise.
> + * @setselfattr:
> + *     Set attribute @attr for the current process.
> + *     Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
> + *     or another negative value otherwise.
>   * @getprocattr:
>   *     Read attribute @name for process @p and store it into @value if allowed.
>   *     Return the length of @value on success, a negative value otherwise.

I'm sure you're already aware of this, but the above will need to be
moved to security.c due to the changes in the lsm/next branch.  That
said, if you're basing on Linus' tree that's fine too, I'll fix it up
during the merge; thankfully it's not a significant merge conflict.

> diff --git a/include/linux/security.h b/include/linux/security.h
> index 8faed81fc3b4..329cd9d2be50 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry,
>                                           struct inode *inode)
>  { }
>
> +static inline int security_getselfattr(unsigned int __user attr,
> +                                      struct lsm_ctx __user *ctx,
> +                                      size_t __user *size, u32 __user flags)
> +{
> +       return -EINVAL;
> +}
> +
> +static inline int security_setselfattr(unsigned int __user attr,
> +                                      struct lsm_ctx __user *ctx,
> +                                      size_t __user size, u32 __user flags)
> +{
> +       return -EINVAL;
> +}

It seems like EOPNOTSUPP might be more appropriate than EINVAL for
both of these dummy implementations.

> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
> index 33a0ee3bcb2e..3feca00cb0c1 100644
> --- a/include/linux/syscalls.h
> +++ b/include/linux/syscalls.h
> @@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>                                             unsigned long home_node,
>                                             unsigned long flags);
> +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> +                                     size_t *size, __u64 flags);
> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
> +                                     __u64 flags);

As the kernel test robot already pointed out, the above needs to be updated.

>  /*
>   * Architecture-specific system calls
> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> index aa3e01867739..adfb55dce2fd 100644
> --- a/include/uapi/linux/lsm.h
> +++ b/include/uapi/linux/lsm.h
> @@ -9,6 +9,39 @@
>  #ifndef _UAPI_LINUX_LSM_H
>  #define _UAPI_LINUX_LSM_H
>
> +#include <linux/types.h>
> +#include <linux/unistd.h>
> +
> +/**
> + * struct lsm_ctx - LSM context information
> + * @id: the LSM id number, see LSM_ID_XXX
> + * @flags: LSM specific flags
> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
> + * @ctx_len: the size of @ctx
> + * @ctx: the LSM context value
> + *
> + * The @len field MUST be equal to the size of the lsm_ctx struct
> + * plus any additional padding and/or data placed after @ctx.
> + *
> + * In all cases @ctx_len MUST be equal to the length of @ctx.
> + * If @ctx is a string value it should be nul terminated with
> + * @ctx_len equal to `strlen(@ctx) + 1`.  Binary values are
> + * supported.
> + *
> + * The @flags and @ctx fields SHOULD only be interpreted by the
> + * LSM specified by @id; they MUST be set to zero/0 when not used.
> + */
> +struct lsm_ctx {
> +       __u64   id;
> +       __u64   flags;
> +       __u64   len;
> +       __u64   ctx_len;
> +       __u8    ctx[];
> +};
> +
> +#include <linux/types.h>
> +#include <linux/unistd.h>

I'm pretty sure the repeated #includes are a typo, right?  Or is there
some uapi trick I'm missing ...

> diff --git a/security/security.c b/security/security.c
> index 87c8796c3c46..2c57fe28c4f7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>  }
>  EXPORT_SYMBOL(security_d_instantiate);
>
> +/**
> + * security_getselfattr - Read an LSM attribute of the current process.
> + * @attr: which attribute to return
> + * @ctx: the user-space destination for the information, or NULL
> + * @size: the size of space available to receive the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Returns the number of attributes found on success, negative value
> + * on error. @size is reset to the total size of the data.
> + * If @size is insufficient to contain the data -E2BIG is returned.
> + */
> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> +                        size_t __user *size, u32 __user flags)
> +{
> +       struct security_hook_list *hp;
> +       void __user *base = (void *)ctx;

The casting seems wrong for a couple of reasons: I don't believe you
need to cast the right side when the left side is a void pointer, and
the right side cast drops the '__user' attribute when the left side is
also a '__user' pointer value.

That said, I think we may want @base to be 'u8 __user *base', more on
that below ...

> +       size_t total = 0;
> +       size_t this;

Naming is hard, but 'this'?  You can do better ...

> +       size_t left;
> +       bool istoobig = false;

Sorry, more naming nits and since it looks like you need to respin
anyway ... please rename @istoobig to @toobig or something else.  The
phrases-as-variable-names has always grated on me.

> +       int count = 0;
> +       int rc;
> +
> +       if (attr == 0)
> +               return -EINVAL;
> +       if (flags != 0)
> +               return -EINVAL;
> +       if (size == NULL)
> +               return -EINVAL;
> +       if (get_user(left, size))
> +               return -EFAULT;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> +               this = left;
> +               if (base)
> +                       ctx = (struct lsm_ctx __user *)(base + total);

Pointer math on void pointers always makes me nervous.  Why not set
@base's type to a 'u8' just to remove any concerns?

> +               rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> +               switch (rc) {
> +               case -EOPNOTSUPP:
> +                       rc = 0;
> +                       continue;
> +               case -E2BIG:
> +                       istoobig = true;
> +                       left = 0;
> +                       break;
> +               case 0:
> +                       left -= this;
> +                       break;
> +               default:
> +                       return rc;

I think the @getselfattr hook should behave similarly to the
associated syscall, returning a non-negative number should indicate
that @rc entries have been added to the @ctx array.  Right now all the
LSMs would just be adding one entry to the array, but we might as well
code this up to be flexible.

> +               }
> +               total += this;
> +               count++;
> +       }
> +       if (count == 0)
> +               return LSM_RET_DEFAULT(getselfattr);
> +       if (put_user(total, size))
> +               return -EFAULT;
> +       if (rc)
> +               return rc;

Is the 'if (rc)' check needed here?  Shouldn't the switch-statement
after the hook catch everything that this check would catch?

> +       if (istoobig)
> +               return -E2BIG;
> +       return count;
> +}
> +
> +/**
> + * security_setselfattr - Set an LSM attribute on the current process.
> + * @attr: which attribute to set
> + * @ctx: the user-space source for the information
> + * @size: the size of the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Set an LSM attribute for the current process. The LSM, attribute
> + * and new value are included in @ctx.
> + *
> + * Returns 0 on success, an LSM specific value on failure.
> + */
> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> +                        size_t __user size, u32 __user flags)
> +{
> +       struct security_hook_list *hp;
> +       struct lsm_ctx lctx;

Shouldn't we check @attr for valid values and return -EINVAL if bogus?

> +       if (flags != 0)
> +               return -EINVAL;
> +       if (size < sizeof(*ctx))
> +               return -EINVAL;

If we're only going to support on 'lsm_ctx' entry in this function we
should verify that the 'len' and 'ctx_len' fields are sane.  Although
more on this below ...

> +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> +               return -EFAULT;
> +
> +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> +               if ((hp->lsmid->id) == lctx.id)
> +                       return hp->hook.setselfattr(attr, ctx, size, flags);

Can anyone think of any good reason why we shouldn't support setting
multiple LSMs in one call, similar to what we do with
security_getselfattr()?  It seems like it might be a nice thing to
have ...

> +       return LSM_RET_DEFAULT(setselfattr);
> +}
> +
>  int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>                          char **value)
>  {
> --
> 2.39.2

--
paul-moore.com

Paul Moore March 30, 2023, 11:24 a.m. UTC | #3

On March 29, 2023 9:12:19 PM Paul Moore <[e-mail]> wrote:
> On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[e-mail]> wrote:

...

>
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> +                        size_t __user size, u32 __user flags)
>> +{
>> +       struct security_hook_list *hp;
>> +       struct lsm_ctx lctx;
>
> Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
>> +       if (flags != 0)
>> +               return -EINVAL;
>> +       if (size < sizeof(*ctx))
>> +               return -EINVAL;
>
> If we're only going to support on 'lsm_ctx' entry in this function we
> should verify that the 'len' and 'ctx_len' fields are sane.  Although
> more on this below ...
>
>> +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> +               return -EFAULT;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> +               if ((hp->lsmid->id) == lctx.id)
>> +                       return hp->hook.setselfattr(attr, ctx, size, flags);
>
> Can anyone think of any good reason why we shouldn't support setting
> multiple LSMs in one call, similar to what we do with
> security_getselfattr()?  It seems like it might be a nice thing to
> have ...

Scratch that, I just reminded myself why attempting to set an attribute on 
multiple LSMs in one operation would be a nightmare. Sorry about the confusion.

--
paul-moore.com

Casey Schaufler March 30, 2023, 8 p.m. UTC | #4

On 3/29/2023 6:12 PM, Paul Moore wrote:
> On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[e-mail]> wrote:
>> Create a system call lsm_get_self_attr() to provide the security
>> module maintained attributes of the current process.
>> Create a system call lsm_set_self_attr() to set a security
>> module maintained attribute of the current process.
>> Historically these attributes have been exposed to user space via
>> entries in procfs under /proc/self/attr.
>>
>> The attribute value is provided in a lsm_ctx structure. The structure
>> identifys the size of the attribute, and the attribute value. The format
> "identifies"
>
>> of the attribute value is defined by the security module. A flags field
>> is included for LSM specific information. It is currently unused and must
>> be 0. The total size of the data, including the lsm_ctx structure and any
>> padding, is maintained as well.
>>
>> struct lsm_ctx {
>>         __u64   id;
>>         __u64   flags;
>>         __u64   len;
>>         __u64   ctx_len;
>>         __u8    ctx[];
>> };
>>
>> Two new LSM hooks are used to interface with the LSMs.
>> security_getselfattr() collects the lsm_ctx values from the
>> LSMs that support the hook, accounting for space requirements.
>> security_setselfattr() identifies which LSM the attribute is
>> intended for and passes it along.
>>
>> Signed-off-by: Casey Schaufler <[e-mail]>
>> ---
>>  Documentation/userspace-api/lsm.rst | 15 +++++
>>  include/linux/lsm_hook_defs.h       |  4 ++
>>  include/linux/lsm_hooks.h           |  9 +++
>>  include/linux/security.h            | 19 ++++++
>>  include/linux/syscalls.h            |  5 ++
>>  include/uapi/linux/lsm.h            | 33 ++++++++++
>>  kernel/sys_ni.c                     |  4 ++
>>  security/Makefile                   |  1 +
>>  security/lsm_syscalls.c             | 55 ++++++++++++++++
>>  security/security.c                 | 97 +++++++++++++++++++++++++++++
>>  10 files changed, 242 insertions(+)
>>  create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 32285ce65419..3c2c4916bd53 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -503,6 +504,14 @@
>>   *     and writing the xattrs as this hook is merely a filter.
>>   * @d_instantiate:
>>   *     Fill in @inode security information for a @dentry if allowed.
>> + * @getselfattr:
>> + *     Read attribute @attr for the current process and store it into @ctx.
>> + *     Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
>> + *     or another negative value otherwise.
>> + * @setselfattr:
>> + *     Set attribute @attr for the current process.
>> + *     Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
>> + *     or another negative value otherwise.
>>   * @getprocattr:
>>   *     Read attribute @name for process @p and store it into @value if allowed.
>>   *     Return the length of @value on success, a negative value otherwise.
> I'm sure you're already aware of this, but the above will need to be
> moved to security.c due to the changes in the lsm/next branch.  That
> said, if you're basing on Linus' tree that's fine too, I'll fix it up
> during the merge; thankfully it's not a significant merge conflict.

I'm based on Linus' tree.

>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 8faed81fc3b4..329cd9d2be50 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -1343,6 +1348,20 @@ static inline void security_d_instantiate(struct dentry *dentry,
>>                                           struct inode *inode)
>>  { }
>>
>> +static inline int security_getselfattr(unsigned int __user attr,
>> +                                      struct lsm_ctx __user *ctx,
>> +                                      size_t __user *size, u32 __user flags)
>> +{
>> +       return -EINVAL;
>> +}
>> +
>> +static inline int security_setselfattr(unsigned int __user attr,
>> +                                      struct lsm_ctx __user *ctx,
>> +                                      size_t __user size, u32 __user flags)
>> +{
>> +       return -EINVAL;
>> +}
> It seems like EOPNOTSUPP might be more appropriate than EINVAL for
> both of these dummy implementations.

Sure.

>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 33a0ee3bcb2e..3feca00cb0c1 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>>  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>>                                             unsigned long home_node,
>>                                             unsigned long flags);
>> +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> +                                     size_t *size, __u64 flags);
>> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> +                                     __u64 flags);
> As the kernel test robot already pointed out, the above needs to be updated.
>
>>  /*
>>   * Architecture-specific system calls
>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index aa3e01867739..adfb55dce2fd 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,39 @@
>>  #ifndef _UAPI_LINUX_LSM_H
>>  #define _UAPI_LINUX_LSM_H
>>
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
>> +
>> +/**
>> + * struct lsm_ctx - LSM context information
>> + * @id: the LSM id number, see LSM_ID_XXX
>> + * @flags: LSM specific flags
>> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
>> + * @ctx_len: the size of @ctx
>> + * @ctx: the LSM context value
>> + *
>> + * The @len field MUST be equal to the size of the lsm_ctx struct
>> + * plus any additional padding and/or data placed after @ctx.
>> + *
>> + * In all cases @ctx_len MUST be equal to the length of @ctx.
>> + * If @ctx is a string value it should be nul terminated with
>> + * @ctx_len equal to `strlen(@ctx) + 1`.  Binary values are
>> + * supported.
>> + *
>> + * The @flags and @ctx fields SHOULD only be interpreted by the
>> + * LSM specified by @id; they MUST be set to zero/0 when not used.
>> + */
>> +struct lsm_ctx {
>> +       __u64   id;
>> +       __u64   flags;
>> +       __u64   len;
>> +       __u64   ctx_len;
>> +       __u8    ctx[];
>> +};
>> +
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
> I'm pretty sure the repeated #includes are a typo, right?  Or is there
> some uapi trick I'm missing ...

An artifact of patch (mis)management. Thanks for noticing.

>> diff --git a/security/security.c b/security/security.c
>> index 87c8796c3c46..2c57fe28c4f7 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>>  }
>>  EXPORT_SYMBOL(security_d_instantiate);
>>
>> +/**
>> + * security_getselfattr - Read an LSM attribute of the current process.
>> + * @attr: which attribute to return
>> + * @ctx: the user-space destination for the information, or NULL
>> + * @size: the size of space available to receive the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Returns the number of attributes found on success, negative value
>> + * on error. @size is reset to the total size of the data.
>> + * If @size is insufficient to contain the data -E2BIG is returned.
>> + */
>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> +                        size_t __user *size, u32 __user flags)
>> +{
>> +       struct security_hook_list *hp;
>> +       void __user *base = (void *)ctx;
> The casting seems wrong for a couple of reasons: I don't believe you
> need to cast the right side when the left side is a void pointer, and
> the right side cast drops the '__user' attribute when the left side is
> also a '__user' pointer value.
>
> That said, I think we may want @base to be 'u8 __user *base', more on
> that below ...
>
>> +       size_t total = 0;
>> +       size_t this;
> Naming is hard, but 'this'?  You can do better ...

It seemed like a good idea at the time, but a rose by any other
name still has thorns. I'll come up with something "better".


>> +       size_t left;
>> +       bool istoobig = false;
> Sorry, more naming nits and since it looks like you need to respin
> anyway ... please rename @istoobig to @toobig or something else.  The
> phrases-as-variable-names has always grated on me.

Sure.

>> +       int count = 0;
>> +       int rc;
>> +
>> +       if (attr == 0)
>> +               return -EINVAL;
>> +       if (flags != 0)
>> +               return -EINVAL;
>> +       if (size == NULL)
>> +               return -EINVAL;
>> +       if (get_user(left, size))
>> +               return -EFAULT;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> +               this = left;
>> +               if (base)
>> +                       ctx = (struct lsm_ctx __user *)(base + total);
> Pointer math on void pointers always makes me nervous.  Why not set
> @base's type to a 'u8' just to remove any concerns?

I can do that. I made it a void pointer to reflect the notion that
the attributes aren't necessarily strings. Making it a u8 may suggest that
the data is a string to some developers.

>> +               rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>> +               switch (rc) {
>> +               case -EOPNOTSUPP:
>> +                       rc = 0;
>> +                       continue;
>> +               case -E2BIG:
>> +                       istoobig = true;
>> +                       left = 0;
>> +                       break;
>> +               case 0:
>> +                       left -= this;
>> +                       break;
>> +               default:
>> +                       return rc;
> I think the @getselfattr hook should behave similarly to the
> associated syscall, returning a non-negative number should indicate
> that @rc entries have been added to the @ctx array.  Right now all the
> LSMs would just be adding one entry to the array, but we might as well
> code this up to be flexible.

Yes, some LSM may decide to have multiple "contexts".

>> +               }
>> +               total += this;
>> +               count++;
>> +       }
>> +       if (count == 0)
>> +               return LSM_RET_DEFAULT(getselfattr);
>> +       if (put_user(total, size))
>> +               return -EFAULT;
>> +       if (rc)
>> +               return rc;
> Is the 'if (rc)' check needed here?  Shouldn't the switch-statement
> after the hook catch everything that this check would catch?

It's necessary because of BPF, which doesn't follow the LSM rules.

>> +       if (istoobig)
>> +               return -E2BIG;
>> +       return count;
>> +}
>> +
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> +                        size_t __user size, u32 __user flags)
>> +{
>> +       struct security_hook_list *hp;
>> +       struct lsm_ctx lctx;
> Shouldn't we check @attr for valid values and return -EINVAL if bogus?

Sure.

>> +       if (flags != 0)
>> +               return -EINVAL;
>> +       if (size < sizeof(*ctx))
>> +               return -EINVAL;
> If we're only going to support on 'lsm_ctx' entry in this function we
> should verify that the 'len' and 'ctx_len' fields are sane.  Although
> more on this below ...

The LSM is going to have to do its own version of sanity checking. Having
sanity checking here as well seems excessive.

>> +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> +               return -EFAULT;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> +               if ((hp->lsmid->id) == lctx.id)
>> +                       return hp->hook.setselfattr(attr, ctx, size, flags);
> Can anyone think of any good reason why we shouldn't support setting
> multiple LSMs in one call, similar to what we do with
> security_getselfattr()?  It seems like it might be a nice thing to
> have ...

If you're setting the context for multiple LSMs and one fails the recovery
process is horrendous. Putting values you've already changed back to their
previous state may not even be possible. We could have a two pass scheme, one
to verify that the request would succeed and a second to do the work. That
doesn't address all the issues, including how to report which attribute failed.
I had planned to do multiple settings, but the weight of the mechanism to
deal with the failure case is considerable for a "nice to have".

>> +       return LSM_RET_DEFAULT(setselfattr);
>> +}
>> +
>>  int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>>                          char **value)
>>  {
>> --
>> 2.39.2
> --
> paul-moore.com

Paul Moore March 30, 2023, 11:22 p.m. UTC | #5

On Thu, Mar 30, 2023 at 4:00 PM Casey Schaufler <[e-mail]> wrote:
> On 3/29/2023 6:12 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:48 PM Casey Schaufler <[e-mail]> wrote:
> >> Create a system call lsm_get_self_attr() to provide the security
> >> module maintained attributes of the current process.
> >> Create a system call lsm_set_self_attr() to set a security
> >> module maintained attribute of the current process.
> >> Historically these attributes have been exposed to user space via
> >> entries in procfs under /proc/self/attr.
> >>
> >> The attribute value is provided in a lsm_ctx structure. The structure
> >> identifys the size of the attribute, and the attribute value. The format
> > "identifies"
> >
> >> of the attribute value is defined by the security module. A flags field
> >> is included for LSM specific information. It is currently unused and must
> >> be 0. The total size of the data, including the lsm_ctx structure and any
> >> padding, is maintained as well.
> >>
> >> struct lsm_ctx {
> >>         __u64   id;
> >>         __u64   flags;
> >>         __u64   len;
> >>         __u64   ctx_len;
> >>         __u8    ctx[];
> >> };
> >>
> >> Two new LSM hooks are used to interface with the LSMs.
> >> security_getselfattr() collects the lsm_ctx values from the
> >> LSMs that support the hook, accounting for space requirements.
> >> security_setselfattr() identifies which LSM the attribute is
> >> intended for and passes it along.
> >>
> >> Signed-off-by: Casey Schaufler <[e-mail]>
> >> ---
> >>  Documentation/userspace-api/lsm.rst | 15 +++++
> >>  include/linux/lsm_hook_defs.h       |  4 ++
> >>  include/linux/lsm_hooks.h           |  9 +++
> >>  include/linux/security.h            | 19 ++++++
> >>  include/linux/syscalls.h            |  5 ++
> >>  include/uapi/linux/lsm.h            | 33 ++++++++++
> >>  kernel/sys_ni.c                     |  4 ++
> >>  security/Makefile                   |  1 +
> >>  security/lsm_syscalls.c             | 55 ++++++++++++++++
> >>  security/security.c                 | 97 +++++++++++++++++++++++++++++
> >>  10 files changed, 242 insertions(+)
> >>  create mode 100644 security/lsm_syscalls.c

...

> >> +       int count = 0;
> >> +       int rc;
> >> +
> >> +       if (attr == 0)
> >> +               return -EINVAL;
> >> +       if (flags != 0)
> >> +               return -EINVAL;
> >> +       if (size == NULL)
> >> +               return -EINVAL;
> >> +       if (get_user(left, size))
> >> +               return -EFAULT;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> >> +               this = left;
> >> +               if (base)
> >> +                       ctx = (struct lsm_ctx __user *)(base + total);
> > Pointer math on void pointers always makes me nervous.  Why not set
> > @base's type to a 'u8' just to remove any concerns?
>
> I can do that. I made it a void pointer to reflect the notion that
> the attributes aren't necessarily strings. Making it a u8 may suggest that
> the data is a string to some developers.

That's a fair concern, but there is plenty of precedence of binary
blobs being stored in 'unsigned char' arrays to make it easier to
pluck data out at random byte offsets.

> >> +               rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> >> +               switch (rc) {
> >> +               case -EOPNOTSUPP:
> >> +                       rc = 0;
> >> +                       continue;
> >> +               case -E2BIG:
> >> +                       istoobig = true;
> >> +                       left = 0;
> >> +                       break;
> >> +               case 0:
> >> +                       left -= this;
> >> +                       break;
> >> +               default:
> >> +                       return rc;
> > I think the @getselfattr hook should behave similarly to the
> > associated syscall, returning a non-negative number should indicate
> > that @rc entries have been added to the @ctx array.  Right now all the
> > LSMs would just be adding one entry to the array, but we might as well
> > code this up to be flexible.
>
> Yes, some LSM may decide to have multiple "contexts".
>
> >> +               }
> >> +               total += this;
> >> +               count++;
> >> +       }
> >> +       if (count == 0)
> >> +               return LSM_RET_DEFAULT(getselfattr);
> >> +       if (put_user(total, size))
> >> +               return -EFAULT;
> >> +       if (rc)
> >> +               return rc;
> > Is the 'if (rc)' check needed here?  Shouldn't the switch-statement
> > after the hook catch everything that this check would catch?
>
> It's necessary because of BPF, which doesn't follow the LSM rules.

I thought if it made it this far in the function the LSM, BPF or not,
would still have gone through the switch statement above which would
have returned early if the the value was something other than one of
the accepted return codes ... right?

> >> +       if (istoobig)
> >> +               return -E2BIG;
> >> +       return count;
> >> +}
> >> +
> >> +/**
> >> + * security_setselfattr - Set an LSM attribute on the current process.
> >> + * @attr: which attribute to set
> >> + * @ctx: the user-space source for the information
> >> + * @size: the size of the data
> >> + * @flags: reserved for future use, must be 0
> >> + *
> >> + * Set an LSM attribute for the current process. The LSM, attribute
> >> + * and new value are included in @ctx.
> >> + *
> >> + * Returns 0 on success, an LSM specific value on failure.
> >> + */
> >> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> >> +                        size_t __user size, u32 __user flags)
> >> +{
> >> +       struct security_hook_list *hp;
> >> +       struct lsm_ctx lctx;
> > Shouldn't we check @attr for valid values and return -EINVAL if bogus?
>
> Sure.
>
> >> +       if (flags != 0)
> >> +               return -EINVAL;
> >> +       if (size < sizeof(*ctx))
> >> +               return -EINVAL;
> > If we're only going to support on 'lsm_ctx' entry in this function we
> > should verify that the 'len' and 'ctx_len' fields are sane.  Although
> > more on this below ...
>
> The LSM is going to have to do its own version of sanity checking. Having
> sanity checking here as well seems excessive.

Yes, the LSM will probably need to do some checks, but we can safely
do the length checking here so we might as well do it simply so every
LSM doesn't have to duplicate the length checks.

> >> +       if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> >> +               return -EFAULT;
> >> +
> >> +       hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> >> +               if ((hp->lsmid->id) == lctx.id)
> >> +                       return hp->hook.setselfattr(attr, ctx, size, flags);
> > Can anyone think of any good reason why we shouldn't support setting
> > multiple LSMs in one call, similar to what we do with
> > security_getselfattr()?  It seems like it might be a nice thing to
> > have ...
>
> If you're setting the context for multiple LSMs ...

See my follow-up to my original reply sent earlier today.

Mickaël Salaün April 3, 2023, 12:04 p.m. UTC | #6

On 15/03/2023 23:46, Casey Schaufler wrote:
> Create a system call lsm_get_self_attr() to provide the security
> module maintained attributes of the current process.
> Create a system call lsm_set_self_attr() to set a security
> module maintained attribute of the current process.
> Historically these attributes have been exposed to user space via
> entries in procfs under /proc/self/attr.
> 
> The attribute value is provided in a lsm_ctx structure. The structure
> identifys the size of the attribute, and the attribute value. The format
> of the attribute value is defined by the security module. A flags field
> is included for LSM specific information. It is currently unused and must
> be 0. The total size of the data, including the lsm_ctx structure and any
> padding, is maintained as well.
> 
> struct lsm_ctx {
>          __u64   id;
>          __u64   flags;
>          __u64   len;
>          __u64   ctx_len;
>          __u8    ctx[];
> };
> 
> Two new LSM hooks are used to interface with the LSMs.
> security_getselfattr() collects the lsm_ctx values from the
> LSMs that support the hook, accounting for space requirements.
> security_setselfattr() identifies which LSM the attribute is
> intended for and passes it along.
> 
> Signed-off-by: Casey Schaufler <[e-mail]>
> ---
>   Documentation/userspace-api/lsm.rst | 15 +++++
>   include/linux/lsm_hook_defs.h       |  4 ++
>   include/linux/lsm_hooks.h           |  9 +++
>   include/linux/security.h            | 19 ++++++
>   include/linux/syscalls.h            |  5 ++
>   include/uapi/linux/lsm.h            | 33 ++++++++++
>   kernel/sys_ni.c                     |  4 ++
>   security/Makefile                   |  1 +
>   security/lsm_syscalls.c             | 55 ++++++++++++++++
>   security/security.c                 | 97 +++++++++++++++++++++++++++++
>   10 files changed, 242 insertions(+)
>   create mode 100644 security/lsm_syscalls.c

[...]

> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> new file mode 100644
> index 000000000000..feee31600219
> --- /dev/null
> +++ b/security/lsm_syscalls.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * System calls implementing the Linux Security Module API.
> + *
> + *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
> + *  Copyright (C) 2022 Intel Corporation
> + */
> +
> +#include <asm/current.h>
> +#include <linux/compiler_types.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/security.h>
> +#include <linux/stddef.h>
> +#include <linux/syscalls.h>
> +#include <linux/types.h>
> +#include <linux/lsm_hooks.h>
> +#include <uapi/linux/lsm.h>
> +
> +/**
> + * sys_lsm_set_self_attr - Set current task's security module attribute
> + * @attr: which attribute to set
> + * @ctx: the LSM contexts
> + * @size: size of @ctx
> + * @flags: reserved for future use
> + *
> + * Sets the calling task's LSM context. On success this function
> + * returns 0. If the attribute specified cannot be set a negative
> + * value indicating the reason for the error is returned.

Do you think it is really worth it to implement syscalls that can get 
and set attributes to several LSMs at the same time, instead of one at a 
time? LSM-specific tools don't care about other LSMs. I still think it 
would be much simpler (for kernel and user space) to pass an LSM ID to 
both syscalls. This would avoid dealing with variable arrays of variable 
element lengths, to both get or set attributes.

Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was 
previously talked about, getting an unknown number of file descriptor 
doesn't look good neither.


> + */
> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> +		ctx, size_t __user, size, u32, flags)
> +{
> +	return security_setselfattr(attr, ctx, size, flags);
> +}
> +
> +/**
> + * sys_lsm_get_self_attr - Return current task's security module attributes
> + * @attr: which attribute to set

attribute to *get*

> + * @ctx: the LSM contexts
> + * @size: size of @ctx, updated on return

I suggest to use a dedicated argument to read the allocated size, and 
another to write the actual/written size.

This would not be required with an LSM ID passed to the syscall because 
attribute sizes should be known by user space, and there is no need to 
help them probe this information.


> + * @flags: reserved for future use
> + *
> + * Returns the calling task's LSM contexts. On success this
> + * function returns the number of @ctx array elements. This value
> + * may be zero if there are no LSM contexts assigned. If @size is
> + * insufficient to contain the return data -E2BIG is returned and
> + * @size is set to the minimum required size.

Doing something (updating a buffer) even when returning an error doesn't 
look right. These sizes should be well-known to user space and part of 
the ABI/UAPI.


> In all other cases
> + * a negative value indicating the error is returned.
> + */
> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> +		ctx, size_t __user *, size, u32, flags)
> +{
> +	return security_getselfattr(attr, ctx, size, flags);
> +}
> diff --git a/security/security.c b/security/security.c
> index 87c8796c3c46..2c57fe28c4f7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>   }
>   EXPORT_SYMBOL(security_d_instantiate);
>   
> +/**
> + * security_getselfattr - Read an LSM attribute of the current process.
> + * @attr: which attribute to return
> + * @ctx: the user-space destination for the information, or NULL
> + * @size: the size of space available to receive the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Returns the number of attributes found on success, negative value
> + * on error. @size is reset to the total size of the data.
> + * If @size is insufficient to contain the data -E2BIG is returned.
> + */
> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> +			 size_t __user *size, u32 __user flags)
> +{
> +	struct security_hook_list *hp;
> +	void __user *base = (void *)ctx;
> +	size_t total = 0;
> +	size_t this;
> +	size_t left;
> +	bool istoobig = false;
> +	int count = 0;
> +	int rc;
> +
> +	if (attr == 0)
> +		return -EINVAL;
> +	if (flags != 0)
> +		return -EINVAL;
> +	if (size == NULL)
> +		return -EINVAL;
> +	if (get_user(left, size))
> +		return -EFAULT;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
> +		this = left;
> +		if (base)
> +			ctx = (struct lsm_ctx __user *)(base + total);
> +		rc = hp->hook.getselfattr(attr, ctx, &this, flags);
> +		switch (rc) {
> +		case -EOPNOTSUPP:
> +			rc = 0;
> +			continue;
> +		case -E2BIG:
> +			istoobig = true;
> +			left = 0;
> +			break;

These two error cases could be directly handled by 
security_getselfattr() instead of relying on each LSM-specific 
implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).


> +		case 0:
> +			left -= this;
> +			break;
> +		default:
> +			return rc;
> +		}
> +		total += this;
> +		count++;
> +	}
> +	if (count == 0)
> +		return LSM_RET_DEFAULT(getselfattr);
> +	if (put_user(total, size))
> +		return -EFAULT;
> +	if (rc)
> +		return rc;
> +	if (istoobig)
> +		return -E2BIG;
> +	return count;
> +}
> +
> +/**
> + * security_setselfattr - Set an LSM attribute on the current process.
> + * @attr: which attribute to set
> + * @ctx: the user-space source for the information
> + * @size: the size of the data
> + * @flags: reserved for future use, must be 0
> + *
> + * Set an LSM attribute for the current process. The LSM, attribute
> + * and new value are included in @ctx.
> + *
> + * Returns 0 on success, an LSM specific value on failure.
> + */
> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> +			 size_t __user size, u32 __user flags)
> +{
> +	struct security_hook_list *hp;
> +	struct lsm_ctx lctx;
> +
> +	if (flags != 0)
> +		return -EINVAL;
> +	if (size < sizeof(*ctx))
> +		return -EINVAL;
> +	if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
> +		return -EFAULT;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
> +		if ((hp->lsmid->id) == lctx.id)
> +			return hp->hook.setselfattr(attr, ctx, size, flags);
> +
> +	return LSM_RET_DEFAULT(setselfattr);
> +}
> +
>   int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
>   			 char **value)
>   {

Casey Schaufler April 3, 2023, 5:36 p.m. UTC | #7

On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>
> On 15/03/2023 23:46, Casey Schaufler wrote:
>> Create a system call lsm_get_self_attr() to provide the security
>> module maintained attributes of the current process.
>> Create a system call lsm_set_self_attr() to set a security
>> module maintained attribute of the current process.
>> Historically these attributes have been exposed to user space via
>> entries in procfs under /proc/self/attr.
>>
>> The attribute value is provided in a lsm_ctx structure. The structure
>> identifys the size of the attribute, and the attribute value. The format
>> of the attribute value is defined by the security module. A flags field
>> is included for LSM specific information. It is currently unused and
>> must
>> be 0. The total size of the data, including the lsm_ctx structure and
>> any
>> padding, is maintained as well.
>>
>> struct lsm_ctx {
>>          __u64   id;
>>          __u64   flags;
>>          __u64   len;
>>          __u64   ctx_len;
>>          __u8    ctx[];
>> };
>>
>> Two new LSM hooks are used to interface with the LSMs.
>> security_getselfattr() collects the lsm_ctx values from the
>> LSMs that support the hook, accounting for space requirements.
>> security_setselfattr() identifies which LSM the attribute is
>> intended for and passes it along.
>>
>> Signed-off-by: Casey Schaufler <[e-mail]>
>> ---
>>   Documentation/userspace-api/lsm.rst | 15 +++++
>>   include/linux/lsm_hook_defs.h       |  4 ++
>>   include/linux/lsm_hooks.h           |  9 +++
>>   include/linux/security.h            | 19 ++++++
>>   include/linux/syscalls.h            |  5 ++
>>   include/uapi/linux/lsm.h            | 33 ++++++++++
>>   kernel/sys_ni.c                     |  4 ++
>>   security/Makefile                   |  1 +
>>   security/lsm_syscalls.c             | 55 ++++++++++++++++
>>   security/security.c                 | 97 +++++++++++++++++++++++++++++
>>   10 files changed, 242 insertions(+)
>>   create mode 100644 security/lsm_syscalls.c
>
> [...]
>
>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>> new file mode 100644
>> index 000000000000..feee31600219
>> --- /dev/null
>> +++ b/security/lsm_syscalls.c
>> @@ -0,0 +1,55 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * System calls implementing the Linux Security Module API.
>> + *
>> + *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
>> + *  Copyright (C) 2022 Intel Corporation
>> + */
>> +
>> +#include <asm/current.h>
>> +#include <linux/compiler_types.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/security.h>
>> +#include <linux/stddef.h>
>> +#include <linux/syscalls.h>
>> +#include <linux/types.h>
>> +#include <linux/lsm_hooks.h>
>> +#include <uapi/linux/lsm.h>
>> +
>> +/**
>> + * sys_lsm_set_self_attr - Set current task's security module attribute
>> + * @attr: which attribute to set
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx
>> + * @flags: reserved for future use
>> + *
>> + * Sets the calling task's LSM context. On success this function
>> + * returns 0. If the attribute specified cannot be set a negative
>> + * value indicating the reason for the error is returned.
>
> Do you think it is really worth it to implement syscalls that can get
> and set attributes to several LSMs at the same time, instead of one at
> a time? 

Setting the values for more than one LSM is impractical due to the possibility
that the Nth value may fail, and unwinding the N-1 values may not be possible.

> LSM-specific tools don't care about other LSMs.

That's part of the problem. Are systemd, dbusd, ps and id LSM specific tools?
They shouldn't be.

> I still think it would be much simpler (for kernel and user space) to
> pass an LSM ID to both syscalls. This would avoid dealing with
> variable arrays of variable element lengths, to both get or set
> attributes.

ps and id should both work regardless of which and how many LSMs provide
context attributes. They shouldn't need to know which LSMs are active in
advance. If a new LSM is introduced, they shouldn't need to be updated to
support it.

>
> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
> previously talked about, getting an unknown number of file descriptor
> doesn't look good neither.

If you have multiple LSM_ATTR_MAGICFD values and can only get one at
a time you have to do something convoluted with flags to get them all.
I don't see that as a good thing.

>
>
>> + */
>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>> lsm_ctx __user *,
>> +        ctx, size_t __user, size, u32, flags)
>> +{
>> +    return security_setselfattr(attr, ctx, size, flags);
>> +}
>> +
>> +/**
>> + * sys_lsm_get_self_attr - Return current task's security module
>> attributes
>> + * @attr: which attribute to set
>
> attribute to *get*
>
>> + * @ctx: the LSM contexts
>> + * @size: size of @ctx, updated on return
>
> I suggest to use a dedicated argument to read the allocated size, and
> another to write the actual/written size.
>
> This would not be required with an LSM ID passed to the syscall
> because attribute sizes should be known by user space, and there is no
> need to help them probe this information.
>
>
>> + * @flags: reserved for future use
>> + *
>> + * Returns the calling task's LSM contexts. On success this
>> + * function returns the number of @ctx array elements. This value
>> + * may be zero if there are no LSM contexts assigned. If @size is
>> + * insufficient to contain the return data -E2BIG is returned and
>> + * @size is set to the minimum required size.
>
> Doing something (updating a buffer) even when returning an error
> doesn't look right. These sizes should be well-known to user space and
> part of the ABI/UAPI.

No. The size of attributes is not well known to user space.
They are usually text strings. The maximum size will be known,
but that's putting additional burden on user space to know
about all possible LSMs. It's not always necessary.

>
>
>> In all other cases
>> + * a negative value indicating the error is returned.
>> + */
>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>> lsm_ctx __user *,
>> +        ctx, size_t __user *, size, u32, flags)
>> +{
>> +    return security_getselfattr(attr, ctx, size, flags);
>> +}
>> diff --git a/security/security.c b/security/security.c
>> index 87c8796c3c46..2c57fe28c4f7 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>> *dentry, struct inode *inode)
>>   }
>>   EXPORT_SYMBOL(security_d_instantiate);
>>   +/**
>> + * security_getselfattr - Read an LSM attribute of the current process.
>> + * @attr: which attribute to return
>> + * @ctx: the user-space destination for the information, or NULL
>> + * @size: the size of space available to receive the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Returns the number of attributes found on success, negative value
>> + * on error. @size is reset to the total size of the data.
>> + * If @size is insufficient to contain the data -E2BIG is returned.
>> + */
>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>> __user *ctx,
>> +             size_t __user *size, u32 __user flags)
>> +{
>> +    struct security_hook_list *hp;
>> +    void __user *base = (void *)ctx;
>> +    size_t total = 0;
>> +    size_t this;
>> +    size_t left;
>> +    bool istoobig = false;
>> +    int count = 0;
>> +    int rc;
>> +
>> +    if (attr == 0)
>> +        return -EINVAL;
>> +    if (flags != 0)
>> +        return -EINVAL;
>> +    if (size == NULL)
>> +        return -EINVAL;
>> +    if (get_user(left, size))
>> +        return -EFAULT;
>> +
>> +    hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> +        this = left;
>> +        if (base)
>> +            ctx = (struct lsm_ctx __user *)(base + total);
>> +        rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>> +        switch (rc) {
>> +        case -EOPNOTSUPP:
>> +            rc = 0;
>> +            continue;
>> +        case -E2BIG:
>> +            istoobig = true;
>> +            left = 0;
>> +            break;
>
> These two error cases could be directly handled by
> security_getselfattr() instead of relying on each LSM-specific
> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).

Yes, they could. My understanding is that Paul wants the LSM layer
to be "thin". Where possible and not insane, the logic should be in
the LSM, not the infrastructure.

>
>
>> +        case 0:
>> +            left -= this;
>> +            break;
>> +        default:
>> +            return rc;
>> +        }
>> +        total += this;
>> +        count++;
>> +    }
>> +    if (count == 0)
>> +        return LSM_RET_DEFAULT(getselfattr);
>> +    if (put_user(total, size))
>> +        return -EFAULT;
>> +    if (rc)
>> +        return rc;
>> +    if (istoobig)
>> +        return -E2BIG;
>> +    return count;
>> +}
>> +
>> +/**
>> + * security_setselfattr - Set an LSM attribute on the current process.
>> + * @attr: which attribute to set
>> + * @ctx: the user-space source for the information
>> + * @size: the size of the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Set an LSM attribute for the current process. The LSM, attribute
>> + * and new value are included in @ctx.
>> + *
>> + * Returns 0 on success, an LSM specific value on failure.
>> + */
>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>> __user *ctx,
>> +             size_t __user size, u32 __user flags)
>> +{
>> +    struct security_hook_list *hp;
>> +    struct lsm_ctx lctx;
>> +
>> +    if (flags != 0)
>> +        return -EINVAL;
>> +    if (size < sizeof(*ctx))
>> +        return -EINVAL;
>> +    if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>> +        return -EFAULT;
>> +
>> +    hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>> +        if ((hp->lsmid->id) == lctx.id)
>> +            return hp->hook.setselfattr(attr, ctx, size, flags);
>> +
>> +    return LSM_RET_DEFAULT(setselfattr);
>> +}
>> +
>>   int security_getprocattr(struct task_struct *p, int lsmid, const
>> char *name,
>>                char **value)
>>   {

Mickaël Salaün April 3, 2023, 6:04 p.m. UTC | #8

On 03/04/2023 19:36, Casey Schaufler wrote:
> On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>>
>> On 15/03/2023 23:46, Casey Schaufler wrote:
>>> Create a system call lsm_get_self_attr() to provide the security
>>> module maintained attributes of the current process.
>>> Create a system call lsm_set_self_attr() to set a security
>>> module maintained attribute of the current process.
>>> Historically these attributes have been exposed to user space via
>>> entries in procfs under /proc/self/attr.
>>>
>>> The attribute value is provided in a lsm_ctx structure. The structure
>>> identifys the size of the attribute, and the attribute value. The format
>>> of the attribute value is defined by the security module. A flags field
>>> is included for LSM specific information. It is currently unused and
>>> must
>>> be 0. The total size of the data, including the lsm_ctx structure and
>>> any
>>> padding, is maintained as well.
>>>
>>> struct lsm_ctx {
>>>           __u64   id;
>>>           __u64   flags;
>>>           __u64   len;
>>>           __u64   ctx_len;
>>>           __u8    ctx[];
>>> };
>>>
>>> Two new LSM hooks are used to interface with the LSMs.
>>> security_getselfattr() collects the lsm_ctx values from the
>>> LSMs that support the hook, accounting for space requirements.
>>> security_setselfattr() identifies which LSM the attribute is
>>> intended for and passes it along.
>>>
>>> Signed-off-by: Casey Schaufler <[e-mail]>
>>> ---
>>>    Documentation/userspace-api/lsm.rst | 15 +++++
>>>    include/linux/lsm_hook_defs.h       |  4 ++
>>>    include/linux/lsm_hooks.h           |  9 +++
>>>    include/linux/security.h            | 19 ++++++
>>>    include/linux/syscalls.h            |  5 ++
>>>    include/uapi/linux/lsm.h            | 33 ++++++++++
>>>    kernel/sys_ni.c                     |  4 ++
>>>    security/Makefile                   |  1 +
>>>    security/lsm_syscalls.c             | 55 ++++++++++++++++
>>>    security/security.c                 | 97 +++++++++++++++++++++++++++++
>>>    10 files changed, 242 insertions(+)
>>>    create mode 100644 security/lsm_syscalls.c
>>
>> [...]
>>
>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>> new file mode 100644
>>> index 000000000000..feee31600219
>>> --- /dev/null
>>> +++ b/security/lsm_syscalls.c
>>> @@ -0,0 +1,55 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * System calls implementing the Linux Security Module API.
>>> + *
>>> + *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
>>> + *  Copyright (C) 2022 Intel Corporation
>>> + */
>>> +
>>> +#include <asm/current.h>
>>> +#include <linux/compiler_types.h>
>>> +#include <linux/err.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/security.h>
>>> +#include <linux/stddef.h>
>>> +#include <linux/syscalls.h>
>>> +#include <linux/types.h>
>>> +#include <linux/lsm_hooks.h>
>>> +#include <uapi/linux/lsm.h>
>>> +
>>> +/**
>>> + * sys_lsm_set_self_attr - Set current task's security module attribute
>>> + * @attr: which attribute to set
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx
>>> + * @flags: reserved for future use
>>> + *
>>> + * Sets the calling task's LSM context. On success this function
>>> + * returns 0. If the attribute specified cannot be set a negative
>>> + * value indicating the reason for the error is returned.
>>
>> Do you think it is really worth it to implement syscalls that can get
>> and set attributes to several LSMs at the same time, instead of one at
>> a time?
> 
> Setting the values for more than one LSM is impractical due to the possibility
> that the Nth value may fail, and unwinding the N-1 values may not be possible.

Indeed, so unless I missed something, why not passing the LSM ID as a 
syscall argument for lsm_set_self_attr() and lsm_get_self_attr(), and 
avoid managing a set of contexts but instead only managing one context 
at a time (to get or set)?


> 
>> LSM-specific tools don't care about other LSMs.
> 
> That's part of the problem. Are systemd, dbusd, ps and id LSM specific tools?
> They shouldn't be.
> 
>> I still think it would be much simpler (for kernel and user space) to
>> pass an LSM ID to both syscalls. This would avoid dealing with
>> variable arrays of variable element lengths, to both get or set
>> attributes.
> 
> ps and id should both work regardless of which and how many LSMs provide
> context attributes. They shouldn't need to know which LSMs are active in
> advance. If a new LSM is introduced, they shouldn't need to be updated to
> support it.

I agree, and making the syscalls simpler doesn't change that.

> 
>>
>> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
>> previously talked about, getting an unknown number of file descriptor
>> doesn't look good neither.
> 
> If you have multiple LSM_ATTR_MAGICFD values and can only get one at
> a time you have to do something convoluted with flags to get them all.
> I don't see that as a good thing.

Yes, that was another argument to *not* deal with a set of contexts.


> 
>>
>>
>>> + */
>>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> +        ctx, size_t __user, size, u32, flags)
>>> +{
>>> +    return security_setselfattr(attr, ctx, size, flags);
>>> +}
>>> +
>>> +/**
>>> + * sys_lsm_get_self_attr - Return current task's security module
>>> attributes
>>> + * @attr: which attribute to set
>>
>> attribute to *get*
>>
>>> + * @ctx: the LSM contexts
>>> + * @size: size of @ctx, updated on return
>>
>> I suggest to use a dedicated argument to read the allocated size, and
>> another to write the actual/written size.
>>
>> This would not be required with an LSM ID passed to the syscall
>> because attribute sizes should be known by user space, and there is no
>> need to help them probe this information.
>>
>>
>>> + * @flags: reserved for future use
>>> + *
>>> + * Returns the calling task's LSM contexts. On success this
>>> + * function returns the number of @ctx array elements. This value
>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>> + * insufficient to contain the return data -E2BIG is returned and
>>> + * @size is set to the minimum required size.
>>
>> Doing something (updating a buffer) even when returning an error
>> doesn't look right. These sizes should be well-known to user space and
>> part of the ABI/UAPI.
> 
> No. The size of attributes is not well known to user space.
> They are usually text strings. The maximum size will be known,
> but that's putting additional burden on user space to know
> about all possible LSMs. It's not always necessary.

Right, I forgot the strings stuff… The lsm_get_self_attr() syscall could 
then return a ctx_actual_size (as one argument), and a ctx pointer (as 
another argument). Similarly, the lsm_set_self_attr() syscall could use 
a dedicated argument for ctx_size and another for the ctx pointer.

> 
>>
>>
>>> In all other cases
>>> + * a negative value indicating the error is returned.
>>> + */
>>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>>> lsm_ctx __user *,
>>> +        ctx, size_t __user *, size, u32, flags)
>>> +{
>>> +    return security_getselfattr(attr, ctx, size, flags);
>>> +}
>>> diff --git a/security/security.c b/security/security.c
>>> index 87c8796c3c46..2c57fe28c4f7 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>>> *dentry, struct inode *inode)
>>>    }
>>>    EXPORT_SYMBOL(security_d_instantiate);
>>>    +/**
>>> + * security_getselfattr - Read an LSM attribute of the current process.
>>> + * @attr: which attribute to return
>>> + * @ctx: the user-space destination for the information, or NULL
>>> + * @size: the size of space available to receive the data
>>> + * @flags: reserved for future use, must be 0
>>> + *
>>> + * Returns the number of attributes found on success, negative value
>>> + * on error. @size is reset to the total size of the data.
>>> + * If @size is insufficient to contain the data -E2BIG is returned.
>>> + */
>>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>>> __user *ctx,
>>> +             size_t __user *size, u32 __user flags)
>>> +{
>>> +    struct security_hook_list *hp;
>>> +    void __user *base = (void *)ctx;
>>> +    size_t total = 0;
>>> +    size_t this;
>>> +    size_t left;
>>> +    bool istoobig = false;
>>> +    int count = 0;
>>> +    int rc;
>>> +
>>> +    if (attr == 0)
>>> +        return -EINVAL;
>>> +    if (flags != 0)
>>> +        return -EINVAL;
>>> +    if (size == NULL)
>>> +        return -EINVAL;
>>> +    if (get_user(left, size))
>>> +        return -EFAULT;
>>> +
>>> +    hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>>> +        this = left;
>>> +        if (base)
>>> +            ctx = (struct lsm_ctx __user *)(base + total);
>>> +        rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>>> +        switch (rc) {
>>> +        case -EOPNOTSUPP:
>>> +            rc = 0;
>>> +            continue;
>>> +        case -E2BIG:
>>> +            istoobig = true;
>>> +            left = 0;
>>> +            break;
>>
>> These two error cases could be directly handled by
>> security_getselfattr() instead of relying on each LSM-specific
>> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
> 
> Yes, they could. My understanding is that Paul wants the LSM layer
> to be "thin". Where possible and not insane, the logic should be in
> the LSM, not the infrastructure.

FWIW, since we are defining new syscalls to make user space's life 
easier, I'm in favor of a well defined common behavior (e.g. returned 
errno) and factoring common code to make each LSM-specific code thin.

> 
>>
>>
>>> +        case 0:
>>> +            left -= this;
>>> +            break;
>>> +        default:
>>> +            return rc;
>>> +        }
>>> +        total += this;
>>> +        count++;
>>> +    }
>>> +    if (count == 0)
>>> +        return LSM_RET_DEFAULT(getselfattr);
>>> +    if (put_user(total, size))
>>> +        return -EFAULT;
>>> +    if (rc)
>>> +        return rc;
>>> +    if (istoobig)
>>> +        return -E2BIG;
>>> +    return count;
>>> +}
>>> +
>>> +/**
>>> + * security_setselfattr - Set an LSM attribute on the current process.
>>> + * @attr: which attribute to set
>>> + * @ctx: the user-space source for the information
>>> + * @size: the size of the data
>>> + * @flags: reserved for future use, must be 0
>>> + *
>>> + * Set an LSM attribute for the current process. The LSM, attribute
>>> + * and new value are included in @ctx.
>>> + *
>>> + * Returns 0 on success, an LSM specific value on failure.
>>> + */
>>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>>> __user *ctx,
>>> +             size_t __user size, u32 __user flags)
>>> +{
>>> +    struct security_hook_list *hp;
>>> +    struct lsm_ctx lctx;
>>> +
>>> +    if (flags != 0)
>>> +        return -EINVAL;
>>> +    if (size < sizeof(*ctx))
>>> +        return -EINVAL;
>>> +    if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>>> +        return -EFAULT;
>>> +
>>> +    hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>>> +        if ((hp->lsmid->id) == lctx.id)
>>> +            return hp->hook.setselfattr(attr, ctx, size, flags);
>>> +
>>> +    return LSM_RET_DEFAULT(setselfattr);
>>> +}
>>> +
>>>    int security_getprocattr(struct task_struct *p, int lsmid, const
>>> char *name,
>>>                 char **value)
>>>    {

Casey Schaufler April 3, 2023, 6:28 p.m. UTC | #9

On 4/3/2023 11:04 AM, Mickaël Salaün wrote:
>
> On 03/04/2023 19:36, Casey Schaufler wrote:
>> On 4/3/2023 5:04 AM, Mickaël Salaün wrote:
>>>
>>> On 15/03/2023 23:46, Casey Schaufler wrote:
>>>> Create a system call lsm_get_self_attr() to provide the security
>>>> module maintained attributes of the current process.
>>>> Create a system call lsm_set_self_attr() to set a security
>>>> module maintained attribute of the current process.
>>>> Historically these attributes have been exposed to user space via
>>>> entries in procfs under /proc/self/attr.
>>>>
>>>> The attribute value is provided in a lsm_ctx structure. The structure
>>>> identifys the size of the attribute, and the attribute value. The
>>>> format
>>>> of the attribute value is defined by the security module. A flags
>>>> field
>>>> is included for LSM specific information. It is currently unused and
>>>> must
>>>> be 0. The total size of the data, including the lsm_ctx structure and
>>>> any
>>>> padding, is maintained as well.
>>>>
>>>> struct lsm_ctx {
>>>>           __u64   id;
>>>>           __u64   flags;
>>>>           __u64   len;
>>>>           __u64   ctx_len;
>>>>           __u8    ctx[];
>>>> };
>>>>
>>>> Two new LSM hooks are used to interface with the LSMs.
>>>> security_getselfattr() collects the lsm_ctx values from the
>>>> LSMs that support the hook, accounting for space requirements.
>>>> security_setselfattr() identifies which LSM the attribute is
>>>> intended for and passes it along.
>>>>
>>>> Signed-off-by: Casey Schaufler <[e-mail]>
>>>> ---
>>>>    Documentation/userspace-api/lsm.rst | 15 +++++
>>>>    include/linux/lsm_hook_defs.h       |  4 ++
>>>>    include/linux/lsm_hooks.h           |  9 +++
>>>>    include/linux/security.h            | 19 ++++++
>>>>    include/linux/syscalls.h            |  5 ++
>>>>    include/uapi/linux/lsm.h            | 33 ++++++++++
>>>>    kernel/sys_ni.c                     |  4 ++
>>>>    security/Makefile                   |  1 +
>>>>    security/lsm_syscalls.c             | 55 ++++++++++++++++
>>>>    security/security.c                 | 97
>>>> +++++++++++++++++++++++++++++
>>>>    10 files changed, 242 insertions(+)
>>>>    create mode 100644 security/lsm_syscalls.c
>>>
>>> [...]
>>>
>>>> diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
>>>> new file mode 100644
>>>> index 000000000000..feee31600219
>>>> --- /dev/null
>>>> +++ b/security/lsm_syscalls.c
>>>> @@ -0,0 +1,55 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * System calls implementing the Linux Security Module API.
>>>> + *
>>>> + *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
>>>> + *  Copyright (C) 2022 Intel Corporation
>>>> + */
>>>> +
>>>> +#include <asm/current.h>
>>>> +#include <linux/compiler_types.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/errno.h>
>>>> +#include <linux/security.h>
>>>> +#include <linux/stddef.h>
>>>> +#include <linux/syscalls.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/lsm_hooks.h>
>>>> +#include <uapi/linux/lsm.h>
>>>> +
>>>> +/**
>>>> + * sys_lsm_set_self_attr - Set current task's security module
>>>> attribute
>>>> + * @attr: which attribute to set
>>>> + * @ctx: the LSM contexts
>>>> + * @size: size of @ctx
>>>> + * @flags: reserved for future use
>>>> + *
>>>> + * Sets the calling task's LSM context. On success this function
>>>> + * returns 0. If the attribute specified cannot be set a negative
>>>> + * value indicating the reason for the error is returned.
>>>
>>> Do you think it is really worth it to implement syscalls that can get
>>> and set attributes to several LSMs at the same time, instead of one at
>>> a time?
>>
>> Setting the values for more than one LSM is impractical due to the
>> possibility
>> that the Nth value may fail, and unwinding the N-1 values may not be
>> possible.
>
> Indeed, so unless I missed something, why not passing the LSM ID as a
> syscall argument for lsm_set_self_attr() and lsm_get_self_attr(), and
> avoid managing a set of contexts but instead only managing one context
> at a time (to get or set)?

The LSM ID is already in the lsm_attr being passed. An additional argument
would be redundant and introduce a potential error when the two values don't
match.

>
>
>>
>>> LSM-specific tools don't care about other LSMs.
>>
>> That's part of the problem. Are systemd, dbusd, ps and id LSM
>> specific tools?
>> They shouldn't be.
>>
>>> I still think it would be much simpler (for kernel and user space) to
>>> pass an LSM ID to both syscalls. This would avoid dealing with
>>> variable arrays of variable element lengths, to both get or set
>>> attributes.
>>
>> ps and id should both work regardless of which and how many LSMs provide
>> context attributes. They shouldn't need to know which LSMs are active in
>> advance. If a new LSM is introduced, they shouldn't need to be
>> updated to
>> support it.
>
> I agree, and making the syscalls simpler doesn't change that.
>
>>
>>>
>>> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
>>> previously talked about, getting an unknown number of file descriptor
>>> doesn't look good neither.
>>
>> If you have multiple LSM_ATTR_MAGICFD values and can only get one at
>> a time you have to do something convoluted with flags to get them all.
>> I don't see that as a good thing.
>
> Yes, that was another argument to *not* deal with a set of contexts.

User space is going to have to deal with multiple values somehow,
either by fetching each possible value independently or by getting
them all at once in a set. Neither is pretty.

>
>>
>>>
>>>
>>>> + */
>>>> +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct
>>>> lsm_ctx __user *,
>>>> +        ctx, size_t __user, size, u32, flags)
>>>> +{
>>>> +    return security_setselfattr(attr, ctx, size, flags);
>>>> +}
>>>> +
>>>> +/**
>>>> + * sys_lsm_get_self_attr - Return current task's security module
>>>> attributes
>>>> + * @attr: which attribute to set
>>>
>>> attribute to *get*
>>>
>>>> + * @ctx: the LSM contexts
>>>> + * @size: size of @ctx, updated on return
>>>
>>> I suggest to use a dedicated argument to read the allocated size, and
>>> another to write the actual/written size.
>>>
>>> This would not be required with an LSM ID passed to the syscall
>>> because attribute sizes should be known by user space, and there is no
>>> need to help them probe this information.
>>>
>>>
>>>> + * @flags: reserved for future use
>>>> + *
>>>> + * Returns the calling task's LSM contexts. On success this
>>>> + * function returns the number of @ctx array elements. This value
>>>> + * may be zero if there are no LSM contexts assigned. If @size is
>>>> + * insufficient to contain the return data -E2BIG is returned and
>>>> + * @size is set to the minimum required size.
>>>
>>> Doing something (updating a buffer) even when returning an error
>>> doesn't look right. These sizes should be well-known to user space and
>>> part of the ABI/UAPI.
>>
>> No. The size of attributes is not well known to user space.
>> They are usually text strings. The maximum size will be known,
>> but that's putting additional burden on user space to know
>> about all possible LSMs. It's not always necessary.
>
> Right, I forgot the strings stuff… The lsm_get_self_attr() syscall
> could then return a ctx_actual_size (as one argument), and a ctx
> pointer (as another argument). Similarly, the lsm_set_self_attr()
> syscall could use a dedicated argument for ctx_size and another for
> the ctx pointer.

That does not meet the design requirement. Paul wants a lsm_attr structure.
I'm not going to deviate from that.

>
>>
>>>
>>>
>>>> In all other cases
>>>> + * a negative value indicating the error is returned.
>>>> + */
>>>> +SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct
>>>> lsm_ctx __user *,
>>>> +        ctx, size_t __user *, size, u32, flags)
>>>> +{
>>>> +    return security_getselfattr(attr, ctx, size, flags);
>>>> +}
>>>> diff --git a/security/security.c b/security/security.c
>>>> index 87c8796c3c46..2c57fe28c4f7 100644
>>>> --- a/security/security.c
>>>> +++ b/security/security.c
>>>> @@ -2168,6 +2168,103 @@ void security_d_instantiate(struct dentry
>>>> *dentry, struct inode *inode)
>>>>    }
>>>>    EXPORT_SYMBOL(security_d_instantiate);
>>>>    +/**
>>>> + * security_getselfattr - Read an LSM attribute of the current
>>>> process.
>>>> + * @attr: which attribute to return
>>>> + * @ctx: the user-space destination for the information, or NULL
>>>> + * @size: the size of space available to receive the data
>>>> + * @flags: reserved for future use, must be 0
>>>> + *
>>>> + * Returns the number of attributes found on success, negative value
>>>> + * on error. @size is reset to the total size of the data.
>>>> + * If @size is insufficient to contain the data -E2BIG is returned.
>>>> + */
>>>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx
>>>> __user *ctx,
>>>> +             size_t __user *size, u32 __user flags)
>>>> +{
>>>> +    struct security_hook_list *hp;
>>>> +    void __user *base = (void *)ctx;
>>>> +    size_t total = 0;
>>>> +    size_t this;
>>>> +    size_t left;
>>>> +    bool istoobig = false;
>>>> +    int count = 0;
>>>> +    int rc;
>>>> +
>>>> +    if (attr == 0)
>>>> +        return -EINVAL;
>>>> +    if (flags != 0)
>>>> +        return -EINVAL;
>>>> +    if (size == NULL)
>>>> +        return -EINVAL;
>>>> +    if (get_user(left, size))
>>>> +        return -EFAULT;
>>>> +
>>>> +    hlist_for_each_entry(hp, &security_hook_heads.getselfattr,
>>>> list) {
>>>> +        this = left;
>>>> +        if (base)
>>>> +            ctx = (struct lsm_ctx __user *)(base + total);
>>>> +        rc = hp->hook.getselfattr(attr, ctx, &this, flags);
>>>> +        switch (rc) {
>>>> +        case -EOPNOTSUPP:
>>>> +            rc = 0;
>>>> +            continue;
>>>> +        case -E2BIG:
>>>> +            istoobig = true;
>>>> +            left = 0;
>>>> +            break;
>>>
>>> These two error cases could be directly handled by
>>> security_getselfattr() instead of relying on each LSM-specific
>>> implementations. See my suggestion on patch 7/11 (lsm_get_attr_size).
>>
>> Yes, they could. My understanding is that Paul wants the LSM layer
>> to be "thin". Where possible and not insane, the logic should be in
>> the LSM, not the infrastructure.
>
> FWIW, since we are defining new syscalls to make user space's life
> easier, I'm in favor of a well defined common behavior (e.g. returned
> errno) and factoring common code to make each LSM-specific code thin.

I appreciate the viewpoint. It's not what I understand the maintainer wants.

>
>>
>>>
>>>
>>>> +        case 0:
>>>> +            left -= this;
>>>> +            break;
>>>> +        default:
>>>> +            return rc;
>>>> +        }
>>>> +        total += this;
>>>> +        count++;
>>>> +    }
>>>> +    if (count == 0)
>>>> +        return LSM_RET_DEFAULT(getselfattr);
>>>> +    if (put_user(total, size))
>>>> +        return -EFAULT;
>>>> +    if (rc)
>>>> +        return rc;
>>>> +    if (istoobig)
>>>> +        return -E2BIG;
>>>> +    return count;
>>>> +}
>>>> +
>>>> +/**
>>>> + * security_setselfattr - Set an LSM attribute on the current
>>>> process.
>>>> + * @attr: which attribute to set
>>>> + * @ctx: the user-space source for the information
>>>> + * @size: the size of the data
>>>> + * @flags: reserved for future use, must be 0
>>>> + *
>>>> + * Set an LSM attribute for the current process. The LSM, attribute
>>>> + * and new value are included in @ctx.
>>>> + *
>>>> + * Returns 0 on success, an LSM specific value on failure.
>>>> + */
>>>> +int security_setselfattr(unsigned int __user attr, struct lsm_ctx
>>>> __user *ctx,
>>>> +             size_t __user size, u32 __user flags)
>>>> +{
>>>> +    struct security_hook_list *hp;
>>>> +    struct lsm_ctx lctx;
>>>> +
>>>> +    if (flags != 0)
>>>> +        return -EINVAL;
>>>> +    if (size < sizeof(*ctx))
>>>> +        return -EINVAL;
>>>> +    if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
>>>> +        return -EFAULT;
>>>> +
>>>> +    hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
>>>> +        if ((hp->lsmid->id) == lctx.id)
>>>> +            return hp->hook.setselfattr(attr, ctx, size, flags);
>>>> +
>>>> +    return LSM_RET_DEFAULT(setselfattr);
>>>> +}
>>>> +
>>>>    int security_getprocattr(struct task_struct *p, int lsmid, const
>>>> char *name,
>>>>                 char **value)
>>>>    {

Paul Moore April 11, 2023, 12:31 a.m. UTC | #10

On Mon, Apr 3, 2023 at 8:04 AM Mickaël Salaün <[e-mail]> wrote:
> On 15/03/2023 23:46, Casey Schaufler wrote:
> > Create a system call lsm_get_self_attr() to provide the security
> > module maintained attributes of the current process.
> > Create a system call lsm_set_self_attr() to set a security
> > module maintained attribute of the current process.
> > Historically these attributes have been exposed to user space via
> > entries in procfs under /proc/self/attr.
> >
> > The attribute value is provided in a lsm_ctx structure. The structure
> > identifys the size of the attribute, and the attribute value. The format
> > of the attribute value is defined by the security module. A flags field
> > is included for LSM specific information. It is currently unused and must
> > be 0. The total size of the data, including the lsm_ctx structure and any
> > padding, is maintained as well.
> >
> > struct lsm_ctx {
> >          __u64   id;
> >          __u64   flags;
> >          __u64   len;
> >          __u64   ctx_len;
> >          __u8    ctx[];
> > };
> >
> > Two new LSM hooks are used to interface with the LSMs.
> > security_getselfattr() collects the lsm_ctx values from the
> > LSMs that support the hook, accounting for space requirements.
> > security_setselfattr() identifies which LSM the attribute is
> > intended for and passes it along.
> >
> > Signed-off-by: Casey Schaufler <[e-mail]>
> > ---
> >   Documentation/userspace-api/lsm.rst | 15 +++++
> >   include/linux/lsm_hook_defs.h       |  4 ++
> >   include/linux/lsm_hooks.h           |  9 +++
> >   include/linux/security.h            | 19 ++++++
> >   include/linux/syscalls.h            |  5 ++
> >   include/uapi/linux/lsm.h            | 33 ++++++++++
> >   kernel/sys_ni.c                     |  4 ++
> >   security/Makefile                   |  1 +
> >   security/lsm_syscalls.c             | 55 ++++++++++++++++
> >   security/security.c                 | 97 +++++++++++++++++++++++++++++
> >   10 files changed, 242 insertions(+)
> >   create mode 100644 security/lsm_syscalls.c
>
> [...]
>
> > diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
> > new file mode 100644
> > index 000000000000..feee31600219
> > --- /dev/null
> > +++ b/security/lsm_syscalls.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * System calls implementing the Linux Security Module API.
> > + *
> > + *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
> > + *  Copyright (C) 2022 Intel Corporation
> > + */
> > +
> > +#include <asm/current.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/err.h>
> > +#include <linux/errno.h>
> > +#include <linux/security.h>
> > +#include <linux/stddef.h>
> > +#include <linux/syscalls.h>
> > +#include <linux/types.h>
> > +#include <linux/lsm_hooks.h>
> > +#include <uapi/linux/lsm.h>
> > +
> > +/**
> > + * sys_lsm_set_self_attr - Set current task's security module attribute
> > + * @attr: which attribute to set
> > + * @ctx: the LSM contexts
> > + * @size: size of @ctx
> > + * @flags: reserved for future use
> > + *
> > + * Sets the calling task's LSM context. On success this function
> > + * returns 0. If the attribute specified cannot be set a negative
> > + * value indicating the reason for the error is returned.
>
> Do you think it is really worth it to implement syscalls that can get
> and set attributes to several LSMs at the same time, instead of one at a
> time?

As mentioned elsewhere, the "set" operations pretty much have to be
one LSM at a time; the error handling is almost impossible otherwise.

However, it would be possible to have a single LSM "get" operation.
We could do this with the proposed lsm_get_self_attr() syscall and a
flag to indicate that only a single LSM's attribute information is
being requested, and that the desired LSM is indicated by the
lsm_ctx::id field (populated by the userspace caller).  I'm imagining
something like this:

  lsm_ctx->id = LSM_ID_MYFAVORITELSM;
  lsm_get_self_attr(LSM_ATTR_CURRENT,
                    lsm_ctx, &lsm_ctx_size, LSM_FLG_SINGLE);

> LSM-specific tools don't care about other LSMs.

That's why they are called "LSM-specific tools" ;)  I think it is a
reasonable request to provide optimizations for that, the
discussion/example above, but I think we also want to support tools
which are LSM "aware" but don't need to be made specific to any one
particular LSM.  Thankfully, I think we can do both.

> I still think it
> would be much simpler (for kernel and user space) to pass an LSM ID to
> both syscalls. This would avoid dealing with variable arrays of variable
> element lengths, to both get or set attributes.

I think we should support "get" operations that support getting an
attribute from multiple LSMs, but I'm perfectly fine with also
supporting a single LSM "get" operation as described in the example
above.

> Furthermore, considering the hypotetical LSM_ATTR_MAGICFD that was
> previously talked about, getting an unknown number of file descriptor
> doesn't look good neither.

We are already in a place where not all LSMs support all of the
attributes, and we handle that.  If you are concerned about a specific
LSM returning some additional, or "richer", attribute data, the
syscall does support that; it is just a matter of the userspace caller
being able to understand the LSM-specific data ... which is true for
even the simple/standard case.

> > + */
> > +SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
> > +             ctx, size_t __user, size, u32, flags)
> > +{
> > +     return security_setselfattr(attr, ctx, size, flags);
> > +}
> > +
> > +/**
> > + * sys_lsm_get_self_attr - Return current task's security module attributes
> > + * @attr: which attribute to set
>
> attribute to *get*
>
> > + * @ctx: the LSM contexts
> > + * @size: size of @ctx, updated on return
>
> I suggest to use a dedicated argument to read the allocated size, and
> another to write the actual/written size.

Can you elaborate on this?  There is plenty of precedence for this approach.

> This would not be required with an LSM ID passed to the syscall because
> attribute sizes should be known by user space, and there is no need to
> help them probe this information.

No.  As we move forward, and LSMs potentially introduce additional
attribute information/types/etc., there will be cases where the kernel
could need more buffer space than userspace would realize.  Keeping
the length flexible allows this, with the extra information ignored by
"legacy" userspace, and utilized by "new" userspace.

> > + * @flags: reserved for future use
> > + *
> > + * Returns the calling task's LSM contexts. On success this
> > + * function returns the number of @ctx array elements. This value
> > + * may be zero if there are no LSM contexts assigned. If @size is
> > + * insufficient to contain the return data -E2BIG is returned and
> > + * @size is set to the minimum required size.
>
> Doing something (updating a buffer) even when returning an error doesn't
> look right.

We could zero the buffer on error/E2BIG if that is a concern, but
unfortunately due the nature of the LSM it is not possible to safely
(no races) determine the size of the buffer before populating it.

> These sizes should be well-known to user space and part of
> the ABI/UAPI.

That may be true for specific LSMs at this point in time, but I
believe it would be a serious mistake to impose that constraint on
these syscalls.

Patch

diff --git a/Documentation/userspace-api/lsm.rst b/Documentation/userspace-api/lsm.rst
index 6ddf5506110b..b45e402302b3 100644
--- a/Documentation/userspace-api/lsm.rst
+++ b/Documentation/userspace-api/lsm.rst
@@ -48,6 +48,21 @@  creating socket objects.
 The proc filesystem provides this value in ``/proc/self/attr/sockcreate``.
 This is supported by the SELinux security module.
 
+Kernel interface
+================
+
+Set a security attribute of the current process
+--------------------------------------------------
+
+.. kernel-doc:: security/lsm_syscalls.c
+    :identifiers: sys_lsm_set_self_attr
+
+Get the specified security attributes of the current process
+--------------------------------------------------
+
+.. kernel-doc:: security/lsm_syscalls.c
+    :identifiers: sys_lsm_get_self_attr
+
 Additional documentation
 ========================
 
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 094b76dc7164..7177d9554f4a 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -261,6 +261,10 @@  LSM_HOOK(int, 0, sem_semop, struct kern_ipc_perm *perm, struct sembuf *sops,
 LSM_HOOK(int, 0, netlink_send, struct sock *sk, struct sk_buff *skb)
 LSM_HOOK(void, LSM_RET_VOID, d_instantiate, struct dentry *dentry,
 	 struct inode *inode)
+LSM_HOOK(int, -EOPNOTSUPP, getselfattr, unsigned int __user attr,
+	 struct lsm_ctx __user *ctx, size_t *size, u32 __user flags)
+LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int __user attr,
+	 struct lsm_ctx __user *ctx, size_t size, u32 __user flags)
 LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name,
 	 char **value)
 LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 32285ce65419..3c2c4916bd53 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -25,6 +25,7 @@ 
 #ifndef __LINUX_LSM_HOOKS_H
 #define __LINUX_LSM_HOOKS_H
 
+#include <uapi/linux/lsm.h>
 #include <linux/security.h>
 #include <linux/init.h>
 #include <linux/rculist.h>
@@ -503,6 +504,14 @@ 
  *	and writing the xattrs as this hook is merely a filter.
  * @d_instantiate:
  *	Fill in @inode security information for a @dentry if allowed.
+ * @getselfattr:
+ *	Read attribute @attr for the current process and store it into @ctx.
+ *	Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
+ *	or another negative value otherwise.
+ * @setselfattr:
+ *	Set attribute @attr for the current process.
+ *	Return 0 on success, -EOPNOTSUPP if the attribute is not supported,
+ *	or another negative value otherwise.
  * @getprocattr:
  *	Read attribute @name for process @p and store it into @value if allowed.
  *	Return the length of @value on success, a negative value otherwise.
diff --git a/include/linux/security.h b/include/linux/security.h
index 8faed81fc3b4..329cd9d2be50 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -60,6 +60,7 @@  struct fs_parameter;
 enum fs_value_type;
 struct watch;
 struct watch_notification;
+struct lsm_ctx;
 
 /* Default (no) options for the capable function */
 #define CAP_OPT_NONE 0x0
@@ -473,6 +474,10 @@  int security_sem_semctl(struct kern_ipc_perm *sma, int cmd);
 int security_sem_semop(struct kern_ipc_perm *sma, struct sembuf *sops,
 			unsigned nsops, int alter);
 void security_d_instantiate(struct dentry *dentry, struct inode *inode);
+int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+			 size_t __user *size, u32 __user flags);
+int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+			 size_t __user size, u32 __user flags);
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
 			 char **value);
 int security_setprocattr(int lsmid, const char *name, void *value, size_t size);
@@ -1343,6 +1348,20 @@  static inline void security_d_instantiate(struct dentry *dentry,
 					  struct inode *inode)
 { }
 
+static inline int security_getselfattr(unsigned int __user attr,
+				       struct lsm_ctx __user *ctx,
+				       size_t __user *size, u32 __user flags)
+{
+	return -EINVAL;
+}
+
+static inline int security_setselfattr(unsigned int __user attr,
+				       struct lsm_ctx __user *ctx,
+				       size_t __user size, u32 __user flags)
+{
+	return -EINVAL;
+}
+
 static inline int security_getprocattr(struct task_struct *p, int lsmid,
 				       const char *name, char **value)
 {
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 33a0ee3bcb2e..3feca00cb0c1 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -71,6 +71,7 @@  struct clone_args;
 struct open_how;
 struct mount_attr;
 struct landlock_ruleset_attr;
+struct lsm_ctx;
 enum landlock_rule_type;
 
 #include <linux/types.h>
@@ -1058,6 +1059,10 @@  asmlinkage long sys_memfd_secret(unsigned int flags);
 asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
 					    unsigned long home_node,
 					    unsigned long flags);
+asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
+				      size_t *size, __u64 flags);
+asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
+				      __u64 flags);
 
 /*
  * Architecture-specific system calls
diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
index aa3e01867739..adfb55dce2fd 100644
--- a/include/uapi/linux/lsm.h
+++ b/include/uapi/linux/lsm.h
@@ -9,6 +9,39 @@ 
 #ifndef _UAPI_LINUX_LSM_H
 #define _UAPI_LINUX_LSM_H
 
+#include <linux/types.h>
+#include <linux/unistd.h>
+
+/**
+ * struct lsm_ctx - LSM context information
+ * @id: the LSM id number, see LSM_ID_XXX
+ * @flags: LSM specific flags
+ * @len: length of the lsm_ctx struct, @ctx and any other data or padding
+ * @ctx_len: the size of @ctx
+ * @ctx: the LSM context value
+ *
+ * The @len field MUST be equal to the size of the lsm_ctx struct
+ * plus any additional padding and/or data placed after @ctx.
+ *
+ * In all cases @ctx_len MUST be equal to the length of @ctx.
+ * If @ctx is a string value it should be nul terminated with
+ * @ctx_len equal to `strlen(@ctx) + 1`.  Binary values are
+ * supported.
+ *
+ * The @flags and @ctx fields SHOULD only be interpreted by the
+ * LSM specified by @id; they MUST be set to zero/0 when not used.
+ */
+struct lsm_ctx {
+	__u64	id;
+	__u64	flags;
+	__u64	len;
+	__u64	ctx_len;
+	__u8	ctx[];
+};
+
+#include <linux/types.h>
+#include <linux/unistd.h>
+
 /*
  * ID tokens to identify Linux Security Modules (LSMs)
  *
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 860b2dcf3ac4..d03c78ef1562 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -262,6 +262,10 @@  COND_SYSCALL_COMPAT(recvmsg);
 /* mm/nommu.c, also with MMU */
 COND_SYSCALL(mremap);
 
+/* security/lsm_syscalls.c */
+COND_SYSCALL(lsm_get_self_attr);
+COND_SYSCALL(lsm_set_self_attr);
+
 /* security/keys/keyctl.c */
 COND_SYSCALL(add_key);
 COND_SYSCALL(request_key);
diff --git a/security/Makefile b/security/Makefile
index 18121f8f85cd..59f238490665 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -7,6 +7,7 @@  obj-$(CONFIG_KEYS)			+= keys/
 
 # always enable default capabilities
 obj-y					+= commoncap.o
+obj-$(CONFIG_SECURITY) 			+= lsm_syscalls.o
 obj-$(CONFIG_MMU)			+= min_addr.o
 
 # Object file lists
diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
new file mode 100644
index 000000000000..feee31600219
--- /dev/null
+++ b/security/lsm_syscalls.c
@@ -0,0 +1,55 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * System calls implementing the Linux Security Module API.
+ *
+ *  Copyright (C) 2022 Casey Schaufler <[e-mail]>
+ *  Copyright (C) 2022 Intel Corporation
+ */
+
+#include <asm/current.h>
+#include <linux/compiler_types.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/security.h>
+#include <linux/stddef.h>
+#include <linux/syscalls.h>
+#include <linux/types.h>
+#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>
+
+/**
+ * sys_lsm_set_self_attr - Set current task's security module attribute
+ * @attr: which attribute to set
+ * @ctx: the LSM contexts
+ * @size: size of @ctx
+ * @flags: reserved for future use
+ *
+ * Sets the calling task's LSM context. On success this function
+ * returns 0. If the attribute specified cannot be set a negative
+ * value indicating the reason for the error is returned.
+ */
+SYSCALL_DEFINE4(lsm_set_self_attr, unsigned int, attr, struct lsm_ctx __user *,
+		ctx, size_t __user, size, u32, flags)
+{
+	return security_setselfattr(attr, ctx, size, flags);
+}
+
+/**
+ * sys_lsm_get_self_attr - Return current task's security module attributes
+ * @attr: which attribute to set
+ * @ctx: the LSM contexts
+ * @size: size of @ctx, updated on return
+ * @flags: reserved for future use
+ *
+ * Returns the calling task's LSM contexts. On success this
+ * function returns the number of @ctx array elements. This value
+ * may be zero if there are no LSM contexts assigned. If @size is
+ * insufficient to contain the return data -E2BIG is returned and
+ * @size is set to the minimum required size. In all other cases
+ * a negative value indicating the error is returned.
+ */
+SYSCALL_DEFINE4(lsm_get_self_attr, unsigned int, attr, struct lsm_ctx __user *,
+		ctx, size_t __user *, size, u32, flags)
+{
+	return security_getselfattr(attr, ctx, size, flags);
+}
diff --git a/security/security.c b/security/security.c
index 87c8796c3c46..2c57fe28c4f7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2168,6 +2168,103 @@  void security_d_instantiate(struct dentry *dentry, struct inode *inode)
 }
 EXPORT_SYMBOL(security_d_instantiate);
 
+/**
+ * security_getselfattr - Read an LSM attribute of the current process.
+ * @attr: which attribute to return
+ * @ctx: the user-space destination for the information, or NULL
+ * @size: the size of space available to receive the data
+ * @flags: reserved for future use, must be 0
+ *
+ * Returns the number of attributes found on success, negative value
+ * on error. @size is reset to the total size of the data.
+ * If @size is insufficient to contain the data -E2BIG is returned.
+ */
+int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+			 size_t __user *size, u32 __user flags)
+{
+	struct security_hook_list *hp;
+	void __user *base = (void *)ctx;
+	size_t total = 0;
+	size_t this;
+	size_t left;
+	bool istoobig = false;
+	int count = 0;
+	int rc;
+
+	if (attr == 0)
+		return -EINVAL;
+	if (flags != 0)
+		return -EINVAL;
+	if (size == NULL)
+		return -EINVAL;
+	if (get_user(left, size))
+		return -EFAULT;
+
+	hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
+		this = left;
+		if (base)
+			ctx = (struct lsm_ctx __user *)(base + total);
+		rc = hp->hook.getselfattr(attr, ctx, &this, flags);
+		switch (rc) {
+		case -EOPNOTSUPP:
+			rc = 0;
+			continue;
+		case -E2BIG:
+			istoobig = true;
+			left = 0;
+			break;
+		case 0:
+			left -= this;
+			break;
+		default:
+			return rc;
+		}
+		total += this;
+		count++;
+	}
+	if (count == 0)
+		return LSM_RET_DEFAULT(getselfattr);
+	if (put_user(total, size))
+		return -EFAULT;
+	if (rc)
+		return rc;
+	if (istoobig)
+		return -E2BIG;
+	return count;
+}
+
+/**
+ * security_setselfattr - Set an LSM attribute on the current process.
+ * @attr: which attribute to set
+ * @ctx: the user-space source for the information
+ * @size: the size of the data
+ * @flags: reserved for future use, must be 0
+ *
+ * Set an LSM attribute for the current process. The LSM, attribute
+ * and new value are included in @ctx.
+ *
+ * Returns 0 on success, an LSM specific value on failure.
+ */
+int security_setselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
+			 size_t __user size, u32 __user flags)
+{
+	struct security_hook_list *hp;
+	struct lsm_ctx lctx;
+
+	if (flags != 0)
+		return -EINVAL;
+	if (size < sizeof(*ctx))
+		return -EINVAL;
+	if (copy_from_user(&lctx, ctx, sizeof(*ctx)))
+		return -EFAULT;
+
+	hlist_for_each_entry(hp, &security_hook_heads.setselfattr, list)
+		if ((hp->lsmid->id) == lctx.id)
+			return hp->hook.setselfattr(attr, ctx, size, flags);
+
+	return LSM_RET_DEFAULT(setselfattr);
+}
+
 int security_getprocattr(struct task_struct *p, int lsmid, const char *name,
 			 char **value)
 {