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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06eb43271d150c5003b00baef7350161@paul-moore.com>
Date: Tue, 07 Jan 2025 22:00:04 -0500
From: Paul Moore <paul@...l-moore.com>
To: Christian Göttsche <cgoettsche@...tendoof.de>, selinux@...r.kernel.org
Cc: Christian Göttsche <cgzones@...glemail.com>, Stephen Smalley <stephen.smalley.work@...il.com>, Ondrej Mosnacek <omosnace@...hat.com>, Nathan Chancellor <nathan@...nel.org>, Nick Desaulniers <ndesaulniers@...gle.com>, Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, Thiébaud Weksteen <tweek@...gle.com>, Bram Bonné <brambonne@...gle.com>, Masahiro Yamada <masahiroy@...nel.org>, linux-kernel@...r.kernel.org, llvm@...ts.linux.dev, Casey Schaufler <casey@...aufler-ca.com>, John Johansen <john.johansen@...onical.com>, GUO Zihua <guozihua@...wei.com>, Canfeng Guo <guocanfeng@...ontech.com>
Subject: Re: [PATCH RFC v2 10/22] selinux: use u16 for security classes

On Dec 16, 2024 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgoettsche@...tendoof.de> wrote:
> 
> Security class identifiers are limited to 2^16, thus use the appropriate
> type u16 consistently.
> 
> Signed-off-by: Christian Göttsche <cgzones@...glemail.com>
> ---
>  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