Re: [PATCH RFC 7/20] selinux: services: update type for umber of class permissions

[Paul Moore <paul@paul-moore.com>]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

On Jul  6, 2023 Gong Ruiqi <gongruiqi1@huawei.com> wrote:
> 
> Security classes have only up to 32 permissions, hence using an u16 is
> sufficient (while improving padding).

Can you explain the improved padding comment?  It looks like you are
only changing the iterator's type so the struct should remain
unchanged, and FWIW, it looks like security_class_wrapping allocates
space for 33 permission strings.

> Also use a fixed sized cast in a bit shift to work correctly on
> architectures where sizeof(unsigned int) != sizeof(u32).
> 
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  security/selinux/ss/services.c | 6 +++---
>  security/selinux/ss/services.h | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 78946b71c1c1..3275cfe2c8f7 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -97,7 +97,6 @@ static int selinux_set_mapping(struct policydb *pol,
>  			       struct selinux_map *out_map)
>  {
>  	u16 i, j;
> -	unsigned k;
>  	bool print_unknown_handle = false;
>  
>  	/* Find number of classes in the input mapping */
> @@ -117,6 +116,7 @@ static int selinux_set_mapping(struct policydb *pol,
>  	while (map[j].name) {
>  		const struct security_class_mapping *p_in = map + (j++);
>  		struct selinux_mapping *p_out = out_map->mapping + j;
> +		u16 k;
>  
>  		/* An empty class string skips ahead */
>  		if (!strcmp(p_in->name, "")) {
> @@ -202,7 +202,7 @@ static void map_decision(struct selinux_map *map,
>  {
>  	if (tclass < map->size) {
>  		struct selinux_mapping *mapping = &map->mapping[tclass];
> -		unsigned int i, n = mapping->num_perms;
> +		u16 i, n = mapping->num_perms;
>  		u32 result;
>  
>  		for (i = 0, result = 0; i < n; i++) {
> @@ -230,7 +230,7 @@ static void map_decision(struct selinux_map *map,
>  		 * should audit that denial
>  		 */
>  		for (; i < (sizeof(u32)*8); i++)
> -			result |= 1<<i;
> +			result |= 1<<((u32)i);
>  		avd->auditdeny = result;
>  	}
>  }
> diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
> index 8a9b85f44b66..b6f99353301e 100644
> --- a/security/selinux/ss/services.h
> +++ b/security/selinux/ss/services.h
> @@ -12,7 +12,7 @@
>  /* Mapping for a single class */
>  struct selinux_mapping {
>  	u16 value; /* policy value for class */
> -	unsigned int num_perms; /* number of permissions in class */
> +	u16 num_perms; /* number of permissions in class */
>  	u32 perms[sizeof(u32) * 8]; /* policy values for permissions */
>  };
>  
> -- 
> 2.40.1

--
paul-moore.com