[patch 4/7] selinux: fix error codes in cond_read_node()

[patch 4/7] selinux: fix error codes in cond_read_node()

[Dan Carpenter]

Originally cond_read_node() returned -1 (-EPERM) on errors which was
incorrect.  Now it either propagates the error codes from lower level
functions next_entry() or cond_read_av_list() or it returns -ENOMEM or
-EINVAL.

next_entry() returns -EINVAL.
cond_read_av_list() returns -EINVAL or -ENOMEM.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 4c39f19..f0de637 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -392,14 +392,14 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 
 	rc = next_entry(buf, fp, sizeof(u32));
 	if (rc < 0)
-		return -1;
+		return rc;
 
 	node->cur_state = le32_to_cpu(buf[0]);
 
 	len = 0;
 	rc = next_entry(buf, fp, sizeof(u32));
 	if (rc < 0)
-		return -1;
+		return rc;
 
 	/* expr */
 	len = le32_to_cpu(buf[0]);
@@ -409,6 +409,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 		if (rc < 0)
 			goto err;
 
+		rc = -ENOMEM;
 		expr = kzalloc(sizeof(struct cond_expr), GFP_KERNEL);
 		if (!expr)
 			goto err;
@@ -417,6 +418,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 		expr->bool = le32_to_cpu(buf[1]);
 
 		if (!expr_isvalid(p, expr)) {
+			rc = -EINVAL;
 			kfree(expr);
 			goto err;
 		}
@@ -428,14 +430,16 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 		last = expr;
 	}
 
-	if (cond_read_av_list(p, fp, &node->true_list, NULL) != 0)
+	rc = cond_read_av_list(p, fp, &node->true_list, NULL);
+	if (rc < 0)
 		goto err;
-	if (cond_read_av_list(p, fp, &node->false_list, node->true_list) != 0)
+	rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
+	if (rc < 0)
 		goto err;
 	return 0;
 err:
 	cond_node_destroy(node);
-	return -1;
+	return rc;
 }
 
 int cond_read_list(struct policydb *p, void *fp)

Re: [patch 4/7] selinux: fix error codes in cond_read_node()

[Eric Paris]

On Mon, Jun 7, 2010 at 5:07 PM, Dan Carpenter <error27@gmail.com> wrote:
> Originally cond_read_node() returned -1 (-EPERM) on errors which was
> incorrect.  Now it either propagates the error codes from lower level
> functions next_entry() or cond_read_av_list() or it returns -ENOMEM or
> -EINVAL.
>
> next_entry() returns -EINVAL.
> cond_read_av_list() returns -EINVAL or -ENOMEM.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 4c39f19..f0de637 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -392,14 +392,14 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>
>        rc = next_entry(buf, fp, sizeof(u32));
>        if (rc < 0)
> -               return -1;
> +               return rc;
>
>        node->cur_state = le32_to_cpu(buf[0]);
>
>        len = 0;
>        rc = next_entry(buf, fp, sizeof(u32));
>        if (rc < 0)
> -               return -1;
> +               return rc;
>
>        /* expr */
>        len = le32_to_cpu(buf[0]);
> @@ -409,6 +409,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>                if (rc < 0)
>                        goto err;
>
> +               rc = -ENOMEM;
>                expr = kzalloc(sizeof(struct cond_expr), GFP_KERNEL);
>                if (!expr)
>                        goto err;
> @@ -417,6 +418,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>                expr->bool = le32_to_cpu(buf[1]);
>
>                if (!expr_isvalid(p, expr)) {
> +                       rc = -EINVAL;
>                        kfree(expr);
>                        goto err;
>                }
> @@ -428,14 +430,16 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>                last = expr;
>        }
>
> -       if (cond_read_av_list(p, fp, &node->true_list, NULL) != 0)
> +       rc = cond_read_av_list(p, fp, &node->true_list, NULL);
> +       if (rc < 0)
>                goto err;
> -       if (cond_read_av_list(p, fp, &node->false_list, node->true_list) != 0)
> +       rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
> +       if (rc < 0)
>                goto err;

I know that lots of the next_entry() calls use if (rc < 0) [unrelated
note I think those need to be fixed too] but most of the code uses if
(rc) and I strongly prefer if (rc) as it's cleaner and faster from a
micro architecture point of view.  Would you mind re-spinning this
one?

-Eric
>        return 0;
>  err:
>        cond_node_destroy(node);
> -       return -1;
> +       return rc;
>  }
>
>  int cond_read_list(struct policydb *p, void *fp)
>
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 4/7] selinux: fix error codes in cond_read_node()

[Dan Carpenter]

On Tue, Jun 08, 2010 at 04:19:18PM -0400, Eric Paris wrote:
> > +       rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
> > +       if (rc < 0)
> >                goto err;
> 
> I know that lots of the next_entry() calls use if (rc < 0) [unrelated
> note I think those need to be fixed too] but most of the code uses if
> (rc) and I strongly prefer if (rc) as it's cleaner and faster from a
> micro architecture point of view.  Would you mind re-spinning this
> one?
> 
> -Eric

The thing I like about if (rc < 0) is that it's makes it explicit that
we expect negative error values.  My gut says this will make it easier
for static checkers to track what the value of "rc" is.

But I don't have strong feelings about this.

I'll resend the whole set tomorrow or the next day with all the changes
you suggested.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html