| Message ID | 20230728155501.39632-4-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 2023/07/28 23:54, Christian Göttsche 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 <cgzones@googlemail.com> Reviewed-by: GONG, Ruiqi <gongruiqi1@huawei.com> > --- > 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 */ > }; >
On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> 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 <cgzones@googlemail.com> > --- > 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
From: Paul Moore > Sent: 04 August 2023 03:20 > > On Jul 28, 2023 =?UTF-8?q?Christian=20G=C3=B6ttsche?= <cgzones@googlemail.com> 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)
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 */ };
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 <cgzones@googlemail.com> --- 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(-)