Re: [PATCH 58/59] LSM: Specify which LSM to display with /proc/self/attr/display

From: Casey Schaufler <casey@schaufler-ca.com>
To: Stephen Smalley <stephen.smalley@gmail.com>
Cc: "Schaufler, Casey" <casey.schaufler@intel.com>,
	James Morris <jmorris@namei.org>,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	casey@schaufler-ca.com
Subject: Re: [PATCH 58/59] LSM: Specify which LSM to display with /proc/self/attr/display
Date: Wed, 10 Apr 2019 10:18:56 -0700	[thread overview]
Message-ID: <3bbc2089-177c-f4f3-c803-cc009fcc7a75@schaufler-ca.com> (raw)
In-Reply-To: <CAB9W1A2Bgr+gVGNxyVOwd7A52Gp8Kx0dPyd91pSjtXKp4UHoxw@mail.gmail.com>

On 4/10/2019 7:09 AM, Stephen Smalley wrote:
> On Wed, Apr 10, 2019 at 8:43 AM Stephen Smalley
> <stephen.smalley@gmail.com> wrote:
>> On Tue, Apr 9, 2019 at 5:42 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> Create a new entry "display" in /proc/.../attr for controlling
>>> which LSM security information is displayed for a process.
>>> The name of an active LSM that supplies hooks for human readable
>>> data may be written to "display" to set the value. The name of
>>> the LSM currently in use can be read from "display".
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> ---
>>>   fs/proc/base.c      |   1 +
>>>   security/security.c | 123 ++++++++++++++++++++++++++++++++++++++++++--
>>>   2 files changed, 121 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index ddef482f1334..7bf70e041315 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2618,6 +2618,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>>          ATTR(NULL, "fscreate",          0666),
>>>          ATTR(NULL, "keycreate",         0666),
>>>          ATTR(NULL, "sockcreate",        0666),
>>> +       ATTR(NULL, "display",           0666),
>>>   #ifdef CONFIG_SECURITY_SMACK
>>>          DIR("smack",                    0555,
>>>              proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>>> diff --git a/security/security.c b/security/security.c
>>> index 29149db3f78a..6e304aa796f9 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -47,9 +47,13 @@ static struct kmem_cache *lsm_inode_cache;
>>>
>>>   char *lsm_names;
>>>
>>> -/* Socket blobs include infrastructure managed data */
>>> +/*
>>> + *     Socket blobs include infrastructure managed data
>>> + *     Cred blobs include context display instructions
>>> + */
>>>   static struct lsm_blob_sizes blob_sizes __lsm_ro_after_init = {
>>>          .lbs_sock = sizeof(struct lsm_export),
>>> +       .lbs_cred = sizeof(struct lsm_one_hooks),
>>>   };
>>>
>>>   /**
>>> @@ -751,7 +755,10 @@ int lsm_superblock_alloc(struct super_block *sb)
>>>
>>>   #define call_one_int_hook(FUNC, IRC, ...) ({                   \
>>>          int RC = IRC;                                           \
>>> -       if (lsm_base_one.FUNC.FUNC)                             \
>>> +       struct lsm_one_hooks *LOH = current_cred()->security;   \
>>> +       if (LOH->FUNC.FUNC)                                     \
>>> +               RC = LOH->FUNC.FUNC(__VA_ARGS__);               \
>>> +       else if (LOH->lsm == NULL && lsm_base_one.FUNC.FUNC)    \
>>>                  RC = lsm_base_one.FUNC.FUNC(__VA_ARGS__);       \
>>>          RC;                                                     \
>>>   })
>>> @@ -1617,6 +1624,7 @@ int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
>>>
>>>   void security_cred_free(struct cred *cred)
>>>   {
>>> +       struct lsm_one_hooks *loh;
>>>          /*
>>>           * There is a failure case in prepare_creds() that
>>>           * may result in a call here with ->security being NULL.
>>> @@ -1626,26 +1634,44 @@ void security_cred_free(struct cred *cred)
>>>
>>>          call_void_hook(cred_free, cred);
>>>
>>> +       loh = cred->security;
>>> +       kfree(loh->lsm);
>>>          kfree(cred->security);
>>>          cred->security = NULL;
>>>   }
>>>
>>> +static int copy_loh(struct lsm_one_hooks *new, struct lsm_one_hooks *old,
>>> +                   gfp_t gfp)
>>> +{
>>> +       *new = *old;
>>> +       if (old->lsm) {
>>> +               new->lsm = kstrdup(old->lsm, gfp);
>>> +               if (unlikely(new->lsm == NULL))
>>> +                       return -ENOMEM;
>>> +       }
>>> +       return 0;
>>> +}
>>> +
>>>   int security_prepare_creds(struct cred *new, const struct cred *old, gfp_t gfp)
>>>   {
>>>          int rc = lsm_cred_alloc(new, gfp);
>>>
>>> -       if (rc)
>>> +       if (unlikely(rc))
>>>                  return rc;
>>>
>>>          rc = call_int_hook(cred_prepare, 0, new, old, gfp);
>>>          if (unlikely(rc))
>>>                  security_cred_free(new);
>>> +       else
>>> +               rc = copy_loh(new->security, old->security, gfp);
>>> +
>>>          return rc;
>>>   }
>>>
>>>   void security_transfer_creds(struct cred *new, const struct cred *old)
>>>   {
>>>          call_void_hook(cred_transfer, new, old);
>>> +       WARN_ON(copy_loh(new->security, old->security, GFP_KERNEL));
>>>   }
>>>
>>>   void security_cred_getsecid(const struct cred *c, struct lsm_export *l)
>>> @@ -1960,10 +1986,28 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>>                                  char **value)
>>>   {
>>>          struct security_hook_list *hp;
>>> +       struct lsm_one_hooks *loh = current_cred()->security;
>>> +       char *s;
>>> +
>>> +       if (!strcmp(name, "display")) {
>>> +               if (loh->lsm)
>>> +                       s = loh->lsm;
>>> +               else if (lsm_base_one.lsm)
>>> +                       s = lsm_base_one.lsm;
>>> +               else
>>> +                       return -EINVAL;
>>> +
>>> +               *value = kstrdup(s, GFP_KERNEL);
>>> +               if (*value)
>>> +                       return strlen(s);
>>> +               return -ENOMEM;
>>> +       }
>>>
>>>          hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>>                  if (lsm != NULL && strcmp(lsm, hp->lsm))
>>>                          continue;
>>> +               if (lsm == NULL && loh->lsm && strcmp(loh->lsm, hp->lsm))
>>> +                       continue;
>>>                  return hp->hook.getprocattr(p, name, value);
>>>          }
>>>          return -EINVAL;
>>> @@ -1973,10 +2017,83 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>>                           size_t size)
>>>   {
>>>          struct security_hook_list *hp;
>>> +       struct lsm_one_hooks *loh = current_cred()->security;
>>> +       bool found = false;
>>> +       char *s;
>>> +
>>> +       /*
>>> +        * End the passed name at a newline.
>>> +        */
>>> +       s = strnchr(value, size, '\n');
>>> +       if (s)
>>> +               *s = '\0';
>>> +
>>> +       if (!strcmp(name, "display")) {
>>> +               union security_list_options secid_to_secctx;
>>> +               union security_list_options secctx_to_secid;
>>> +               union security_list_options socket_getpeersec_stream;
>>> +
>>> +               if (size == 0 || size >= 100)
>>> +                       return -EINVAL;
>>> +
>>> +               secid_to_secctx.secid_to_secctx = NULL;
>>> +               hlist_for_each_entry(hp, &security_hook_heads.secid_to_secctx,
>>> +                                    list) {
>>> +                       if (size >= strlen(hp->lsm) &&
>>> +                           !strncmp(value, hp->lsm, size)) {
>>> +                               secid_to_secctx = hp->hook;
>>> +                               found = true;
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               secctx_to_secid.secctx_to_secid = NULL;
>>> +               hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid,
>>> +                                    list) {
>>> +                       if (size >= strlen(hp->lsm) &&
>>> +                           !strncmp(value, hp->lsm, size)) {
>>> +                               secctx_to_secid = hp->hook;
>>> +                               found = true;
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               socket_getpeersec_stream.socket_getpeersec_stream = NULL;
>>> +               hlist_for_each_entry(hp,
>>> +                               &security_hook_heads.socket_getpeersec_stream,
>>> +                                    list) {
>>> +                       if (size >= strlen(hp->lsm) &&
>>> +                           !strncmp(value, hp->lsm, size)) {
>>> +                               socket_getpeersec_stream = hp->hook;
>>> +                               found = true;
>>> +                               break;
>>> +                       }
>>> +               }
>>> +               if (!found)
>>> +                       return -EINVAL;
>>> +
>>> +               /*
>>> +                * The named lsm is active and supplies one or more
>>> +                * of the relevant hooks. Switch to it.
>>> +                */
>>> +               s = kmemdup(value, size + 1, GFP_KERNEL);
>>> +               if (s == NULL)
>>> +                       return -ENOMEM;
>>> +               s[size] = '\0';
>>> +
>>> +               if (loh->lsm)
>>> +                       kfree(loh->lsm);
>>> +               loh->lsm = s;
>>> +               loh->secid_to_secctx = secid_to_secctx;
>>> +               loh->secctx_to_secid = secctx_to_secid;
>>> +               loh->socket_getpeersec_stream = socket_getpeersec_stream;
>> You can't just write to the cred security blob like this; it is a
>> shared data structure, not per-task.
> To be clear, you either need to perform a new = prepare_creds(); /*
> modify new->security as desired */; commit_creds(new); sequence here,
> or use the task security blob instead of the cred security blob.  The
> latter seems more amenable to your needs.

You're right. The task blob makes more sense.