The extensible bitmap supports bit positions up to U32_MAX due to the
type of the member highbit being u32. Use u32 consistently as the type
for bit positions to announce to callers what range of values is
supported.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2: avoid declarations in init-clauses of for loops
---
security/selinux/ss/ebitmap.c | 32 ++++++++++++++++----------------
security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
2 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
index 77875ad355f7..6ab2baf4cfb5 100644
--- a/security/selinux/ss/ebitmap.c
+++ b/security/selinux/ss/ebitmap.c
@@ -24,7 +24,7 @@
#include "ebitmap.h"
#include "policydb.h"
-#define BITS_PER_U64 (sizeof(u64) * 8)
+#define BITS_PER_U64 ((u32)(sizeof(u64) * 8))
static struct kmem_cache *ebitmap_node_cachep __ro_after_init;
@@ -82,7 +82,8 @@ int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src)
int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2)
{
struct ebitmap_node *n;
- int bit, rc;
+ u32 bit;
+ int rc;
ebitmap_init(dst);
@@ -113,8 +114,7 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
{
struct ebitmap_node *e_iter = ebmap->node;
unsigned long e_map;
- u32 offset;
- unsigned int iter;
+ u32 offset, iter;
int rc;
if (e_iter == NULL) {
@@ -259,7 +259,7 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 las
return 1;
}
-int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
+int ebitmap_get_bit(const struct ebitmap *e, u32 bit)
{
const struct ebitmap_node *n;
@@ -276,7 +276,7 @@ int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
return 0;
}
-int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
+int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value)
{
struct ebitmap_node *n, *prev, *new;
@@ -287,7 +287,7 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
if (value) {
ebitmap_node_set_bit(n, bit);
} else {
- unsigned int s;
+ u32 s;
ebitmap_node_clr_bit(n, bit);
@@ -365,12 +365,12 @@ void ebitmap_destroy(struct ebitmap *e)
int ebitmap_read(struct ebitmap *e, void *fp)
{
struct ebitmap_node *n = NULL;
- u32 mapunit, count, startbit, index;
+ u32 mapunit, count, startbit, index, i;
__le32 ebitmap_start;
u64 map;
__le64 mapbits;
__le32 buf[3];
- int rc, i;
+ int rc;
ebitmap_init(e);
@@ -384,7 +384,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
if (mapunit != BITS_PER_U64) {
pr_err("SELinux: ebitmap: map size %u does not "
- "match my size %zd (high bit was %d)\n",
+ "match my size %d (high bit was %d)\n",
mapunit, BITS_PER_U64, e->highbit);
goto bad;
}
@@ -471,18 +471,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
int ebitmap_write(const struct ebitmap *e, void *fp)
{
struct ebitmap_node *n;
- u32 count;
+ u32 bit, count, last_bit, last_startbit;
__le32 buf[3];
u64 map;
- int bit, last_bit, last_startbit, rc;
+ int rc;
buf[0] = cpu_to_le32(BITS_PER_U64);
count = 0;
last_bit = 0;
- last_startbit = -1;
+ last_startbit = (u32)-1;
ebitmap_for_each_positive_bit(e, n, bit) {
- if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
+ if (last_startbit == (u32)-1 || rounddown(bit, BITS_PER_U64) > last_startbit) {
count++;
last_startbit = rounddown(bit, BITS_PER_U64);
}
@@ -496,9 +496,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
return rc;
map = 0;
- last_startbit = INT_MIN;
+ last_startbit = (u32)-1;
ebitmap_for_each_positive_bit(e, n, bit) {
- if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
+ if (last_startbit == (u32)-1 || rounddown(bit, BITS_PER_U64) > last_startbit) {
__le64 buf64[1];
/* this is the very first bit */
diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h
index e3c807cfad90..43c32077d483 100644
--- a/security/selinux/ss/ebitmap.h
+++ b/security/selinux/ss/ebitmap.h
@@ -44,10 +44,10 @@ struct ebitmap {
#define ebitmap_length(e) ((e)->highbit)
-static inline unsigned int ebitmap_start_positive(const struct ebitmap *e,
+static inline u32 ebitmap_start_positive(const struct ebitmap *e,
struct ebitmap_node **n)
{
- unsigned int ofs;
+ u32 ofs;
for (*n = e->node; *n; *n = (*n)->next) {
ofs = find_first_bit((*n)->maps, EBITMAP_SIZE);
@@ -62,11 +62,11 @@ static inline void ebitmap_init(struct ebitmap *e)
memset(e, 0, sizeof(*e));
}
-static inline unsigned int ebitmap_next_positive(const struct ebitmap *e,
+static inline u32 ebitmap_next_positive(const struct ebitmap *e,
struct ebitmap_node **n,
- unsigned int bit)
+ u32 bit)
{
- unsigned int ofs;
+ u32 ofs;
ofs = find_next_bit((*n)->maps, EBITMAP_SIZE, bit - (*n)->startbit + 1);
if (ofs < EBITMAP_SIZE)
@@ -86,10 +86,10 @@ static inline unsigned int ebitmap_next_positive(const struct ebitmap *e,
(((bit) - (node)->startbit) % EBITMAP_UNIT_SIZE)
static inline int ebitmap_node_get_bit(const struct ebitmap_node *n,
- unsigned int bit)
+ u32 bit)
{
- unsigned int index = EBITMAP_NODE_INDEX(n, bit);
- unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+ u32 index = EBITMAP_NODE_INDEX(n, bit);
+ u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
BUG_ON(index >= EBITMAP_UNIT_NUMS);
if ((n->maps[index] & (EBITMAP_BIT << ofs)))
@@ -98,20 +98,20 @@ static inline int ebitmap_node_get_bit(const struct ebitmap_node *n,
}
static inline void ebitmap_node_set_bit(struct ebitmap_node *n,
- unsigned int bit)
+ u32 bit)
{
- unsigned int index = EBITMAP_NODE_INDEX(n, bit);
- unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+ u32 index = EBITMAP_NODE_INDEX(n, bit);
+ u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
BUG_ON(index >= EBITMAP_UNIT_NUMS);
n->maps[index] |= (EBITMAP_BIT << ofs);
}
static inline void ebitmap_node_clr_bit(struct ebitmap_node *n,
- unsigned int bit)
+ u32 bit)
{
- unsigned int index = EBITMAP_NODE_INDEX(n, bit);
- unsigned int ofs = EBITMAP_NODE_OFFSET(n, bit);
+ u32 index = EBITMAP_NODE_INDEX(n, bit);
+ u32 ofs = EBITMAP_NODE_OFFSET(n, bit);
BUG_ON(index >= EBITMAP_UNIT_NUMS);
n->maps[index] &= ~(EBITMAP_BIT << ofs);
@@ -126,8 +126,8 @@ int ebitmap_cmp(const struct ebitmap *e1, const struct ebitmap *e2);
int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src);
int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2);
int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 last_e2bit);
-int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit);
-int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value);
+int ebitmap_get_bit(const struct ebitmap *e, u32 bit);
+int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value);
void ebitmap_destroy(struct ebitmap *e);
int ebitmap_read(struct ebitmap *e, void *fp);
int ebitmap_write(const struct ebitmap *e, void *fp);
--
2.40.1
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]>
---
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;
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)
{
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;
struct role_allow *ra, *lra = NULL;
for (i = 0; i < SYM_NUM; i++) {
@@ -1154,8 +1154,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)
@@ -1220,13 +1220,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++) {
@@ -1318,8 +1318,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)
@@ -1412,7 +1412,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;
@@ -1468,7 +1469,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;
@@ -1542,7 +1544,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;
@@ -1683,7 +1686,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: "
@@ -1719,7 +1722,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: "
@@ -1834,9 +1837,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;
@@ -2082,9 +2085,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;
@@ -2123,8 +2126,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;
@@ -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;
@@ -2429,9 +2451,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;
@@ -3282,7 +3304,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];
@@ -3631,10 +3654,10 @@ static int filename_trans_write(struct policydb *p, void *fp)
*/
int policydb_write(struct policydb *p, void *fp)
{
- unsigned int i, num_syms;
int rc;
+ unsigned int num_syms;
__le32 buf[4];
- u32 config;
+ u32 config, i;
size_t len;
const struct policydb_compat_info *info;
--
2.40.1
Use an unsigned type as loop iterator.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2: avoid declarations in init-clauses of for loops
---
security/selinux/nlmsgtab.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
index 2ee7b4ed43ef..2f8fab949633 100644
--- a/security/selinux/nlmsgtab.c
+++ b/security/selinux/nlmsgtab.c
@@ -153,7 +153,8 @@ static const struct nlmsg_perm nlmsg_audit_perms[] = {
static int nlmsg_perm(u16 nlmsg_type, u32 *perm, const struct nlmsg_perm *tab, size_t tabsize)
{
- int i, err = -EINVAL;
+ u32 i;
+ int err = -EINVAL;
for (i = 0; i < tabsize/sizeof(struct nlmsg_perm); i++)
if (nlmsg_type == tab[i].nlmsg_type) {
--
2.40.1
Return u32 from avtab_hash() instead of int, since the hashing is done
on u32 and the result is used as an index on the hash array.
Use the type of the limit in for loops.
Avoid signed to unsigned conversion of multiplication result in
avtab_hash_eval().
Use unsigned loop iterator for index operations, to avoid sign
extension.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2: avoid declarations in init-clauses of for loops
---
security/selinux/ss/avtab.c | 23 ++++++++++++-----------
1 file changed, 12 insertions(+), 11 deletions(-)
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 32f92da00b0e..8a508018e696 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -29,7 +29,7 @@ static struct kmem_cache *avtab_xperms_cachep __ro_after_init;
/* Based on MurmurHash3, written by Austin Appleby and placed in the
* public domain.
*/
-static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
+static inline u32 avtab_hash(const struct avtab_key *keyp, u32 mask)
{
static const u32 c1 = 0xcc9e2d51;
static const u32 c2 = 0x1b873593;
@@ -66,7 +66,7 @@ static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
}
static struct avtab_node*
-avtab_insert_node(struct avtab *h, int hvalue,
+avtab_insert_node(struct avtab *h, u32 hvalue,
struct avtab_node *prev,
const struct avtab_key *key, const struct avtab_datum *datum)
{
@@ -106,7 +106,7 @@ avtab_insert_node(struct avtab *h, int hvalue,
static int avtab_insert(struct avtab *h, const struct avtab_key *key,
const struct avtab_datum *datum)
{
- int hvalue;
+ u32 hvalue;
struct avtab_node *prev, *cur, *newnode;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
@@ -152,7 +152,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
const struct avtab_key *key,
const struct avtab_datum *datum)
{
- int hvalue;
+ u32 hvalue;
struct avtab_node *prev, *cur;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
@@ -186,7 +186,7 @@ struct avtab_node *avtab_insert_nonunique(struct avtab *h,
struct avtab_node *avtab_search_node(struct avtab *h,
const struct avtab_key *key)
{
- int hvalue;
+ u32 hvalue;
struct avtab_node *cur;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
@@ -246,7 +246,7 @@ avtab_search_node_next(struct avtab_node *node, u16 specified)
void avtab_destroy(struct avtab *h)
{
- int i;
+ u32 i;
struct avtab_node *cur, *temp;
if (!h)
@@ -324,7 +324,8 @@ int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
void avtab_hash_eval(struct avtab *h, const char *tag)
{
- int i, chain_len, slots_used, max_chain_len;
+ u32 i;
+ unsigned int chain_len, slots_used, max_chain_len;
unsigned long long chain2_len_sum;
struct avtab_node *cur;
@@ -372,13 +373,13 @@ int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
{
__le16 buf16[4];
u16 enabled;
- u32 items, items2, val, vers = pol->policyvers;
+ u32 items, items2, val, i;
struct avtab_key key;
struct avtab_datum datum;
struct avtab_extended_perms xperms;
__le32 buf32[ARRAY_SIZE(xperms.perms.p)];
- int i, rc;
- unsigned set;
+ int rc;
+ unsigned int set, vers = pol->policyvers;
memset(&key, 0, sizeof(struct avtab_key));
memset(&datum, 0, sizeof(struct avtab_datum));
@@ -614,7 +615,7 @@ int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
int avtab_write(struct policydb *p, struct avtab *a, void *fp)
{
- unsigned int i;
+ u32 i;
int rc = 0;
struct avtab_node *cur;
__le32 buf[1];
--
2.40.1
Use u32 as the output parameter type in security_get_classes() and
security_get_permissions(), based on the type of the symtab nprim
member.
Declare the read-only class string parameter of
security_get_permissions() const.
Avoid several implicit conversions by using the identical type for the
destination.
Use the type identical to the source for local variables.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2: avoid declarations in init-clauses of for loops
---
security/selinux/include/security.h | 4 ++--
security/selinux/selinuxfs.c | 7 ++++---
security/selinux/ss/services.c | 23 ++++++++++++-----------
3 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 668e393a9709..074d439fe9ad 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -312,9 +312,9 @@ int security_net_peersid_resolve(u32 nlbl_sid, u32 nlbl_type,
u32 *peer_sid);
int security_get_classes(struct selinux_policy *policy,
- char ***classes, int *nclasses);
+ char ***classes, u32 *nclasses);
int security_get_permissions(struct selinux_policy *policy,
- char *class, char ***perms, int *nperms);
+ const char *class, char ***perms, u32 *nperms);
int security_get_reject_unknown(void);
int security_get_allow_unknown(void);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index f79e96f0f221..b969e87fd870 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1798,7 +1798,8 @@ static int sel_make_perm_files(struct selinux_policy *newpolicy,
char *objclass, int classvalue,
struct dentry *dir)
{
- int i, rc, nperms;
+ u32 i, nperms;
+ int rc;
char **perms;
rc = security_get_permissions(newpolicy, objclass, &perms, &nperms);
@@ -1868,8 +1869,8 @@ static int sel_make_classes(struct selinux_policy *newpolicy,
struct dentry *class_dir,
unsigned long *last_class_ino)
{
-
- int rc, nclasses, i;
+ u32 i, nclasses;
+ int rc;
char **classes;
rc = security_get_classes(newpolicy, &classes, &nclasses);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index cf4b87ec4a0e..3a03243f52e7 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -856,7 +856,7 @@ int security_bounded_transition(u32 old_sid, u32 new_sid)
struct sidtab *sidtab;
struct sidtab_entry *old_entry, *new_entry;
struct type_datum *type;
- int index;
+ u32 index;
int rc;
if (!selinux_initialized())
@@ -1511,7 +1511,7 @@ static int security_context_to_sid_core(const char *scontext, u32 scontext_len,
return -ENOMEM;
if (!selinux_initialized()) {
- int i;
+ u32 i;
for (i = 1; i < SECINITSID_NUM; i++) {
const char *s = initial_sid_to_string[i];
@@ -2821,7 +2821,6 @@ static inline int __security_genfs_sid(struct selinux_policy *policy,
{
struct policydb *policydb = &policy->policydb;
struct sidtab *sidtab = policy->sidtab;
- int len;
u16 sclass;
struct genfs *genfs;
struct ocontext *c;
@@ -2843,7 +2842,7 @@ static inline int __security_genfs_sid(struct selinux_policy *policy,
return -ENOENT;
for (c = genfs->head; c; c = c->next) {
- len = strlen(c->u.name);
+ size_t len = strlen(c->u.name);
if ((!c->v.sclass || sclass == c->v.sclass) &&
(strncmp(c->u.name, path, len) == 0))
break;
@@ -3331,7 +3330,7 @@ static int get_classes_callback(void *k, void *d, void *args)
{
struct class_datum *datum = d;
char *name = k, **classes = args;
- int value = datum->value - 1;
+ u32 value = datum->value - 1;
classes[value] = kstrdup(name, GFP_ATOMIC);
if (!classes[value])
@@ -3341,7 +3340,7 @@ static int get_classes_callback(void *k, void *d, void *args)
}
int security_get_classes(struct selinux_policy *policy,
- char ***classes, int *nclasses)
+ char ***classes, u32 *nclasses)
{
struct policydb *policydb;
int rc;
@@ -3357,7 +3356,8 @@ int security_get_classes(struct selinux_policy *policy,
rc = hashtab_map(&policydb->p_classes.table, get_classes_callback,
*classes);
if (rc) {
- int i;
+ u32 i;
+
for (i = 0; i < *nclasses; i++)
kfree((*classes)[i]);
kfree(*classes);
@@ -3371,7 +3371,7 @@ static int get_permissions_callback(void *k, void *d, void *args)
{
struct perm_datum *datum = d;
char *name = k, **perms = args;
- int value = datum->value - 1;
+ u32 value = datum->value - 1;
perms[value] = kstrdup(name, GFP_ATOMIC);
if (!perms[value])
@@ -3381,10 +3381,11 @@ static int get_permissions_callback(void *k, void *d, void *args)
}
int security_get_permissions(struct selinux_policy *policy,
- char *class, char ***perms, int *nperms)
+ const char *class, char ***perms, u32 *nperms)
{
struct policydb *policydb;
- int rc, i;
+ u32 i;
+ int rc;
struct class_datum *match;
policydb = &policy->policydb;
@@ -3599,7 +3600,7 @@ int selinux_audit_rule_init(u32 field, u32 op, char *rulestr, void **vrule)
/* Check to see if the rule contains any selinux fields */
int selinux_audit_rule_known(struct audit_krule *rule)
{
- int i;
+ u32 i;
for (i = 0; i < rule->field_count; i++) {
struct audit_field *f = &rule->fields[i];
--
2.40.1
Security classes have only up to 32 permissions, hence using an u16 is
sufficient (while improving padding in struct selinux_mapping).
Also use a fixed sized cast in a bit shift to avoid (well defined)
overflows on architectures where sizeof(unsigned int) != sizeof(u32)
resulting in no bits set.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2:
update commit description:
- mention struct selinux_mapping in the padding argument
(currently between the first and second member there are 2 bytes
padding)
- mention overflow in the cast argument and the result of setting
no bits due to it
---
security/selinux/ss/services.c | 6 +++---
security/selinux/ss/services.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2c5be06fbada..cf4b87ec4a0e 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -97,7 +97,6 @@ static int selinux_set_mapping(struct policydb *pol,
struct selinux_map *out_map)
{
u16 i, j;
- unsigned k;
bool print_unknown_handle = false;
/* Find number of classes in the input mapping */
@@ -117,6 +116,7 @@ static int selinux_set_mapping(struct policydb *pol,
while (map[j].name) {
const struct security_class_mapping *p_in = map + (j++);
struct selinux_mapping *p_out = out_map->mapping + j;
+ u16 k;
/* An empty class string skips ahead */
if (!strcmp(p_in->name, "")) {
@@ -202,7 +202,7 @@ static void map_decision(struct selinux_map *map,
{
if (tclass < map->size) {
struct selinux_mapping *mapping = &map->mapping[tclass];
- unsigned int i, n = mapping->num_perms;
+ u16 i, n = mapping->num_perms;
u32 result;
for (i = 0, result = 0; i < n; i++) {
@@ -230,7 +230,7 @@ static void map_decision(struct selinux_map *map,
* should audit that denial
*/
for (; i < (sizeof(u32)*8); i++)
- result |= 1<<i;
+ result |= 1<<((u32)i);
avd->auditdeny = result;
}
}
diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h
index ed2ee6600467..d24b0a3d198e 100644
--- a/security/selinux/ss/services.h
+++ b/security/selinux/ss/services.h
@@ -12,7 +12,7 @@
/* Mapping for a single class */
struct selinux_mapping {
u16 value; /* policy value for class */
- unsigned int num_perms; /* number of permissions in class */
+ u16 num_perms; /* number of permissions in class */
u32 perms[sizeof(u32) * 8]; /* policy values for permissions */
};
--
2.40.1
Use the identical type u32 for the loop iterator.
Signed-off-by: Christian Göttsche <[email protected]>
---
v2: avoid declarations in init-clauses of for loops
---
security/selinux/ss/hashtab.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/selinux/ss/hashtab.c b/security/selinux/ss/hashtab.c
index 30532ec319ce..7df9640554be 100644
--- a/security/selinux/ss/hashtab.c
+++ b/security/selinux/ss/hashtab.c
@@ -137,7 +137,8 @@ int hashtab_duplicate(struct hashtab *new, struct hashtab *orig,
void *args)
{
struct hashtab_node *cur, *tmp, *tail;
- int i, rc;
+ u32 i;
+ int rc;
memset(new, 0, sizeof(*new));
--
2.40.1
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> Use u32 as the output parameter type in security_get_classes() and
> security_get_permissions(), based on the type of the symtab nprim
> member.
>
> Declare the read-only class string parameter of
> security_get_permissions() const.
>
> Avoid several implicit conversions by using the identical type for the
> destination.
>
> Use the type identical to the source for local variables.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/include/security.h | 4 ++--
> security/selinux/selinuxfs.c | 7 ++++---
> security/selinux/ss/services.c | 23 ++++++++++++-----------
> 3 files changed, 18 insertions(+), 16 deletions(-)
Merged into selinux/next, thanks.
--
paul-moore.com
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> Security classes have only up to 32 permissions, hence using an u16 is
> sufficient (while improving padding in struct selinux_mapping).
>
> Also use a fixed sized cast in a bit shift to avoid (well defined)
> overflows on architectures where sizeof(unsigned int) != sizeof(u32)
> resulting in no bits set.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2:
> update commit description:
> - mention struct selinux_mapping in the padding argument
> (currently between the first and second member there are 2 bytes
> padding)
> - mention overflow in the cast argument and the result of setting
> no bits due to it
> ---
> security/selinux/ss/services.c | 6 +++---
> security/selinux/ss/services.h | 2 +-
> 2 files changed, 4 insertions(+), 4 deletions(-)
This looks good, I would just like to request one small change
(see below).
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 2c5be06fbada..cf4b87ec4a0e 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -97,7 +97,6 @@ static int selinux_set_mapping(struct policydb *pol,
> struct selinux_map *out_map)
> {
> u16 i, j;
> - unsigned k;
> bool print_unknown_handle = false;
>
> /* Find number of classes in the input mapping */
> @@ -117,6 +116,7 @@ static int selinux_set_mapping(struct policydb *pol,
> while (map[j].name) {
> const struct security_class_mapping *p_in = map + (j++);
> struct selinux_mapping *p_out = out_map->mapping + j;
> + u16 k;
>
> /* An empty class string skips ahead */
> if (!strcmp(p_in->name, "")) {
> @@ -202,7 +202,7 @@ static void map_decision(struct selinux_map *map,
> {
> if (tclass < map->size) {
> struct selinux_mapping *mapping = &map->mapping[tclass];
> - unsigned int i, n = mapping->num_perms;
> + u16 i, n = mapping->num_perms;
> u32 result;
>
> for (i = 0, result = 0; i < n; i++) {
> @@ -230,7 +230,7 @@ static void map_decision(struct selinux_map *map,
> * should audit that denial
> */
> for (; i < (sizeof(u32)*8); i++)
> - result |= 1<<i;
> + result |= 1<<((u32)i);
Given that the for-loop bounds the value of 'i' to a maximum of 32
(31 within the valid portion of the loop), this cast seems
unnecessary and potentially problematic in the future. Please drop
this casting.
> avd->auditdeny = result;
> }
> }
--
paul-moore.com
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> Use the identical type u32 for the loop iterator.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/ss/hashtab.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
Merged into selinux/next, thanks.
--
paul-moore.com
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> The extensible bitmap supports bit positions up to U32_MAX due to the
> type of the member highbit being u32. Use u32 consistently as the type
> for bit positions to announce to callers what range of values is
> supported.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/ss/ebitmap.c | 32 ++++++++++++++++----------------
> security/selinux/ss/ebitmap.h | 32 ++++++++++++++++----------------
> 2 files changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c
> index 77875ad355f7..6ab2baf4cfb5 100644
> --- a/security/selinux/ss/ebitmap.c
> +++ b/security/selinux/ss/ebitmap.c
> @@ -24,7 +24,7 @@
> #include "ebitmap.h"
> #include "policydb.h"
>
> -#define BITS_PER_U64 (sizeof(u64) * 8)
> +#define BITS_PER_U64 ((u32)(sizeof(u64) * 8))
>
> static struct kmem_cache *ebitmap_node_cachep __ro_after_init;
>
> @@ -82,7 +82,8 @@ int ebitmap_cpy(struct ebitmap *dst, const struct ebitmap *src)
> int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2)
> {
> struct ebitmap_node *n;
> - int bit, rc;
> + u32 bit;
> + int rc;
>
> ebitmap_init(dst);
>
> @@ -113,8 +114,7 @@ int ebitmap_netlbl_export(struct ebitmap *ebmap,
> {
> struct ebitmap_node *e_iter = ebmap->node;
> unsigned long e_map;
> - u32 offset;
> - unsigned int iter;
> + u32 offset, iter;
> int rc;
In this function 'iter' is used to iterate through ebitmap_node::maps
and it thus only indirectly related to an ebitmap spot/offset.
I don't think this change harms anything, but it isn't strictly
necessary.
> if (e_iter == NULL) {
> @@ -259,7 +259,7 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 las
> return 1;
> }
>
> -int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
> +int ebitmap_get_bit(const struct ebitmap *e, u32 bit)
> {
> const struct ebitmap_node *n;
>
> @@ -276,7 +276,7 @@ int ebitmap_get_bit(const struct ebitmap *e, unsigned long bit)
> return 0;
> }
>
> -int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
> +int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value)
> {
> struct ebitmap_node *n, *prev, *new;
>
> @@ -287,7 +287,7 @@ int ebitmap_set_bit(struct ebitmap *e, unsigned long bit, int value)
> if (value) {
> ebitmap_node_set_bit(n, bit);
> } else {
> - unsigned int s;
> + u32 s;
>
> ebitmap_node_clr_bit(n, bit);
>
> @@ -365,12 +365,12 @@ void ebitmap_destroy(struct ebitmap *e)
> int ebitmap_read(struct ebitmap *e, void *fp)
> {
> struct ebitmap_node *n = NULL;
> - u32 mapunit, count, startbit, index;
> + u32 mapunit, count, startbit, index, i;
> __le32 ebitmap_start;
> u64 map;
> __le64 mapbits;
> __le32 buf[3];
> - int rc, i;
> + int rc;
>
> ebitmap_init(e);
>
> @@ -384,7 +384,7 @@ int ebitmap_read(struct ebitmap *e, void *fp)
>
> if (mapunit != BITS_PER_U64) {
> pr_err("SELinux: ebitmap: map size %u does not "
> - "match my size %zd (high bit was %d)\n",
> + "match my size %d (high bit was %d)\n",
> mapunit, BITS_PER_U64, e->highbit);
> goto bad;
> }
> @@ -471,18 +471,18 @@ int ebitmap_read(struct ebitmap *e, void *fp)
> int ebitmap_write(const struct ebitmap *e, void *fp)
> {
> struct ebitmap_node *n;
> - u32 count;
> + u32 bit, count, last_bit, last_startbit;
> __le32 buf[3];
> u64 map;
> - int bit, last_bit, last_startbit, rc;
> + int rc;
>
> buf[0] = cpu_to_le32(BITS_PER_U64);
>
> count = 0;
> last_bit = 0;
> - last_startbit = -1;
> + last_startbit = (u32)-1;
I can't say I'm as current on all of the C standards and compilier
oddities as some other in the Linux kernel space, but my
understanding is that on assignment the right value is always
implicitly type cast to the type of the left variable, is that not
true? Assuming it is true, I think this explicit cast isn't
necessary and could actually be harmful if we need to change the
ebitmap types in the future.
> ebitmap_for_each_positive_bit(e, n, bit) {
> - if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> + if (last_startbit == (u32)-1 || rounddown(bit, BITS_PER_U64) > last_startbit) {
This is a little more challenging as I know the rules for integer
comparisons are not quite as simple as assignments, but I do question
if the above change is an improvement.
One possibility would be to explicitly match the types, for example:
x == (typeof(x))-1
> count++;
> last_startbit = rounddown(bit, BITS_PER_U64);
> }
> @@ -496,9 +496,9 @@ int ebitmap_write(const struct ebitmap *e, void *fp)
> return rc;
>
> map = 0;
> - last_startbit = INT_MIN;
> + last_startbit = (u32)-1;
> ebitmap_for_each_positive_bit(e, n, bit) {
> - if (rounddown(bit, (int)BITS_PER_U64) > last_startbit) {
> + if (last_startbit == (u32)-1 || rounddown(bit, BITS_PER_U64) > last_startbit) {
> __le64 buf64[1];
Both of these changes are discussed above.
> /* this is the very first bit */
--
paul-moore.com
On Jul 28, 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]>
> ---
> 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
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> Return u32 from avtab_hash() instead of int, since the hashing is done
> on u32 and the result is used as an index on the hash array.
>
> Use the type of the limit in for loops.
>
> Avoid signed to unsigned conversion of multiplication result in
> avtab_hash_eval().
>
> Use unsigned loop iterator for index operations, to avoid sign
> extension.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/ss/avtab.c | 23 ++++++++++++-----------
> 1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 32f92da00b0e..8a508018e696 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
...
> @@ -324,7 +324,8 @@ int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
>
> void avtab_hash_eval(struct avtab *h, const char *tag)
> {
> - int i, chain_len, slots_used, max_chain_len;
> + u32 i;
> + unsigned int chain_len, slots_used, max_chain_len;
Since the total number of elements in the hash table and the number
of hash buckets/slots are both u32, it seems reasonable to me that
we would also want the 'chain_len', 'slots_used', and 'max_chain_len'
variables as u32, yes?
> unsigned long long chain2_len_sum;
> struct avtab_node *cur;
>
--
paul-moore.com
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
>
> Use an unsigned type as loop iterator.
>
> Signed-off-by: Christian Göttsche <[email protected]>
> ---
> v2: avoid declarations in init-clauses of for loops
> ---
> security/selinux/nlmsgtab.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/nlmsgtab.c b/security/selinux/nlmsgtab.c
> index 2ee7b4ed43ef..2f8fab949633 100644
> --- a/security/selinux/nlmsgtab.c
> +++ b/security/selinux/nlmsgtab.c
> @@ -153,7 +153,8 @@ static const struct nlmsg_perm nlmsg_audit_perms[] = {
>
> static int nlmsg_perm(u16 nlmsg_type, u32 *perm, const struct nlmsg_perm *tab, size_t tabsize)
> {
> - int i, err = -EINVAL;
> + u32 i;
> + int err = -EINVAL;
I understand wanting to make 'i' unsigned, but I think unsigned int
is a better fit for an iterator and array index.
> for (i = 0; i < tabsize/sizeof(struct nlmsg_perm); i++)
> if (nlmsg_type == tab[i].nlmsg_type) {
> --
> 2.40.1
--
paul-moore.com
From: Paul Moore
> Sent: 04 August 2023 03:20
>
> On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
....
> > + last_startbit = (u32)-1;
>
> I can't say I'm as current on all of the C standards and compilier
> oddities as some other in the Linux kernel space, but my
> understanding is that on assignment the right value is always
> implicitly type cast to the type of the left variable, is that not
> true? Assuming it is true, I think this explicit cast isn't
> necessary and could actually be harmful if we need to change the
> ebitmap types in the future.
The only question is where any required sign extend happens.
If you do:
u64 val = -1;
then the signed int is first sign extended to 64 bit and then
converted to unsigned (which just copies the bit pattern on any
sane system that Linux might run on).
Whereas:
u64 val = (u32)-1;
Converts an (assumed) 32bit -1 to unsigned and then zero extends it.
What you should really be using is a named constant that is
(for the current implementation) (~0u) and doesn't ever need
any casts and is always unsigned.
If you are actually worried about 'int' being other than 32bits
then there will be a lot more places that need fixing.
But you could use ((u32)~(u32)0) if you really want to allow
for 'u32' being both smaller and larger than 'int' and for
non 2's compliment (eg 1's compliment and sign overpunch)
systems.
(Good luck on finding a working C compiler for either of those.)
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Paul Moore
> Sent: 04 August 2023 03:20
>
> On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]> wrote:
> >
...
> > + u16 i, n = mapping->num_perms;
...
> > for (; i < (sizeof(u32)*8); i++)
Don't dop arithmetic on types smaller than int.
You are pretty much requesting the compiler add code to mask the
result down to 16 bits after very operations.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)