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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
http://www.paul-moore.com
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 <[email protected]>
> ---
> 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
http://www.paul-moore.com
Hi Paul,
On Thu, Jun 19, 2014 at 4:36 AM, Paul Moore <[email protected]> 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
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 <[email protected]>
>> ---
>> 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).
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 <[email protected]> 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
http://www.paul-moore.com