diff mbox series

[v2,4/5] selinux: declare data arrays const

Message ID 20220308165527.45456-1-cgzones@googlemail.com (mailing list archive)
State Superseded
Delegated to: Paul Moore
Headers show
Series None | expand

Commit Message

Christian Göttsche March 8, 2022, 4:55 p.m. UTC
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.

Do not override the classmap and the initial sid list in the build time
script genheaders, by using a static buffer in the conversion function
stoupperx().  In cases we need to compare or print more than one
identifier allocate a temporary copy.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
   Drop const exemption for genheaders script by rewriting stoupperx().
---
 scripts/selinux/genheaders/genheaders.c       | 76 ++++++++++---------
 scripts/selinux/mdp/mdp.c                     |  4 +-
 security/selinux/avc.c                        |  2 +-
 security/selinux/include/avc_ss.h             |  2 +-
 security/selinux/include/classmap.h           |  2 +-
 .../selinux/include/initial_sid_to_string.h   |  3 +-
 security/selinux/include/policycap.h          |  2 +-
 security/selinux/include/policycap_names.h    |  2 +-
 security/selinux/ss/services.c                |  4 +-
 9 files changed, 51 insertions(+), 46 deletions(-)

Comments

Paul Moore April 4, 2022, 8:03 p.m. UTC | #1
On Tue, Mar 8, 2022 at 11:55 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.
>
> Do not override the classmap and the initial sid list in the build time
> script genheaders, by using a static buffer in the conversion function
> stoupperx().  In cases we need to compare or print more than one
> identifier allocate a temporary copy.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
> v2:
>    Drop const exemption for genheaders script by rewriting stoupperx().
> ---
>  scripts/selinux/genheaders/genheaders.c       | 76 ++++++++++---------
>  scripts/selinux/mdp/mdp.c                     |  4 +-
>  security/selinux/avc.c                        |  2 +-
>  security/selinux/include/avc_ss.h             |  2 +-
>  security/selinux/include/classmap.h           |  2 +-
>  .../selinux/include/initial_sid_to_string.h   |  3 +-
>  security/selinux/include/policycap.h          |  2 +-
>  security/selinux/include/policycap_names.h    |  2 +-
>  security/selinux/ss/services.c                |  4 +-
>  9 files changed, 51 insertions(+), 46 deletions(-)
>
> diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
> index f355b3e0e968..a2caff3c997f 100644
> --- a/scripts/selinux/genheaders/genheaders.c
> +++ b/scripts/selinux/genheaders/genheaders.c
> @@ -26,19 +26,23 @@ static void usage(void)
>         exit(1);
>  }
>
> -static char *stoupperx(const char *s)
> +static const char *stoupperx(const char *s)
>  {
> -       char *s2 = strdup(s);
> -       char *p;
> +       static char buffer[256];
> +       unsigned int i;
> +       char *p = buffer;
>
> -       if (!s2) {
> -               fprintf(stderr, "%s:  out of memory\n", progname);
> +       for (i = 0; i < (sizeof(buffer) - 1) && *s; i++)
> +               *p++ = toupper(*s++);
> +
> +       if (*s) {
> +               fprintf(stderr, "%s:  buffer too small\n", progname);
>                 exit(3);
>         }
>
> -       for (p = s2; *p; p++)
> -               *p = toupper(*p);
> -       return s2;
> +       *p = '\0';
> +
> +       return buffer;
>  }

Hmmm.  I recognize this is just build time code so it's not as
critical, but I still don't like the idea of passing back a static
buffer to the caller; it just seems like we are asking for future
trouble.  I'm also curious as to why you made this choice in this
revision when the existing code should have worked (passed a const,
returned a non-const).  I'm sure I'm missing something obvious, but
can you help me understand why this is necessary?
diff mbox series

Patch

diff --git a/scripts/selinux/genheaders/genheaders.c b/scripts/selinux/genheaders/genheaders.c
index f355b3e0e968..a2caff3c997f 100644
--- a/scripts/selinux/genheaders/genheaders.c
+++ b/scripts/selinux/genheaders/genheaders.c
@@ -26,19 +26,23 @@  static void usage(void)
 	exit(1);
 }
 
-static char *stoupperx(const char *s)
+static const char *stoupperx(const char *s)
 {
-	char *s2 = strdup(s);
-	char *p;
+	static char buffer[256];
+	unsigned int i;
+	char *p = buffer;
 
-	if (!s2) {
-		fprintf(stderr, "%s:  out of memory\n", progname);
+	for (i = 0; i < (sizeof(buffer) - 1) && *s; i++)
+		*p++ = toupper(*s++);
+
+	if (*s) {
+		fprintf(stderr, "%s:  buffer too small\n", progname);
 		exit(3);
 	}
 
-	for (p = s2; *p; p++)
-		*p = toupper(*p);
-	return s2;
+	*p = '\0';
+
+	return buffer;
 }
 
 int main(int argc, char *argv[])
@@ -59,35 +63,19 @@  int main(int argc, char *argv[])
 		exit(2);
 	}
 
-	for (i = 0; secclass_map[i].name; i++) {
-		struct security_class_mapping *map = &secclass_map[i];
-		map->name = stoupperx(map->name);
-		for (j = 0; map->perms[j]; j++)
-			map->perms[j] = stoupperx(map->perms[j]);
-	}
-
-	isids_len = sizeof(initial_sid_to_string) / sizeof (char *);
-	for (i = 1; i < isids_len; i++) {
-		const char *s = initial_sid_to_string[i];
-
-		if (s)
-			initial_sid_to_string[i] = stoupperx(s);
-	}
-
 	fprintf(fout, "/* This file is automatically generated.  Do not edit. */\n");
 	fprintf(fout, "#ifndef _SELINUX_FLASK_H_\n#define _SELINUX_FLASK_H_\n\n");
 
-	for (i = 0; secclass_map[i].name; i++) {
-		struct security_class_mapping *map = &secclass_map[i];
-		fprintf(fout, "#define SECCLASS_%-39s %2d\n", map->name, i+1);
-	}
+	for (i = 0; secclass_map[i].name; i++)
+		fprintf(fout, "#define SECCLASS_%-39s %2d\n", stoupperx(secclass_map[i].name), i+1);
 
 	fprintf(fout, "\n");
 
+	isids_len = sizeof(initial_sid_to_string) / sizeof(char *);
 	for (i = 1; i < isids_len; i++) {
 		const char *s = initial_sid_to_string[i];
 		if (s)
-			fprintf(fout, "#define SECINITSID_%-39s %2d\n", s, i);
+			fprintf(fout, "#define SECINITSID_%-39s %2d\n", stoupperx(s), i);
 	}
 	fprintf(fout, "\n#define SECINITSID_NUM %d\n", i-1);
 	fprintf(fout, "\nstatic inline bool security_is_socket_class(u16 kern_tclass)\n");
@@ -96,10 +84,18 @@  int main(int argc, char *argv[])
 	fprintf(fout, "\tswitch (kern_tclass) {\n");
 	for (i = 0; secclass_map[i].name; i++) {
 		static char s[] = "SOCKET";
-		struct security_class_mapping *map = &secclass_map[i];
-		int len = strlen(map->name), l = sizeof(s) - 1;
-		if (len >= l && memcmp(map->name + len - l, s, l) == 0)
-			fprintf(fout, "\tcase SECCLASS_%s:\n", map->name);
+		int len, l;
+		char *name = strdup(stoupperx(secclass_map[i].name));
+
+		if (!name) {
+			fprintf(stderr, "%s:  out of memory\n", progname);
+			exit(3);
+		}
+		len = strlen(name);
+		l = sizeof(s) - 1;
+		if (len >= l && memcmp(name + len - l, s, l) == 0)
+			fprintf(fout, "\tcase SECCLASS_%s:\n", name);
+		free(name);
 	}
 	fprintf(fout, "\t\tsock = true;\n");
 	fprintf(fout, "\t\tbreak;\n");
@@ -123,17 +119,25 @@  int main(int argc, char *argv[])
 	fprintf(fout, "#ifndef _SELINUX_AV_PERMISSIONS_H_\n#define _SELINUX_AV_PERMISSIONS_H_\n\n");
 
 	for (i = 0; secclass_map[i].name; i++) {
-		struct security_class_mapping *map = &secclass_map[i];
-		int len = strlen(map->name);
+		const struct security_class_mapping *map = &secclass_map[i];
+		int len;
+		char *name = strdup(stoupperx(map->name));
+
+		if (!name) {
+			fprintf(stderr, "%s:  out of memory\n", progname);
+			exit(3);
+		}
+		len = strlen(name);
 		for (j = 0; map->perms[j]; j++) {
 			if (j >= 32) {
 				fprintf(stderr, "Too many permissions to fit into an access vector at (%s, %s).\n",
 					map->name, map->perms[j]);
 				exit(5);
 			}
-			fprintf(fout, "#define %s__%-*s 0x%08xU\n", map->name,
-				39-len, map->perms[j], 1U<<j);
+			fprintf(fout, "#define %s__%-*s 0x%08xU\n", name,
+				39-len, stoupperx(map->perms[j]), 1U<<j);
 		}
+		free(name);
 	}
 
 	fprintf(fout, "\n#endif\n");
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..ff757ae5f253 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -38,7 +38,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 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..05cc51085437 100644
--- a/security/selinux/include/initial_sid_to_string.h
+++ b/security/selinux/include/initial_sid_to_string.h
@@ -1,5 +1,6 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
-static const char *initial_sid_to_string[] =
+
+static const char *const initial_sid_to_string[] =
 {
 	NULL,
 	"kernel",
diff --git a/security/selinux/include/policycap.h b/security/selinux/include/policycap.h
index 2680aa21205c..f35d3458e71d 100644
--- a/security/selinux/include/policycap.h
+++ b/security/selinux/include/policycap.h
@@ -16,6 +16,6 @@  enum {
 };
 #define POLICYDB_CAP_MAX (__POLICYDB_CAP_MAX - 1)
 
-extern const char *selinux_policycap_names[__POLICYDB_CAP_MAX];
+extern const char *const selinux_policycap_names[__POLICYDB_CAP_MAX];
 
 #endif /* _SELINUX_POLICYCAP_H_ */
diff --git a/security/selinux/include/policycap_names.h b/security/selinux/include/policycap_names.h
index 100da7d043db..2a87fc3702b8 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_CAP_MAX] = {
+const char *const selinux_policycap_names[__POLICYDB_CAP_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 */