[PATCH v7 10/11] SELinux: Add selfattr hooks

Paul Moore paul at paul-moore.com
Thu Mar 30 23:32:54 UTC 2023


On Thu, Mar 30, 2023 at 4:55 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
> On 3/29/2023 6:13 PM, Paul Moore wrote:
> > On Wed, Mar 15, 2023 at 6:52 PM Casey Schaufler <casey at schaufler-ca.com> wrote:
> >> Add hooks for setselfattr and getselfattr. These hooks are not very
> >> different from their setprocattr and getprocattr equivalents, and
> >> much of the code is shared.
> >>
> >> Signed-off-by: Casey Schaufler <casey at schaufler-ca.com>
> >> Cc: selinux at vger.kernel.org
> >> Cc: Paul Moore <paul at paul-moore.com>
> >> ---
> >>  security/selinux/hooks.c | 147 +++++++++++++++++++++++++++++++--------
> >>  1 file changed, 117 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 9403aee75981..8896edf80aa9 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -6348,8 +6348,7 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
> >>                 inode_doinit_with_dentry(inode, dentry);
> >>  }
> >>
> >> -static int selinux_getprocattr(struct task_struct *p,
> >> -                              const char *name, char **value)
> >> +static int do_getattr(unsigned int attr, struct task_struct *p, char **value)
> > Are you ready for more naming nitpicks? ;)
>
> I would expect nothing less. :)
>
> > Let's call this 'selinux_lsm_getattr()', and the matching setter
> > should be 'selinux_lsm_setattr()'.
>
> As you wish. It's your LSM.
>
>
> >>  {
> >>         const struct task_security_struct *__tsec;
> >>         u32 sid;
> >> @@ -6367,20 +6366,27 @@ static int selinux_getprocattr(struct task_struct *p,
> >>                         goto bad;
> >>         }
> >>
> >> -       if (!strcmp(name, "current"))
> >> +       switch (attr) {
> >> +       case LSM_ATTR_CURRENT:
> >>                 sid = __tsec->sid;
> >> -       else if (!strcmp(name, "prev"))
> >> +               break;
> >> +       case LSM_ATTR_PREV:
> >>                 sid = __tsec->osid;
> >> -       else if (!strcmp(name, "exec"))
> >> +               break;
> >> +       case LSM_ATTR_EXEC:
> >>                 sid = __tsec->exec_sid;
> >> -       else if (!strcmp(name, "fscreate"))
> >> +               break;
> >> +       case LSM_ATTR_FSCREATE:
> >>                 sid = __tsec->create_sid;
> >> -       else if (!strcmp(name, "keycreate"))
> >> +               break;
> >> +       case LSM_ATTR_KEYCREATE:
> >>                 sid = __tsec->keycreate_sid;
> >> -       else if (!strcmp(name, "sockcreate"))
> >> +               break;
> >> +       case LSM_ATTR_SOCKCREATE:
> >>                 sid = __tsec->sockcreate_sid;
> >> -       else {
> >> -               error = -EINVAL;
> >> +               break;
> >> +       default:
> >> +               error = -EOPNOTSUPP;
> > The error should probably be -EINVAL.
>
> It's possible that we may add an attribute that SELinux doesn't
> support, say LSM_ATTR_CRYPTO_KEY, that another LSM does. This is
> the same behavior the other LSMs exhibit in the face of a request
> for attributes such as LSM_ATTR_KEYCREATE that they don't support.

Okay, I'll accept that argument, but I would ask that add some
additional handling in selinux_getprocattr() so that it returns
-EINVAL in this case just as it does today.

> >>                 goto bad;
> >>         }
> >>         rcu_read_unlock();
> >> @@ -6398,7 +6404,7 @@ static int selinux_getprocattr(struct task_struct *p,
> >>         return error;
> >>  }
> >>
> >> -static int selinux_setprocattr(const char *name, void *value, size_t size)
> >> +static int do_setattr(u64 attr, void *value, size_t size)
> >>  {
> >>         struct task_security_struct *tsec;
> >>         struct cred *new;
> >> @@ -6409,28 +6415,36 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> >>         /*
> >>          * Basic control over ability to set these attributes at all.
> >>          */
> >> -       if (!strcmp(name, "exec"))
> >> +       switch (attr) {
> >> +       case LSM_ATTR_CURRENT:
> >> +               error = avc_has_perm(&selinux_state,
> >> +                                    mysid, mysid, SECCLASS_PROCESS,
> >> +                                    PROCESS__SETCURRENT, NULL);
> >> +               break;
> >> +       case LSM_ATTR_EXEC:
> >>                 error = avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETEXEC, NULL);
> >> -       else if (!strcmp(name, "fscreate"))
> >> +               break;
> >> +       case LSM_ATTR_FSCREATE:
> >>                 error = avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETFSCREATE, NULL);
> >> -       else if (!strcmp(name, "keycreate"))
> >> +               break;
> >> +       case LSM_ATTR_KEYCREATE:
> >>                 error = avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETKEYCREATE, NULL);
> >> -       else if (!strcmp(name, "sockcreate"))
> >> +               break;
> >> +       case LSM_ATTR_SOCKCREATE:
> >>                 error = avc_has_perm(&selinux_state,
> >>                                      mysid, mysid, SECCLASS_PROCESS,
> >>                                      PROCESS__SETSOCKCREATE, NULL);
> >> -       else if (!strcmp(name, "current"))
> >> -               error = avc_has_perm(&selinux_state,
> >> -                                    mysid, mysid, SECCLASS_PROCESS,
> >> -                                    PROCESS__SETCURRENT, NULL);
> >> -       else
> >> -               error = -EINVAL;
> >> +               break;
> >> +       default:
> >> +               error = -EOPNOTSUPP;
> > Same as above, should be -EINVAL.
>
> Same as above, there may be attributes SELinux doesn't support.

Also, same as above.

> >> +               break;
> >> +       }
> >>         if (error)
> >>                 return error;
> >>
> >> @@ -6442,13 +6456,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
> >>                 }
> >>                 error = security_context_to_sid(&selinux_state, value, size,
> >>                                                 &sid, GFP_KERNEL);
> >> -               if (error == -EINVAL && !strcmp(name, "fscreate")) {
> >> +               if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
> >>                         if (!has_cap_mac_admin(true)) {
> >>                                 struct audit_buffer *ab;
> >>                                 size_t audit_size;
> >>
> >> -                               /* We strip a nul only if it is at the end, otherwise the
> >> -                                * context contains a nul and we should audit that */
> >> +                               /* We strip a nul only if it is at the end,
> >> +                                * otherwise the context contains a nul and
> >> +                                * we should audit that */
> > You *do* get gold stars for fixing line lengths in close proximity ;)
>
> I guess I'm the Last User of the 80 character terminal.

I'm still a big fan and I'm sticking to the 80 char limit for the LSM
layer as well as the SELinux, audit, and labeled networking
subsystems.  Longer lines either predate me or I simply didn't catch
them during review/merge.

-- 
paul-moore.com



More information about the Linux-security-module-archive mailing list