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
