[PATCH 08/10] LSM: SafeSetID: add read handler

[Kees Cook]

On Wed, Apr 10, 2019 at 9:56 AM Micah Morton <mortonm at chromium.org> wrote:
>
> From: Jann Horn <jannh at google.com>
>
> For debugging a running system, it is very helpful to be able to see what
> policy the system is using. Add a read handler that can dump out a copy of
> the loaded policy.
>
> Signed-off-by: Jann Horn <jannh at google.com>
> Signed-off-by: Micah Morton <mortonm at chromium.org>
> ---
>  security/safesetid/lsm.h        |  3 +++
>  security/safesetid/securityfs.c | 38 +++++++++++++++++++++++++++++++--
>  2 files changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index 4a34f558d964..9380329fe30a 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -17,6 +17,7 @@
>  #include <linux/types.h>
>  #include <linux/uidgid.h>
>  #include <linux/hashtable.h>
> +#include <linux/refcount.h>
>
>  /* Flag indicating whether initialization completed */
>  extern int safesetid_initialized;
> @@ -41,7 +42,9 @@ struct setuid_rule {
>
>  struct setuid_ruleset {
>         DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> +       char *policy_str;
>         struct rcu_head rcu;
> +       refcount_t refcount;
>  };

refcount seems like overkill? Why not just use the spinlock? Neither
read nor write are "fast path".

-Kees

>
>  enum sid_policy_type _setuid_policy_lookup(struct setuid_ruleset *policy,
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 13fce4c10930..7a08fff2bc14 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -67,12 +67,14 @@ static void __release_ruleset(struct rcu_head *rcu)
>
>         hash_for_each_safe(pol->rules, bucket, tmp, rule, next)
>                 kfree(rule);
> +       kfree(pol->policy_str);
>         kfree(pol);
>  }
>
>  static void release_ruleset(struct setuid_ruleset *pol)
>  {
> -       call_rcu(&pol->rcu, __release_ruleset);
> +       if (pol != NULL && refcount_dec_and_test(&pol->refcount))
> +               call_rcu(&pol->rcu, __release_ruleset);
>  }
>
>  static ssize_t handle_policy_update(struct file *file,
> @@ -85,6 +87,8 @@ static ssize_t handle_policy_update(struct file *file,
>         pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
>         if (!pol)
>                 return -ENOMEM;
> +       refcount_set(&pol->refcount, 1);
> +       pol->policy_str = NULL;
>         hash_init(pol->rules);
>
>         p = buf = memdup_user_nul(ubuf, len);
> @@ -92,6 +96,11 @@ static ssize_t handle_policy_update(struct file *file,
>                 err = PTR_ERR(buf);
>                 goto out_free_pol;
>         }
> +       pol->policy_str = kstrdup(buf, GFP_KERNEL);
> +       if (pol->policy_str == NULL) {
> +               err = -ENOMEM;
> +               goto out_free_buf;
> +       }
>
>         /* policy lines, including the last one, end with \n */
>         while (*p != '\0') {
> @@ -162,7 +171,32 @@ static ssize_t safesetid_file_write(struct file *file,
>         return handle_policy_update(file, buf, len);
>  }
>
> +static ssize_t safesetid_file_read(struct file *file, char __user *buf,
> +                                  size_t len, loff_t *ppos)
> +{
> +       ssize_t res;
> +       struct setuid_ruleset *pol;
> +       const char *kbuf;
> +
> +       rcu_read_lock();
> +       pol = rcu_dereference(safesetid_setuid_rules);
> +       if (!pol) {
> +               rcu_read_unlock();
> +               return 0;
> +       }
> +       if (!refcount_inc_not_zero(&pol->refcount)) {
> +               rcu_read_unlock();
> +               return -EBUSY;
> +       }
> +       rcu_read_unlock();
> +       kbuf = pol->policy_str;
> +       res = simple_read_from_buffer(buf, len, ppos, kbuf, strlen(kbuf));
> +       release_ruleset(pol);
> +       return res;
> +}
> +
>  static const struct file_operations safesetid_file_fops = {
> +       .read = safesetid_file_read,
>         .write = safesetid_file_write,
>  };
>
> @@ -181,7 +215,7 @@ static int __init safesetid_init_securityfs(void)
>                 goto error;
>         }
>
> -       policy_file = securityfs_create_file("whitelist_policy", 0200,
> +       policy_file = securityfs_create_file("whitelist_policy", 0600,
>                         policy_dir, NULL, &safesetid_file_fops);
>         if (IS_ERR(policy_file)) {
>                 ret = PTR_ERR(policy_file);
> --
> 2.21.0.392.gf8f6787159e-goog
>


-- 
Kees Cook