| Message ID | 20230706132337.15924-7-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC,01/20] selinux: check for multiplication overflow in put_entry() | expand |
Hi Christian, First of all, there's a typo in the subject: umber -> number ;) On 2023/07/06 21:23, Christian Göttsche wrote: > Security classes have only up to 32 permissions, hence using an u16 is > sufficient (while improving padding). > > Also use a fixed sized cast in a bit shift to work correctly on > architectures where sizeof(unsigned int) != sizeof(u32). > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > 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 78946b71c1c1..3275cfe2c8f7 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); Is it really necessary to do explicit conversion here? Its value is known to be small, and IIUC, u16 will be implicitly promoted to int as an operand of <<, as described here: https://en.cppreference.com/w/c/language/conversion#Integer_promotions > avd->auditdeny = result; > } > } > diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h > index 8a9b85f44b66..b6f99353301e 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 6, 2023 Gong Ruiqi <gongruiqi1@huawei.com> wrote: > > Security classes have only up to 32 permissions, hence using an u16 is > sufficient (while improving padding). Can you explain the improved padding comment? It looks like you are only changing the iterator's type so the struct should remain unchanged, and FWIW, it looks like security_class_wrapping allocates space for 33 permission strings. > Also use a fixed sized cast in a bit shift to work correctly on > architectures where sizeof(unsigned int) != sizeof(u32). > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > 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 78946b71c1c1..3275cfe2c8f7 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 8a9b85f44b66..b6f99353301e 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 -- paul-moore.com
On 2023/07/19 6:01, Paul Moore wrote: > On Jul 6, 2023 Gong Ruiqi <gongruiqi1@huawei.com> wrote: >> >> Security classes have only up to 32 permissions, hence using an u16 is >> sufficient (while improving padding). > > Can you explain the improved padding comment? I think what Christian means is that struct selinux_mapping will occupy less memory since num_perms is changed from uint (32 bits) to u16, which saves 16 bits of space due to padding. > [...] > >> diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h >> index 8a9b85f44b66..b6f99353301e 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 */ >> }; Check here. >> >> -- >> 2.40.1 > > -- > paul-moore.com
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 78946b71c1c1..3275cfe2c8f7 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 8a9b85f44b66..b6f99353301e 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). Also use a fixed sized cast in a bit shift to work correctly on architectures where sizeof(unsigned int) != sizeof(u32). Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- security/selinux/ss/services.c | 6 +++--- security/selinux/ss/services.h | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-)