diff mbox series

[05/10] LSM: SafeSetID: refactor policy parsing

Message ID 20190410165548.211254-1-mortonm@chromium.org (mailing list archive)
State New, archived
Headers show
Series [01/10] LSM: SafeSetID: fix pr_warn() to include newline | expand

Commit Message

Micah Morton April 10, 2019, 4:55 p.m. UTC 
From: Jann Horn <jannh@google.com>

In preparation for changing the policy parsing logic, refactor the line
parsing logic to be less verbose and move it into a separate function.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Micah Morton <mortonm@chromium.org>
---
I made a minor change to Jann's original patch to use u32 instead of
s32 for the 'parsed_parent' and 'parsed_child' variables.

 security/safesetid/securityfs.c | 84 +++++++++++++--------------------
 1 file changed, 33 insertions(+), 51 deletions(-)

Comments

Kees Cook April 10, 2019, 5:15 p.m. UTC | #1 
On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
>
> From: Jann Horn <jannh@google.com>
>
> In preparation for changing the policy parsing logic, refactor the line
> parsing logic to be less verbose and move it into a separate function.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> Signed-off-by: Micah Morton <mortonm@chromium.org>

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

-Kees

> ---
> I made a minor change to Jann's original patch to use u32 instead of
> s32 for the 'parsed_parent' and 'parsed_child' variables.
>
>  security/safesetid/securityfs.c | 84 +++++++++++++--------------------
>  1 file changed, 33 insertions(+), 51 deletions(-)
>
> diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> index 2c6c829be044..90784a8d950a 100644
> --- a/security/safesetid/securityfs.c
> +++ b/security/safesetid/securityfs.c
> @@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = {
>
>  /*
>   * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> - * variables pointed to by 'parent' and 'child' will get updated but this
> + * variables pointed to by @parent and @child will get updated but this
>   * function will return an error.
> + * Contents of @buf may be modified.
>   */
> -static int parse_safesetid_whitelist_policy(const char __user *buf,
> -                                           size_t len,
> -                                           kuid_t *parent,
> -                                           kuid_t *child)
> +static int parse_policy_line(
> +       struct file *file, char *buf, kuid_t *parent, kuid_t *child)
>  {
> -       char *kern_buf;
> -       char *parent_buf;
> -       char *child_buf;
> -       const char separator[] = ":";
> +       char *child_str;
>         int ret;
> -       size_t first_substring_length;
> -       long parsed_parent;
> -       long parsed_child;
> +       u32 parsed_parent, parsed_child;
>
> -       /* Duplicate string from user memory and NULL-terminate */
> -       kern_buf = memdup_user_nul(buf, len);
> -       if (IS_ERR(kern_buf))
> -               return PTR_ERR(kern_buf);
> -
> -       /*
> -        * Format of |buf| string should be <UID>:<UID>.
> -        * Find location of ":" in kern_buf (copied from |buf|).
> -        */
> -       first_substring_length = strcspn(kern_buf, separator);
> -       if (first_substring_length == 0 || first_substring_length == len) {
> -               ret = -EINVAL;
> -               goto free_kern;
> -       }
> -
> -       parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
> -       if (!parent_buf) {
> -               ret = -ENOMEM;
> -               goto free_kern;
> -       }
> +       /* Format of |buf| string should be <UID>:<UID>. */
> +       child_str = strchr(buf, ':');
> +       if (child_str == NULL)
> +               return -EINVAL;
> +       *child_str = '\0';
> +       child_str++;
>
> -       ret = kstrtol(parent_buf, 0, &parsed_parent);
> +       ret = kstrtou32(buf, 0, &parsed_parent);
>         if (ret)
> -               goto free_both;
> +               return ret;
>
> -       child_buf = kern_buf + first_substring_length + 1;
> -       ret = kstrtol(child_buf, 0, &parsed_child);
> +       ret = kstrtou32(child_str, 0, &parsed_child);
>         if (ret)
> -               goto free_both;
> +               return ret;
>
>         *parent = make_kuid(current_user_ns(), parsed_parent);
> -       if (!uid_valid(*parent)) {
> -               ret = -EINVAL;
> -               goto free_both;
> -       }
> -
>         *child = make_kuid(current_user_ns(), parsed_child);
> -       if (!uid_valid(*child)) {
> -               ret = -EINVAL;
> -               goto free_both;
> -       }
> +       if (!uid_valid(*parent) || !uid_valid(*child))
> +               return -EINVAL;
>
> -free_both:
> -       kfree(parent_buf);
> -free_kern:
> +       return 0;
> +}
> +
> +static int parse_safesetid_whitelist_policy(
> +       struct file *file, const char __user *buf, size_t len,
> +       kuid_t *parent, kuid_t *child)
> +{
> +       char *kern_buf = memdup_user_nul(buf, len);
> +       int ret;
> +
> +       if (IS_ERR(kern_buf))
> +               return PTR_ERR(kern_buf);
> +       ret = parse_policy_line(file, kern_buf, parent, child);
>         kfree(kern_buf);
>         return ret;
>  }
> @@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file,
>                 flush_safesetid_whitelist_entries();
>                 break;
>         case SAFESETID_WHITELIST_ADD:
> -               ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> -                                                                &child);
> +               ret = parse_safesetid_whitelist_policy(file, buf, len,
> +                                                      &parent, &child);
>                 if (ret)
>                         return ret;
>
> --
> 2.21.0.392.gf8f6787159e-goog
>
Micah Morton May 7, 2019, 3:01 p.m. UTC | #2 
Ready for merge.

On Wed, Apr 10, 2019 at 10:15 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 10, 2019 at 9:55 AM Micah Morton <mortonm@chromium.org> wrote:
> >
> > From: Jann Horn <jannh@google.com>
> >
> > In preparation for changing the policy parsing logic, refactor the line
> > parsing logic to be less verbose and move it into a separate function.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Micah Morton <mortonm@chromium.org>
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> > ---
> > I made a minor change to Jann's original patch to use u32 instead of
> > s32 for the 'parsed_parent' and 'parsed_child' variables.
> >
> >  security/safesetid/securityfs.c | 84 +++++++++++++--------------------
> >  1 file changed, 33 insertions(+), 51 deletions(-)
> >
> > diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
> > index 2c6c829be044..90784a8d950a 100644
> > --- a/security/safesetid/securityfs.c
> > +++ b/security/safesetid/securityfs.c
> > @@ -33,68 +33,50 @@ static struct safesetid_file_entry safesetid_files[] = {
> >
> >  /*
> >   * In the case the input buffer contains one or more invalid UIDs, the kuid_t
> > - * variables pointed to by 'parent' and 'child' will get updated but this
> > + * variables pointed to by @parent and @child will get updated but this
> >   * function will return an error.
> > + * Contents of @buf may be modified.
> >   */
> > -static int parse_safesetid_whitelist_policy(const char __user *buf,
> > -                                           size_t len,
> > -                                           kuid_t *parent,
> > -                                           kuid_t *child)
> > +static int parse_policy_line(
> > +       struct file *file, char *buf, kuid_t *parent, kuid_t *child)
> >  {
> > -       char *kern_buf;
> > -       char *parent_buf;
> > -       char *child_buf;
> > -       const char separator[] = ":";
> > +       char *child_str;
> >         int ret;
> > -       size_t first_substring_length;
> > -       long parsed_parent;
> > -       long parsed_child;
> > +       u32 parsed_parent, parsed_child;
> >
> > -       /* Duplicate string from user memory and NULL-terminate */
> > -       kern_buf = memdup_user_nul(buf, len);
> > -       if (IS_ERR(kern_buf))
> > -               return PTR_ERR(kern_buf);
> > -
> > -       /*
> > -        * Format of |buf| string should be <UID>:<UID>.
> > -        * Find location of ":" in kern_buf (copied from |buf|).
> > -        */
> > -       first_substring_length = strcspn(kern_buf, separator);
> > -       if (first_substring_length == 0 || first_substring_length == len) {
> > -               ret = -EINVAL;
> > -               goto free_kern;
> > -       }
> > -
> > -       parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
> > -       if (!parent_buf) {
> > -               ret = -ENOMEM;
> > -               goto free_kern;
> > -       }
> > +       /* Format of |buf| string should be <UID>:<UID>. */
> > +       child_str = strchr(buf, ':');
> > +       if (child_str == NULL)
> > +               return -EINVAL;
> > +       *child_str = '\0';
> > +       child_str++;
> >
> > -       ret = kstrtol(parent_buf, 0, &parsed_parent);
> > +       ret = kstrtou32(buf, 0, &parsed_parent);
> >         if (ret)
> > -               goto free_both;
> > +               return ret;
> >
> > -       child_buf = kern_buf + first_substring_length + 1;
> > -       ret = kstrtol(child_buf, 0, &parsed_child);
> > +       ret = kstrtou32(child_str, 0, &parsed_child);
> >         if (ret)
> > -               goto free_both;
> > +               return ret;
> >
> >         *parent = make_kuid(current_user_ns(), parsed_parent);
> > -       if (!uid_valid(*parent)) {
> > -               ret = -EINVAL;
> > -               goto free_both;
> > -       }
> > -
> >         *child = make_kuid(current_user_ns(), parsed_child);
> > -       if (!uid_valid(*child)) {
> > -               ret = -EINVAL;
> > -               goto free_both;
> > -       }
> > +       if (!uid_valid(*parent) || !uid_valid(*child))
> > +               return -EINVAL;
> >
> > -free_both:
> > -       kfree(parent_buf);
> > -free_kern:
> > +       return 0;
> > +}
> > +
> > +static int parse_safesetid_whitelist_policy(
> > +       struct file *file, const char __user *buf, size_t len,
> > +       kuid_t *parent, kuid_t *child)
> > +{
> > +       char *kern_buf = memdup_user_nul(buf, len);
> > +       int ret;
> > +
> > +       if (IS_ERR(kern_buf))
> > +               return PTR_ERR(kern_buf);
> > +       ret = parse_policy_line(file, kern_buf, parent, child);
> >         kfree(kern_buf);
> >         return ret;
> >  }
> > @@ -121,8 +103,8 @@ static ssize_t safesetid_file_write(struct file *file,
> >                 flush_safesetid_whitelist_entries();
> >                 break;
> >         case SAFESETID_WHITELIST_ADD:
> > -               ret = parse_safesetid_whitelist_policy(buf, len, &parent,
> > -                                                                &child);
> > +               ret = parse_safesetid_whitelist_policy(file, buf, len,
> > +                                                      &parent, &child);
> >                 if (ret)
> >                         return ret;
> >
> > --
> > 2.21.0.392.gf8f6787159e-goog
> >
>
>
> --
> Kees Cook
diff mbox series

Patch

diff --git a/security/safesetid/securityfs.c b/security/safesetid/securityfs.c
index 2c6c829be044..90784a8d950a 100644
--- a/security/safesetid/securityfs.c
+++ b/security/safesetid/securityfs.c
@@ -33,68 +33,50 @@  static struct safesetid_file_entry safesetid_files[] = {
 
 /*
  * In the case the input buffer contains one or more invalid UIDs, the kuid_t
- * variables pointed to by 'parent' and 'child' will get updated but this
+ * variables pointed to by @parent and @child will get updated but this
  * function will return an error.
+ * Contents of @buf may be modified.
  */
-static int parse_safesetid_whitelist_policy(const char __user *buf,
-					    size_t len,
-					    kuid_t *parent,
-					    kuid_t *child)
+static int parse_policy_line(
+	struct file *file, char *buf, kuid_t *parent, kuid_t *child)
 {
-	char *kern_buf;
-	char *parent_buf;
-	char *child_buf;
-	const char separator[] = ":";
+	char *child_str;
 	int ret;
-	size_t first_substring_length;
-	long parsed_parent;
-	long parsed_child;
+	u32 parsed_parent, parsed_child;
 
-	/* Duplicate string from user memory and NULL-terminate */
-	kern_buf = memdup_user_nul(buf, len);
-	if (IS_ERR(kern_buf))
-		return PTR_ERR(kern_buf);
-
-	/*
-	 * Format of |buf| string should be <UID>:<UID>.
-	 * Find location of ":" in kern_buf (copied from |buf|).
-	 */
-	first_substring_length = strcspn(kern_buf, separator);
-	if (first_substring_length == 0 || first_substring_length == len) {
-		ret = -EINVAL;
-		goto free_kern;
-	}
-
-	parent_buf = kmemdup_nul(kern_buf, first_substring_length, GFP_KERNEL);
-	if (!parent_buf) {
-		ret = -ENOMEM;
-		goto free_kern;
-	}
+	/* Format of |buf| string should be <UID>:<UID>. */
+	child_str = strchr(buf, ':');
+	if (child_str == NULL)
+		return -EINVAL;
+	*child_str = '\0';
+	child_str++;
 
-	ret = kstrtol(parent_buf, 0, &parsed_parent);
+	ret = kstrtou32(buf, 0, &parsed_parent);
 	if (ret)
-		goto free_both;
+		return ret;
 
-	child_buf = kern_buf + first_substring_length + 1;
-	ret = kstrtol(child_buf, 0, &parsed_child);
+	ret = kstrtou32(child_str, 0, &parsed_child);
 	if (ret)
-		goto free_both;
+		return ret;
 
 	*parent = make_kuid(current_user_ns(), parsed_parent);
-	if (!uid_valid(*parent)) {
-		ret = -EINVAL;
-		goto free_both;
-	}
-
 	*child = make_kuid(current_user_ns(), parsed_child);
-	if (!uid_valid(*child)) {
-		ret = -EINVAL;
-		goto free_both;
-	}
+	if (!uid_valid(*parent) || !uid_valid(*child))
+		return -EINVAL;
 
-free_both:
-	kfree(parent_buf);
-free_kern:
+	return 0;
+}
+
+static int parse_safesetid_whitelist_policy(
+	struct file *file, const char __user *buf, size_t len,
+	kuid_t *parent, kuid_t *child)
+{
+	char *kern_buf = memdup_user_nul(buf, len);
+	int ret;
+
+	if (IS_ERR(kern_buf))
+		return PTR_ERR(kern_buf);
+	ret = parse_policy_line(file, kern_buf, parent, child);
 	kfree(kern_buf);
 	return ret;
 }
@@ -121,8 +103,8 @@  static ssize_t safesetid_file_write(struct file *file,
 		flush_safesetid_whitelist_entries();
 		break;
 	case SAFESETID_WHITELIST_ADD:
-		ret = parse_safesetid_whitelist_policy(buf, len, &parent,
-								 &child);
+		ret = parse_safesetid_whitelist_policy(file, buf, len,
+						       &parent, &child);
 		if (ret)
 			return ret;