| Message ID | 20230728155501.39632-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [v2,1/9] selinux: avoid implicit conversions in avtab code | expand |
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> 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 <cgzones@googlemail.com> > --- > 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
From: Paul Moore > Sent: 04 August 2023 03:20 > > On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> 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)
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);
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 <cgzones@googlemail.com> --- 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(-)