[PATCH 1/2] selinux: simple cleanup for cond_read_node()

[PATCH 1/2] selinux: simple cleanup for cond_read_node()

[Namhyung Kim]

The node->cur_state and len can be read in a single call of next_entry().
And setting len before reading is a dead write so can be eliminated.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 security/selinux/ss/conditional.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 377d148e7157..4766a38fae9a 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 	int rc;
 	struct cond_expr *expr = NULL, *last = NULL;
 
-	rc = next_entry(buf, fp, sizeof(u32));
+	rc = next_entry(buf, fp, sizeof(buf));
 	if (rc)
 		return rc;
 
 	node->cur_state = le32_to_cpu(buf[0]);
 
-	len = 0;
-	rc = next_entry(buf, fp, sizeof(u32));
-	if (rc)
-		return rc;
-
 	/* expr */
-	len = le32_to_cpu(buf[0]);
+	len = le32_to_cpu(buf[1]);
 
 	for (i = 0; i < len; i++) {
 		rc = next_entry(buf, fp, sizeof(u32) * 2);
-- 
2.0.0

[PATCH 2/2] selinux: fix a possible memory leak in cond_read_node()

[Namhyung Kim]

The cond_read_node() should free the given node on error path as it's
not linked to p->cond_list yet.  This is done via cond_node_destroy()
but it's not called when next_entry() fails before the expr loop.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 security/selinux/ss/conditional.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 4766a38fae9a..470d5cca8d14 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -404,7 +404,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
 
 	rc = next_entry(buf, fp, sizeof(buf));
 	if (rc)
-		return rc;
+		goto err;
 
 	node->cur_state = le32_to_cpu(buf[0]);
 
-- 
2.0.0

Re: [PATCH 1/2] selinux: simple cleanup for cond_read_node()

[Paul Moore]

On Sunday, June 15, 2014 01:19:01 AM Namhyung Kim wrote:
> The node->cur_state and len can be read in a single call of next_entry().
> And setting len before reading is a dead write so can be eliminated.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  security/selinux/ss/conditional.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/security/selinux/ss/conditional.c
> b/security/selinux/ss/conditional.c index 377d148e7157..4766a38fae9a 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p, struct
> cond_node *node, void *fp) int rc;
>  	struct cond_expr *expr = NULL, *last = NULL;
> 
> -	rc = next_entry(buf, fp, sizeof(u32));
> +	rc = next_entry(buf, fp, sizeof(buf));

This is a bit nit-picky, but how about using "sizeof(u32) * 2"?  It is more 
consistent with the rest of the function and helps underscore that we are 
reading two 32-bit values.

Assuming you're okay with the change I can fix it up when I apply the patch.

>  	if (rc)
>  		return rc;
> 
>  	node->cur_state = le32_to_cpu(buf[0]);
> 
> -	len = 0;
> -	rc = next_entry(buf, fp, sizeof(u32));
> -	if (rc)
> -		return rc;
> -
>  	/* expr */
> -	len = le32_to_cpu(buf[0]);
> +	len = le32_to_cpu(buf[1]);
> 
>  	for (i = 0; i < len; i++) {
>  		rc = next_entry(buf, fp, sizeof(u32) * 2);

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] selinux: fix a possible memory leak in cond_read_node()

[Paul Moore]

On Sunday, June 15, 2014 01:19:02 AM Namhyung Kim wrote:
> The cond_read_node() should free the given node on error path as it's
> not linked to p->cond_list yet.  This is done via cond_node_destroy()
> but it's not called when next_entry() fails before the expr loop.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  security/selinux/ss/conditional.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, nice catch.  This patch looks good to me but it is dependent on patch 
1/2 which I commented on ... 

> diff --git a/security/selinux/ss/conditional.c
> b/security/selinux/ss/conditional.c index 4766a38fae9a..470d5cca8d14 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -404,7 +404,7 @@ static int cond_read_node(struct policydb *p, struct
> cond_node *node, void *fp)
> 
>  	rc = next_entry(buf, fp, sizeof(buf));
>  	if (rc)
> -		return rc;
> +		goto err;
> 
>  	node->cur_state = le32_to_cpu(buf[0]);

-- 
paul moore
www.paul-moore.com

Re: [PATCH 1/2] selinux: simple cleanup for cond_read_node()

[Namhyung Kim]

Hi Paul,

On Thu, Jun 19, 2014 at 4:36 AM, Paul Moore <paul@paul-moore.com> wrote:
> > @@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p, struct
>> cond_node *node, void *fp) int rc;
>>       struct cond_expr *expr = NULL, *last = NULL;
>>
>> -     rc = next_entry(buf, fp, sizeof(u32));
>> +     rc = next_entry(buf, fp, sizeof(buf));
>
> This is a bit nit-picky, but how about using "sizeof(u32) * 2"?  It is more
> consistent with the rest of the function and helps underscore that we are
> reading two 32-bit values.
>
> Assuming you're okay with the change I can fix it up when I apply the patch.

I'm okay with it. :)

Thanks,
Namhyung

Re: [PATCH 1/2] selinux: simple cleanup for cond_read_node()

[Stephen Smalley]

On 06/18/2014 03:36 PM, Paul Moore wrote:
> On Sunday, June 15, 2014 01:19:01 AM Namhyung Kim wrote:
>> The node->cur_state and len can be read in a single call of next_entry().
>> And setting len before reading is a dead write so can be eliminated.
>>
>> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>> ---
>>  security/selinux/ss/conditional.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/security/selinux/ss/conditional.c
>> b/security/selinux/ss/conditional.c index 377d148e7157..4766a38fae9a 100644
>> --- a/security/selinux/ss/conditional.c
>> +++ b/security/selinux/ss/conditional.c
>> @@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p, struct
>> cond_node *node, void *fp) int rc;
>>  	struct cond_expr *expr = NULL, *last = NULL;
>>
>> -	rc = next_entry(buf, fp, sizeof(u32));
>> +	rc = next_entry(buf, fp, sizeof(buf));
> 
> This is a bit nit-picky, but how about using "sizeof(u32) * 2"?  It is more 
> consistent with the rest of the function and helps underscore that we are 

Concur - I don't want to assume that the buf size is always the same as
the next read size (e.g. we sometimes use the same buf for multiple reads).

Re: [PATCH 1/2] selinux: simple cleanup for cond_read_node()

[Paul Moore]

On Thursday, June 19, 2014 08:58:31 AM Namhyung Kim wrote:
> Hi Paul,
> 
> On Thu, Jun 19, 2014 at 4:36 AM, Paul Moore <paul@paul-moore.com> wrote:
> > > @@ -402,19 +402,14 @@ static int cond_read_node(struct policydb *p,
> > > struct
> >> 
> >> cond_node *node, void *fp) int rc;
> >> 
> >>       struct cond_expr *expr = NULL, *last = NULL;
> >> 
> >> -     rc = next_entry(buf, fp, sizeof(u32));
> >> +     rc = next_entry(buf, fp, sizeof(buf));
> > 
> > This is a bit nit-picky, but how about using "sizeof(u32) * 2"?  It is
> > more
> > consistent with the rest of the function and helps underscore that we are
> > reading two 32-bit values.
> > 
> > Assuming you're okay with the change I can fix it up when I apply the
> > patch.
>
> I'm okay with it. :)

Great, both patches are applied.

-- 
paul moore
www.paul-moore.com