[PATCH] selinux: Streamline type determination in security_compute_sid

[PATCH] selinux: Streamline type determination in security_compute_sid

[Canfeng Guo]

Simplifies the logic for determining the security context type in
security_compute_sid, enhancing readability and efficiency.

Consolidates default type assignment logic next to type transition
checks, removing redundancy and improving code flow.

Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
---
 security/selinux/ss/services.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e33e55384b75..0c07ebf0b1e7 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1804,21 +1804,7 @@ static int security_compute_sid(u32 ssid,
 			newcontext.role = OBJECT_R_VAL;
 	}
 
-	/* Set the type to default values. */
-	if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
-		newcontext.type = scontext->type;
-	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
-		newcontext.type = tcontext->type;
-	} else {
-		if ((tclass == policydb->process_class) || sock) {
-			/* Use the type of process. */
-			newcontext.type = scontext->type;
-		} else {
-			/* Use the type of the related object. */
-			newcontext.type = tcontext->type;
-		}
-	}
-
+	/* Set the type. */
 	/* Look for a type transition/member/change rule. */
 	avkey.source_type = scontext->type;
 	avkey.target_type = tcontext->type;
@@ -1837,9 +1823,23 @@ static int security_compute_sid(u32 ssid,
 		}
 	}
 
+	/* If a permanent rule is found, use the type from */
+	/* the type transition/member/change rule. Otherwise, */
+	/* set the type to its default values. */
 	if (avnode) {
-		/* Use the type from the type transition/member/change rule. */
 		newcontext.type = avnode->datum.u.data;
+	} else if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
+		newcontext.type = scontext->type;
+	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
+		newcontext.type = tcontext->type;
+	} else {
+		if ((tclass == policydb->process_class) || sock) {
+			/* Use the type of process. */
+			newcontext.type = scontext->type;
+		} else {
+			/* Use the type of the related object. */
+			newcontext.type = tcontext->type;
+		}
 	}
 
 	/* if we have a objname this is a file trans check so check those rules */
-- 
2.20.1

Re: [PATCH] selinux: Streamline type determination in security_compute_sid

[Paul Moore]

On Jun 29, 2024 Canfeng Guo <guocanfeng@uniontech.com> wrote:
> 
> Simplifies the logic for determining the security context type in
> security_compute_sid, enhancing readability and efficiency.
> 
> Consolidates default type assignment logic next to type transition
> checks, removing redundancy and improving code flow.
> 
> Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
> ---
>  security/selinux/ss/services.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index e33e55384b75..0c07ebf0b1e7 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1804,21 +1804,7 @@ static int security_compute_sid(u32 ssid,
>  			newcontext.role = OBJECT_R_VAL;
>  	}
>  
> -	/* Set the type to default values. */
> -	if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
> -		newcontext.type = scontext->type;
> -	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
> -		newcontext.type = tcontext->type;
> -	} else {
> -		if ((tclass == policydb->process_class) || sock) {
> -			/* Use the type of process. */
> -			newcontext.type = scontext->type;
> -		} else {
> -			/* Use the type of the related object. */
> -			newcontext.type = tcontext->type;
> -		}
> -	}
> -
> +	/* Set the type. */
>  	/* Look for a type transition/member/change rule. */
>  	avkey.source_type = scontext->type;
>  	avkey.target_type = tcontext->type;
> @@ -1837,9 +1823,23 @@ static int security_compute_sid(u32 ssid,
>  		}
>  	}
>  
> +	/* If a permanent rule is found, use the type from */
> +	/* the type transition/member/change rule. Otherwise, */
> +	/* set the type to its default values. */

In general this patch looks fine with the exception of the comment
block above, you can either follow the multi-line comment used elsewhere
in this source file, example:

 /* line one
    line two
    line three */

... or you can follow the generally accepted style for multi-line
comments in the Linux kernel:

 /* line one
  * line two
  * line three
  */

See the link below for more information:

* https://docs.kernel.org/process/coding-style.html#commenting

>  	if (avnode) {
> -		/* Use the type from the type transition/member/change rule. */
>  		newcontext.type = avnode->datum.u.data;
> +	} else if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
> +		newcontext.type = scontext->type;
> +	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
> +		newcontext.type = tcontext->type;
> +	} else {
> +		if ((tclass == policydb->process_class) || sock) {
> +			/* Use the type of process. */
> +			newcontext.type = scontext->type;
> +		} else {
> +			/* Use the type of the related object. */
> +			newcontext.type = tcontext->type;
> +		}
>  	}
>  
>  	/* if we have a objname this is a file trans check so check those rules */
> -- 
> 2.20.1

--
paul-moore.com

Re: [PATCH] selinux: Streamline type determination in security_compute_sid

[Paul Moore]

On Thu, Jul 11, 2024 at 5:10 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jul 2, 2024 at 10:56 PM Canfeng Guo <guocanfeng@uniontech.com> wrote:
> >
> > Simplifies the logic for determining the security context type in
> > security_compute_sid, enhancing readability and efficiency.
> >
> > Consolidates default type assignment logic next to type transition
> > checks, removing redundancy and improving code flow.
> >
> > Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
> > ---
> > v2:
> >    Modify the format to follow the generally accepted style for
> >    multi-line comments in the Linux kernel.
> > ---
> >  security/selinux/ss/services.c | 36 ++++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 17 deletions(-)
>
> Thanks for the revised patch, it looks good to me, but it is too late
> in the development cycle to merge it into the selinux/dev branch; I'm
> going to merge it into selinux/dev-staging for testing and I'll move
> it to the selinux/dev branch after the upcoming merge window closes.

A quick note to let you know that this is now in the selinux/dev branch, thanks!

-- 
paul-moore.com

Re: [PATCH] selinux: Streamline type determination in security_compute_sid

[Paul Moore]

On Tue, Jul 2, 2024 at 10:56 PM Canfeng Guo <guocanfeng@uniontech.com> wrote:
>
> Simplifies the logic for determining the security context type in
> security_compute_sid, enhancing readability and efficiency.
>
> Consolidates default type assignment logic next to type transition
> checks, removing redundancy and improving code flow.
>
> Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
> ---
> v2:
>    Modify the format to follow the generally accepted style for
>    multi-line comments in the Linux kernel.
> ---
>  security/selinux/ss/services.c | 36 ++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)

Thanks for the revised patch, it looks good to me, but it is too late
in the development cycle to merge it into the selinux/dev branch; I'm
going to merge it into selinux/dev-staging for testing and I'll move
it to the selinux/dev branch after the upcoming merge window closes.

-- 
paul-moore.com

[PATCH] selinux: Streamline type determination in security_compute_sid

[Canfeng Guo]

Simplifies the logic for determining the security context type in
security_compute_sid, enhancing readability and efficiency.

Consolidates default type assignment logic next to type transition
checks, removing redundancy and improving code flow.

Signed-off-by: Canfeng Guo <guocanfeng@uniontech.com>
---
v2:
   Modify the format to follow the generally accepted style for
   multi-line comments in the Linux kernel.
---
 security/selinux/ss/services.c | 36 ++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index e33e55384b75..a9830fbfc5c6 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1804,22 +1804,9 @@ static int security_compute_sid(u32 ssid,
 			newcontext.role = OBJECT_R_VAL;
 	}
 
-	/* Set the type to default values. */
-	if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
-		newcontext.type = scontext->type;
-	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
-		newcontext.type = tcontext->type;
-	} else {
-		if ((tclass == policydb->process_class) || sock) {
-			/* Use the type of process. */
-			newcontext.type = scontext->type;
-		} else {
-			/* Use the type of the related object. */
-			newcontext.type = tcontext->type;
-		}
-	}
-
-	/* Look for a type transition/member/change rule. */
+	/* Set the type.
+	 * Look for a type transition/member/change rule.
+	 */
 	avkey.source_type = scontext->type;
 	avkey.target_type = tcontext->type;
 	avkey.target_class = tclass;
@@ -1837,9 +1824,24 @@ static int security_compute_sid(u32 ssid,
 		}
 	}
 
+	/* If a permanent rule is found, use the type from
+	 * the type transition/member/change rule. Otherwise,
+	 * set the type to its default values.
+	 */
 	if (avnode) {
-		/* Use the type from the type transition/member/change rule. */
 		newcontext.type = avnode->datum.u.data;
+	} else if (cladatum && cladatum->default_type == DEFAULT_SOURCE) {
+		newcontext.type = scontext->type;
+	} else if (cladatum && cladatum->default_type == DEFAULT_TARGET) {
+		newcontext.type = tcontext->type;
+	} else {
+		if ((tclass == policydb->process_class) || sock) {
+			/* Use the type of process. */
+			newcontext.type = scontext->type;
+		} else {
+			/* Use the type of the related object. */
+			newcontext.type = tcontext->type;
+		}
 	}
 
 	/* if we have a objname this is a file trans check so check those rules */
-- 
2.20.1