On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@xxxxxxxxxxxxxx> wrote:
>
> Use the identical type for local variables, e.g. loop counters.
>
> Declare members of struct policydb_compat_info unsigned to consistently
> use unsigned iterators. They hold read-only non-negative numbers in the
> global variable policydb_compat.
>
> Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> ---
> v2:
> - avoid declarations in init-clauses of for loops
> - declare members of struct policydb_compat_info unsigned
> ---
> security/selinux/ss/policydb.c | 93 +++++++++++++++++++++-------------
> 1 file changed, 58 insertions(+), 35 deletions(-)
>
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index dc66868ff62c..aa2371a422af 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -55,9 +55,9 @@ static const char *const symtab_name[SYM_NUM] = {
> #endif
>
> struct policydb_compat_info {
> - int version;
> - int sym_num;
> - int ocon_num;
> + unsigned int version;
> + unsigned int sym_num;
> + unsigned int ocon_num;
> };
>
> /* These need to be updated if SYM_NUM or OCON_NUM changes */
> @@ -159,9 +159,9 @@ static const struct policydb_compat_info policydb_compat[] = {
> },
> };
>
> -static const struct policydb_compat_info *policydb_lookup_compat(int version)
> +static const struct policydb_compat_info *policydb_lookup_compat(unsigned int version)
> {
> - int i;
> + u32 i;
Another question of 'why u32'? I can understand making the iterator
unsigned, but why explicitly make it 32-bits? Why not just an
unsigned int?
> for (i = 0; i < ARRAY_SIZE(policydb_compat); i++) {
> if (policydb_compat[i].version == version)
> @@ -359,7 +359,7 @@ static int role_tr_destroy(void *key, void *datum, void *p)
> return 0;
> }
>
> -static void ocontext_destroy(struct ocontext *c, int i)
> +static void ocontext_destroy(struct ocontext *c, u32 i)
Yes, this should be unsigned, but why not an unsigned it?
> {
> if (!c)
> return;
> @@ -781,7 +781,7 @@ void policydb_destroy(struct policydb *p)
> {
> struct ocontext *c, *ctmp;
> struct genfs *g, *gtmp;
> - int i;
> + u32 i;
Same.
> struct role_allow *ra, *lra = NULL;
>
> for (i = 0; i < SYM_NUM; i++) {
> @@ -2237,8 +2240,9 @@ static int genfs_read(struct policydb *p, void *fp)
> static int ocontext_read(struct policydb *p, const struct policydb_compat_info *info,
> void *fp)
> {
> - int i, j, rc;
> - u32 nel, len;
> + int rc;
> + unsigned int i;
> + u32 j, nel, len, val;
> __be64 prefixbuf[1];
> __le32 buf[3];
> struct ocontext *l, *c;
> @@ -2299,9 +2303,27 @@ static int ocontext_read(struct policydb *p, const struct policydb_compat_info *
> rc = next_entry(buf, fp, sizeof(u32)*3);
> if (rc)
> goto out;
> - c->u.port.protocol = le32_to_cpu(buf[0]);
> - c->u.port.low_port = le32_to_cpu(buf[1]);
> - c->u.port.high_port = le32_to_cpu(buf[2]);
> +
> + rc = -EINVAL;
> +
> + val = le32_to_cpu(buf[0]);
> + if (val > U8_MAX)
> + goto out;
> + c->u.port.protocol = val;
> +
> + val = le32_to_cpu(buf[1]);
> + if (val > U16_MAX)
> + goto out;
> + c->u.port.low_port = val;
> +
> + val = le32_to_cpu(buf[2]);
> + if (val > U16_MAX)
> + goto out;
> + c->u.port.high_port = val;
> +
> + if (c->u.port.low_port > c->u.port.high_port)
> + goto out;
> +
> rc = context_read_and_validate(&c->context[0], p, fp);
> if (rc)
> goto out;
This entire block of bounds checking for protocols and ports should
be pulled out into its own patch, especially since it isn't mentioned
in the commit description.
--
paul-moore.com
