diff mbox series

[v2,08/10] LSM: SafeSetID: add read handler

Message ID 20190411201154.167617-1-mortonm@chromium.org (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Micah Morton April 11, 2019, 8:11 p.m. UTC 
From: Jann Horn <jannh@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@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
Changes since the last patch set: Instead of doing refcounting, change
policy_update_lock to a mutex and hold the mutex across the policy read.
 security/safesetid/lsm.h        |  1 +
 security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 4 deletions(-)

Comments

Kees Cook April 11, 2019, 8:37 p.m. UTC | #1 
On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@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@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
> Changes since the last patch set: Instead of doing refcounting, change
> policy_update_lock to a mutex and hold the mutex across the policy read.
>  security/safesetid/lsm.h        |  1 +
>  security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> index 4a34f558d964..db6d16e6bbc3 100644
> --- a/security/safesetid/lsm.h
> +++ b/security/safesetid/lsm.h
> @@ -41,6 +41,7 @@ struct setuid_rule {
>
>  struct setuid_ruleset {
>         DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> +       char *policy_str;
>         struct rcu_head rcu;
>  };
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 250d59e046c1..997b403c6255 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -19,7 +19,7 @@
>
>  #include "lsm.h"
>
> -static DEFINE_SPINLOCK(policy_update_lock);
> +static DEFINE_MUTEX(policy_update_lock);
>
>  /*
>   * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> @@ -67,6 +67,7 @@ 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);
>  }
>
> @@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file,
>         pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
>         if (!pol)
>                 return -ENOMEM;
> +       pol->policy_str = NULL;
>         hash_init(pol->rules);
>
>         p = buf = memdup_user_nul(ubuf, len);
> @@ -92,6 +94,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') {
> @@ -135,10 +142,10 @@ static ssize_t handle_policy_update(struct file *file,
>          * What we really want here is an xchg() wrapper for RCU, but since that
>          * doesn't currently exist, just use a spinlock for now.
>          */
> -       spin_lock(&policy_update_lock);
> +       mutex_lock(&policy_update_lock);
>         rcu_swap_protected(safesetid_setuid_rules, pol,
>                            lockdep_is_held(&policy_update_lock));
> -       spin_unlock(&policy_update_lock);
> +       mutex_unlock(&policy_update_lock);
>         err = len;
>
>  out_free_buf:
> @@ -162,7 +169,27 @@ 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 = 0;
> +       struct setuid_ruleset *pol;
> +       const char *kbuf;
> +
> +       mutex_lock(&policy_update_lock);
> +       pol = rcu_dereference_protected(safesetid_setuid_rules,
> +                                       lockdep_is_held(&policy_update_lock));
> +       if (pol) {
> +               kbuf = pol->policy_str;
> +               res = simple_read_from_buffer(buf, len, ppos,
> +                                             kbuf, strlen(kbuf));
> +       }
> +       mutex_unlock(&policy_update_lock);
> +       return res;
> +}
> +
>  static const struct file_operations safesetid_file_fops = {
> +       .read = safesetid_file_read,
>         .write = safesetid_file_write,
>  };
>
> @@ -181,7 +208,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
Micah Morton May 7, 2019, 3:02 p.m. UTC | #2 
Ready for merge.

On Thu, Apr 11, 2019 at 1:38 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Thu, Apr 11, 2019 at 1:12 PM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@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@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > Changes since the last patch set: Instead of doing refcounting, change
> > policy_update_lock to a mutex and hold the mutex across the policy read.
> >  security/safesetid/lsm.h        |  1 +
> >  security/safesetid/securityfs.c | 35 +++++++++++++++++++++++++++++----
> >  2 files changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
> > index 4a34f558d964..db6d16e6bbc3 100644
> > --- a/security/safesetid/lsm.h
> > +++ b/security/safesetid/lsm.h
> > @@ -41,6 +41,7 @@ struct setuid_rule {
> >
> >  struct setuid_ruleset {
> >         DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
> > +       char *policy_str;
> >         struct rcu_head rcu;
> >  };
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 250d59e046c1..997b403c6255 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -19,7 +19,7 @@
> >
> >  #include "lsm.h"
> >
> > -static DEFINE_SPINLOCK(policy_update_lock);
> > +static DEFINE_MUTEX(policy_update_lock);
> >
> >  /*
> >   * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> > @@ -67,6 +67,7 @@ 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);
> >  }
> >
> > @@ -85,6 +86,7 @@ static ssize_t handle_policy_update(struct file *file,
> >         pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
> >         if (!pol)
> >                 return -ENOMEM;
> > +       pol->policy_str = NULL;
> >         hash_init(pol->rules);
> >
> >         p = buf = memdup_user_nul(ubuf, len);
> > @@ -92,6 +94,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') {
> > @@ -135,10 +142,10 @@ static ssize_t handle_policy_update(struct file *file,
> >          * What we really want here is an xchg() wrapper for RCU, but since that
> >          * doesn't currently exist, just use a spinlock for now.
> >          */
> > -       spin_lock(&policy_update_lock);
> > +       mutex_lock(&policy_update_lock);
> >         rcu_swap_protected(safesetid_setuid_rules, pol,
> >                            lockdep_is_held(&policy_update_lock));
> > -       spin_unlock(&policy_update_lock);
> > +       mutex_unlock(&policy_update_lock);
> >         err = len;
> >
> >  out_free_buf:
> > @@ -162,7 +169,27 @@ 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 = 0;
> > +       struct setuid_ruleset *pol;
> > +       const char *kbuf;
> > +
> > +       mutex_lock(&policy_update_lock);
> > +       pol = rcu_dereference_protected(safesetid_setuid_rules,
> > +                                       lockdep_is_held(&policy_update_lock));
> > +       if (pol) {
> > +               kbuf = pol->policy_str;
> > +               res = simple_read_from_buffer(buf, len, ppos,
> > +                                             kbuf, strlen(kbuf));
> > +       }
> > +       mutex_unlock(&policy_update_lock);
> > +       return res;
> > +}
> > +
> >  static const struct file_operations safesetid_file_fops = {
> > +       .read = safesetid_file_read,
> >         .write = safesetid_file_write,
> >  };
> >
> > @@ -181,7 +208,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
diff mbox series

Patch

diff --git a/security/safesetid/lsm.h b/security/safesetid/lsm.h
index 4a34f558d964..db6d16e6bbc3 100644
--- a/security/safesetid/lsm.h
+++ b/security/safesetid/lsm.h
@@ -41,6 +41,7 @@  struct setuid_rule {
 
 struct setuid_ruleset {
 	DECLARE_HASHTABLE(rules, SETID_HASH_BITS);
+	char *policy_str;
 	struct rcu_head rcu;
 };
 
diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 250d59e046c1..997b403c6255 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -19,7 +19,7 @@ 
 
 #include "lsm.h"
 
-static DEFINE_SPINLOCK(policy_update_lock);
+static DEFINE_MUTEX(policy_update_lock);
 
 /*
  * In the case the input buffer contains one or more invalid UIDs, the kuid_t
@@ -67,6 +67,7 @@  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);
 }
 
@@ -85,6 +86,7 @@  static ssize_t handle_policy_update(struct file *file,
 	pol = kmalloc(sizeof(struct setuid_ruleset), GFP_KERNEL);
 	if (!pol)
 		return -ENOMEM;
+	pol->policy_str = NULL;
 	hash_init(pol->rules);
 
 	p = buf = memdup_user_nul(ubuf, len);
@@ -92,6 +94,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') {
@@ -135,10 +142,10 @@  static ssize_t handle_policy_update(struct file *file,
 	 * What we really want here is an xchg() wrapper for RCU, but since that
 	 * doesn't currently exist, just use a spinlock for now.
 	 */
-	spin_lock(&policy_update_lock);
+	mutex_lock(&policy_update_lock);
 	rcu_swap_protected(safesetid_setuid_rules, pol,
 			   lockdep_is_held(&policy_update_lock));
-	spin_unlock(&policy_update_lock);
+	mutex_unlock(&policy_update_lock);
 	err = len;
 
 out_free_buf:
@@ -162,7 +169,27 @@  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 = 0;
+	struct setuid_ruleset *pol;
+	const char *kbuf;
+
+	mutex_lock(&policy_update_lock);
+	pol = rcu_dereference_protected(safesetid_setuid_rules,
+					lockdep_is_held(&policy_update_lock));
+	if (pol) {
+		kbuf = pol->policy_str;
+		res = simple_read_from_buffer(buf, len, ppos,
+					      kbuf, strlen(kbuf));
+	}
+	mutex_unlock(&policy_update_lock);
+	return res;
+}
+
 static const struct file_operations safesetid_file_fops = {
+	.read = safesetid_file_read,
 	.write = safesetid_file_write,
 };
 
@@ -181,7 +208,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);