On Fri, Jan 31, 2020 at 5:05 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> >
> > Since it is fixed-size after allocation and we know the size beforehand,
> > using a plain old array is simpler and more efficient.
> >
> > While there, also fix signedness of some related variables/parameters.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
> > ---
> > security/selinux/include/conditional.h | 6 +--
> > security/selinux/selinuxfs.c | 4 +-
> > security/selinux/ss/conditional.c | 54 ++++++++++----------------
> > security/selinux/ss/conditional.h | 3 +-
> > security/selinux/ss/policydb.c | 2 +-
> > security/selinux/ss/policydb.h | 3 +-
> > security/selinux/ss/services.c | 28 ++++++-------
> > 7 files changed, 43 insertions(+), 57 deletions(-)
> >
> > diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
> > index 0ab316f61da0..ffb9a33341f8 100644
> > --- a/security/selinux/include/conditional.h
> > +++ b/security/selinux/include/conditional.h
> > @@ -14,12 +14,12 @@
> > #include "security.h"
> >
> > int security_get_bools(struct selinux_state *state,
> > - int *len, char ***names, int **values);
> > + u32 *len, char ***names, int **values);
> >
> > int security_set_bools(struct selinux_state *state,
> > - int len, int *values);
> > + u32 len, int *values);
>
> In the future, if you're making changes like this, you might as well
> put it all on one line (it fits under 80 chars).
Good point, will do that in v3.
>
> > int security_get_bool_value(struct selinux_state *state,
> > - int index);
> > + u32 index);
>
> Same here.
>
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index 04593062008d..c8a02c9b23ee 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
> >
> > p->bool_val_to_struct = NULL;
> > p->cond_list = NULL;
> > + p->cond_list_len = 0;
> >
> > rc = avtab_init(&p->te_cond_avtab);
> > if (rc)
> > @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
> > }
> > cond_av_list_destroy(node->true_list);
> > cond_av_list_destroy(node->false_list);
> > - kfree(node);
> > }
> >
> > -static void cond_list_destroy(struct cond_node *list)
> > +static void cond_list_destroy(struct policydb *p)
> > {
> > - struct cond_node *next, *cur;
> > + u32 i;
> >
> > - if (list == NULL)
> > - return;
> > -
> > - for (cur = list; cur; cur = next) {
> > - next = cur->next;
> > - cond_node_destroy(cur);
> > - }
> > + for (i = 0; i < p->cond_list_len; i++)
> > + cond_node_destroy(&p->cond_list[i]);
> > + kfree(p->cond_list);
> > }
> >
> > void cond_policydb_destroy(struct policydb *p)
> > {
> > kfree(p->bool_val_to_struct);
> > avtab_destroy(&p->te_cond_avtab);
> > - cond_list_destroy(p->cond_list);
> > + cond_list_destroy(p);
> > }
> >
> > int cond_init_bool_indexes(struct policydb *p)
> > @@ -447,7 +443,6 @@ err:
> >
> > int cond_read_list(struct policydb *p, void *fp)
> > {
> > - struct cond_node *node, *last = NULL;
> > __le32 buf[1];
> > u32 i, len;
> > int rc;
> > @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
> >
> > len = le32_to_cpu(buf[0]);
> >
> > + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> > + if (!p->cond_list)
> > + return rc;
> > +
> > rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
> > if (rc)
> > goto err;
> >
> > for (i = 0; i < len; i++) {
> > - rc = -ENOMEM;
> > - node = kzalloc(sizeof(*node), GFP_KERNEL);
> > - if (!node)
> > - goto err;
> > -
> > - rc = cond_read_node(p, node, fp);
> > + rc = cond_read_node(p, &p->cond_list[i], fp);
> > if (rc)
> > goto err;
> > -
> > - if (i == 0)
> > - p->cond_list = node;
> > - else
> > - last->next = node;
> > - last = node;
> > }
> > +
> > + p->cond_list_len = len;
>
> Hmmm. Since we don't update p->cond_list_len until here, if we fail
> in the for-loop above and end up jumping to "err" we might not cleanup
> all the nodes, right?
You're right. I'll fix it by moving the p->cond_list_len assignment
before the for-loop - since the array is zero-initialized by
kcalloc(), cond_node_destroy() can be called safely also on unread
items. (That is a bit inefficient, but I see little value in
micro-optimizing the error path.)
>
> > return 0;
> > err:
> > - cond_list_destroy(p->cond_list);
> > + cond_list_destroy(p);
> > p->cond_list = NULL;
> > return rc;
> > }
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
