Re: [PATCH v8 04/11] LSM: syscalls for current process attributes

[Paul Moore]

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

...

> >> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
> >> index f27c9a9cc376..b10dfab8a4d9 100644
> >> --- a/include/uapi/linux/lsm.h
> >> +++ b/include/uapi/linux/lsm.h
> >> @@ -9,6 +9,36 @@
> >> #ifndef _UAPI_LINUX_LSM_H
> >> #define _UAPI_LINUX_LSM_H
> >>
> >> +#include <linux/types.h>
> >> +#include <linux/unistd.h>
> >> +
> >> +/**
> >> + * struct lsm_ctx - LSM context information
> >> + * @id: the LSM id number, see LSM_ID_XXX
> >> + * @flags: LSM specific flags
> >> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
> >> + * @ctx_len: the size of @ctx
> >> + * @ctx: the LSM context value
> >> + *
> >> + * The @len field MUST be equal to the size of the lsm_ctx struct
> >> + * plus any additional padding and/or data placed after @ctx.
> >> + *
> >> + * In all cases @ctx_len MUST be equal to the length of @ctx.
> >> + * If @ctx is a string value it should be nul terminated with
> >> + * @ctx_len equal to `strlen(@ctx) + 1`.  Binary values are
> >> + * supported.
> >> + *
> >> + * The @flags and @ctx fields SHOULD only be interpreted by the
> >> + * LSM specified by @id; they MUST be set to zero/0 when not used.
> >> + */
> >> +struct lsm_ctx {
> >> +       __u64   id;
> >> +       __u64   flags;
> >> +       __u64   len;
> >> +       __u64   ctx_len;
> >> +       __u8    ctx[];
> >> +};
> > Sorry, style nitpick since this needs to be respun anyway for the
> > syscalls.h fix at the very least ... I *really* dislike when variable
> > declarations, and field declarations in the case composite variables,
> > are aligned with the vertically neighboring declarations; just use a
> > single space please:
>
> I'll do it, but the tab after type has been accepted for forever.
> As an aside, I pulled out my 1978 K&R to prove my point and discovered
> that it isn't consistent regarding this style.

Yeah, it's not a question of acceptance or rejection, it's a matter of
personal preference.  I guess one of the few perks of this job is that
I can occasionally ask people to write code in a style that makes me
happy :)

> >   struct lsm_ctx {
> >   <tab>__u64 id;
> >   <tab>__u64 flags;
> >   <tab>__u64 len;
> >   <tab>__u64 ctx_len;
> >   <tab>__u8 ctx[];
> >   }
> >
> >> diff --git a/security/security.c b/security/security.c
> >> index 38ca0e646cac..bfe9a1a426b2 100644
> >> --- a/security/security.c
> >> +++ b/security/security.c
> >> @@ -2167,6 +2167,104 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
> >>  }
> >>  EXPORT_SYMBOL(security_d_instantiate);
> >>
> >> +/**
> >> + * security_getselfattr - Read an LSM attribute of the current process.
> >> + * @attr: which attribute to return
> >> + * @ctx: the user-space destination for the information, or NULL
> >> + * @size: the size of space available to receive the data
> >> + * @flags: reserved for future use, must be 0
> >> + *
> >> + * Returns the number of attributes found on success, negative value
> >> + * on error. @size is reset to the total size of the data.
> >> + * If @size is insufficient to contain the data -E2BIG is returned.
> >> + */
> >> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
> >> +                        size_t __user *size, u32 __user flags)
> >> +{
> >> +       struct security_hook_list *hp;
> >> +       u8 __user *base = (u8 __user *)ctx;
> >> +       size_t total = 0;
> >> +       size_t entrysize;
> >> +       size_t left;
> >> +       bool toobig = false;
> >> +       int count = 0;
> >> +       int rc;
> >> +
> >> +       if (attr == 0)
> >> +               return -EINVAL;
> >> +       if (flags)
> >> +               return -EINVAL;
> >
> > I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which
> > allows one to request a single LSM's attribute.
>
> I don't, but I'll incorporate it.

Thank you.

> >   I don't think that
> > support has to be part of this initial patchset, but I do think it
> > would be good to have it in the same PR that goes up to Linus during
> > the merge window.
>
> As this patch set is intended to be what goes to Linus (isn't it?)
> I'll put it in.

Yes, but it goes to Linus via the lsm/next branch, which will likely
contain other patches as well; one of these other patches could be a
LSM_FLG_SINGLE patch if you wanted to focus on what you have here and
leave that to someone else.  Either way, it all goes up to Linus in
one PR so I'm not too worried either way.

What I do want to avoid is shipping the initial support in version N,
and shipping LSM_FLG_SINGLE in release N+1.

-- 
paul-moore.com