Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 03 Aug 2023 22:20:22 -0400
Message-ID: <d3b3791b6d71b7ee6f0a020ee9280e2d.paul@paul-moore.com>
From:   Paul Moore <paul@paul-moore.com>
To:     =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com>,
        selinux@vger.kernel.org
Cc:     Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>,
        Ondrej Mosnacek <omosnace@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 8/9] selinux: policydb: implicit conversions
References: <20230728155501.39632-7-cgzones@googlemail.com>
In-Reply-To: <20230728155501.39632-7-cgzones@googlemail.com>
Precedence: bulk

On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> 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@googlemail.com>
> ---
> 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