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).
> 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?
> 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
