[PATCH 01/46] selinux: Use kmalloc_array() in cond_init_bool_indexes()

[PATCH 01/46] selinux: Use kmalloc_array() in cond_init_bool_indexes()

[Paul Moore]

On Sun, Jan 15, 2017 at 9:56 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 10:48:28 +0100
>
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "kmalloc_array".
>
>   This issue was detected by using the Coccinelle software.
>
> * Replace the specification of a data type by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/conditional.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Merged, thanks.  Sorry for the delay.

> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 34afeadd9e73..fcfab2635c11 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -176,8 +176,9 @@ void cond_policydb_destroy(struct policydb *p)
>  int cond_init_bool_indexes(struct policydb *p)
>  {
>         kfree(p->bool_val_to_struct);
> -       p->bool_val_to_struct =
> -               kmalloc(p->p_bools.nprim * sizeof(struct cond_bool_datum *), GFP_KERNEL);
> +       p->bool_val_to_struct = kmalloc_array(p->p_bools.nprim,
> +                                             sizeof(*p->bool_val_to_struct),
> +                                             GFP_KERNEL);
>         if (!p->bool_val_to_struct)
>                 return -ENOMEM;
>         return 0;
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/46] selinux: Delete an unnecessary return statement in cond_compute_av()

[Paul Moore]

On Sun, Jan 15, 2017 at 9:58 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 11:00:23 +0100
>
> The script "checkpatch.pl" pointed information out like the following.
>
> WARNING: void function return statements are not generally useful
>
> Thus remove such a statement in the affected function.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/conditional.c | 1 -
>  1 file changed, 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index fcfab2635c11..4a3bf29f7565 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -664,5 +664,4 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
>                                 (node->key.specified & AVTAB_XPERMS))
>                         services_compute_xperms_drivers(xperms, node);
>         }
> -       return;
>  }
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/46] selinux: Improve size determinations in four functions

[Paul Moore]

On Sun, Jan 15, 2017 at 10:00 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 11:22:12 +0100
>
> Replace the specification of data structures by pointer dereferences
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/conditional.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 4a3bf29f7565..771c96afe1d5 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -227,7 +227,7 @@ int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp)
>         u32 len;
>         int rc;
>
> -       booldatum = kzalloc(sizeof(struct cond_bool_datum), GFP_KERNEL);
> +       booldatum = kzalloc(sizeof(*booldatum), GFP_KERNEL);
>         if (!booldatum)
>                 return -ENOMEM;
>
> @@ -332,7 +332,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
>                 goto err;
>         }
>
> -       list = kzalloc(sizeof(struct cond_av_list), GFP_KERNEL);
> +       list = kzalloc(sizeof(*list), GFP_KERNEL);
>         if (!list) {
>                 rc = -ENOMEM;
>                 goto err;
> @@ -421,7 +421,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
>                         goto err;
>
>                 rc = -ENOMEM;
> -               expr = kzalloc(sizeof(struct cond_expr), GFP_KERNEL);
> +               expr = kzalloc(sizeof(*expr), GFP_KERNEL);
>                 if (!expr)
>                         goto err;
>
> @@ -472,7 +472,7 @@ int cond_read_list(struct policydb *p, void *fp)
>
>         for (i = 0; i < len; i++) {
>                 rc = -ENOMEM;
> -               node = kzalloc(sizeof(struct cond_node), GFP_KERNEL);
> +               node = kzalloc(sizeof(*node), GFP_KERNEL);
>                 if (!node)
>                         goto err;
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/46] selinux: Use kmalloc_array() in hashtab_create()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:01 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 12:06:13 +0100
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/hashtab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
> index 2cc496149842..dc99fff64ecb 100644
> --- a/security/selinux/ss/hashtab.c
> +++ b/security/selinux/ss/hashtab.c
> @@ -24,7 +24,7 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *
>         p->nel = 0;
>         p->hash_value = hash_value;
>         p->keycmp = keycmp;
> -       p->htable = kmalloc(sizeof(*(p->htable)) * size, GFP_KERNEL);
> +       p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL);
>         if (p->htable == NULL) {
>                 kfree(p);
>                 return NULL;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/46] selinux: Adjust four checks for null pointers

[Paul Moore]

On Sun, Jan 15, 2017 at 10:02 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 12:36:59 +0100
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The script "checkpatch.pl" pointed information out like the following.
>
> Comparison to NULL could be written !?
>
> Thus fix affected source code places.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/hashtab.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I'm not generally in favor of style changes like this, but I'll accept
it since there is something to be said for better compliance with
checkpatch.pl.

> diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
> index dc99fff64ecb..3858706a29fb 100644
> --- a/security/selinux/ss/hashtab.c
> +++ b/security/selinux/ss/hashtab.c
> @@ -17,7 +17,7 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *
>         u32 i;
>
>         p = kzalloc(sizeof(*p), GFP_KERNEL);
> -       if (p == NULL)
> +       if (!p)
>                 return p;
>
>         p->size = size;
> @@ -25,7 +25,7 @@ struct hashtab *hashtab_create(u32 (*hash_value)(struct hashtab *h, const void *
>         p->hash_value = hash_value;
>         p->keycmp = keycmp;
>         p->htable = kmalloc_array(size, sizeof(*p->htable), GFP_KERNEL);
> -       if (p->htable == NULL) {
> +       if (!p->htable) {
>                 kfree(p);
>                 return NULL;
>         }
> @@ -58,7 +58,7 @@ int hashtab_insert(struct hashtab *h, void *key, void *datum)
>                 return -EEXIST;
>
>         newnode = kzalloc(sizeof(*newnode), GFP_KERNEL);
> -       if (newnode == NULL)
> +       if (!newnode)
>                 return -ENOMEM;
>         newnode->key = key;
>         newnode->datum = datum;
> @@ -87,7 +87,7 @@ void *hashtab_search(struct hashtab *h, const void *key)
>         while (cur && h->keycmp(h, key, cur->key) > 0)
>                 cur = cur->next;
>
> -       if (cur == NULL || (h->keycmp(h, key, cur->key) != 0))
> +       if (!cur || (h->keycmp(h, key, cur->key) != 0))
>                 return NULL;
>
>         return cur->datum;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/46] selinux: Use kcalloc() in policydb_index()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:03 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 13:08:59 +0100
>
> Multiplications for the size determination of memory allocations
> indicated that array data structures should be processed.
> Thus use the corresponding function "kcalloc".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index d719db4219cd..21869b622c0c 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -540,23 +540,23 @@ static int policydb_index(struct policydb *p)
>  #endif
>
>         rc = -ENOMEM;
> -       p->class_val_to_struct =
> -               kzalloc(p->p_classes.nprim * sizeof(*(p->class_val_to_struct)),
> -                       GFP_KERNEL);
> +       p->class_val_to_struct = kcalloc(p->p_classes.nprim,
> +                                        sizeof(*p->class_val_to_struct),
> +                                        GFP_KERNEL);
>         if (!p->class_val_to_struct)
>                 goto out;
>
>         rc = -ENOMEM;
> -       p->role_val_to_struct =
> -               kzalloc(p->p_roles.nprim * sizeof(*(p->role_val_to_struct)),
> -                       GFP_KERNEL);
> +       p->role_val_to_struct = kcalloc(p->p_roles.nprim,
> +                                       sizeof(*p->role_val_to_struct),
> +                                       GFP_KERNEL);
>         if (!p->role_val_to_struct)
>                 goto out;
>
>         rc = -ENOMEM;
> -       p->user_val_to_struct =
> -               kzalloc(p->p_users.nprim * sizeof(*(p->user_val_to_struct)),
> -                       GFP_KERNEL);
> +       p->user_val_to_struct = kcalloc(p->p_users.nprim,
> +                                       sizeof(*p->user_val_to_struct),
> +                                       GFP_KERNEL);
>         if (!p->user_val_to_struct)
>                 goto out;
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/46] selinux: Delete unnecessary variable assignments in policydb_index()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:04 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 13:40:25 +0100
>
> The local variable "rc" was reset with an error code up to five times
> before a memory allocation failure was detected.
>
> Add a jump target so that this assignment will only be performed after
> a concrete software failure.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

Like you, I generally despise the notion of unconditionally setting
return codes to an error value *before* the faulting call; see Eric's
response on patch 0/46, my preferred style is different from Eric.
However, I agree with Casey that this patch is mostly just code churn
so I'm going to drop this from your series.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 21869b622c0c..4d4ba1ad910d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -539,34 +539,30 @@ static int policydb_index(struct policydb *p)
>         symtab_hash_eval(p->symtab);
>  #endif
>
> -       rc = -ENOMEM;
>         p->class_val_to_struct = kcalloc(p->p_classes.nprim,
>                                          sizeof(*p->class_val_to_struct),
>                                          GFP_KERNEL);
>         if (!p->class_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
> -       rc = -ENOMEM;
>         p->role_val_to_struct = kcalloc(p->p_roles.nprim,
>                                         sizeof(*p->role_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->role_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
> -       rc = -ENOMEM;
>         p->user_val_to_struct = kcalloc(p->p_users.nprim,
>                                         sizeof(*p->user_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->user_val_to_struct)
> -               goto out;
> +               goto failure_indication;
>
>         /* Yes, I want the sizeof the pointer, not the structure */
> -       rc = -ENOMEM;
>         p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
>                                                        p->p_types.nprim,
>                                                        GFP_KERNEL | __GFP_ZERO);
>         if (!p->type_val_to_struct_array)
> -               goto out;
> +               goto failure_indication;
>
>         rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
>                                  p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
> @@ -578,12 +574,11 @@ static int policydb_index(struct policydb *p)
>                 goto out;
>
>         for (i = 0; i < SYM_NUM; i++) {
> -               rc = -ENOMEM;
>                 p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
>                                                          p->symtab[i].nprim,
>                                                          GFP_KERNEL | __GFP_ZERO);
>                 if (!p->sym_val_to_name[i])
> -                       goto out;
> +                       goto failure_indication;
>
>                 rc = flex_array_prealloc(p->sym_val_to_name[i],
>                                          0, p->symtab[i].nprim,
> @@ -598,6 +593,9 @@ static int policydb_index(struct policydb *p)
>         rc = 0;
>  out:
>         return rc;
> +failure_indication:
> +       rc = -ENOMEM;
> +       goto out;
>  }
>
>  /*
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete unnecessary variable assignments in policydb_index()

[SF Markus Elfring]

> However, I agree with Casey that this patch is mostly just code churn
> so I'm going to drop this from your series.

How do you think about to return only constant error codes in this function?
Would it be acceptable to replace any statements ?goto out;? with
?return -ENOMEM;? here instead?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete unnecessary variable assignments in policydb_index()

[Paul Moore]

On Mon, Mar 27, 2017 at 2:24 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> However, I agree with Casey that this patch is mostly just code churn
>> so I'm going to drop this from your series.
>
> How do you think about to return only constant error codes in this function?
> Would it be acceptable to replace any statements ?goto out;? with
> ?return -ENOMEM;? here instead?

Yes.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/46] selinux: Delete an unnecessary return statement in policydb_destroy()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:06 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 14:00:02 +0100
>
> The script "checkpatch.pl" pointed information out like the following.
>
> WARNING: void function return statements are not generally useful
>
> Thus remove such a statement in the affected function.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 --
>  1 file changed, 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 4d4ba1ad910d..fe8992382a71 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -878,8 +878,6 @@ void policydb_destroy(struct policydb *p)
>         ebitmap_destroy(&p->filename_trans_ttypes);
>         ebitmap_destroy(&p->policycaps);
>         ebitmap_destroy(&p->permissive_map);
> -
> -       return;
>  }
>
>  /*
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/46] selinux: Delete an error message for a failed memory allocation in policydb_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:07 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 14:20:41 +0100
>
> Omit an extra message for a memory allocation failure in this function.
>
> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

I'm not going to remove an error message without some better reasoning
in the patch description.  Providing a link to slides is fine, but
your commit message needs to convey the important information and I
don't think that is the case here (what happens when that URL dies?).

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index fe8992382a71..53e6d06e772a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2269,11 +2269,8 @@ int policydb_read(struct policydb *p, void *fp)
>
>         rc = -ENOMEM;
>         policydb_str = kmalloc(len + 1, GFP_KERNEL);
> -       if (!policydb_str) {
> -               printk(KERN_ERR "SELinux:  unable to allocate memory for policydb "
> -                      "string of length %d\n", len);
> +       if (!policydb_str)
>                 goto bad;
> -       }
>
>         rc = next_entry(policydb_str, fp, len);
>         if (rc) {
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete an error message for a failed memory allocation in policydb_read()

[SF Markus Elfring]

>> Omit an extra message for a memory allocation failure in this function.
>>
>> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  security/selinux/ss/policydb.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> I'm not going to remove an error message without some better reasoning
> in the patch description.  Providing a link to slides is fine, but
> your commit message needs to convey the important information and I
> don't think that is the case here (what happens when that URL dies?).

Do you need an explicit reminder there that the function ?kmalloc? provides its own
error reporting already because the flag ?__GFP_NOWARN? was not passed here?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete an error message for a failed memory allocation in policydb_read()

[Paul Moore]

On Fri, Mar 24, 2017 at 8:13 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> Omit an extra message for a memory allocation failure in this function.
>>>
>>> Link: http://events.linuxfoundation.org/sites/events/files/slides/LCJ16-Refactor_Strings-WSang_0.pdf
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>>  security/selinux/ss/policydb.c | 5 +----
>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> I'm not going to remove an error message without some better reasoning
>> in the patch description.  Providing a link to slides is fine, but
>> your commit message needs to convey the important information and I
>> don't think that is the case here (what happens when that URL dies?).
>
> Do you need an explicit reminder there that the function ?kmalloc? provides its own
> error reporting already because the flag ?__GFP_NOWARN? was not passed here?

That is what I said by "better reasoning in the patch description",
however, now that I'm looking at this again, I don't think I'm going
to merge this.  Yes, maybe in some cases it is a bit wasteful, but I
like the error message.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete an error message for a failed memory allocation in policydb_read()

[SF Markus Elfring]

> ?, but I like the error message.

How do you think about to pass the flag ?__GFP_NOWARN? if you like
this information more than the default error reporting of the function ?kmalloc??

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Delete an error message for a failed memory allocation in policydb_read()

[Paul Moore]

On Mon, Mar 27, 2017 at 1:56 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> ?, but I like the error message.
>
> How do you think about to pass the flag ?__GFP_NOWARN? if you like
> this information more than the default error reporting of the function ?kmalloc??

Possibly, although I would encourage you to just leave it as-is for
the moment.  Reviewing and merging patches carries a cost, and I would
very much prefer to allocate my time/resources on changes that have a
more significant impact.  I don't want to discourage your from
contributing to SELinux development, but I do want to strongly
encourage you to contribute more meaningful patches.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/46] selinux: Move some assignments for the variable "rc" in policydb_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:10 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 15:22:29 +0100
>
> One local variable was set to an error code in some cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++-----------------
>  1 file changed, 35 insertions(+), 24 deletions(-)

More code churn with no real advantage.  I agree with the style you
are using, and would support changing it if you are in the function
fixing bugs or doing other substantial changes in that code, but I
can't justify it as a standalone change, sorry.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 53e6d06e772a..506b0228d1f1 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2250,15 +2250,14 @@ int policydb_read(struct policydb *p, void *fp)
>         if (rc)
>                 goto bad;
>
> -       rc = -EINVAL;
>         if (le32_to_cpu(buf[0]) != POLICYDB_MAGIC) {
>                 printk(KERN_ERR "SELinux:  policydb magic number 0x%x does "
>                        "not match expected magic number 0x%x\n",
>                        le32_to_cpu(buf[0]), POLICYDB_MAGIC);
> +               rc = -EINVAL;
>                 goto bad;
>         }
>
> -       rc = -EINVAL;
>         len = le32_to_cpu(buf[1]);
>         if (len != strlen(POLICYDB_STRING)) {
>                 printk(KERN_ERR "SELinux:  policydb string length %d does not "
> @@ -2265,11 +2265,13 @@ int policydb_read(struct policydb *p, void *fp)
>                        len, strlen(POLICYDB_STRING));
> +               rc = -EINVAL;
>                 goto bad;
>         }
>
> -       rc = -ENOMEM;
>         policydb_str = kmalloc(len + 1, GFP_KERNEL);
> -       if (!policydb_str)
> +       if (!policydb_str) {
> +               rc = -ENOMEM;
>                 goto bad;
> +       }
>
>         rc = next_entry(policydb_str, fp, len);
>         if (rc) {
> @@ -2279,12 +2280,12 @@ int policydb_read(struct policydb *p, void *fp)
>                 goto bad;
>         }
>
> -       rc = -EINVAL;
>         policydb_str[len] = '\0';
>         if (strcmp(policydb_str, POLICYDB_STRING)) {
>                 printk(KERN_ERR "SELinux:  policydb string %s does not match "
>                        "my string %s\n", policydb_str, POLICYDB_STRING);
>                 kfree(policydb_str);
> +               rc = -EINVAL;
>                 goto bad;
>         }
>         /* Done with policydb_str. */
> @@ -2296,24 +2297,24 @@ int policydb_read(struct policydb *p, void *fp)
>         if (rc)
>                 goto bad;
>
> -       rc = -EINVAL;
>         p->policyvers = le32_to_cpu(buf[0]);
>         if (p->policyvers < POLICYDB_VERSION_MIN ||
>             p->policyvers > POLICYDB_VERSION_MAX) {
>                 printk(KERN_ERR "SELinux:  policydb version %d does not match "
>                        "my version range %d-%d\n",
>                        le32_to_cpu(buf[0]), POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX);
> +               rc = -EINVAL;
>                 goto bad;
>         }
>
>         if ((le32_to_cpu(buf[1]) & POLICYDB_CONFIG_MLS)) {
>                 p->mls_enabled = 1;
>
> -               rc = -EINVAL;
>                 if (p->policyvers < POLICYDB_VERSION_MLS) {
>                         printk(KERN_ERR "SELinux: security policydb version %d "
>                                 "(MLS) not backwards compatible\n",
>                                 p->policyvers);
> +                       rc = -EINVAL;
>                         goto bad;
>                 }
>         }
> @@ -2332,21 +2333,21 @@ int policydb_read(struct policydb *p, void *fp)
>                         goto bad;
>         }
>
> -       rc = -EINVAL;
>         info = policydb_lookup_compat(p->policyvers);
>         if (!info) {
>                 printk(KERN_ERR "SELinux:  unable to find policy compat info "
>                        "for version %d\n", p->policyvers);
> +               rc = -EINVAL;
>                 goto bad;
>         }
>
> -       rc = -EINVAL;
>         if (le32_to_cpu(buf[2]) != info->sym_num ||
>                 le32_to_cpu(buf[3]) != info->ocon_num) {
>                 printk(KERN_ERR "SELinux:  policydb table sizes (%d,%d) do "
>                        "not match mine (%d,%d)\n", le32_to_cpu(buf[2]),
>                         le32_to_cpu(buf[3]),
>                        info->sym_num, info->ocon_num);
> +               rc = -EINVAL;
>                 goto bad;
>         }
>
> @@ -2365,10 +2366,11 @@ int policydb_read(struct policydb *p, void *fp)
>                 p->symtab[i].nprim = nprim;
>         }
>
> -       rc = -EINVAL;
>         p->process_class = string_to_security_class(p, "process");
> -       if (!p->process_class)
> +       if (!p->process_class) {
> +               rc = -EINVAL;
>                 goto bad;
> +       }
>
>         rc = avtab_read(&p->te_avtab, fp, p);
>         if (rc)
> @@ -2386,10 +2388,12 @@ int policydb_read(struct policydb *p, void *fp)
>         nel = le32_to_cpu(buf[0]);
>         ltr = NULL;
>         for (i = 0; i < nel; i++) {
> -               rc = -ENOMEM;
>                 tr = kzalloc(sizeof(*tr), GFP_KERNEL);
> -               if (!tr)
> +               if (!tr) {
> +                       rc = -ENOMEM;
>                         goto bad;
> +               }
> +
>                 if (ltr)
>                         ltr->next = tr;
>                 else
> @@ -2398,7 +2402,6 @@ int policydb_read(struct policydb *p, void *fp)
>                 if (rc)
>                         goto bad;
>
> -               rc = -EINVAL;
>                 tr->role = le32_to_cpu(buf[0]);
>                 tr->type = le32_to_cpu(buf[1]);
>                 tr->new_role = le32_to_cpu(buf[2]);
> @@ -2410,12 +2413,14 @@ int policydb_read(struct policydb *p, void *fp)
>                 } else
>                         tr->tclass = p->process_class;
>
> -               rc = -EINVAL;
>                 if (!policydb_role_isvalid(p, tr->role) ||
>                     !policydb_type_isvalid(p, tr->type) ||
>                     !policydb_class_isvalid(p, tr->tclass) ||
> -                   !policydb_role_isvalid(p, tr->new_role))
> +                   !policydb_role_isvalid(p, tr->new_role)) {
> +                       rc = -EINVAL;
>                         goto bad;
> +               }
> +
>                 ltr = tr;
>         }
>
> @@ -2425,10 +2430,12 @@ int policydb_read(struct policydb *p, void *fp)
>         nel = le32_to_cpu(buf[0]);
>         lra = NULL;
>         for (i = 0; i < nel; i++) {
> -               rc = -ENOMEM;
>                 ra = kzalloc(sizeof(*ra), GFP_KERNEL);
> -               if (!ra)
> +               if (!ra) {
> +                       rc = -ENOMEM;
>                         goto bad;
> +               }
> +
>                 if (lra)
>                         lra->next = ra;
>                 else
> @@ -2437,12 +2444,14 @@ int policydb_read(struct policydb *p, void *fp)
>                 if (rc)
>                         goto bad;
>
> -               rc = -EINVAL;
>                 ra->role = le32_to_cpu(buf[0]);
>                 ra->new_role = le32_to_cpu(buf[1]);
>                 if (!policydb_role_isvalid(p, ra->role) ||
> -                   !policydb_role_isvalid(p, ra->new_role))
> +                   !policydb_role_isvalid(p, ra->new_role)) {
> +                       rc = -EINVAL;
>                         goto bad;
> +               }
> +
>                 lra = ra;
>         }
>
> @@ -2454,11 +2463,12 @@ int policydb_read(struct policydb *p, void *fp)
>         if (rc)
>                 goto bad;
>
> -       rc = -EINVAL;
>         p->process_trans_perms = string_to_av_perm(p, p->process_class, "transition");
>         p->process_trans_perms |= string_to_av_perm(p, p->process_class, "dyntransition");
> -       if (!p->process_trans_perms)
> +       if (!p->process_trans_perms) {
> +               rc = -EINVAL;
>                 goto bad;
> +       }
>
>         rc = ocontext_read(p, info, fp);
>         if (rc)
> @@ -2472,12 +2482,13 @@ int policydb_read(struct policydb *p, void *fp)
>         if (rc)
>                 goto bad;
>
> -       rc = -ENOMEM;
>         p->type_attr_map_array = flex_array_alloc(sizeof(struct ebitmap),
>                                                   p->p_types.nprim,
>                                                   GFP_KERNEL | __GFP_ZERO);
> -       if (!p->type_attr_map_array)
> +       if (!p->type_attr_map_array) {
> +               rc = -ENOMEM;
>                 goto bad;
> +       }
>
>         /* preallocate so we don't have to worry about the put ever failing */
>         rc = flex_array_prealloc(p->type_attr_map_array, 0, p->p_types.nprim,
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Move some assignments for the variable "rc" in policydb_read()

[SF Markus Elfring]

>> One local variable was set to an error code in some cases before
>> a concrete error situation was detected. Thus move the corresponding
>> assignments into if branches to indicate a software failure there.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++-----------------
>>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> More code churn with no real advantage.

There are different opinions about the mentioned implementation details.


> I agree with the style you are using,

Thanks for such feedback.


> and would support changing it if you are in the function fixing bugs
> or doing other substantial changes in that code,

Is this expectation a contradiction for a desired patch granularity?


> but I can't justify it as a standalone change, sorry.

This update suggestion seems to be not attractive enough for you at the moment
as another change step of my patch series.
Would you like to check if there are other effects worthwhile besides the proposed
coding style adjustment here?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Move some assignments for the variable "rc" in policydb_read()

[Paul Moore]

On Fri, Mar 24, 2017 at 6:09 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>>> One local variable was set to an error code in some cases before
>>> a concrete error situation was detected. Thus move the corresponding
>>> assignments into if branches to indicate a software failure there.
>>>
>>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>>> ---
>>>  security/selinux/ss/policydb.c | 59 +++++++++++++++++++++++++-----------------
>>>  1 file changed, 35 insertions(+), 24 deletions(-)
>>
>> More code churn with no real advantage.
>
> There are different opinions about the mentioned implementation details.
>
>> I agree with the style you are using,
>
> Thanks for such feedback.
>
>> and would support changing it if you are in the function fixing bugs
>> or doing other substantial changes in that code,
>
> Is this expectation a contradiction for a desired patch granularity?
>
>> but I can't justify it as a standalone change, sorry.
>
> This update suggestion seems to be not attractive enough for you at the moment
> as another change step of my patch series.
> Would you like to check if there are other effects worthwhile besides the proposed
> coding style adjustment here?

To be honest, I would just leave it alone for now.  If you want to
contribute, focus on meaningful improvements such as bug fixing and/or
new features; changing only the code style isn't very interesting or
appealing, even if I happen to agree with your changes.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/46] selinux: Return directly after a failed next_entry() in genfs_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:11 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 16:34:25 +0100
>
> Return directly after a call of the function "next_entry" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 506b0228d1f1..754f829d2027 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2004,7 +2004,7 @@ static int genfs_read(struct policydb *p, void *fp)
>
>         rc = next_entry(buf, fp, sizeof(u32));
>         if (rc)
> -               goto out;
> +               return rc;
>         nel = le32_to_cpu(buf[0]);
>
>         for (i = 0; i < nel; i++) {
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 12/46] selinux: Move assignments for two pointers in genfs_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:12 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 16:56:51 +0100
>
> Move the assignment for the local variables "newc" and "newgenfs" behind
> a call of the function "next_entry" at the beginning so that they will
> only be set after a successful call.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I think it is cleaner, and arguably safer, the way it is now.  I'm not
worried about performance gains in the error path, especially when
those gains are probably not measurable.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 754f829d2027..7544e374dec9 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1997,14 +1997,14 @@ static int genfs_read(struct policydb *p, void *fp)
>         int i, j, rc;
>         u32 nel, nel2, len, len2;
>         __le32 buf[1];
> -       struct ocontext *l, *c;
> -       struct ocontext *newc = NULL;
> -       struct genfs *genfs_p, *genfs;
> -       struct genfs *newgenfs = NULL;
> +       struct ocontext *l, *c, *newc;
> +       struct genfs *genfs_p, *genfs, *newgenfs;
>
>         rc = next_entry(buf, fp, sizeof(u32));
>         if (rc)
>                 return rc;
> +       newc = NULL;
> +       newgenfs = NULL;
>         nel = le32_to_cpu(buf[0]);
>
>         for (i = 0; i < nel; i++) {
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 13/46] selinux: Move four assignments for the variable "rc" in genfs_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:13 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 17:21:59 +0100
>
> One local variable was set to an error code in four cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

See my previous comments.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 7544e374dec9..a12d9166f0e4 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2012,11 +2012,11 @@ static int genfs_read(struct policydb *p, void *fp)
>                 if (rc)
>                         goto out;
>                 len = le32_to_cpu(buf[0]);
> -
> -               rc = -ENOMEM;
>                 newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL);
> -               if (!newgenfs)
> +               if (!newgenfs) {
> +                       rc = -ENOMEM;
>                         goto out;
> +               }
>
>                 rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
>                 if (rc)
> @@ -2024,10 +2024,10 @@ static int genfs_read(struct policydb *p, void *fp)
>
>                 for (genfs_p = NULL, genfs = p->genfs; genfs;
>                      genfs_p = genfs, genfs = genfs->next) {
> -                       rc = -EINVAL;
>                         if (strcmp(newgenfs->fstype, genfs->fstype) == 0) {
>                                 printk(KERN_ERR "SELinux:  dup genfs fstype %s\n",
>                                        newgenfs->fstype);
> +                               rc = -EINVAL;
>                                 goto out;
>                         }
>                         if (strcmp(newgenfs->fstype, genfs->fstype) < 0)
> @@ -2051,11 +2051,11 @@ static int genfs_read(struct policydb *p, void *fp)
>                         if (rc)
>                                 goto out;
>                         len = le32_to_cpu(buf[0]);
> -
> -                       rc = -ENOMEM;
>                         newc = kzalloc(sizeof(*newc), GFP_KERNEL);
> -                       if (!newc)
> +                       if (!newc) {
> +                               rc = -ENOMEM;
>                                 goto out;
> +                       }
>
>                         rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
>                         if (rc)
> @@ -2072,12 +2072,12 @@ static int genfs_read(struct policydb *p, void *fp)
>
>                         for (l = NULL, c = genfs->head; c;
>                              l = c, c = c->next) {
> -                               rc = -EINVAL;
>                                 if (!strcmp(newc->u.name, c->u.name) &&
>                                     (!c->v.sclass || !newc->v.sclass ||
>                                      newc->v.sclass == c->v.sclass)) {
>                                         printk(KERN_ERR "SELinux:  dup genfs entry (%s,%s)\n",
>                                                genfs->fstype, c->u.name);
> +                                       rc = -EINVAL;
>                                         goto out;
>                                 }
>                                 len = strlen(newc->u.name);
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 14/46] selinux: One function call less in genfs_read() after null pointer detection

[Paul Moore]

On Sun, Jan 15, 2017 at 10:14 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 17:43:47 +0100
>
> Call the function "kfree" at the end only after it was determined
> that the local variable "newgenfs" contained a non-null pointer.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index a12d9166f0e4..5dc31faa601f 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2096,9 +2096,10 @@ static int genfs_read(struct policydb *p, void *fp)
>         }
>         rc = 0;
>  out:
> -       if (newgenfs)
> +       if (newgenfs) {
>                 kfree(newgenfs->fstype);
> -       kfree(newgenfs);
> +               kfree(newgenfs);
> +       }
>         ocontext_destroy(newc, OCON_FSUSE);
>
>         return rc;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 15/46] selinux: One check and function call less in genfs_read() after error detection

[Paul Moore]

On Tue, Jan 17, 2017 at 12:53 PM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 1/17/2017 8:37 AM, SF Markus Elfring wrote:
>>>> @@ -2015,7 +2015,7 @@ static int genfs_read(struct policydb *p, void *fp)
>>>>             newgenfs = kzalloc(sizeof(*newgenfs), GFP_KERNEL);
>>>>             if (!newgenfs) {
>>>>                     rc = -ENOMEM;
>>>> -                   goto out;
>>>> +                   goto exit;
>>>>             }
>>>>
>>>>             rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
>>>> @@ -2101,7 +2101,7 @@ static int genfs_read(struct policydb *p, void *fp)
>>>>             kfree(newgenfs);
>>>>     }
>>>>     ocontext_destroy(newc, OCON_FSUSE);
>>>> -
>>>> +exit:
>>>>     return rc;
>>> Why not replace the "goto out" with "return rc" rather
>>> than add a target?
>> Would you accept to use the statement "return -ENOMEM;" there instead?
>
> That would be even better.

I *hate* code that does a jump to a label only to then do a
return/exit.  That said, see my earlier comments about not worrying
too much about performance of the error path and in this case I like
the "feel good" nature of the code where ever failure in the loop goes
to "out".

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 16/46] selinux: Move two assignments for the variable "rc" in filename_trans_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:16 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 18:50:52 +0100
>
> One local variable was set to an error code in two cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

See previous comments.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index e7b882251da8..106a1da1d68a 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1930,16 +1930,17 @@ static int filename_trans_read(struct policydb *p, void *fp)
>                 ft = NULL;
>                 otype = NULL;
>                 name = NULL;
> -
> -               rc = -ENOMEM;
>                 ft = kzalloc(sizeof(*ft), GFP_KERNEL);
> -               if (!ft)
> +               if (!ft) {
> +                       rc = -ENOMEM;
>                         goto out;
> +               }
>
> -               rc = -ENOMEM;
>                 otype = kmalloc(sizeof(*otype), GFP_KERNEL);
> -               if (!otype)
> +               if (!otype) {
> +                       rc = -ENOMEM;
>                         goto out;
> +               }
>
>                 /* length of the path component string */
>                 rc = next_entry(buf, fp, sizeof(u32));
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 17/46] selinux: Delete an unnecessary variable assignment in filename_trans_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:17 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 19:02:42 +0100
>
> The local variable "ft" was set to a null pointer despite of an
> immediate reassignment.
> Thus remove this statement from the beginning of a loop.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 1 -
>  1 file changed, 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 106a1da1d68a..2be5b18eb149 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1927,7 +1927,6 @@ static int filename_trans_read(struct policydb *p, void *fp)
>         nel = le32_to_cpu(buf[0]);
>
>         for (i = 0; i < nel; i++) {
> -               ft = NULL;
>                 otype = NULL;
>                 name = NULL;
>                 ft = kzalloc(sizeof(*ft), GFP_KERNEL);
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 18/46] selinux: One function call less in filename_trans_read() after error detection

[Paul Moore]

On Sun, Jan 15, 2017 at 10:18 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 19:19:42 +0100
>
> Adjust a jump target to avoid a function call at the end after a memory
> allocation failed for the local variable "ft".
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

It doesn't seem like this is worth adding another jump label.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 2be5b18eb149..5f122e846332 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1932,7 +1932,7 @@ static int filename_trans_read(struct policydb *p, void *fp)
>                 ft = kzalloc(sizeof(*ft), GFP_KERNEL);
>                 if (!ft) {
>                         rc = -ENOMEM;
> -                       goto out;
> +                       goto free_name;
>                 }
>
>                 otype = kmalloc(sizeof(*otype), GFP_KERNEL);
> @@ -1986,6 +1986,7 @@ static int filename_trans_read(struct policydb *p, void *fp)
>         return 0;
>  out:
>         kfree(ft);
> +free_name:
>         kfree(name);
>         kfree(otype);
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 19/46] selinux: Return directly after a failed next_entry() in range_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:19 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 19:35:59 +0100
>
> Return directly after a call of the function "next_entry" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 5f122e846332..a696876fc327 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1850,7 +1850,7 @@ static int range_read(struct policydb *p, void *fp)
>
>         rc = next_entry(buf, fp, sizeof(u32));
>         if (rc)
> -               goto out;
> +               return rc;
>
>         nel = le32_to_cpu(buf[0]);
>         for (i = 0; i < nel; i++) {
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 20/46] selinux: Move four assignments for the variable "rc" in range_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:20 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 19:55:00 +0100
>
> One local variable was set to an error code in four cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)

See previous comments.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index a696876fc327..4cd96ce51322 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1854,10 +1854,11 @@ static int range_read(struct policydb *p, void *fp)
>
>         nel = le32_to_cpu(buf[0]);
>         for (i = 0; i < nel; i++) {
> -               rc = -ENOMEM;
>                 rt = kzalloc(sizeof(*rt), GFP_KERNEL);
> -               if (!rt)
> +               if (!rt) {
> +                       rc = -ENOMEM;
>                         goto out;
> +               }
>
>                 rc = next_entry(buf, fp, (sizeof(u32) * 2));
>                 if (rc)
> @@ -1873,24 +1874,26 @@ static int range_read(struct policydb *p, void *fp)
>                 } else
>                         rt->target_class = p->process_class;
>
> -               rc = -EINVAL;
>                 if (!policydb_type_isvalid(p, rt->source_type) ||
>                     !policydb_type_isvalid(p, rt->target_type) ||
> -                   !policydb_class_isvalid(p, rt->target_class))
> +                   !policydb_class_isvalid(p, rt->target_class)) {
> +                       rc = -EINVAL;
>                         goto out;
> +               }
>
> -               rc = -ENOMEM;
>                 r = kzalloc(sizeof(*r), GFP_KERNEL);
> -               if (!r)
> +               if (!r) {
> +                       rc = -ENOMEM;
>                         goto out;
> +               }
>
>                 rc = mls_read_range_helper(r, fp);
>                 if (rc)
>                         goto out;
>
> -               rc = -EINVAL;
>                 if (!mls_range_isvalid(p, r)) {
>                         printk(KERN_WARNING "SELinux:  rangetrans:  invalid range\n");
> +                       rc = -EINVAL;
>                         goto out;
>                 }
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 21/46] selinux: Two function calls less in range_read() after error detection

[Paul Moore]

On Tue, Jan 17, 2017 at 11:35 AM, Casey Schaufler
<casey@schaufler-ca.com> wrote:
> On 1/15/2017 7:21 AM, SF Markus Elfring wrote:
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sat, 14 Jan 2017 20:20:15 +0100
>>
>> Adjust a jump target to avoid two calls of the function "kfree" at the end
>> after a memory allocation failed for the local variable "rt".
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>>  security/selinux/ss/policydb.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
>> index 4cd96ce51322..0d2f64558c0a 100644
>> --- a/security/selinux/ss/policydb.c
>> +++ b/security/selinux/ss/policydb.c
>> @@ -1857,7 +1857,7 @@ static int range_read(struct policydb *p, void *fp)
>>               rt = kzalloc(sizeof(*rt), GFP_KERNEL);
>>               if (!rt) {
>>                       rc = -ENOMEM;
>> -                     goto out;
>> +                     goto exit;
>
> Why not "return rc;"?
> goto to a return is wrong.

Agree with Casey, but also see my previous comments about the
convenience of using a single error handling goto for loops like this.

>>               }
>>
>>               rc = next_entry(buf, fp, (sizeof(u32) * 2));
>> @@ -1909,6 +1909,7 @@ static int range_read(struct policydb *p, void *fp)
>>  out:
>>       kfree(rt);
>>       kfree(r);
>> +exit:
>>       return rc;
>>  }
>>
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 22/46] selinux: Delete an unnecessary variable initialisation in range_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:22 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 20:40:12 +0100
>
> The local variable "rt" will be set to an appropriate pointer a bit later.
> Thus omit the explicit initialisation at the beginning which became
> unnecessary with a previous update step.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 0d2f64558c0a..6121a26ada64 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1839,7 +1839,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
>
>  static int range_read(struct policydb *p, void *fp)
>  {
> -       struct range_trans *rt = NULL;
> +       struct range_trans *rt;
>         struct mls_range *r = NULL;
>         int i, rc;
>         __le32 buf[2];
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 23/46] selinux: Move an assignment for a pointer in range_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:23 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 21:00:45 +0100
>
> Move the assignment for the local variable "r" behind a call of the
> function "next_entry" at the beginning so that it will only be set
> after a successful call.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

See previous comments.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 6121a26ada64..5101592ae172 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1840,7 +1840,7 @@ u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name)
>  static int range_read(struct policydb *p, void *fp)
>  {
>         struct range_trans *rt;
> -       struct mls_range *r = NULL;
> +       struct mls_range *r;
>         int i, rc;
>         __le32 buf[2];
>         u32 nel;
> @@ -1852,6 +1852,7 @@ static int range_read(struct policydb *p, void *fp)
>         if (rc)
>                 return rc;
>
> +       r = NULL;
>         nel = le32_to_cpu(buf[0]);
>         for (i = 0; i < nel; i++) {
>                 rt = kzalloc(sizeof(*rt), GFP_KERNEL);
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 00/46] SELinux: Fine-tuning for several function implementations

[Paul Moore]

On Sun, Jan 15, 2017 at 9:55 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 15:15:14 +0100
>
> Several update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (46): ....

Hi Markus,

Thank you for your patience with this patchset, I realize it has taken
me quite a long time to get around to merging them.  I just wanted to
send a quick summary: I've made it through the first half (23/46) and
of those I've merged patches 1, 2, 3, 4, 5, 6, 7, 11, 14, 17, 19, and
22.  Unfortunately, this is all the time I have for today, but I'll
resume working on the remaining patches soon.

Thanks for the patches and your understanding.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SELinux: Fine-tuning for several function implementations

[SF Markus Elfring]

> I've made it through the first half (23/46) and of those
> I've merged patches ?, 7, ?

It seems that you dropped this one while integrating the eighth
update step.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

SELinux: Fine-tuning for several function implementations

[Paul Moore]

On Mon, Mar 27, 2017 at 1:48 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> I've made it through the first half (23/46) and of those
>> I've merged patches ?, 7, ?
>
> It seems that you dropped this one while integrating the eighth
> update step.

Yes, my mistake, as I said in my response to that patch, I do not plan
on merging 07/46.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 24/46] selinux: Return directly after a failed kzalloc() in cat_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:24 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 21:20:43 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 5101592ae172..eb898dcbe502 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1635,10 +1635,9 @@ static int cat_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[3];
>         u32 len;
>
> -       rc = -ENOMEM;
>         catdatum = kzalloc(sizeof(*catdatum), GFP_ATOMIC);
>         if (!catdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         rc = next_entry(buf, fp, sizeof buf);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 25/46] selinux: Return directly after a failed kzalloc() in sens_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:25 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 21:42:02 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index eb898dcbe502..5caa1fa5ea80 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1593,10 +1593,9 @@ static int sens_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[2];
>         u32 len;
>
> -       rc = -ENOMEM;
>         levdatum = kzalloc(sizeof(*levdatum), GFP_ATOMIC);
>         if (!levdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         rc = next_entry(buf, fp, sizeof buf);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 26/46] selinux: Improve another size determination in sens_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:26 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 21:52:55 +0100
>
> Replace the specification of a data type by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 5caa1fa5ea80..edfcfd3bbc60 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1609,7 +1609,7 @@ static int sens_read(struct policydb *p, struct hashtab *h, void *fp)
>                 goto bad;
>
>         rc = -ENOMEM;
> -       levdatum->level = kmalloc(sizeof(struct mls_level), GFP_ATOMIC);
> +       levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_ATOMIC);
>         if (!levdatum->level)
>                 goto bad;
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 27/46] selinux: Move an assignment for the variable "rc" in sens_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:27 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 22:02:14 +0100
>
> A local variable was set to an error code in one case before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Dropped for reasons previously discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index edfcfd3bbc60..3e43556e67b8 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1608,10 +1608,11 @@ static int sens_read(struct policydb *p, struct hashtab *h, void *fp)
>         if (rc)
>                 goto bad;
>
> -       rc = -ENOMEM;
>         levdatum->level = kmalloc(sizeof(*levdatum->level), GFP_ATOMIC);
> -       if (!levdatum->level)
> +       if (!levdatum->level) {
> +               rc = -ENOMEM;
>                 goto bad;
> +       }
>
>         rc = mls_read_level(levdatum->level, fp);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 28/46] selinux: Return directly after a failed kzalloc() in user_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:28 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 22:08:22 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 3e43556e67b8..1c046d39e2a7 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1542,10 +1542,9 @@ static int user_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[3];
>         u32 len;
>
> -       rc = -ENOMEM;
>         usrdatum = kzalloc(sizeof(*usrdatum), GFP_KERNEL);
>         if (!usrdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>                 to_read = 3;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 29/46] selinux: Return directly after a failed kzalloc() in type_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:29 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 22:15:54 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 1c046d39e2a7..662139365449 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1467,10 +1467,9 @@ static int type_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[4];
>         u32 len;
>
> -       rc = -ENOMEM;
>         typdatum = kzalloc(sizeof(*typdatum), GFP_KERNEL);
>         if (!typdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>                 to_read = 4;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 30/46] selinux: Return directly after a failed kzalloc() in role_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:30 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 22:20:25 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 662139365449..34b670227c4d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1410,10 +1410,9 @@ static int role_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[3];
>         u32 len;
>
> -       rc = -ENOMEM;
>         role = kzalloc(sizeof(*role), GFP_KERNEL);
>         if (!role)
> -               goto bad;
> +               return -ENOMEM;
>
>         if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
>                 to_read = 3;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 32/46] selinux: Return directly after a failed kzalloc() in class_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:32 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 14 Jan 2017 22:30:51 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 49fc5d8990e9..3af2b0849495 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1316,10 +1316,9 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp)
>         u32 len, len2, ncons, nel;
>         int i, rc;
>
> -       rc = -ENOMEM;
>         cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
>         if (!cladatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         rc = next_entry(buf, fp, sizeof(u32)*6);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 33/46] selinux: Move an assignment for the variable "rc" in class_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:33 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:10:39 +0100
>
> A local variable was set to an error code in one case before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Dropped for reasons already discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 3af2b0849495..9035e5329ceb 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1345,10 +1345,10 @@ static int class_read(struct policydb *p, struct hashtab *h, void *fp)
>                 if (rc)
>                         goto bad;
>
> -               rc = -EINVAL;
>                 cladatum->comdatum = hashtab_search(p->p_commons.table, cladatum->comkey);
>                 if (!cladatum->comdatum) {
>                         printk(KERN_ERR "SELinux:  unknown common %s\n", cladatum->comkey);
> +                       rc = -EINVAL;
>                         goto bad;
>                 }
>         }
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 34/46] selinux: Return directly after a failed kzalloc() in common_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:34 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:15:19 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 9035e5329ceb..551685283399 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1150,10 +1150,9 @@ static int common_read(struct policydb *p, struct hashtab *h, void *fp)
>         u32 len, nel;
>         int i, rc;
>
> -       rc = -ENOMEM;
>         comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
>         if (!comdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         rc = next_entry(buf, fp, sizeof buf);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 35/46] selinux: Return directly after a failed kzalloc() in perm_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:35 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:20:13 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 551685283399..9b595f2e0d9f 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -1116,10 +1116,9 @@ static int perm_read(struct policydb *p, struct hashtab *h, void *fp)
>         __le32 buf[2];
>         u32 len;
>
> -       rc = -ENOMEM;
>         perdatum = kzalloc(sizeof(*perdatum), GFP_KERNEL);
>         if (!perdatum)
> -               goto bad;
> +               return -ENOMEM;
>
>         rc = next_entry(buf, fp, sizeof buf);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 36/46] selinux: Move an assignment for the variable "rc" in mls_read_range_helper()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:36 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:22:23 +0100
>
> A local variable was set to an error code in one case before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Dropped as previously discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 9b595f2e0d9f..7cf635c650dc 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -996,10 +996,10 @@ static int mls_read_range_helper(struct mls_range *r, void *fp)
>         if (rc)
>                 goto out;
>
> -       rc = -EINVAL;
>         items = le32_to_cpu(buf[0]);
>         if (items > ARRAY_SIZE(buf)) {
>                 printk(KERN_ERR "SELinux: mls:  range overflow\n");
> +               rc = -EINVAL;
>                 goto out;
>         }
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 37/46] selinux: Move an assignment for the variable "rc" in policydb_load_isids()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:37 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:24:51 +0100
>
> A local variable was set to an error code in one case before a concrete
> error situation was detected. Thus move the corresponding assignment into
> an if branch to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Dropped as previously discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 7cf635c650dc..faa6ecc2450d 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -897,10 +897,10 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
>
>         head = p->ocontexts[OCON_ISID];
>         for (c = head; c; c = c->next) {
> -               rc = -EINVAL;
>                 if (!c->context[0].user) {
>                         printk(KERN_ERR "SELinux:  SID %s was never defined.\n",
>                                 c->u.name);
> +                       rc = -EINVAL;
>                         goto out;
>                 }
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 38/46] selinux: One function call less in five functions after null pointer detection

[Paul Moore]

On Sun, Jan 15, 2017 at 10:38 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:28:02 +0100
>
> Call the function "kfree" at the end only after it was determined
> that the passed parameter contained a non-null pointer.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index faa6ecc2450d..88730b372277 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -620,8 +620,8 @@ static int common_destroy(void *key, void *datum, void *p)
>                 comdatum = datum;
>                 hashtab_map(comdatum->permissions.table, perm_destroy, NULL);
>                 hashtab_destroy(comdatum->permissions.table);
> +               kfree(datum);
>         }
> -       kfree(datum);
>         return 0;
>  }

I'm dropping this patch because it really doesn't do much other than
add some code churn.  I suppose you can argue it removes one
conditional if datum is NULL, but that is such a corner case I'm not
worried about it.

If I were to merge this, I would probably expect "kfree(datum)" to be
converted to "kfree(comdatum)" (and similar in the cases below).

> @@ -675,8 +675,8 @@ static int cls_destroy(void *key, void *datum, void *p)
>                         kfree(ctemp);
>                 }
>                 kfree(cladatum->comkey);
> +               kfree(datum);
>         }
> -       kfree(datum);
>         return 0;
>  }
>
> @@ -689,8 +689,8 @@ static int role_destroy(void *key, void *datum, void *p)
>                 role = datum;
>                 ebitmap_destroy(&role->dominates);
>                 ebitmap_destroy(&role->types);
> +               kfree(datum);
>         }
> -       kfree(datum);
>         return 0;
>  }
>
> @@ -712,8 +712,8 @@ static int user_destroy(void *key, void *datum, void *p)
>                 ebitmap_destroy(&usrdatum->range.level[0].cat);
>                 ebitmap_destroy(&usrdatum->range.level[1].cat);
>                 ebitmap_destroy(&usrdatum->dfltlevel.cat);
> +               kfree(datum);
>         }
> -       kfree(datum);
>         return 0;
>  }
>
> @@ -726,8 +726,8 @@ static int sens_destroy(void *key, void *datum, void *p)
>                 levdatum = datum;
>                 ebitmap_destroy(&levdatum->level->cat);
>                 kfree(levdatum->level);
> +               kfree(datum);
>         }
> -       kfree(datum);
>         return 0;
>  }
>
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 39/46] selinux: Move two assignments for the variable "rc" in ocontext_read()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:39 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 11:30:12 +0100
>
> One local variable was set to an error code in two cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

Dropped as previously discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 88730b372277..8b9ed3f1b132 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -2121,10 +2121,11 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
>
>                 l = NULL;
>                 for (j = 0; j < nel; j++) {
> -                       rc = -ENOMEM;
>                         c = kzalloc(sizeof(*c), GFP_KERNEL);
> -                       if (!c)
> +                       if (!c) {
> +                               rc = -ENOMEM;
>                                 goto out;
> +                       }
>                         if (l)
>                                 l->next = c;
>                         else
> @@ -2186,13 +2187,13 @@ static int ocontext_read(struct policydb *p, struct policydb_compat_info *info,
>                                 if (rc)
>                                         goto out;
>
> -                               rc = -EINVAL;
>                                 c->v.behavior = le32_to_cpu(buf[0]);
>                                 /* Determined at runtime, not in policy DB. */
> -                               if (c->v.behavior == SECURITY_FS_USE_MNTPOINT)
> -                                       goto out;
> -                               if (c->v.behavior > SECURITY_FS_USE_MAX)
> +                               if (c->v.behavior == SECURITY_FS_USE_MNTPOINT ||
> +                                   c->v.behavior > SECURITY_FS_USE_MAX) {
> +                                       rc = -EINVAL;
>                                         goto out;
> +                               }
>
>                                 len = le32_to_cpu(buf[1]);
>                                 rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 40/46] selinux: Return directly after a failed kzalloc() in roles_init()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:40 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 12:10:09 +0100
>
> Return directly after a call of the function "kzalloc" failed
> at the beginning.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 8b9ed3f1b132..ccc146bfd4c2 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -178,10 +178,9 @@ static int roles_init(struct policydb *p)
>         int rc;
>         struct role_datum *role;
>
> -       rc = -ENOMEM;
>         role = kzalloc(sizeof(*role), GFP_KERNEL);
>         if (!role)
> -               goto out;
> +               return -ENOMEM;
>
>         rc = -EINVAL;
>         role->value = ++p->p_roles.nprim;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 41/46] selinux: Move two assignments for the variable "rc" in roles_init()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:41 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 12:40:35 +0100
>
> One local variable was set to an error code in two cases before
> a concrete error situation was detected. Thus move the corresponding
> assignments into if branches to indicate a software failure there.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Dropped for reasons previously discussed.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index ccc146bfd4c2..be445abf047b 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -182,15 +182,17 @@ static int roles_init(struct policydb *p)
>         if (!role)
>                 return -ENOMEM;
>
> -       rc = -EINVAL;
>         role->value = ++p->p_roles.nprim;
> -       if (role->value != OBJECT_R_VAL)
> +       if (role->value != OBJECT_R_VAL) {
> +               rc = -EINVAL;
>                 goto out;
> +       }
>
> -       rc = -ENOMEM;
>         key = kstrdup(OBJECT_R, GFP_KERNEL);
> -       if (!key)
> +       if (!key) {
> +               rc = -ENOMEM;
>                 goto out;
> +       }
>
>         rc = hashtab_insert(p->p_roles.table, key, role);
>         if (rc)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 42/46] selinux: One function call less in roles_init() after error detection

[Paul Moore]

On Sun, Jan 15, 2017 at 10:42 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 12:56:54 +0100
>
> The kfree() function was called in up to two cases by the
> roles_init() function during error handling even if the passed variable
> contained a null pointer.
>
> * Adjust a jump target according to the Linux coding style convention.
>
> * Delete an initialisation for the variable "key" at the beginning
>   which became unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Dropped due to unnecessary code churn with no significant advantage.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index be445abf047b..7dd5c6f7786f 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -174,7 +174,7 @@ static struct policydb_compat_info *policydb_lookup_compat(int version)
>   */
>  static int roles_init(struct policydb *p)
>  {
> -       char *key = NULL;
> +       char *key;
>         int rc;
>         struct role_datum *role;
>
> @@ -185,13 +185,13 @@ static int roles_init(struct policydb *p)
>         role->value = ++p->p_roles.nprim;
>         if (role->value != OBJECT_R_VAL) {
>                 rc = -EINVAL;
> -               goto out;
> +               goto free_role;
>         }
>
>         key = kstrdup(OBJECT_R, GFP_KERNEL);
>         if (!key) {
>                 rc = -ENOMEM;
> -               goto out;
> +               goto free_role;
>         }
>
>         rc = hashtab_insert(p->p_roles.table, key, role);
> @@ -201,6 +201,7 @@ static int roles_init(struct policydb *p)
>         return 0;
>  out:
>         kfree(key);
> +free_role:
>         kfree(role);
>         return rc;
>  }
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 43/46] selinux: Use kmalloc_array() in sidtab_init()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:43 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 13:13:19 +0100
>
> A multiplication for the size determination of a memory allocation
> indicated that an array data structure should be processed.
> Thus use the corresponding function "kmalloc_array".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Merged, thanks.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index 5840a35155fc..c9533b21942b 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -18,7 +18,7 @@ int sidtab_init(struct sidtab *s)
>  {
>         int i;
>
> -       s->htable = kmalloc(sizeof(*(s->htable)) * SIDTAB_SIZE, GFP_ATOMIC);
> +       s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC);
>         if (!s->htable)
>                 return -ENOMEM;
>         for (i = 0; i < SIDTAB_SIZE; i++)
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 44/46] selinux: Adjust two checks for null pointers

[Paul Moore]

On Sun, Jan 15, 2017 at 10:44 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 13:30:20 +0100
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> The script "checkpatch.pl" pointed information out like the following.
>
> Comparison to NULL could be written !?
>
> Thus fix affected source code places.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Merged.

Definitely code churn, but arguably there is some minor value in
making checkpatch.pl happy.  In the future changes like this should be
bundled with more significant changes to make them more appealing.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index c9533b21942b..f6915f257486 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -54,7 +54,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>         }
>
>         newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
> -       if (newnode == NULL) {
> +       if (!newnode) {
>                 rc = -ENOMEM;
>                 goto out;
>         }
> @@ -98,7 +98,7 @@ static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
>         if (force && cur && sid == cur->sid && cur->context.len)
>                 return &cur->context;
>
> -       if (cur == NULL || sid != cur->sid || cur->context.len) {
> +       if (!cur || sid != cur->sid || cur->context.len) {
>                 /* Remap invalid SIDs to the unlabeled SID. */
>                 sid = SECINITSID_UNLABELED;
>                 hvalue = SIDTAB_HASH(sid);
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 46/46] selinuxfs: Use seq_puts() in sel_avc_stats_seq_show()

[Paul Moore]

On Sun, Jan 15, 2017 at 10:46 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 Jan 2017 14:04:53 +0100
>
> A string which did not contain data format specifications should be put
> into a sequence. Thus use the corresponding function "seq_puts".
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/selinuxfs.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Merged, thanks.

> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 55345f84f17d..0d81ebf7ff23 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1445,10 +1445,10 @@ static int sel_avc_stats_seq_show(struct seq_file *seq, void *v)
>  {
>         struct avc_cache_stats *st = v;
>
> -       if (v == SEQ_START_TOKEN)
> -               seq_printf(seq, "lookups hits misses allocations reclaims "
> -                          "frees\n");
> -       else {
> +       if (v == SEQ_START_TOKEN) {
> +               seq_puts(seq,
> +                        "lookups hits misses allocations reclaims frees\n");
> +       } else {
>                 unsigned int lookups = st->lookups;
>                 unsigned int misses = st->misses;
>                 unsigned int hits = lookups - misses;
> --
> 2.11.0
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] SELinux: Fine-tuning for two function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 4 Apr 2017 13:02:03 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Return directly after a failed memory allocation in policydb_index()
  Return an error code only as a constant in sidtab_insert()
  Use an other error code for an input validation failure in sidtab_insert()

 security/selinux/ss/policydb.c | 15 +++++----------
 security/selinux/ss/sidtab.c   | 27 ++++++++++-----------------
 2 files changed, 15 insertions(+), 27 deletions(-)

-- 
2.12.2


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

[PATCH 1/3] selinux: Return directly after a failed memory allocation in policydb_index()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 4 Apr 2017 10:20:46 +0200

Replace five goto statements (and previous variable assignments) by
direct returns after a memory allocation failure in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/policydb.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 0080122760ad..87d645d3a39f 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -538,34 +538,30 @@ static int policydb_index(struct policydb *p)
 	symtab_hash_eval(p->symtab);
 #endif
 
-	rc = -ENOMEM;
 	p->class_val_to_struct = kcalloc(p->p_classes.nprim,
 					 sizeof(*p->class_val_to_struct),
 					 GFP_KERNEL);
 	if (!p->class_val_to_struct)
-		goto out;
+		return -ENOMEM;
 
-	rc = -ENOMEM;
 	p->role_val_to_struct = kcalloc(p->p_roles.nprim,
 					sizeof(*p->role_val_to_struct),
 					GFP_KERNEL);
 	if (!p->role_val_to_struct)
-		goto out;
+		return -ENOMEM;
 
-	rc = -ENOMEM;
 	p->user_val_to_struct = kcalloc(p->p_users.nprim,
 					sizeof(*p->user_val_to_struct),
 					GFP_KERNEL);
 	if (!p->user_val_to_struct)
-		goto out;
+		return -ENOMEM;
 
 	/* Yes, I want the sizeof the pointer, not the structure */
-	rc = -ENOMEM;
 	p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
 						       p->p_types.nprim,
 						       GFP_KERNEL | __GFP_ZERO);
 	if (!p->type_val_to_struct_array)
-		goto out;
+		return -ENOMEM;
 
 	rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
 				 p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
@@ -577,12 +573,11 @@ static int policydb_index(struct policydb *p)
 		goto out;
 
 	for (i = 0; i < SYM_NUM; i++) {
-		rc = -ENOMEM;
 		p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
 							 p->symtab[i].nprim,
 							 GFP_KERNEL | __GFP_ZERO);
 		if (!p->sym_val_to_name[i])
-			goto out;
+			return -ENOMEM;
 
 		rc = flex_array_prealloc(p->sym_val_to_name[i],
 					 0, p->symtab[i].nprim,
-- 
2.12.2

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

[PATCH 1/3] selinux: Return directly after a failed memory allocation in policydb_index()

[Paul Moore]

On Tue, Apr 4, 2017 at 7:12 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 4 Apr 2017 10:20:46 +0200
>
> Replace five goto statements (and previous variable assignments) by
> direct returns after a memory allocation failure in this function.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/policydb.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)

Merged.

> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 0080122760ad..87d645d3a39f 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -538,34 +538,30 @@ static int policydb_index(struct policydb *p)
>         symtab_hash_eval(p->symtab);
>  #endif
>
> -       rc = -ENOMEM;
>         p->class_val_to_struct = kcalloc(p->p_classes.nprim,
>                                          sizeof(*p->class_val_to_struct),
>                                          GFP_KERNEL);
>         if (!p->class_val_to_struct)
> -               goto out;
> +               return -ENOMEM;
>
> -       rc = -ENOMEM;
>         p->role_val_to_struct = kcalloc(p->p_roles.nprim,
>                                         sizeof(*p->role_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->role_val_to_struct)
> -               goto out;
> +               return -ENOMEM;
>
> -       rc = -ENOMEM;
>         p->user_val_to_struct = kcalloc(p->p_users.nprim,
>                                         sizeof(*p->user_val_to_struct),
>                                         GFP_KERNEL);
>         if (!p->user_val_to_struct)
> -               goto out;
> +               return -ENOMEM;
>
>         /* Yes, I want the sizeof the pointer, not the structure */
> -       rc = -ENOMEM;
>         p->type_val_to_struct_array = flex_array_alloc(sizeof(struct type_datum *),
>                                                        p->p_types.nprim,
>                                                        GFP_KERNEL | __GFP_ZERO);
>         if (!p->type_val_to_struct_array)
> -               goto out;
> +               return -ENOMEM;
>
>         rc = flex_array_prealloc(p->type_val_to_struct_array, 0,
>                                  p->p_types.nprim, GFP_KERNEL | __GFP_ZERO);
> @@ -577,12 +573,11 @@ static int policydb_index(struct policydb *p)
>                 goto out;
>
>         for (i = 0; i < SYM_NUM; i++) {
> -               rc = -ENOMEM;
>                 p->sym_val_to_name[i] = flex_array_alloc(sizeof(char *),
>                                                          p->symtab[i].nprim,
>                                                          GFP_KERNEL | __GFP_ZERO);
>                 if (!p->sym_val_to_name[i])
> -                       goto out;
> +                       return -ENOMEM;
>
>                 rc = flex_array_prealloc(p->sym_val_to_name[i],
>                                          0, p->symtab[i].nprim,
> --
> 2.12.2
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] selinux: Return an error code only as a constant in sidtab_insert()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 4 Apr 2017 11:33:53 +0200

* Return an error code without storing it in an intermediate variable.

* Delete the local variable "rc" and the jump label "out" which became
  unnecessary with this refactoring.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/sidtab.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index f6915f257486..c5f436b15d19 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -32,13 +32,11 @@ int sidtab_init(struct sidtab *s)
 
 int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 {
-	int hvalue, rc = 0;
+	int hvalue;
 	struct sidtab_node *prev, *cur, *newnode;
 
-	if (!s) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!s)
+		return -ENOMEM;
 
 	hvalue = SIDTAB_HASH(sid);
 	prev = NULL;
@@ -48,21 +46,17 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 		cur = cur->next;
 	}
 
-	if (cur && sid == cur->sid) {
-		rc = -EEXIST;
-		goto out;
-	}
+	if (cur && sid == cur->sid)
+		return -EEXIST;
 
 	newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
-	if (!newnode) {
-		rc = -ENOMEM;
-		goto out;
-	}
+	if (!newnode)
+		return -ENOMEM;
+
 	newnode->sid = sid;
 	if (context_cpy(&newnode->context, context)) {
 		kfree(newnode);
-		rc = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	if (prev) {
@@ -78,8 +72,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	s->nel++;
 	if (sid >= s->next_sid)
 		s->next_sid = sid + 1;
-out:
-	return rc;
+	return 0;
 }
 
 static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
-- 
2.12.2

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

[PATCH 2/3] selinux: Return an error code only as a constant in sidtab_insert()

[Paul Moore]

On Tue, Apr 4, 2017 at 7:14 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 4 Apr 2017 11:33:53 +0200
>
> * Return an error code without storing it in an intermediate variable.
>
> * Delete the local variable "rc" and the jump label "out" which became
>   unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)

Merged.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index f6915f257486..c5f436b15d19 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -32,13 +32,11 @@ int sidtab_init(struct sidtab *s)
>
>  int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>  {
> -       int hvalue, rc = 0;
> +       int hvalue;
>         struct sidtab_node *prev, *cur, *newnode;
>
> -       if (!s) {
> -               rc = -ENOMEM;
> -               goto out;
> -       }
> +       if (!s)
> +               return -ENOMEM;
>
>         hvalue = SIDTAB_HASH(sid);
>         prev = NULL;
> @@ -48,21 +46,17 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>                 cur = cur->next;
>         }
>
> -       if (cur && sid == cur->sid) {
> -               rc = -EEXIST;
> -               goto out;
> -       }
> +       if (cur && sid == cur->sid)
> +               return -EEXIST;
>
>         newnode = kmalloc(sizeof(*newnode), GFP_ATOMIC);
> -       if (!newnode) {
> -               rc = -ENOMEM;
> -               goto out;
> -       }
> +       if (!newnode)
> +               return -ENOMEM;
> +
>         newnode->sid = sid;
>         if (context_cpy(&newnode->context, context)) {
>                 kfree(newnode);
> -               rc = -ENOMEM;
> -               goto out;
> +               return -ENOMEM;
>         }
>
>         if (prev) {
> @@ -78,8 +72,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>         s->nel++;
>         if (sid >= s->next_sid)
>                 s->next_sid = sid + 1;
> -out:
> -       return rc;
> +       return 0;
>  }
>
>  static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force)
> --
> 2.12.2
>



-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] selinux: Use an other error code for an input validation failure in sidtab_insert()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Tue, 4 Apr 2017 12:23:41 +0200

The error code "-ENOMEM" was also returned so far when the parameter "s"
of this function contained a null pointer.
Now I find that the code "-EINVAL" is more appropriate in this case.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 security/selinux/ss/sidtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index c5f436b15d19..2eb2a54b88d2 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -36,7 +36,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
 	struct sidtab_node *prev, *cur, *newnode;
 
 	if (!s)
-		return -ENOMEM;
+		return -EINVAL;
 
 	hvalue = SIDTAB_HASH(sid);
 	prev = NULL;
-- 
2.12.2

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

[PATCH 3/3] selinux: Use an other error code for an input validation failure in sidtab_insert()

[Paul Moore]

On Tue, Apr 4, 2017 at 7:16 AM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Tue, 4 Apr 2017 12:23:41 +0200
>
> The error code "-ENOMEM" was also returned so far when the parameter "s"
> of this function contained a null pointer.
> Now I find that the code "-EINVAL" is more appropriate in this case.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  security/selinux/ss/sidtab.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Have you tested this to determine any impact it may have on the
SELinux userspace?  I would agree that EINVAL is probably more
appropriate in this case, but changing this return code has very
little value and may disrupt userspace if it assumes EINVAL means
something else when the policy load fails.  Without a demonstration
that all the code paths have been tested I'm not inclined to merge
this patch.

> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index c5f436b15d19..2eb2a54b88d2 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -36,7 +36,7 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context)
>         struct sidtab_node *prev, *cur, *newnode;
>
>         if (!s)
> -               return -ENOMEM;
> +               return -EINVAL;
>
>         hvalue = SIDTAB_HASH(sid);
>         prev = NULL;
> --
> 2.12.2

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

selinux: Use an other error code for an input validation failure in sidtab_insert()

[SF Markus Elfring]

> Have you tested this to determine any impact it may have on the
> SELinux userspace?

Not yet.


> I would agree that EINVAL is probably more appropriate in this case,

Thanks that a part of your view seems to fit also to mine.


> but changing this return code has very little value

I would appreciate if this aspect can clarified a bit more.


> and may disrupt userspace if it assumes EINVAL means something else
> when the policy load fails.

Would you find an other error code better there?

Do you care to distinguish an input validation failure in a specific
function implementation from other error situations?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html