[PATCH 0/5] selinux: minor cleanup suggestions

[PATCH 0/5] selinux: minor cleanup suggestions

[Rasmus Villemoes]

A few random things I stumbled on.

While I'm pretty sure of the change in 1/5, I'm also confused, because
the doc for the reverse security_sid_to_context state that
@scontext_len is set to "the length of the string", which one would
normally interpret as being what strlen() would give (i.e., without
the \0). However, security_sid_to_context_core clearly includes the \0
in the return value, and I think callers rely on that.

Rasmus Villemoes (5):
  selinux: introduce security_context_str_to_sid
  selinux: remove pointless cast in selinux_inode_setsecurity()
  selinux: use kmemdup in security_sid_to_context_core()
  selinux: use kstrdup() in security_get_bools()
  selinux: use sprintf return value

 security/selinux/hooks.c            | 14 +++++---------
 security/selinux/include/security.h |  2 ++
 security/selinux/selinuxfs.c        | 26 +++++++++-----------------
 security/selinux/ss/services.c      | 22 +++++++++-------------
 4 files changed, 25 insertions(+), 39 deletions(-)

-- 
2.1.3

[PATCH 1/5] selinux: introduce security_context_str_to_sid

[Rasmus Villemoes]

There seems to be a little confusion as to whether the scontext_len
parameter of security_context_to_sid() includes the nul-byte or
not. Reading security_context_to_sid_core(), it seems that the
expectation is that it does not (both the string copying and the test
for scontext_len being zero hint at that).

Introduce the helper security_context_str_to_sid() to do the strlen()
call and fix all callers.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 security/selinux/hooks.c            | 12 ++++--------
 security/selinux/include/security.h |  2 ++
 security/selinux/selinuxfs.c        | 26 +++++++++-----------------
 security/selinux/ss/services.c      |  5 +++++
 4 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e4369d86e588..fd50cd5ac2ec 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -674,10 +674,9 @@ static int selinux_set_mnt_opts(struct super_block *sb,
 
 		if (flags[i] == SBLABEL_MNT)
 			continue;
-		rc = security_context_to_sid(mount_options[i],
-					     strlen(mount_options[i]), &sid, GFP_KERNEL);
+		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
 		if (rc) {
-			printk(KERN_WARNING "SELinux: security_context_to_sid"
+			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
 			       "(%s) failed for (dev %s, type %s) errno=%d\n",
 			       mount_options[i], sb->s_id, name, rc);
 			goto out;
@@ -2617,15 +2616,12 @@ static int selinux_sb_remount(struct super_block *sb, void *data)
 
 	for (i = 0; i < opts.num_mnt_opts; i++) {
 		u32 sid;
-		size_t len;
 
 		if (flags[i] == SBLABEL_MNT)
 			continue;
-		len = strlen(mount_options[i]);
-		rc = security_context_to_sid(mount_options[i], len, &sid,
-					     GFP_KERNEL);
+		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
 		if (rc) {
-			printk(KERN_WARNING "SELinux: security_context_to_sid"
+			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
 			       "(%s) failed for (dev %s, type %s) errno=%d\n",
 			       mount_options[i], sb->s_id, sb->s_type->name, rc);
 			goto out_free_opts;
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 6a681d26bf20..223e9fd15d66 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -166,6 +166,8 @@ int security_sid_to_context_force(u32 sid, char **scontext, u32 *scontext_len);
 int security_context_to_sid(const char *scontext, u32 scontext_len,
 			    u32 *out_sid, gfp_t gfp);
 
+int security_context_str_to_sid(const char *scontext, u32 *out_sid, gfp_t gfp);
+
 int security_context_to_sid_default(const char *scontext, u32 scontext_len,
 				    u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
 
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 5bed7716f8ab..c02da25d7b63 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -731,13 +731,11 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -819,13 +817,11 @@ static ssize_t sel_write_create(struct file *file, char *buf, size_t size)
 		objname = namebuf;
 	}
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -882,13 +878,11 @@ static ssize_t sel_write_relabel(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -940,7 +934,7 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s", con, user) != 2)
 		goto out;
 
-	length = security_context_to_sid(con, strlen(con) + 1, &sid, GFP_KERNEL);
+	length = security_context_str_to_sid(con, &sid, GFP_KERNEL);
 	if (length)
 		goto out;
 
@@ -1000,13 +994,11 @@ static ssize_t sel_write_member(struct file *file, char *buf, size_t size)
 	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
 		goto out;
 
-	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
 	if (length)
 		goto out;
 
-	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
-					 GFP_KERNEL);
+	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
 	if (length)
 		goto out;
 
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index b7df12ba61d8..c550df0e0ff1 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1476,6 +1476,11 @@ int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid,
 					    sid, SECSID_NULL, gfp, 0);
 }
 
+int security_context_str_to_sid(const char *scontext, u32 *sid, gfp_t gfp)
+{
+	return security_context_to_sid(scontext, strlen(scontext), sid, gfp);
+}
+
 /**
  * security_context_to_sid_default - Obtain a SID for a given security context,
  * falling back to specified default if needed.
-- 
2.1.3

[PATCH 2/5] selinux: remove pointless cast in selinux_inode_setsecurity()

[Rasmus Villemoes]

security_context_to_sid() expects a const char* argument, so there's
no point in casting away the const qualifier of value.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 security/selinux/hooks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fd50cd5ac2ec..5edb57df86f8 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3162,7 +3162,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
 	if (!value || !size)
 		return -EACCES;
 
-	rc = security_context_to_sid((void *)value, size, &newsid, GFP_KERNEL);
+	rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
 	if (rc)
 		return rc;
 
-- 
2.1.3

[PATCH 3/5] selinux: use kmemdup in security_sid_to_context_core()

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 security/selinux/ss/services.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index c550df0e0ff1..994c824a34c6 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1259,12 +1259,12 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
 			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
 			if (!scontext)
 				goto out;
-			scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
+			scontextp = kmemdup(initial_sid_to_string[sid],
+					    *scontext_len, GFP_ATOMIC);
 			if (!scontextp) {
 				rc = -ENOMEM;
 				goto out;
 			}
-			strcpy(scontextp, initial_sid_to_string[sid]);
 			*scontext = scontextp;
 			goto out;
 		}
-- 
2.1.3

[PATCH 4/5] selinux: use kstrdup() in security_get_bools()

[Rasmus Villemoes]

This is much simpler.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 security/selinux/ss/services.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 994c824a34c6..aa2bdcb20848 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2609,18 +2609,12 @@ int security_get_bools(int *len, char ***names, int **values)
 		goto err;
 
 	for (i = 0; i < *len; i++) {
-		size_t name_len;
-
 		(*values)[i] = policydb.bool_val_to_struct[i]->state;
-		name_len = strlen(sym_name(&policydb, SYM_BOOLS, i)) + 1;
 
 		rc = -ENOMEM;
-		(*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC);
+		(*names)[i] = kstrdup(sym_name(&policydb, SYM_BOOLS, i), GFP_ATOMIC);
 		if (!(*names)[i])
 			goto err;
-
-		strncpy((*names)[i], sym_name(&policydb, SYM_BOOLS, i), name_len);
-		(*names)[i][name_len - 1] = 0;
 	}
 	rc = 0;
 out:
-- 
2.1.3

[PATCH 5/5] selinux: use sprintf return value

[Rasmus Villemoes]

sprintf returns the number of characters printed (excluding '\0'), so
we can use that and avoid duplicating the length computation.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 security/selinux/ss/services.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index aa2bdcb20848..ebb5eb3c318c 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1218,13 +1218,10 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
 	/*
 	 * Copy the user name, role name and type name into the context.
 	 */
-	sprintf(scontextp, "%s:%s:%s",
+	scontextp += sprintf(scontextp, "%s:%s:%s",
 		sym_name(&policydb, SYM_USERS, context->user - 1),
 		sym_name(&policydb, SYM_ROLES, context->role - 1),
 		sym_name(&policydb, SYM_TYPES, context->type - 1));
-	scontextp += strlen(sym_name(&policydb, SYM_USERS, context->user - 1)) +
-		     1 + strlen(sym_name(&policydb, SYM_ROLES, context->role - 1)) +
-		     1 + strlen(sym_name(&policydb, SYM_TYPES, context->type - 1));
 
 	mls_sid_to_context(context, &scontextp);
 
-- 
2.1.3

Re: [PATCH 0/5] selinux: minor cleanup suggestions

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> A few random things I stumbled on.
>
> While I'm pretty sure of the change in 1/5, I'm also confused, because
> the doc for the reverse security_sid_to_context state that
> @scontext_len is set to "the length of the string", which one would
> normally interpret as being what strlen() would give (i.e., without
> the \0). However, security_sid_to_context_core clearly includes the \0
> in the return value, and I think callers rely on that.

It is historical; originally security_context_to_sid() required 
@scontext to be NUL-terminated and @scontext_len to include the NUL byte 
in the length, and security_sid_to_context() returned a NUL-terminated 
@scontext and included the NUL byte in the returned length.  However, 
when we switched SELinux to using xattrs rather than its own persistent 
label mapping, security_context_to_sid() was changed to accept contexts 
that did not already include the NUL because setfattr did not consider 
the NUL to be part of the attribute value for strings.  So presently it 
accepts either form, although we prefer them to be NUL-terminated and 
canonicalize them to that form before returning to userspace.

>
> Rasmus Villemoes (5):
>    selinux: introduce security_context_str_to_sid
>    selinux: remove pointless cast in selinux_inode_setsecurity()
>    selinux: use kmemdup in security_sid_to_context_core()
>    selinux: use kstrdup() in security_get_bools()
>    selinux: use sprintf return value
>
>   security/selinux/hooks.c            | 14 +++++---------
>   security/selinux/include/security.h |  2 ++
>   security/selinux/selinuxfs.c        | 26 +++++++++-----------------
>   security/selinux/ss/services.c      | 22 +++++++++-------------
>   4 files changed, 25 insertions(+), 39 deletions(-)
>

Re: [PATCH 1/5] selinux: introduce security_context_str_to_sid

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> There seems to be a little confusion as to whether the scontext_len
> parameter of security_context_to_sid() includes the nul-byte or
> not. Reading security_context_to_sid_core(), it seems that the
> expectation is that it does not (both the string copying and the test
> for scontext_len being zero hint at that).
>
> Introduce the helper security_context_str_to_sid() to do the strlen()
> call and fix all callers.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c            | 12 ++++--------
>   security/selinux/include/security.h |  2 ++
>   security/selinux/selinuxfs.c        | 26 +++++++++-----------------
>   security/selinux/ss/services.c      |  5 +++++
>   4 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d86e588..fd50cd5ac2ec 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -674,10 +674,9 @@ static int selinux_set_mnt_opts(struct super_block *sb,
>
>   		if (flags[i] == SBLABEL_MNT)
>   			continue;
> -		rc = security_context_to_sid(mount_options[i],
> -					     strlen(mount_options[i]), &sid, GFP_KERNEL);
> +		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
>   		if (rc) {
> -			printk(KERN_WARNING "SELinux: security_context_to_sid"
> +			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
>   			       "(%s) failed for (dev %s, type %s) errno=%d\n",
>   			       mount_options[i], sb->s_id, name, rc);
>   			goto out;
> @@ -2617,15 +2616,12 @@ static int selinux_sb_remount(struct super_block *sb, void *data)
>
>   	for (i = 0; i < opts.num_mnt_opts; i++) {
>   		u32 sid;
> -		size_t len;
>
>   		if (flags[i] == SBLABEL_MNT)
>   			continue;
> -		len = strlen(mount_options[i]);
> -		rc = security_context_to_sid(mount_options[i], len, &sid,
> -					     GFP_KERNEL);
> +		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
>   		if (rc) {
> -			printk(KERN_WARNING "SELinux: security_context_to_sid"
> +			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
>   			       "(%s) failed for (dev %s, type %s) errno=%d\n",
>   			       mount_options[i], sb->s_id, sb->s_type->name, rc);
>   			goto out_free_opts;
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index 6a681d26bf20..223e9fd15d66 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -166,6 +166,8 @@ int security_sid_to_context_force(u32 sid, char **scontext, u32 *scontext_len);
>   int security_context_to_sid(const char *scontext, u32 scontext_len,
>   			    u32 *out_sid, gfp_t gfp);
>
> +int security_context_str_to_sid(const char *scontext, u32 *out_sid, gfp_t gfp);
> +
>   int security_context_to_sid_default(const char *scontext, u32 scontext_len,
>   				    u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 5bed7716f8ab..c02da25d7b63 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -731,13 +731,11 @@ static ssize_t sel_write_access(struct file *file, char *buf, size_t size)
>   	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
>   		goto out;
>
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> @@ -819,13 +817,11 @@ static ssize_t sel_write_create(struct file *file, char *buf, size_t size)
>   		objname = namebuf;
>   	}
>
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> @@ -882,13 +878,11 @@ static ssize_t sel_write_relabel(struct file *file, char *buf, size_t size)
>   	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
>   		goto out;
>
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> @@ -940,7 +934,7 @@ static ssize_t sel_write_user(struct file *file, char *buf, size_t size)
>   	if (sscanf(buf, "%s %s", con, user) != 2)
>   		goto out;
>
> -	length = security_context_to_sid(con, strlen(con) + 1, &sid, GFP_KERNEL);
> +	length = security_context_str_to_sid(con, &sid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> @@ -1000,13 +994,11 @@ static ssize_t sel_write_member(struct file *file, char *buf, size_t size)
>   	if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass) != 3)
>   		goto out;
>
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>   	if (length)
>   		goto out;
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b7df12ba61d8..c550df0e0ff1 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1476,6 +1476,11 @@ int security_context_to_sid(const char *scontext, u32 scontext_len, u32 *sid,
>   					    sid, SECSID_NULL, gfp, 0);
>   }
>
> +int security_context_str_to_sid(const char *scontext, u32 *sid, gfp_t gfp)
> +{
> +	return security_context_to_sid(scontext, strlen(scontext), sid, gfp);
> +}
> +
>   /**
>    * security_context_to_sid_default - Obtain a SID for a given security context,
>    * falling back to specified default if needed.
>

Re: [PATCH 2/5] selinux: remove pointless cast in selinux_inode_setsecurity()

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> security_context_to_sid() expects a const char* argument, so there's
> no point in casting away the const qualifier of value.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/hooks.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fd50cd5ac2ec..5edb57df86f8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3162,7 +3162,7 @@ static int selinux_inode_setsecurity(struct inode *inode, const char *name,
>   	if (!value || !size)
>   		return -EACCES;
>
> -	rc = security_context_to_sid((void *)value, size, &newsid, GFP_KERNEL);
> +	rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
>   	if (rc)
>   		return rc;
>
>

Re: [PATCH 3/5] selinux: use kmemdup in security_sid_to_context_core()

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/ss/services.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index c550df0e0ff1..994c824a34c6 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1259,12 +1259,12 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
>   			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
>   			if (!scontext)
>   				goto out;
> -			scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
> +			scontextp = kmemdup(initial_sid_to_string[sid],
> +					    *scontext_len, GFP_ATOMIC);
>   			if (!scontextp) {
>   				rc = -ENOMEM;
>   				goto out;
>   			}
> -			strcpy(scontextp, initial_sid_to_string[sid]);
>   			*scontext = scontextp;
>   			goto out;
>   		}
>

Re: [PATCH 4/5] selinux: use kstrdup() in security_get_bools()

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> This is much simpler.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/ss/services.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 994c824a34c6..aa2bdcb20848 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2609,18 +2609,12 @@ int security_get_bools(int *len, char ***names, int **values)
>   		goto err;
>
>   	for (i = 0; i < *len; i++) {
> -		size_t name_len;
> -
>   		(*values)[i] = policydb.bool_val_to_struct[i]->state;
> -		name_len = strlen(sym_name(&policydb, SYM_BOOLS, i)) + 1;
>
>   		rc = -ENOMEM;
> -		(*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC);
> +		(*names)[i] = kstrdup(sym_name(&policydb, SYM_BOOLS, i), GFP_ATOMIC);
>   		if (!(*names)[i])
>   			goto err;
> -
> -		strncpy((*names)[i], sym_name(&policydb, SYM_BOOLS, i), name_len);
> -		(*names)[i][name_len - 1] = 0;
>   	}
>   	rc = 0;
>   out:
>

Re: [PATCH 5/5] selinux: use sprintf return value

[Stephen Smalley]

On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
> sprintf returns the number of characters printed (excluding '\0'), so
> we can use that and avoid duplicating the length computation.
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/ss/services.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index aa2bdcb20848..ebb5eb3c318c 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1218,13 +1218,10 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
>   	/*
>   	 * Copy the user name, role name and type name into the context.
>   	 */
> -	sprintf(scontextp, "%s:%s:%s",
> +	scontextp += sprintf(scontextp, "%s:%s:%s",
>   		sym_name(&policydb, SYM_USERS, context->user - 1),
>   		sym_name(&policydb, SYM_ROLES, context->role - 1),
>   		sym_name(&policydb, SYM_TYPES, context->type - 1));
> -	scontextp += strlen(sym_name(&policydb, SYM_USERS, context->user - 1)) +
> -		     1 + strlen(sym_name(&policydb, SYM_ROLES, context->role - 1)) +
> -		     1 + strlen(sym_name(&policydb, SYM_TYPES, context->type - 1));
>
>   	mls_sid_to_context(context, &scontextp);
>
>

Re: [PATCH 5/5] selinux: use sprintf return value

[Rasmus Villemoes]

On Tue, Sep 29 2015, Stephen Smalley <sds@tycho.nsa.gov> wrote:

> On 09/25/2015 06:34 PM, Rasmus Villemoes wrote:
>> sprintf returns the number of characters printed (excluding '\0'), so
>> we can use that and avoid duplicating the length computation.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>
> Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

Thanks. I also like Joe's add-on suggestion (except I'd declare user,
role, type as const char*).

Rasmus

Re: [PATCH 1/5] selinux: introduce security_context_str_to_sid

[Paul Moore]

On Saturday, September 26, 2015 12:34:15 AM Rasmus Villemoes wrote:
> There seems to be a little confusion as to whether the scontext_len
> parameter of security_context_to_sid() includes the nul-byte or
> not. Reading security_context_to_sid_core(), it seems that the
> expectation is that it does not (both the string copying and the test
> for scontext_len being zero hint at that).
> 
> Introduce the helper security_context_str_to_sid() to do the strlen()
> call and fix all callers.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  security/selinux/hooks.c            | 12 ++++--------
>  security/selinux/include/security.h |  2 ++
>  security/selinux/selinuxfs.c        | 26 +++++++++-----------------
>  security/selinux/ss/services.c      |  5 +++++
>  4 files changed, 20 insertions(+), 25 deletions(-)

Applied.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e4369d86e588..fd50cd5ac2ec 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -674,10 +674,9 @@ static int selinux_set_mnt_opts(struct super_block *sb,
> 
>  		if (flags[i] == SBLABEL_MNT)
>  			continue;
> -		rc = security_context_to_sid(mount_options[i],
> -					     strlen(mount_options[i]), &sid, GFP_KERNEL);
> +		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
>  		if (rc) {
> -			printk(KERN_WARNING "SELinux: security_context_to_sid"
> +			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
>  			       "(%s) failed for (dev %s, type %s) errno=%d\n",
>  			       mount_options[i], sb->s_id, name, rc);
>  			goto out;
> @@ -2617,15 +2616,12 @@ static int selinux_sb_remount(struct super_block
> *sb, void *data)
> 
>  	for (i = 0; i < opts.num_mnt_opts; i++) {
>  		u32 sid;
> -		size_t len;
> 
>  		if (flags[i] == SBLABEL_MNT)
>  			continue;
> -		len = strlen(mount_options[i]);
> -		rc = security_context_to_sid(mount_options[i], len, &sid,
> -					     GFP_KERNEL);
> +		rc = security_context_str_to_sid(mount_options[i], &sid, GFP_KERNEL);
>  		if (rc) {
> -			printk(KERN_WARNING "SELinux: security_context_to_sid"
> +			printk(KERN_WARNING "SELinux: security_context_str_to_sid"
>  			       "(%s) failed for (dev %s, type %s) errno=%d\n",
>  			       mount_options[i], sb->s_id, sb->s_type->name, rc);
>  			goto out_free_opts;
> diff --git a/security/selinux/include/security.h
> b/security/selinux/include/security.h index 6a681d26bf20..223e9fd15d66
> 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -166,6 +166,8 @@ int security_sid_to_context_force(u32 sid, char
> **scontext, u32 *scontext_len); int security_context_to_sid(const char
> *scontext, u32 scontext_len, u32 *out_sid, gfp_t gfp);
> 
> +int security_context_str_to_sid(const char *scontext, u32 *out_sid, gfp_t
> gfp); +
>  int security_context_to_sid_default(const char *scontext, u32 scontext_len,
> u32 *out_sid, u32 def_sid, gfp_t gfp_flags);
> 
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 5bed7716f8ab..c02da25d7b63 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -731,13 +731,11 @@ static ssize_t sel_write_access(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -819,13 +817,11 @@ static ssize_t sel_write_create(struct file *file,
> char *buf, size_t size) objname = namebuf;
>  	}
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -882,13 +878,11 @@ static ssize_t sel_write_relabel(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -940,7 +934,7 @@ static ssize_t sel_write_user(struct file *file, char
> *buf, size_t size) if (sscanf(buf, "%s %s", con, user) != 2)
>  		goto out;
> 
> -	length = security_context_to_sid(con, strlen(con) + 1, &sid, GFP_KERNEL);
> +	length = security_context_str_to_sid(con, &sid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> @@ -1000,13 +994,11 @@ static ssize_t sel_write_member(struct file *file,
> char *buf, size_t size) if (sscanf(buf, "%s %s %hu", scon, tcon, &tclass)
> != 3)
>  		goto out;
> 
> -	length = security_context_to_sid(scon, strlen(scon) + 1, &ssid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(scon, &ssid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> -	length = security_context_to_sid(tcon, strlen(tcon) + 1, &tsid,
> -					 GFP_KERNEL);
> +	length = security_context_str_to_sid(tcon, &tsid, GFP_KERNEL);
>  	if (length)
>  		goto out;
> 
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index b7df12ba61d8..c550df0e0ff1 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1476,6 +1476,11 @@ int security_context_to_sid(const char *scontext, u32
> scontext_len, u32 *sid, sid, SECSID_NULL, gfp, 0);
>  }
> 
> +int security_context_str_to_sid(const char *scontext, u32 *sid, gfp_t gfp)
> +{
> +	return security_context_to_sid(scontext, strlen(scontext), sid, gfp);
> +}
> +
>  /**
>   * security_context_to_sid_default - Obtain a SID for a given security
> context, * falling back to specified default if needed.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/5] selinux: remove pointless cast in selinux_inode_setsecurity()

[Paul Moore]

On Saturday, September 26, 2015 12:34:16 AM Rasmus Villemoes wrote:
> security_context_to_sid() expects a const char* argument, so there's
> no point in casting away the const qualifier of value.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  security/selinux/hooks.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied.

> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index fd50cd5ac2ec..5edb57df86f8 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3162,7 +3162,7 @@ static int selinux_inode_setsecurity(struct inode
> *inode, const char *name, if (!value || !size)
>  		return -EACCES;
> 
> -	rc = security_context_to_sid((void *)value, size, &newsid, GFP_KERNEL);
> +	rc = security_context_to_sid(value, size, &newsid, GFP_KERNEL);
>  	if (rc)
>  		return rc;

-- 
paul moore
www.paul-moore.com

Re: [PATCH 3/5] selinux: use kmemdup in security_sid_to_context_core()

[Paul Moore]

On Saturday, September 26, 2015 12:34:17 AM Rasmus Villemoes wrote:
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  security/selinux/ss/services.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index c550df0e0ff1..994c824a34c6 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1259,12 +1259,12 @@ static int security_sid_to_context_core(u32 sid,
> char **scontext, *scontext_len = strlen(initial_sid_to_string[sid]) + 1;
>  			if (!scontext)
>  				goto out;
> -			scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
> +			scontextp = kmemdup(initial_sid_to_string[sid],
> +					    *scontext_len, GFP_ATOMIC);
>  			if (!scontextp) {
>  				rc = -ENOMEM;
>  				goto out;
>  			}
> -			strcpy(scontextp, initial_sid_to_string[sid]);
>  			*scontext = scontextp;
>  			goto out;
>  		}

-- 
paul moore
www.paul-moore.com

Re: [PATCH 4/5] selinux: use kstrdup() in security_get_bools()

[Paul Moore]

On Saturday, September 26, 2015 12:34:18 AM Rasmus Villemoes wrote:
> This is much simpler.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  security/selinux/ss/services.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

Applied.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 994c824a34c6..aa2bdcb20848 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2609,18 +2609,12 @@ int security_get_bools(int *len, char ***names, int
> **values) goto err;
> 
>  	for (i = 0; i < *len; i++) {
> -		size_t name_len;
> -
>  		(*values)[i] = policydb.bool_val_to_struct[i]->state;
> -		name_len = strlen(sym_name(&policydb, SYM_BOOLS, i)) + 1;
> 
>  		rc = -ENOMEM;
> -		(*names)[i] = kmalloc(sizeof(char) * name_len, GFP_ATOMIC);
> +		(*names)[i] = kstrdup(sym_name(&policydb, SYM_BOOLS, i), GFP_ATOMIC);
>  		if (!(*names)[i])
>  			goto err;
> -
> -		strncpy((*names)[i], sym_name(&policydb, SYM_BOOLS, i), name_len);
> -		(*names)[i][name_len - 1] = 0;
>  	}
>  	rc = 0;
>  out:

-- 
paul moore
www.paul-moore.com

Re: [PATCH 5/5] selinux: use sprintf return value

[Paul Moore]

On Saturday, September 26, 2015 12:34:19 AM Rasmus Villemoes wrote:
> sprintf returns the number of characters printed (excluding '\0'), so
> we can use that and avoid duplicating the length computation.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  security/selinux/ss/services.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Applied, thank you.

I just pushed all five of your patches to the SELinux next branch so they 
should be included in the next linux-next release.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index aa2bdcb20848..ebb5eb3c318c 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1218,13 +1218,10 @@ static int context_struct_to_string(struct context
> *context, char **scontext, u3 /*
>  	 * Copy the user name, role name and type name into the context.
>  	 */
> -	sprintf(scontextp, "%s:%s:%s",
> +	scontextp += sprintf(scontextp, "%s:%s:%s",
>  		sym_name(&policydb, SYM_USERS, context->user - 1),
>  		sym_name(&policydb, SYM_ROLES, context->role - 1),
>  		sym_name(&policydb, SYM_TYPES, context->type - 1));
> -	scontextp += strlen(sym_name(&policydb, SYM_USERS, context->user - 1)) +
> -		     1 + strlen(sym_name(&policydb, SYM_ROLES, context->role - 1)) +
> -		     1 + strlen(sym_name(&policydb, SYM_TYPES, context->type - 1));
> 
>  	mls_sid_to_context(context, &scontextp);

-- 
paul moore
www.paul-moore.com