[RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Paul Moore]

While looking at the SELinux secid/secctx conversion functions I realized they
could probably do with a little cleanup to reduce the amount of code and make
better use of existing string processing functions in the kernel.  Making use
of the kernel's existing string processing functions is a good idea as many
architectures have specialized/optimized routines which should be an
improvement over the generic code in the SELinux security server.
---

 security/selinux/ss/mls.c      |   61 +++++--------
 security/selinux/ss/mls.h      |    3 -
 security/selinux/ss/services.c |  194 ++++++++++++++++------------------------
 3 files changed, 103 insertions(+), 155 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index feaf0a5..ba211b4 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -239,8 +239,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
  * Policy read-lock must be held for sidtab lookup.
  *
  */
-int mls_context_to_sid(char oldc,
-		       char **scontext,
+int mls_context_to_sid(char **scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid)
@@ -250,10 +249,10 @@ int mls_context_to_sid(char oldc,
 	char *scontextp, *p, *rngptr;
 	struct level_datum *levdatum;
 	struct cat_datum *catdatum, *rngdatum;
-	int l, rc = -EINVAL;
+	int l, rc;
 
 	if (!selinux_mls_enabled) {
-		if (def_sid != SECSID_NULL && oldc)
+		if (def_sid != SECSID_NULL && *scontext && **scontext)
 			*scontext += strlen(*scontext)+1;
 		return 0;
 	}
@@ -262,18 +261,17 @@ int mls_context_to_sid(char oldc,
 	 * No MLS component to the security context, try and map to
 	 * default if provided.
 	 */
-	if (!oldc) {
+	if (!(*scontext) || !(**scontext)) {
 		struct context *defcon;
 
 		if (def_sid == SECSID_NULL)
-			goto out;
+			return -EINVAL;
 
 		defcon = sidtab_search(s, def_sid);
 		if (!defcon)
-			goto out;
+			return -EINVAL;
 
-		rc = mls_context_cpy(context, defcon);
-		goto out;
+		return mls_context_cpy(context, defcon);
 	}
 
 	/* Extract low sensitivity. */
@@ -287,10 +285,8 @@ int mls_context_to_sid(char oldc,
 
 	for (l = 0; l < 2; l++) {
 		levdatum = hashtab_search(policydb.p_levels.table, scontextp);
-		if (!levdatum) {
-			rc = -EINVAL;
-			goto out;
-		}
+		if (!levdatum)
+			return -EINVAL;
 
 		context->range.level[l].sens = levdatum->level->sens;
 
@@ -312,35 +308,29 @@ int mls_context_to_sid(char oldc,
 
 				catdatum = hashtab_search(policydb.p_cats.table,
 				                          scontextp);
-				if (!catdatum) {
-					rc = -EINVAL;
-					goto out;
-				}
+				if (!catdatum)
+					return -EINVAL;
 
 				rc = ebitmap_set_bit(&context->range.level[l].cat,
 				                     catdatum->value - 1, 1);
 				if (rc)
-					goto out;
+					return rc;
 
 				/* If range, set all categories in range */
 				if (rngptr) {
 					int i;
 
 					rngdatum = hashtab_search(policydb.p_cats.table, rngptr);
-					if (!rngdatum) {
-						rc = -EINVAL;
-						goto out;
-					}
+					if (!rngdatum)
+						return -EINVAL;
 
-					if (catdatum->value >= rngdatum->value) {
-						rc = -EINVAL;
-						goto out;
-					}
+					if (catdatum->value >= rngdatum->value)
+						return -EINVAL;
 
 					for (i = catdatum->value; i < rngdatum->value; i++) {
 						rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
 						if (rc)
-							goto out;
+							return rc;
 					}
 				}
 
@@ -366,12 +356,10 @@ int mls_context_to_sid(char oldc,
 		rc = ebitmap_cpy(&context->range.level[1].cat,
 				 &context->range.level[0].cat);
 		if (rc)
-			goto out;
+			return rc;
 	}
 	*scontext = ++p;
-	rc = 0;
-out:
-	return rc;
+	return 0;
 }
 
 /*
@@ -391,13 +379,10 @@ int mls_from_string(char *str, struct context *context, gfp_t gfp_mask)
 	/* we need freestr because mls_context_to_sid will change
 	   the value of tmpstr */
 	tmpstr = freestr = kstrdup(str, gfp_mask);
-	if (!tmpstr) {
-		rc = -ENOMEM;
-	} else {
-		rc = mls_context_to_sid(':', &tmpstr, context,
-		                        NULL, SECSID_NULL);
-		kfree(freestr);
-	}
+	if (!tmpstr)
+		return -ENOMEM;
+	rc = mls_context_to_sid(&tmpstr, context, NULL, SECSID_NULL);
+	kfree(freestr);
 
 	return rc;
 }
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index ab53663..b364f61 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -30,8 +30,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c);
 int mls_range_isvalid(struct policydb *p, struct mls_range *r);
 int mls_level_isvalid(struct policydb *p, struct mls_level *l);
 
-int mls_context_to_sid(char oldc,
-	               char **scontext,
+int mls_context_to_sid(char **scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index f374186..4aab8a5 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -573,47 +573,43 @@ out:
 	return rc;
 }
 
-/*
- * Write the security context string representation of
- * the context structure `context' into a dynamically
- * allocated string of the correct size.  Set `*scontext'
- * to point to this string and set `*scontext_len' to
- * the length of the string.
+/**
+ * context_struct_to_string - Generate a string representation of a context
+ * @context: native security context
+ * @scontext: security context string
+ * @scontext_len: length of security context string
+ *
+ * Description:
+ * Write the security context string representation of @context into
+ * @scontext.  The caller must free @scontext when finished.  Returns zero on
+ * success, negative values on failure.
+ *
  */
-static int context_struct_to_string(struct context *context, char **scontext, u32 *scontext_len)
+static int context_struct_to_string(struct context *context,
+				    char **scontext, u32 *scontext_len)
 {
-	char *scontextp;
+	char *ctx;
 
-	*scontext = NULL;
-	*scontext_len = 0;
-
-	/* Compute the size of the context. */
-	*scontext_len += strlen(policydb.p_user_val_to_name[context->user - 1]) + 1;
-	*scontext_len += strlen(policydb.p_role_val_to_name[context->role - 1]) + 1;
-	*scontext_len += strlen(policydb.p_type_val_to_name[context->type - 1]) + 1;
-	*scontext_len += mls_compute_context_len(context);
+	*scontext_len = strlen(policydb.p_user_val_to_name[context->user - 1]) +
+		strlen(policydb.p_role_val_to_name[context->role - 1]) +
+		strlen(policydb.p_type_val_to_name[context->type - 1]) +
+		mls_compute_context_len(context) + 3;
 
 	/* Allocate space for the context; caller must free this space. */
-	scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
-	if (!scontextp) {
+	*scontext = kmalloc(*scontext_len, GFP_ATOMIC);
+	if (!*scontext) {
+		*scontext_len = 0;
 		return -ENOMEM;
 	}
-	*scontext = scontextp;
 
-	/*
-	 * Copy the user name, role name and type name into the context.
-	 */
-	sprintf(scontextp, "%s:%s:%s",
-		policydb.p_user_val_to_name[context->user - 1],
-		policydb.p_role_val_to_name[context->role - 1],
-		policydb.p_type_val_to_name[context->type - 1]);
-	scontextp += strlen(policydb.p_user_val_to_name[context->user - 1]) +
-	             1 + strlen(policydb.p_role_val_to_name[context->role - 1]) +
-	             1 + strlen(policydb.p_type_val_to_name[context->type - 1]);
-
-	mls_sid_to_context(context, &scontextp);
-
-	*scontextp = 0;
+	ctx = *scontext;
+	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
+	strcat(ctx, ":");
+	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
+	strcat(ctx, ":");
+	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);
+	ctx += strlen(ctx);
+	mls_sid_to_context(context, &ctx);
 
 	return 0;
 }
@@ -640,68 +636,62 @@ const char *security_get_initial_sid_context(u32 sid)
 int security_sid_to_context(u32 sid, char **scontext, u32 *scontext_len)
 {
 	struct context *context;
-	int rc = 0;
-
-	*scontext = NULL;
-	*scontext_len  = 0;
+	int rc;
 
 	if (!ss_initialized) {
-		if (sid <= SECINITSID_NUM) {
-			char *scontextp;
-
-			*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
-			scontextp = kmalloc(*scontext_len,GFP_ATOMIC);
-			if (!scontextp) {
-				rc = -ENOMEM;
-				goto out;
-			}
-			strcpy(scontextp, initial_sid_to_string[sid]);
-			*scontext = scontextp;
-			goto out;
+		if (sid > SECINITSID_NUM) {
+			printk(KERN_ERR
+			       "security_sid_to_context:  called before "
+			       "initial load_policy on unknown SID %d\n", sid);
+			goto err;
 		}
-		printk(KERN_ERR "security_sid_to_context:  called before initial "
-		       "load_policy on unknown SID %d\n", sid);
-		rc = -EINVAL;
-		goto out;
+		*scontext = kstrdup(initial_sid_to_string[sid], GFP_ATOMIC);
+		if (*scontext == NULL)
+			return -ENOMEM;
+		*scontext_len = strlen(*scontext) + 1;
+		return 0;
 	}
+
 	POLICY_RDLOCK;
 	context = sidtab_search(&sidtab, sid);
 	if (!context) {
-		printk(KERN_ERR "security_sid_to_context:  unrecognized SID "
-		       "%d\n", sid);
-		rc = -EINVAL;
-		goto out_unlock;
+		POLICY_RDUNLOCK;
+		printk(KERN_ERR
+		       "security_sid_to_context:  unrecognized SID %d\n", sid);
+		goto err;
 	}
 	rc = context_struct_to_string(context, scontext, scontext_len);
-out_unlock:
 	POLICY_RDUNLOCK;
-out:
 	return rc;
 
+err:
+	*scontext = NULL;
+	*scontext_len = 0;
+	return -EINVAL;
 }
 
-static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *sid, u32 def_sid)
+static int security_context_to_sid_core(char *scontext, u32 scontext_len,
+					u32 *sid, u32 def_sid)
 {
-	char *scontext2;
+	char *scontext_dup, *scontext_iter, *scontext_next;
 	struct context context;
-	struct role_datum *role;
+	struct role_datum *roledatum;
 	struct type_datum *typdatum;
 	struct user_datum *usrdatum;
-	char *scontextp, *p, oldc;
-	int rc = 0;
+	int rc = -EINVAL;
 
 	if (!ss_initialized) {
 		int i;
 
-		for (i = 1; i < SECINITSID_NUM; i++) {
+		for (i = 1; i < SECINITSID_NUM; i++)
 			if (!strcmp(initial_sid_to_string[i], scontext)) {
 				*sid = i;
-				goto out;
+				return 0;
 			}
-		}
 		*sid = SECINITSID_KERNEL;
-		goto out;
+		return 0;
 	}
+
 	*sid = SECSID_NULL;
 
 	/* Copy the string so that we can modify the copy as we parse it.
@@ -709,73 +699,46 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 	   null suffix to the copy to avoid problems with the existing
 	   attr package, which doesn't view the null terminator as part
 	   of the attribute value. */
-	scontext2 = kmalloc(scontext_len+1,GFP_KERNEL);
-	if (!scontext2) {
-		rc = -ENOMEM;
-		goto out;
-	}
-	memcpy(scontext2, scontext, scontext_len);
-	scontext2[scontext_len] = 0;
+	scontext_dup = kmemdup(scontext, scontext_len + 1, GFP_KERNEL);
+	if (!scontext_dup)
+		return -ENOMEM;
+	scontext_dup[scontext_len] = 0;
 
 	context_init(&context);
-	*sid = SECSID_NULL;
+	scontext_next = scontext_dup;
 
 	POLICY_RDLOCK;
 
-	/* Parse the security context. */
-
-	rc = -EINVAL;
-	scontextp = (char *) scontext2;
-
 	/* Extract the user. */
-	p = scontextp;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	usrdatum = hashtab_search(policydb.p_users.table, scontextp);
+	usrdatum = hashtab_search(policydb.p_users.table, scontext_iter);
 	if (!usrdatum)
 		goto out_unlock;
-
 	context.user = usrdatum->value;
 
 	/* Extract role. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-
-	if (*p == 0)
+	scontext_iter = strsep(&scontext_next, ":");
+	if (!scontext_next)
 		goto out_unlock;
-
-	*p++ = 0;
-
-	role = hashtab_search(policydb.p_roles.table, scontextp);
-	if (!role)
+	roledatum = hashtab_search(policydb.p_roles.table, scontext_iter);
+	if (!roledatum)
 		goto out_unlock;
-	context.role = role->value;
+	context.role = roledatum->value;
 
 	/* Extract type. */
-	scontextp = p;
-	while (*p && *p != ':')
-		p++;
-	oldc = *p;
-	*p++ = 0;
-
-	typdatum = hashtab_search(policydb.p_types.table, scontextp);
+	scontext_iter = strsep(&scontext_next, ":");
+	typdatum = hashtab_search(policydb.p_types.table, scontext_iter);
 	if (!typdatum)
 		goto out_unlock;
-
 	context.type = typdatum->value;
 
-	rc = mls_context_to_sid(oldc, &p, &context, &sidtab, def_sid);
+	/* Extract MLS range. */
+	rc = mls_context_to_sid(&scontext_next, &context, &sidtab, def_sid);
 	if (rc)
 		goto out_unlock;
-
-	if ((p - scontext2) < scontext_len) {
+	if (scontext_next - scontext_dup < scontext_len) {
 		rc = -EINVAL;
 		goto out_unlock;
 	}
@@ -785,13 +748,14 @@ static int security_context_to_sid_core(char *scontext, u32 scontext_len, u32 *s
 		rc = -EINVAL;
 		goto out_unlock;
 	}
+
 	/* Obtain the new sid. */
 	rc = sidtab_context_to_sid(&sidtab, &context, sid);
+
 out_unlock:
 	POLICY_RDUNLOCK;
 	context_destroy(&context);
-	kfree(scontext2);
-out:
+	kfree(scontext_dup);
 	return rc;
 }
 


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[James Morris]

On Thu, 28 Feb 2008, Paul Moore wrote:

> While looking at the SELinux secid/secctx conversion functions I realized they
> could probably do with a little cleanup to reduce the amount of code and make
> better use of existing string processing functions in the kernel.  Making use
> of the kernel's existing string processing functions is a good idea as many
> architectures have specialized/optimized routines which should be an
> improvement over the generic code in the SELinux security server.

Please always include a signed-off-by line, in case the patch gets 
applied.

> -static int context_struct_to_string(struct context *context, char **scontext, u32 *scontext_len)
> +static int context_struct_to_string(struct context *context,
> +				    char **scontext, u32 *scontext_len)

I tend to not prefer the 80-column rule for function declarations, as it 
makes the code harder to grep (not a show-stopper, but generally, don't 
try and fix these).

- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Paul Moore]

On Friday 29 February 2008 4:49:59 am James Morris wrote:
> On Thu, 28 Feb 2008, Paul Moore wrote:
> > While looking at the SELinux secid/secctx conversion functions I realized
> > they could probably do with a little cleanup to reduce the amount of code
> > and make better use of existing string processing functions in the
> > kernel.  Making use of the kernel's existing string processing functions
> > is a good idea as many architectures have specialized/optimized routines
> > which should be an improvement over the generic code in the SELinux
> > security server.
>
> Please always include a signed-off-by line, in case the patch gets
> applied.

Not including my sign-off is intentional as I've found it the only reliable to 
keep patches from being merged upstream before I'm happy with them.  In this 
particular case I've only booted the patch once or twice and was posting it 
to see if others felt it worthwhile before I spent any more time on it.

> > -static int context_struct_to_string(struct context *context, char
> > **scontext, u32 *scontext_len) +static int
> > context_struct_to_string(struct context *context,
> > +				    char **scontext, u32 *scontext_len)
>
> I tend to not prefer the 80-column rule for function declarations, as it
> makes the code harder to grep (not a show-stopper, but generally, don't
> try and fix these).

I find myself sticking pretty religiously to the 80-column rule, but you make 
a valid point that if I'm not changing the arguments I probably shouldn't 
mess with them.  I'll fix that up.

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Stephen Smalley]

On Thu, 2008-02-28 at 16:41 -0500, Paul Moore wrote:
> While looking at the SELinux secid/secctx conversion functions I realized they
> could probably do with a little cleanup to reduce the amount of code and make
> better use of existing string processing functions in the kernel.  Making use
> of the kernel's existing string processing functions is a good idea as many
> architectures have specialized/optimized routines which should be an
> improvement over the generic code in the SELinux security server.
> ---
> 
>  security/selinux/ss/mls.c      |   61 +++++--------
>  security/selinux/ss/mls.h      |    3 -
>  security/selinux/ss/services.c |  194 ++++++++++++++++------------------------
>  3 files changed, 103 insertions(+), 155 deletions(-)
> 

The snippet below looks like a step backward rather than an improvement
- single sprintf replaced by series of strcat calls.  That can't be more
efficient.

> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index f374186..4aab8a5 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
<snip>
> -	/*
> -	 * Copy the user name, role name and type name into the context.
> -	 */
> -	sprintf(scontextp, "%s:%s:%s",
> -		policydb.p_user_val_to_name[context->user - 1],
> -		policydb.p_role_val_to_name[context->role - 1],
> -		policydb.p_type_val_to_name[context->type - 1]);
> -	scontextp += strlen(policydb.p_user_val_to_name[context->user - 1]) +
> -	             1 + strlen(policydb.p_role_val_to_name[context->role - 1]) +
> -	             1 + strlen(policydb.p_type_val_to_name[context->type - 1]);
> -
> -	mls_sid_to_context(context, &scontextp);
> -
> -	*scontextp = 0;
> +	ctx = *scontext;
> +	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
> +	strcat(ctx, ":");
> +	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
> +	strcat(ctx, ":");
> +	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);

-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Stephen Smalley]

On Fri, 2008-02-29 at 08:54 -0500, Stephen Smalley wrote:
> On Thu, 2008-02-28 at 16:41 -0500, Paul Moore wrote:
> > While looking at the SELinux secid/secctx conversion functions I realized they
> > could probably do with a little cleanup to reduce the amount of code and make
> > better use of existing string processing functions in the kernel.  Making use
> > of the kernel's existing string processing functions is a good idea as many
> > architectures have specialized/optimized routines which should be an
> > improvement over the generic code in the SELinux security server.
> > ---
> > 
> >  security/selinux/ss/mls.c      |   61 +++++--------
> >  security/selinux/ss/mls.h      |    3 -
> >  security/selinux/ss/services.c |  194 ++++++++++++++++------------------------
> >  3 files changed, 103 insertions(+), 155 deletions(-)
> > 
> 
> The snippet below looks like a step backward rather than an improvement
> - single sprintf replaced by series of strcat calls.  That can't be more
> efficient.

Hmm...well, maybe I'm wrong (after looking at the implementations).
Pity that Linux doesn't have stpcpy (as in glibc) - that is much nicer
than a series of strcat's since it returns the end pointer and doesn't
require finding the end of string each time.

> 
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index f374186..4aab8a5 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> <snip>
> > -	/*
> > -	 * Copy the user name, role name and type name into the context.
> > -	 */
> > -	sprintf(scontextp, "%s:%s:%s",
> > -		policydb.p_user_val_to_name[context->user - 1],
> > -		policydb.p_role_val_to_name[context->role - 1],
> > -		policydb.p_type_val_to_name[context->type - 1]);
> > -	scontextp += strlen(policydb.p_user_val_to_name[context->user - 1]) +
> > -	             1 + strlen(policydb.p_role_val_to_name[context->role - 1]) +
> > -	             1 + strlen(policydb.p_type_val_to_name[context->type - 1]);
> > -
> > -	mls_sid_to_context(context, &scontextp);
> > -
> > -	*scontextp = 0;
> > +	ctx = *scontext;
> > +	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
> > +	strcat(ctx, ":");
> > +	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
> > +	strcat(ctx, ":");
> > +	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);
> 
-- 
Stephen Smalley
National Security Agency


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Paul Moore]

On Friday 29 February 2008 9:13:46 am Stephen Smalley wrote:
> On Fri, 2008-02-29 at 08:54 -0500, Stephen Smalley wrote:
> > On Thu, 2008-02-28 at 16:41 -0500, Paul Moore wrote:
> > > While looking at the SELinux secid/secctx conversion functions I
> > > realized they could probably do with a little cleanup to reduce the
> > > amount of code and make better use of existing string processing
> > > functions in the kernel.  Making use of the kernel's existing string
> > > processing functions is a good idea as many architectures have
> > > specialized/optimized routines which should be an improvement over the
> > > generic code in the SELinux security server. ---
> > >
> > >  security/selinux/ss/mls.c      |   61 +++++--------
> > >  security/selinux/ss/mls.h      |    3 -
> > >  security/selinux/ss/services.c |  194
> > > ++++++++++++++++------------------------ 3 files changed, 103
> > > insertions(+), 155 deletions(-)
> >
> > The snippet below looks like a step backward rather than an improvement
> > - single sprintf replaced by series of strcat calls.  That can't be more
> > efficient.
>
> Hmm...well, maybe I'm wrong (after looking at the implementations).

That is exactly why I decided the multiple strcpy()/strcat() calls would be 
faster.  Our formatting needs here are pretty simple and the kernel's 
sprintf() implementation looks very involved versus strcpy()/strcat().

> Pity that Linux doesn't have stpcpy (as in glibc) - that is much nicer
> than a series of strcat's since it returns the end pointer and doesn't
> require finding the end of string each time.

That would be nice here.  However, I think at least moving away from sprintf() 
should yield an advantage.

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[James Antill]

[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]


On Fri, 2008-02-29 at 09:13 -0500, Stephen Smalley wrote:
> On Fri, 2008-02-29 at 08:54 -0500, Stephen Smalley wrote:
> > The snippet below looks like a step backward rather than an improvement
> > - single sprintf replaced by series of strcat calls.  That can't be more
> > efficient.
> 
> Hmm...well, maybe I'm wrong (after looking at the implementations).
> Pity that Linux doesn't have stpcpy (as in glibc) - that is much nicer
> than a series of strcat's since it returns the end pointer and doesn't
> require finding the end of string each time.

 Note that you can do (only slightly abusing the interface):

	ctx = *scontext;

	ctx += strlcpy(ctx, policydb.p_user_val_to_name[context->user - 1], -1);
	ctx += strlcpy(ctx, ":", -1);
	ctx += strlcpy(ctx, policydb.p_role_val_to_name[context->role - 1], -1);
	ctx += strlcpy(ctx, ":", -1);
	ctx += strlcpy(ctx, policydb.p_type_val_to_name[context->type - 1], -1);

...which is basically a memcpy() with a simple if test.

> > > -	*scontextp = 0;
> > > +	ctx = *scontext;
> > > +	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
> > > +	strcat(ctx, ":");
> > > +	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
> > > +	strcat(ctx, ":");
> > > +	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);
> > 
-- 
James Antill <james.antill@redhat.com>
Red Hat

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Paul Moore]

On Friday 29 February 2008 11:40:55 am James Antill wrote:
> On Fri, 2008-02-29 at 09:13 -0500, Stephen Smalley wrote:
> > On Fri, 2008-02-29 at 08:54 -0500, Stephen Smalley wrote:
> > > The snippet below looks like a step backward rather than an
> > > improvement - single sprintf replaced by series of strcat calls. 
> > > That can't be more efficient.
> >
> > Hmm...well, maybe I'm wrong (after looking at the implementations).
> > Pity that Linux doesn't have stpcpy (as in glibc) - that is much
> > nicer than a series of strcat's since it returns the end pointer
> > and doesn't require finding the end of string each time.
>
>  Note that you can do (only slightly abusing the interface):
>
> 	ctx = *scontext;
>
> 	ctx += strlcpy(ctx, policydb.p_user_val_to_name[context->user - 1],
> -1); ctx += strlcpy(ctx, ":", -1);
> 	ctx += strlcpy(ctx, policydb.p_role_val_to_name[context->role - 1],
> -1); ctx += strlcpy(ctx, ":", -1);
> 	ctx += strlcpy(ctx, policydb.p_type_val_to_name[context->type - 1],
> -1);
>
> ...which is basically a memcpy() with a simple if test.

... except for that strlen() call right at the top, there are also two 
comparisons (you only accounted for one) and a add/subtract 
operation :)

[NOTE: I'm comparing the architecture independent versions of these 
functions]

The strlcpy() implementation does a strlen() and a memcpy() call along 
with two comparisons and some trivial math.  The strcat() 
implementation does pretty much the same thing but with less 
comparisons and no additional math.  The strcpy() implementation only 
does a byte-by-byte copy which should be faster then strlcpy() even 
when one factors in an optimized memcpy() implementation due to the 
strlen() call.

My conclusion is that the strcpy()/strcat() approach should be at least 
as fast as the strlcpy() approach if not a smidge faster.

> > > > -	*scontextp = 0;
> > > > +	ctx = *scontext;
> > > > +	strcpy(ctx, policydb.p_user_val_to_name[context->user - 1]);
> > > > +	strcat(ctx, ":");
> > > > +	strcat(ctx, policydb.p_role_val_to_name[context->role - 1]);
> > > > +	strcat(ctx, ":");
> > > > +	strcat(ctx, policydb.p_type_val_to_name[context->type - 1]);

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

RE: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Todd Miller]

Paul Moore wrote:
> The strlcpy() implementation does a strlen() and a memcpy() call along
> with two comparisons and some trivial math.  The strcat()
> implementation does pretty much the same thing but with less
> comparisons and no additional math.  The strcpy() implementation only
> does a byte-by-byte copy which should be faster then strlcpy() even
> when one factors in an optimized memcpy() implementation due to the
> strlen() call.

The difference is that with strcat() you are also finding the length
of dest whereas which strlcpy() you only need to find the length of
src.  I really doubt it makes a measurable difference and things may
be muddied by gcc inlining some of the standard str* functions.

However, you definately do not need the final strlen(ctx) in either
case since you know that *scontext_len - 1 == strlen(ctx).

I'm not sanctioning James' abuse of strlcpy(); I think it is pointless
to try and guess which is faster without actually benchmarking it.

 - todd


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [RFC PATCH] SELinux: Cleanup the secid/secctx conversion functions

[Paul Moore]

On Friday 29 February 2008 1:41:54 pm Todd Miller wrote:
> Paul Moore wrote:
> > The strlcpy() implementation does a strlen() and a memcpy() call
> > along with two comparisons and some trivial math.  The strcat()
> > implementation does pretty much the same thing but with less
> > comparisons and no additional math.  The strcpy() implementation
> > only does a byte-by-byte copy which should be faster then strlcpy()
> > even when one factors in an optimized memcpy() implementation due
> > to the strlen() call.
>
> The difference is that with strcat() you are also finding the length
> of dest whereas which strlcpy() you only need to find the length of
> src.

True, strcat() does do a strlen() on dest while strlcpy() does a 
strlen() on src and in this case dest is almost always going to be 
bigger then src.

> I really doubt it makes a measurable difference and things may 
> be muddied by gcc inlining some of the standard str* functions.

Agree.

Also, (this starts getting into architecture specifics) on x86 (not 
x86_64) I'm pretty sure all function arguments are passed on the stack 
and strlcpy() requires three arguments versus strcat()/strcpy() which 
require only two.  However, at this point I think the argument is 
starting to get a bit ridiculous.

> However, you definately do not need the final strlen(ctx) in either
> case since you know that *scontext_len - 1 == strlen(ctx).

Disagree.  At the point in time of the final strlen(ctx) call ctx looks 
like this (note the lack of MLS range):

    user_u:role_r:type_t

... where as scontext_len is equal to the strlen() of the full context 
(MLS included).  The results of strlen(*scontext) and *scontext_len are 
not the same until after the call to mls_sid_to_context().

> I'm not sanctioning James' abuse of strlcpy(); I think it is
> pointless to try and guess which is faster without actually
> benchmarking it.

Yep ... slow news day and this discussion seems a lot safer then the 
current labeled NFS discussion ;)

-- 
paul moore
linux security @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.