| Message ID | 20220217142133.72205-3-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [1/5] selinux: drop return statement at end of void functions | expand |
On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > The arrays for the policy capability names, the initial sid identifiers > and the class and permission names are not changed at runtime. Declare > them const to avoid accidental modification. > > The build time script genheaders needs to be exempted, since it converts > the entries to uppercase. > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > scripts/selinux/genheaders/genheaders.c | 2 ++ > scripts/selinux/mdp/mdp.c | 4 ++-- > security/selinux/avc.c | 2 +- > security/selinux/include/avc_ss.h | 2 +- > security/selinux/include/classmap.h | 8 +++++++- > security/selinux/include/initial_sid_to_string.h | 9 ++++++++- > security/selinux/include/policycap.h | 2 +- > security/selinux/include/policycap_names.h | 2 +- > security/selinux/ss/services.c | 4 ++-- > 9 files changed, 25 insertions(+), 10 deletions(-) ... > diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c > index f355b3e0e968..5f7c0b7d9260 100644 > --- a/scripts/selinux/genheaders/genheaders.c > +++ b/scripts/selinux/genheaders/genheaders.c > @@ -15,6 +15,8 @@ struct security_class_mapping { > const char *perms[sizeof(unsigned) * 8 + 1]; > }; > > +/* Allow to convert entries in mappings to uppercase */ > +#define __SELINUX_GENHEADERS__ > #include "classmap.h" > #include "initial_sid_to_string.h" ... > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h > index 35aac62a662e..07ade4af85ff 100644 > --- a/security/selinux/include/classmap.h > +++ b/security/selinux/include/classmap.h > @@ -2,6 +2,12 @@ > #include <linux/capability.h> > #include <linux/socket.h> > > +#ifdef __SELINUX_GENHEADERS__ > +# define const_qual > +#else > +# define const_qual const > +#endif > + > #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ > "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" > > @@ -38,7 +44,7 @@ > * Note: The name for any socket class should be suffixed by "socket", > * and doesn't contain more than one substr of "socket". > */ > -struct security_class_mapping secclass_map[] = { > +const_qual struct security_class_mapping secclass_map[] = { > { "security", > { "compute_av", "compute_create", "compute_member", > "check_context", "load_policy", "compute_relabel", ... > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > index 5d332aeb8b6c..915283cd89bd 100644 > --- a/security/selinux/include/initial_sid_to_string.h > +++ b/security/selinux/include/initial_sid_to_string.h > @@ -1,5 +1,12 @@ > /* SPDX-License-Identifier: GPL-2.0 */ > -static const char *initial_sid_to_string[] = > + > +#ifdef __SELINUX_GENHEADERS__ > +# define const_qual > +#else > +# define const_qual const > +#endif > + > +static const char *const_qual initial_sid_to_string[] = > { > NULL, > "kernel", Thanks for this Christian. I generally like when we can const'ify things like this, but I'm not excited about the const_qual hack on core SELinux kernel code to satisfy genheaders.c. I understand why it is needed, but I would rather clutter the genheaders.c code than the core SELinux kernel code. If we can't cast away the const'ification in genheaders.c could we simply allocate duplicate arrays in genheaders.c and store the transformed strings into the new arrays?
On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > > index 5d332aeb8b6c..915283cd89bd 100644 > > --- a/security/selinux/include/initial_sid_to_string.h > > +++ b/security/selinux/include/initial_sid_to_string.h > > @@ -1,5 +1,12 @@ > > /* SPDX-License-Identifier: GPL-2.0 */ > > -static const char *initial_sid_to_string[] = > > + > > +#ifdef __SELINUX_GENHEADERS__ > > +# define const_qual > > +#else > > +# define const_qual const > > +#endif > > + > > +static const char *const_qual initial_sid_to_string[] = > > { > > NULL, > > "kernel", > > Thanks for this Christian. I generally like when we can const'ify > things like this, but I'm not excited about the const_qual hack on > core SELinux kernel code to satisfy genheaders.c. I understand why it > is needed, but I would rather clutter the genheaders.c code than the > core SELinux kernel code. If we can't cast away the const'ification > in genheaders.c could we simply allocate duplicate arrays in > genheaders.c and store the transformed strings into the new arrays? Note: casting off const is UB. I've had to fix multiple bugs where clang will drop writes to variables declared const but had const'ness casted away.
On Fri, Feb 18, 2022 at 12:24 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > On Fri, Feb 18, 2022 at 8:13 AM Paul Moore <paul@paul-moore.com> wrote: > > On Thu, Feb 17, 2022 at 9:21 AM Christian Göttsche > > <cgzones@googlemail.com> wrote: > > > > > > diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h > > > index 5d332aeb8b6c..915283cd89bd 100644 > > > --- a/security/selinux/include/initial_sid_to_string.h > > > +++ b/security/selinux/include/initial_sid_to_string.h > > > @@ -1,5 +1,12 @@ > > > /* SPDX-License-Identifier: GPL-2.0 */ > > > -static const char *initial_sid_to_string[] = > > > + > > > +#ifdef __SELINUX_GENHEADERS__ > > > +# define const_qual > > > +#else > > > +# define const_qual const > > > +#endif > > > + > > > +static const char *const_qual initial_sid_to_string[] = > > > { > > > NULL, > > > "kernel", > > > > Thanks for this Christian. I generally like when we can const'ify > > things like this, but I'm not excited about the const_qual hack on > > core SELinux kernel code to satisfy genheaders.c. I understand why it > > is needed, but I would rather clutter the genheaders.c code than the > > core SELinux kernel code. If we can't cast away the const'ification > > in genheaders.c could we simply allocate duplicate arrays in > > genheaders.c and store the transformed strings into the new arrays? > > Note: casting off const is UB. I've had to fix multiple bugs where > clang will drop writes to variables declared const but had const'ness > casted away. Then let's just memcpy the array in genheaders.c. I'm okay with genheaders being a little ugly if it helps keep the core code cleaner.
diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c index f355b3e0e968..5f7c0b7d9260 100644 --- a/scripts/selinux/genheaders/genheaders.c +++ b/scripts/selinux/genheaders/genheaders.c @@ -15,6 +15,8 @@ struct security_class_mapping { const char *perms[sizeof(unsigned) * 8 + 1]; }; +/* Allow to convert entries in mappings to uppercase */ +#define __SELINUX_GENHEADERS__ #include "classmap.h" #include "initial_sid_to_string.h" diff --git a/scripts/selinux/mdp/mdp.c b/scripts/selinux/mdp/mdp.c index 105c1c31a316..1415604c3d24 100644 --- a/scripts/selinux/mdp/mdp.c +++ b/scripts/selinux/mdp/mdp.c @@ -82,7 +82,7 @@ int main(int argc, char *argv[]) /* print out the class permissions */ for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "class %s\n", map->name); fprintf(fout, "{\n"); for (j = 0; map->perms[j]; j++) @@ -103,7 +103,7 @@ int main(int argc, char *argv[]) #define SYSTEMLOW "s0" #define SYSTEMHIGH "s1:c0.c1" for (i = 0; secclass_map[i].name; i++) { - struct security_class_mapping *map = &secclass_map[i]; + const struct security_class_mapping *map = &secclass_map[i]; fprintf(fout, "mlsconstrain %s {\n", map->name); for (j = 0; map->perms[j]; j++) diff --git a/security/selinux/avc.c b/security/selinux/avc.c index abcd9740d10f..020985a53d8f 100644 --- a/security/selinux/avc.c +++ b/security/selinux/avc.c @@ -668,7 +668,7 @@ static void avc_audit_pre_callback(struct audit_buffer *ab, void *a) struct common_audit_data *ad = a; struct selinux_audit_data *sad = ad->selinux_audit_data; u32 av = sad->audited; - const char **perms; + const char *const *perms; int i, perm; audit_log_format(ab, "avc: %s ", sad->denied ? "denied" : "granted"); diff --git a/security/selinux/include/avc_ss.h b/security/selinux/include/avc_ss.h index 88c384c5c09e..b38974e22d81 100644 --- a/security/selinux/include/avc_ss.h +++ b/security/selinux/include/avc_ss.h @@ -18,7 +18,7 @@ struct security_class_mapping { const char *perms[sizeof(u32) * 8 + 1]; }; -extern struct security_class_mapping secclass_map[]; +extern const struct security_class_mapping secclass_map[]; #endif /* _SELINUX_AVC_SS_H_ */ diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 35aac62a662e..07ade4af85ff 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -2,6 +2,12 @@ #include <linux/capability.h> #include <linux/socket.h> +#ifdef __SELINUX_GENHEADERS__ +# define const_qual +#else +# define const_qual const +#endif + #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \ "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map" @@ -38,7 +44,7 @@ * Note: The name for any socket class should be suffixed by "socket", * and doesn't contain more than one substr of "socket". */ -struct security_class_mapping secclass_map[] = { +const_qual struct security_class_mapping secclass_map[] = { { "security", { "compute_av", "compute_create", "compute_member", "check_context", "load_policy", "compute_relabel", diff --git a/security/selinux/include/initial_sid_to_string.h b/security/selinux/include/initial_sid_to_string.h index 5d332aeb8b6c..915283cd89bd 100644 --- a/security/selinux/include/initial_sid_to_string.h +++ b/security/selinux/include/initial_sid_to_string.h @@ -1,5 +1,12 @@ /* SPDX-License-Identifier: GPL-2.0 */ -static const char *initial_sid_to_string[] = + +#ifdef __SELINUX_GENHEADERS__ +# define const_qual +#else +# define const_qual const +#endif + +static const char *const_qual initial_sid_to_string[] = { NULL, "kernel", diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h index 2ec038efbb03..3207a4e8c899 100644 --- a/security/selinux/include/policycap.h +++ b/security/selinux/include/policycap.h @@ -15,6 +15,6 @@ enum { }; #define POLICYDB_CAPABILITY_MAX (__POLICYDB_CAPABILITY_MAX - 1) -extern const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; +extern const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX]; #endif /* _SELINUX_POLICYCAP_H_ */ diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h index b89289f092c9..51da36e37d21 100644 --- a/security/selinux/include/policycap_names.h +++ b/security/selinux/include/policycap_names.h @@ -5,7 +5,7 @@ #include "policycap.h" /* Policy capability names */ -const char *selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { +const char *const selinux_policycap_names[__POLICYDB_CAPABILITY_MAX] = { "network_peer_controls", "open_perms", "extended_socket_class", diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 7865926962ab..25c287324059 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -99,7 +99,7 @@ static void context_struct_compute_av(struct policydb *policydb, struct extended_perms *xperms); static int selinux_set_mapping(struct policydb *pol, - struct security_class_mapping *map, + const struct security_class_mapping *map, struct selinux_map *out_map) { u16 i, j; @@ -121,7 +121,7 @@ static int selinux_set_mapping(struct policydb *pol, /* Store the raw class and permission values */ j = 0; while (map[j].name) { - struct security_class_mapping *p_in = map + (j++); + const struct security_class_mapping *p_in = map + (j++); struct selinux_mapping *p_out = out_map->mapping + j; /* An empty class string skips ahead */
The arrays for the policy capability names, the initial sid identifiers and the class and permission names are not changed at runtime. Declare them const to avoid accidental modification. The build time script genheaders needs to be exempted, since it converts the entries to uppercase. Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- scripts/selinux/genheaders/genheaders.c | 2 ++ scripts/selinux/mdp/mdp.c | 4 ++-- security/selinux/avc.c | 2 +- security/selinux/include/avc_ss.h | 2 +- security/selinux/include/classmap.h | 8 +++++++- security/selinux/include/initial_sid_to_string.h | 9 ++++++++- security/selinux/include/policycap.h | 2 +- security/selinux/include/policycap_names.h | 2 +- security/selinux/ss/services.c | 4 ++-- 9 files changed, 25 insertions(+), 10 deletions(-)