2023-08-07 18:08:36

by Christian Göttsche

[permalink] [raw]
Subject: [PATCH v3 6/7] selinux: avoid implicit conversions in policydb code

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 <[email protected]>
---
v3:
- use unsigned int instead of u32 for iterators where the loop bound
is known at compile time and small (<100)
/@Paul: keep u32 iterator in policydb_destroy() due to
/ for (i = 0; i < p->p_types.nprim; i++)
/
- drop not mentioned protocol and port checks regarding out of range
values; there are a couple more of them and those changes are
suitable for a different patchset
v2:
- avoid declarations in init-clauses of for loops
- declare members of struct policydb_compat_info unsigned
---
security/selinux/ss/policydb.c | 69 ++++++++++++++++++----------------
1 file changed, 37 insertions(+), 32 deletions(-)

diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index a424997c79eb..c3ffe78ef144 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;
+ unsigned int i;

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, unsigned int i)
{
if (!c)
return;
@@ -782,7 +782,7 @@ void policydb_destroy(struct policydb *p)
{
struct ocontext *c, *ctmp;
struct genfs *g, *gtmp;
- int i;
+ u32 i;
struct role_allow *ra, *lra = NULL;

for (i = 0; i < SYM_NUM; i++) {
@@ -1155,8 +1155,8 @@ static int common_read(struct policydb *p, struct symtab *s, void *fp)
char *key = NULL;
struct common_datum *comdatum;
__le32 buf[4];
- u32 len, nel;
- int i, rc;
+ u32 i, len, nel;
+ int rc;

comdatum = kzalloc(sizeof(*comdatum), GFP_KERNEL);
if (!comdatum)
@@ -1221,13 +1221,13 @@ static int type_set_read(struct type_set *t, void *fp)

static int read_cons_helper(struct policydb *p,
struct constraint_node **nodep,
- int ncons, int allowxtarget, void *fp)
+ u32 ncons, int allowxtarget, void *fp)
{
struct constraint_node *c, *lc;
struct constraint_expr *e, *le;
__le32 buf[3];
- u32 nexpr;
- int rc, i, j, depth;
+ u32 i, j, nexpr;
+ int rc, depth;

lc = NULL;
for (i = 0; i < ncons; i++) {
@@ -1319,8 +1319,8 @@ static int class_read(struct policydb *p, struct symtab *s, void *fp)
char *key = NULL;
struct class_datum *cladatum;
__le32 buf[6];
- u32 len, len2, ncons, nel;
- int i, rc;
+ u32 i, len, len2, ncons, nel;
+ int rc;

cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL);
if (!cladatum)
@@ -1413,7 +1413,8 @@ static int role_read(struct policydb *p, struct symtab *s, void *fp)
{
char *key = NULL;
struct role_datum *role;
- int rc, to_read = 2;
+ int rc;
+ unsigned int to_read = 2;
__le32 buf[3];
u32 len;

@@ -1469,7 +1470,8 @@ static int type_read(struct policydb *p, struct symtab *s, void *fp)
{
char *key = NULL;
struct type_datum *typdatum;
- int rc, to_read = 3;
+ int rc;
+ unsigned int to_read = 3;
__le32 buf[4];
u32 len;

@@ -1543,7 +1545,8 @@ static int user_read(struct policydb *p, struct symtab *s, void *fp)
{
char *key = NULL;
struct user_datum *usrdatum;
- int rc, to_read = 2;
+ int rc;
+ unsigned int to_read = 2;
__le32 buf[3];
u32 len;

@@ -1684,7 +1687,7 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap)
upper = user = datum;
while (upper->bounds) {
struct ebitmap_node *node;
- unsigned long bit;
+ u32 bit;

if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
pr_err("SELinux: user %s: "
@@ -1720,7 +1723,7 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap)
upper = role = datum;
while (upper->bounds) {
struct ebitmap_node *node;
- unsigned long bit;
+ u32 bit;

if (++depth == POLICYDB_BOUNDS_MAXDEPTH) {
pr_err("SELinux: role %s: "
@@ -1835,9 +1838,9 @@ static int range_read(struct policydb *p, void *fp)
{
struct range_trans *rt = NULL;
struct mls_range *r = NULL;
- int i, rc;
+ int rc;
__le32 buf[2];
- u32 nel;
+ u32 i, nel;

if (p->policyvers < POLICYDB_VERSION_MLS)
return 0;
@@ -2083,9 +2086,9 @@ static int filename_trans_read_helper(struct policydb *p, void *fp)

static int filename_trans_read(struct policydb *p, void *fp)
{
- u32 nel;
+ u32 nel, i;
__le32 buf[1];
- int rc, i;
+ int rc;

if (p->policyvers < POLICYDB_VERSION_FILENAME_TRANS)
return 0;
@@ -2124,8 +2127,8 @@ static int filename_trans_read(struct policydb *p, void *fp)

static int genfs_read(struct policydb *p, void *fp)
{
- int i, j, rc;
- u32 nel, nel2, len, len2;
+ int rc;
+ u32 i, j, nel, nel2, len, len2;
__le32 buf[1];
struct ocontext *l, *c;
struct ocontext *newc = NULL;
@@ -2238,8 +2241,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;
__be64 prefixbuf[1];
__le32 buf[3];
struct ocontext *l, *c;
@@ -2430,9 +2434,9 @@ int policydb_read(struct policydb *p, void *fp)
struct role_allow *ra, *lra;
struct role_trans_key *rtk = NULL;
struct role_trans_datum *rtd = NULL;
- int i, j, rc;
+ int rc;
__le32 buf[4];
- u32 len, nprim, nel, perm;
+ u32 i, j, len, nprim, nel, perm;

char *policydb_str;
const struct policydb_compat_info *info;
@@ -3283,7 +3287,8 @@ static int (*const write_f[SYM_NUM]) (void *key, void *datum, void *datap) = {
static int ocontext_write(struct policydb *p, const struct policydb_compat_info *info,
void *fp)
{
- unsigned int i, j, rc;
+ unsigned int i, j;
+ int rc;
size_t nel, len;
__be64 prefixbuf[1];
__le32 buf[3];
@@ -3632,10 +3637,10 @@ static int filename_trans_write(struct policydb *p, void *fp)
*/
int policydb_write(struct policydb *p, void *fp)
{
- unsigned int i, num_syms;
+ unsigned int num_syms;
int rc;
__le32 buf[4];
- u32 config;
+ u32 config, i;
size_t len;
const struct policydb_compat_info *info;

--
2.40.1



2023-08-09 23:58:24

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v3 6/7] selinux: avoid implicit conversions in policydb code

On Aug 7, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> 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 <[email protected]>
> ---
> v3:
> - use unsigned int instead of u32 for iterators where the loop bound
> is known at compile time and small (<100)
> /@Paul: keep u32 iterator in policydb_destroy() due to
> / for (i = 0; i < p->p_types.nprim; i++)
> /
> - drop not mentioned protocol and port checks regarding out of range
> values; there are a couple more of them and those changes are
> suitable for a different patchset
> v2:
> - avoid declarations in init-clauses of for loops
> - declare members of struct policydb_compat_info unsigned
> ---
> security/selinux/ss/policydb.c | 69 ++++++++++++++++++----------------
> 1 file changed, 37 insertions(+), 32 deletions(-)

Merged into selinux/next, thanks.

--
paul-moore.com