[PATCH] selinux: remove the sidtab context conversion indirect calls

[PATCH] selinux: remove the sidtab context conversion indirect calls

[Paul Moore]

The sidtab conversion code has support for multiple context
conversion routines through the use of function pointers and
indirect calls.  However, the reality is that all current users rely
on the same conversion routine: convert_context().  This patch does
away with this extra complexity and replaces the indirect calls
with direct function calls; allowing us to remove a layer of
obfuscation and create cleaner, more maintainable code.

Signed-off-by: Paul Moore <paul@paul-moore.com>
---
 security/selinux/ss/services.c | 51 ++++++++++++++--------------------
 security/selinux/ss/services.h | 14 ++++++++--
 security/selinux/ss/sidtab.c   | 21 ++++++++------
 security/selinux/ss/sidtab.h   |  3 +-
 4 files changed, 45 insertions(+), 44 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index fe5fcf571c564..e63c4f942fd6a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -68,12 +68,6 @@
 #include "policycap_names.h"
 #include "ima.h"
 
-struct convert_context_args {
-	struct selinux_state *state;
-	struct policydb *oldp;
-	struct policydb *newp;
-};
-
 struct selinux_policy_convert_data {
 	struct convert_context_args args;
 	struct sidtab_convert_params sidtab_params;
@@ -2014,17 +2008,20 @@ static inline int convert_context_handle_invalid_context(
 	return 0;
 }
 
-/*
- * Convert the values in the security context
- * structure `oldc' from the values specified
- * in the policy `p->oldp' to the values specified
- * in the policy `p->newp', storing the new context
- * in `newc'.  Verify that the context is valid
- * under the new policy.
+/**
+ * services_convert_context - Convert a security context across policies.
+ * @args: populated convert_context_args struct
+ * @oldc: original context
+ * @newc: converted context
+ *
+ * Convert the values in the security context structure @oldc from the values
+ * specified in the policy @args->oldp to the values specified in the policy
+ * @args->newp, storing the new context in @newc, and verifying that the
+ * context is valid under the new policy.
  */
-static int convert_context(struct context *oldc, struct context *newc, void *p)
+int services_convert_context(struct convert_context_args *args,
+			     struct context *oldc, struct context *newc)
 {
-	struct convert_context_args *args;
 	struct ocontext *oc;
 	struct role_datum *role;
 	struct type_datum *typdatum;
@@ -2033,15 +2030,12 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
 	u32 len;
 	int rc;
 
-	args = p;
-
 	if (oldc->str) {
 		s = kstrdup(oldc->str, GFP_KERNEL);
 		if (!s)
 			return -ENOMEM;
 
-		rc = string_to_context_struct(args->newp, NULL, s,
-					      newc, SECSID_NULL);
+		rc = string_to_context_struct(args->newp, NULL, s, newc, SECSID_NULL);
 		if (rc == -EINVAL) {
 			/*
 			 * Retain string representation for later mapping.
@@ -2072,8 +2066,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
 
 	/* Convert the user. */
 	usrdatum = symtab_search(&args->newp->p_users,
-				 sym_name(args->oldp,
-					  SYM_USERS, oldc->user - 1));
+				 sym_name(args->oldp, SYM_USERS, oldc->user - 1));
 	if (!usrdatum)
 		goto bad;
 	newc->user = usrdatum->value;
@@ -2087,8 +2080,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
 
 	/* Convert the type. */
 	typdatum = symtab_search(&args->newp->p_types,
-				 sym_name(args->oldp,
-					  SYM_TYPES, oldc->type - 1));
+				 sym_name(args->oldp, SYM_TYPES, oldc->type - 1));
 	if (!typdatum)
 		goto bad;
 	newc->type = typdatum->value;
@@ -2122,8 +2114,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
 	/* Check the validity of the new context. */
 	if (!policydb_context_isvalid(args->newp, newc)) {
 		rc = convert_context_handle_invalid_context(args->state,
-							args->oldp,
-							oldc);
+							    args->oldp, oldc);
 		if (rc)
 			goto bad;
 	}
@@ -2332,21 +2323,21 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
 		goto err_free_isids;
 	}
 
+	/*
+	 * Convert the internal representations of contexts
+	 * in the new SID table.
+	 */
+
 	convert_data = kmalloc(sizeof(*convert_data), GFP_KERNEL);
 	if (!convert_data) {
 		rc = -ENOMEM;
 		goto err_free_isids;
 	}
 
-	/*
-	 * Convert the internal representations of contexts
-	 * in the new SID table.
-	 */
 	convert_data->args.state = state;
 	convert_data->args.oldp = &oldpolicy->policydb;
 	convert_data->args.newp = &newpolicy->policydb;
 
-	convert_data->sidtab_params.func = convert_context;
 	convert_data->sidtab_params.args = &convert_data->args;
 	convert_data->sidtab_params.target = newpolicy->sidtab;
 
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index 9555ad074303c..6348c95ff0e52 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -29,10 +29,18 @@ struct selinux_policy {
 	u32 latest_granting;
 } __randomize_layout;
 
-void services_compute_xperms_drivers(struct extended_perms *xperms,
-				struct avtab_node *node);
+struct convert_context_args {
+	struct selinux_state *state;
+	struct policydb *oldp;
+	struct policydb *newp;
+};
 
+void services_compute_xperms_drivers(struct extended_perms *xperms,
+				     struct avtab_node *node);
 void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
-					struct avtab_node *node);
+				      struct avtab_node *node);
+
+int services_convert_context(struct convert_context_args *args,
+			     struct context *oldc, struct context *newc);
 
 #endif	/* _SS_SERVICES_H_ */
diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
index a54b8652bfb50..1c3d2cda6b92a 100644
--- a/security/selinux/ss/sidtab.c
+++ b/security/selinux/ss/sidtab.c
@@ -18,6 +18,7 @@
 #include "flask.h"
 #include "security.h"
 #include "sidtab.h"
+#include "services.h"
 
 struct sidtab_str_cache {
 	struct rcu_head rcu_member;
@@ -292,7 +293,6 @@ int sidtab_context_to_sid(struct sidtab *s, struct context *context,
 	}
 
 	count = s->count;
-	convert = s->convert;
 
 	/* bail out if we already reached max entries */
 	rc = -EOVERFLOW;
@@ -316,25 +316,28 @@ int sidtab_context_to_sid(struct sidtab *s, struct context *context,
 	 * if we are building a new sidtab, we need to convert the context
 	 * and insert it there as well
 	 */
+	convert = s->convert;
 	if (convert) {
+		struct sidtab *target = convert->target;
+
 		rc = -ENOMEM;
-		dst_convert = sidtab_do_lookup(convert->target, count, 1);
+		dst_convert = sidtab_do_lookup(target, count, 1);
 		if (!dst_convert) {
 			context_destroy(&dst->context);
 			goto out_unlock;
 		}
 
-		rc = convert->func(context, &dst_convert->context,
-				   convert->args);
+		rc = services_convert_context(convert->args,
+					      context, &dst_convert->context);
 		if (rc) {
 			context_destroy(&dst->context);
 			goto out_unlock;
 		}
 		dst_convert->sid = index_to_sid(count);
 		dst_convert->hash = context_compute_hash(&dst_convert->context);
-		convert->target->count = count + 1;
+		target->count = count + 1;
 
-		hash_add_rcu(convert->target->context_to_sid,
+		hash_add_rcu(target->context_to_sid,
 			     &dst_convert->list, dst_convert->hash);
 	}
 
@@ -402,9 +405,9 @@ static int sidtab_convert_tree(union sidtab_entry_inner *edst,
 		}
 		i = 0;
 		while (i < SIDTAB_LEAF_ENTRIES && *pos < count) {
-			rc = convert->func(&esrc->ptr_leaf->entries[i].context,
-					   &edst->ptr_leaf->entries[i].context,
-					   convert->args);
+			rc = services_convert_context(convert->args,
+					&esrc->ptr_leaf->entries[i].context,
+					&edst->ptr_leaf->entries[i].context);
 			if (rc)
 				return rc;
 			(*pos)++;
diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
index 4eff0e49dcb22..72810a080e77b 100644
--- a/security/selinux/ss/sidtab.h
+++ b/security/selinux/ss/sidtab.h
@@ -65,8 +65,7 @@ struct sidtab_isid_entry {
 };
 
 struct sidtab_convert_params {
-	int (*func)(struct context *oldc, struct context *newc, void *args);
-	void *args;
+	struct convert_context_args *args;
 	struct sidtab *target;
 };
 
-- 
2.38.1

Re: [PATCH] selinux: remove the sidtab context conversion indirect calls

[Ondrej Mosnacek]

On Wed, Nov 9, 2022 at 5:02 AM Paul Moore <paul@paul-moore.com> wrote:
> The sidtab conversion code has support for multiple context
> conversion routines through the use of function pointers and
> indirect calls.  However, the reality is that all current users rely
> on the same conversion routine: convert_context().  This patch does
> away with this extra complexity and replaces the indirect calls
> with direct function calls; allowing us to remove a layer of
> obfuscation and create cleaner, more maintainable code.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
> ---
>  security/selinux/ss/services.c | 51 ++++++++++++++--------------------
>  security/selinux/ss/services.h | 14 ++++++++--
>  security/selinux/ss/sidtab.c   | 21 ++++++++------
>  security/selinux/ss/sidtab.h   |  3 +-
>  4 files changed, 45 insertions(+), 44 deletions(-)

The goal of the callback abstraction was to avoid the awkward coupling
between services.c and sidtab.c, but both ways are ugly in some way,
so I consider it a matter of maintainer preference. So if you prefer
this version, feel free to go with it :)

Reviewed-by: Ondrej Mosnacek <omosnace@redhat.com>

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: [PATCH] selinux: remove the sidtab context conversion indirect calls

[Paul Moore]

On Wed, Nov 9, 2022 at 4:26 AM Ondrej Mosnacek <omosnace@redhat.com> wrote
> On Wed, Nov 9, 2022 at 5:02 AM Paul Moore <paul@paul-moore.com> wrote:
> > The sidtab conversion code has support for multiple context
> > conversion routines through the use of function pointers and
> > indirect calls.  However, the reality is that all current users rely
> > on the same conversion routine: convert_context().  This patch does
> > away with this extra complexity and replaces the indirect calls
> > with direct function calls; allowing us to remove a layer of
> > obfuscation and create cleaner, more maintainable code.
> >
> > Signed-off-by: Paul Moore <paul@paul-moore.com>
> > ---
> >  security/selinux/ss/services.c | 51 ++++++++++++++--------------------
> >  security/selinux/ss/services.h | 14 ++++++++--
> >  security/selinux/ss/sidtab.c   | 21 ++++++++------
> >  security/selinux/ss/sidtab.h   |  3 +-
> >  4 files changed, 45 insertions(+), 44 deletions(-)
>
> The goal of the callback abstraction was to avoid the awkward coupling
> between services.c and sidtab.c, but both ways are ugly in some way,
> so I consider it a matter of maintainer preference. So if you prefer
> this version, feel free to go with it :)

While function pointers do have their place, e.g. the network stack
and VFS, history has shown that indirect calls aren't without risk.
In addition, Linus implied that he wanted this removed, and I don't
feel strongly enough about it to argue.

Merged into selinux/next.

-- 
paul-moore.com

Re: [PATCH] selinux: remove the sidtab context conversion indirect calls

[Casey Schaufler]

On 11/8/2022 8:00 PM, Paul Moore wrote:
> The sidtab conversion code has support for multiple context
> conversion routines through the use of function pointers and
> indirect calls.  However, the reality is that all current users rely
> on the same conversion routine: convert_context().  This patch does
> away with this extra complexity and replaces the indirect calls
> with direct function calls; allowing us to remove a layer of
> obfuscation and create cleaner, more maintainable code.
>
> Signed-off-by: Paul Moore <paul@paul-moore.com>

The SELinux code is enhanced by this sort of clean-up.

Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>

> ---
>  security/selinux/ss/services.c | 51 ++++++++++++++--------------------
>  security/selinux/ss/services.h | 14 ++++++++--
>  security/selinux/ss/sidtab.c   | 21 ++++++++------
>  security/selinux/ss/sidtab.h   |  3 +-
>  4 files changed, 45 insertions(+), 44 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index fe5fcf571c564..e63c4f942fd6a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -68,12 +68,6 @@
>  #include "policycap_names.h"
>  #include "ima.h"
>  
> -struct convert_context_args {
> -	struct selinux_state *state;
> -	struct policydb *oldp;
> -	struct policydb *newp;
> -};
> -
>  struct selinux_policy_convert_data {
>  	struct convert_context_args args;
>  	struct sidtab_convert_params sidtab_params;
> @@ -2014,17 +2008,20 @@ static inline int convert_context_handle_invalid_context(
>  	return 0;
>  }
>  
> -/*
> - * Convert the values in the security context
> - * structure `oldc' from the values specified
> - * in the policy `p->oldp' to the values specified
> - * in the policy `p->newp', storing the new context
> - * in `newc'.  Verify that the context is valid
> - * under the new policy.
> +/**
> + * services_convert_context - Convert a security context across policies.
> + * @args: populated convert_context_args struct
> + * @oldc: original context
> + * @newc: converted context
> + *
> + * Convert the values in the security context structure @oldc from the values
> + * specified in the policy @args->oldp to the values specified in the policy
> + * @args->newp, storing the new context in @newc, and verifying that the
> + * context is valid under the new policy.
>   */
> -static int convert_context(struct context *oldc, struct context *newc, void *p)
> +int services_convert_context(struct convert_context_args *args,
> +			     struct context *oldc, struct context *newc)
>  {
> -	struct convert_context_args *args;
>  	struct ocontext *oc;
>  	struct role_datum *role;
>  	struct type_datum *typdatum;
> @@ -2033,15 +2030,12 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>  	u32 len;
>  	int rc;
>  
> -	args = p;
> -
>  	if (oldc->str) {
>  		s = kstrdup(oldc->str, GFP_KERNEL);
>  		if (!s)
>  			return -ENOMEM;
>  
> -		rc = string_to_context_struct(args->newp, NULL, s,
> -					      newc, SECSID_NULL);
> +		rc = string_to_context_struct(args->newp, NULL, s, newc, SECSID_NULL);
>  		if (rc == -EINVAL) {
>  			/*
>  			 * Retain string representation for later mapping.
> @@ -2072,8 +2066,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>  
>  	/* Convert the user. */
>  	usrdatum = symtab_search(&args->newp->p_users,
> -				 sym_name(args->oldp,
> -					  SYM_USERS, oldc->user - 1));
> +				 sym_name(args->oldp, SYM_USERS, oldc->user - 1));
>  	if (!usrdatum)
>  		goto bad;
>  	newc->user = usrdatum->value;
> @@ -2087,8 +2080,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>  
>  	/* Convert the type. */
>  	typdatum = symtab_search(&args->newp->p_types,
> -				 sym_name(args->oldp,
> -					  SYM_TYPES, oldc->type - 1));
> +				 sym_name(args->oldp, SYM_TYPES, oldc->type - 1));
>  	if (!typdatum)
>  		goto bad;
>  	newc->type = typdatum->value;
> @@ -2122,8 +2114,7 @@ static int convert_context(struct context *oldc, struct context *newc, void *p)
>  	/* Check the validity of the new context. */
>  	if (!policydb_context_isvalid(args->newp, newc)) {
>  		rc = convert_context_handle_invalid_context(args->state,
> -							args->oldp,
> -							oldc);
> +							    args->oldp, oldc);
>  		if (rc)
>  			goto bad;
>  	}
> @@ -2332,21 +2323,21 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len,
>  		goto err_free_isids;
>  	}
>  
> +	/*
> +	 * Convert the internal representations of contexts
> +	 * in the new SID table.
> +	 */
> +
>  	convert_data = kmalloc(sizeof(*convert_data), GFP_KERNEL);
>  	if (!convert_data) {
>  		rc = -ENOMEM;
>  		goto err_free_isids;
>  	}
>  
> -	/*
> -	 * Convert the internal representations of contexts
> -	 * in the new SID table.
> -	 */
>  	convert_data->args.state = state;
>  	convert_data->args.oldp = &oldpolicy->policydb;
>  	convert_data->args.newp = &newpolicy->policydb;
>  
> -	convert_data->sidtab_params.func = convert_context;
>  	convert_data->sidtab_params.args = &convert_data->args;
>  	convert_data->sidtab_params.target = newpolicy->sidtab;
>  
> diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
> index 9555ad074303c..6348c95ff0e52 100644
> --- a/security/selinux/ss/services.h
> +++ b/security/selinux/ss/services.h
> @@ -29,10 +29,18 @@ struct selinux_policy {
>  	u32 latest_granting;
>  } __randomize_layout;
>  
> -void services_compute_xperms_drivers(struct extended_perms *xperms,
> -				struct avtab_node *node);
> +struct convert_context_args {
> +	struct selinux_state *state;
> +	struct policydb *oldp;
> +	struct policydb *newp;
> +};
>  
> +void services_compute_xperms_drivers(struct extended_perms *xperms,
> +				     struct avtab_node *node);
>  void services_compute_xperms_decision(struct extended_perms_decision *xpermd,
> -					struct avtab_node *node);
> +				      struct avtab_node *node);
> +
> +int services_convert_context(struct convert_context_args *args,
> +			     struct context *oldc, struct context *newc);
>  
>  #endif	/* _SS_SERVICES_H_ */
> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c
> index a54b8652bfb50..1c3d2cda6b92a 100644
> --- a/security/selinux/ss/sidtab.c
> +++ b/security/selinux/ss/sidtab.c
> @@ -18,6 +18,7 @@
>  #include "flask.h"
>  #include "security.h"
>  #include "sidtab.h"
> +#include "services.h"
>  
>  struct sidtab_str_cache {
>  	struct rcu_head rcu_member;
> @@ -292,7 +293,6 @@ int sidtab_context_to_sid(struct sidtab *s, struct context *context,
>  	}
>  
>  	count = s->count;
> -	convert = s->convert;
>  
>  	/* bail out if we already reached max entries */
>  	rc = -EOVERFLOW;
> @@ -316,25 +316,28 @@ int sidtab_context_to_sid(struct sidtab *s, struct context *context,
>  	 * if we are building a new sidtab, we need to convert the context
>  	 * and insert it there as well
>  	 */
> +	convert = s->convert;
>  	if (convert) {
> +		struct sidtab *target = convert->target;
> +
>  		rc = -ENOMEM;
> -		dst_convert = sidtab_do_lookup(convert->target, count, 1);
> +		dst_convert = sidtab_do_lookup(target, count, 1);
>  		if (!dst_convert) {
>  			context_destroy(&dst->context);
>  			goto out_unlock;
>  		}
>  
> -		rc = convert->func(context, &dst_convert->context,
> -				   convert->args);
> +		rc = services_convert_context(convert->args,
> +					      context, &dst_convert->context);
>  		if (rc) {
>  			context_destroy(&dst->context);
>  			goto out_unlock;
>  		}
>  		dst_convert->sid = index_to_sid(count);
>  		dst_convert->hash = context_compute_hash(&dst_convert->context);
> -		convert->target->count = count + 1;
> +		target->count = count + 1;
>  
> -		hash_add_rcu(convert->target->context_to_sid,
> +		hash_add_rcu(target->context_to_sid,
>  			     &dst_convert->list, dst_convert->hash);
>  	}
>  
> @@ -402,9 +405,9 @@ static int sidtab_convert_tree(union sidtab_entry_inner *edst,
>  		}
>  		i = 0;
>  		while (i < SIDTAB_LEAF_ENTRIES && *pos < count) {
> -			rc = convert->func(&esrc->ptr_leaf->entries[i].context,
> -					   &edst->ptr_leaf->entries[i].context,
> -					   convert->args);
> +			rc = services_convert_context(convert->args,
> +					&esrc->ptr_leaf->entries[i].context,
> +					&edst->ptr_leaf->entries[i].context);
>  			if (rc)
>  				return rc;
>  			(*pos)++;
> diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h
> index 4eff0e49dcb22..72810a080e77b 100644
> --- a/security/selinux/ss/sidtab.h
> +++ b/security/selinux/ss/sidtab.h
> @@ -65,8 +65,7 @@ struct sidtab_isid_entry {
>  };
>  
>  struct sidtab_convert_params {
> -	int (*func)(struct context *oldc, struct context *newc, void *args);
> -	void *args;
> +	struct convert_context_args *args;
>  	struct sidtab *target;
>  };
>