Re: [PATCH RFC v2 10/22] selinux: use u16 for security classes

[Paul Moore]

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@xxxxxxxxxxxxx> wrote:
> 
> Security class identifiers are limited to 2^16, thus use the appropriate
> type u16 consistently.
> 
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
>  security/selinux/ss/policydb.c | 52 +++++++++++++++++++++++++---------
>  security/selinux/ss/policydb.h | 10 +++----
>  security/selinux/ss/services.c |  2 +-
>  3 files changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 2408c3e8ae39..eeca470cc90c 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -927,7 +927,7 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s)
>  	return 0;
>  }
>  
> -int policydb_class_isvalid(struct policydb *p, unsigned int class)
> +int policydb_class_isvalid(struct policydb *p, u16 class)
>  {
>  	if (!class || class > p->p_classes.nprim)
>  		return 0;
> @@ -1321,7 +1321,7 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>  	char *key = NULL;
>  	struct class_datum *cladatum;
>  	__le32 buf[6];
> -	u32 i, len, len2, ncons, nel;
> +	u32 i, len, len2, ncons, nel, val;
>  	int rc;
>  
>  	cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
> @@ -1334,9 +1334,14 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
>  
>  	len = le32_to_cpu(buf[0]);
>  	len2 = le32_to_cpu(buf[1]);
> -	cladatum->value = le32_to_cpu(buf[2]);
>  	nel = le32_to_cpu(buf[4]);
>  
> +	val = le32_to_cpu(buf[2]);
> +	rc = -EINVAL;
> +	if (val >= U16_MAX)
> +		goto bad;

While this is a major issue, isn't U16_MAX technically still valid?  In
other words should this be: '(val > U16_MAX)'?

> @@ -1939,7 +1948,11 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
>  
>  	stype = le32_to_cpu(buf[0]);
>  	key.ttype = le32_to_cpu(buf[1]);
> -	key.tclass = le32_to_cpu(buf[2]);
> +	val = le32_to_cpu(buf[2]);
> +	rc = -EINVAL;
> +	if (val > U16_MAX || !policydb_class_isvalid(p, val))
> +		goto out;

We should split out the class validity check into a separate patch and
keep this just as the subject states: consolidate the class type to u16.

As an aside, I'm going to do a quick review pass on the rest of the
patches in this series, but I'm not going to merge them as I keep
hitting a number of merge failures due to this patch not being applied
and I'd rather not have to fix them all up :)

--
paul-moore.com