fs: define a string representation of the kernel_read_file_id enumeration

RFC PATCH Superseded Mimi Zohar 2 participants 3 messages Apr 14, 2016 – Apr 15, 2016

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

Mimi Zohar

author

2016-04-14 21:17 UTC #0 lore raw

RFC PATCH 1/1 fs: define a string representation of the kernel_read_file_id enumeration

(This patch is being posted as an RFC and has not been compiled.)

A string representation of the kernel_read_file_id enumeration is needed for displaying messages (eg. pr_info, auditing). We assume that the string representation of the enumeration will be needed by multiple LSMs and the integrity subsystem. Instead of each defining their own string representation, this patch defines a common one.

Each time a new enumeration entry is defined, it will need to be reflected in the list of strings. To simplify keeping the list of strings in sync with the enumeration, this patch proposes using two preprocessing
macros: stringify_1 and an a new macro named enumify.

In general, preprocessing macros are not recommended. The question is whether using preprocessing macros is preferable to having to remember to update the list each time a new enumeration is defined.

With these changes, the simplified version of kernel_read_file_id_str() could be moved to a header.

Signed-off-by Mimi Zohar <[email protected]>

---
 fs/exec.c          | 28 ++++++++++++++--------------
 include/linux/fs.h | 17 +++++++++++------
 2 files changed, 25 insertions(+), 20 deletions(-)

fs/exec.c +14 ~~-14~~

index 05e71b6..e9b9b85 100644
@@ -819,25 +819,25 @@ struct file *open_exec(const char *name)
 }
 EXPORT_SYMBOL(open_exec);
 
+static char *kernel_read_file_str[READING_MAX_ID];
 const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 {
-	switch (id) {
-	case READING_FIRMWARE:
-		return "firmware";
-	case READING_MODULE:
-		return "kernel-module";
-	case READING_KEXEC_IMAGE:
-		return "kexec-image";
-	case READING_KEXEC_INITRAMFS:
-		return "kexec-initramfs";
-	case READING_POLICY:
-		return "security-policy";
-	default:
-		return "unknown";
-	}
+	return kernel_read_file_str[id];
 }
 EXPORT_SYMBOL(kernel_read_file_id_str);
 
+void __init kernel_read_file_init()
+{
+	const char *kernel_read_file_upper_str[] = {
+		 __kernel_read_file_id(__stringify_1)
+	};
+
+	for (i = 0; i < READING_MAX_ID; i++) {
+		kernel_read_file_str[i] = strdup(kernel_read_file_upper_str[i];
+		lower_case(kernel_read_file_str[i];
+	}
+}
+
 int kernel_read(struct file *file, loff_t offset,
 		char *addr, unsigned long count)
 {

include/linux/fs.h +11 -6

index 23ea886..35ed80f 100644
@@ -2580,13 +2580,18 @@ static inline void i_readcount_inc(struct inode *inode)
 #endif
 extern int do_pipe_flags(int *, int);
 
+#define __kernel_read_file_id(id) \
+	id(UNKNOWN)		\
+	id(FIRMWARE)		\
+	id(MODULE)		\
+	id(KEXEC_IMAGE)		\
+	id(KEXEC_INITRAMFS)	\
+	id(POLICY)		\
+	id(MAX_ID)		\
+#define __enumify(ENUM) READING_ ## ENUM,
+
 enum kernel_read_file_id {
-	READING_FIRMWARE = 1,
-	READING_MODULE,
-	READING_KEXEC_IMAGE,
-	READING_KEXEC_INITRAMFS,
-	READING_POLICY,
-	READING_MAX_ID
+	__kernel_read_file_id(__enumify)
 };
 
 extern const char *kernel_read_file_id_str(enum kernel_read_file_id id);

Signature · 1 line

2.1.0

Kees Cook

2016-04-14 22:46 UTC #1 lore raw

Quoted 42 lines · Mimi Zohar

(This patch is being posted as an RFC and has not been compiled.)
A string representation of the kernel_read_file_id enumeration is needed for displaying messages (eg. pr_info, auditing). We assume that the string representation of the enumeration will be needed by multiple LSMs and the integrity subsystem. Instead of each defining their own string representation, this patch defines a common one.
Each time a new enumeration entry is defined, it will need to be reflected in the list of strings. To simplify keeping the list of strings in sync with the enumeration, this patch proposes using two preprocessing
macros: stringify_1 and an a new macro named enumify.
In general, preprocessing macros are not recommended. The question is whether using preprocessing macros is preferable to having to remember to update the list each time a new enumeration is defined.
With these changes, the simplified version of kernel_read_file_id_str() could be moved to a header.
Signed-off-by Mimi Zohar <[email protected]>
---
 fs/exec.c          | 28 ++++++++++++++--------------
 include/linux/fs.h | 17 +++++++++++------
 2 files changed, 25 insertions(+), 20 deletions(-)
fs/exec.c +1 -5
index 05e71b6..e9b9b85 100644
@@ -819,25 +819,25 @@ struct file *open_exec(const char *name)
 }
 EXPORT_SYMBOL(open_exec);
 
+static char *kernel_read_file_str[READING_MAX_ID];
 const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 {
-       switch (id) {
-       case READING_FIRMWARE:
-               return "firmware";
-       case READING_MODULE:
-               return "kernel-module";

fs/exec.c struct file *open_exec(const char *name) · line 825

Quoted 10 lines

fs/exec.c +1 -9

-       case READING_KEXEC_IMAGE:
-               return "kexec-image";
-       case READING_KEXEC_INITRAMFS:
-               return "kexec-initramfs";
-       case READING_POLICY:
-               return "security-policy";
-       default:
-               return "unknown";
-       }
+       return kernel_read_file_str[id];

(Whatever is decided, I'd still prefer an explicit bounds-check on the "id" argument here.)

-Kees

Quoted 50 lines

fs/exec.c +12 -0

 }
 EXPORT_SYMBOL(kernel_read_file_id_str);
 
+void __init kernel_read_file_init()
+{
+       const char *kernel_read_file_upper_str[] = {
+                __kernel_read_file_id(__stringify_1)
+       };
+
+       for (i = 0; i < READING_MAX_ID; i++) {
+               kernel_read_file_str[i] = strdup(kernel_read_file_upper_str[i];
+               lower_case(kernel_read_file_str[i];
+       }
+}
+
 int kernel_read(struct file *file, loff_t offset,
                char *addr, unsigned long count)
 {

include/linux/fs.h +11 -7

index 23ea886..35ed80f 100644
@@ -2580,13 +2580,18 @@ static inline void i_readcount_inc(struct inode *inode)
 #endif
 extern int do_pipe_flags(int *, int);
 
+#define __kernel_read_file_id(id) \
+       id(UNKNOWN)             \
+       id(FIRMWARE)            \
+       id(MODULE)              \
+       id(KEXEC_IMAGE)         \
+       id(KEXEC_INITRAMFS)     \
+       id(POLICY)              \
+       id(MAX_ID)              \
+#define __enumify(ENUM) READING_ ## ENUM,
+
 enum kernel_read_file_id {
-       READING_FIRMWARE = 1,
-       READING_MODULE,
-       READING_KEXEC_IMAGE,
-       READING_KEXEC_INITRAMFS,
-       READING_POLICY,
-       READING_MAX_ID
+       __kernel_read_file_id(__enumify)
 };
 
 extern const char *kernel_read_file_id_str(enum kernel_read_file_id id);
--

2.1.0

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security

Mimi Zohar

author

2016-04-15 11:39 UTC #2 lore raw

Quoted 48 lines · Kees Cook

Quoted 47 lines · Mimi Zohar
(This patch is being posted as an RFC and has not been compiled.)
A string representation of the kernel_read_file_id enumeration is needed for displaying messages (eg. pr_info, auditing). We assume that the string representation of the enumeration will be needed by multiple LSMs and the integrity subsystem. Instead of each defining their own string representation, this patch defines a common one.
Each time a new enumeration entry is defined, it will need to be reflected in the list of strings. To simplify keeping the list of strings in sync with the enumeration, this patch proposes using two preprocessing
macros: stringify_1 and an a new macro named enumify.
In general, preprocessing macros are not recommended. The question is whether using preprocessing macros is preferable to having to remember to update the list each time a new enumeration is defined.
With these changes, the simplified version of kernel_read_file_id_str() could be moved to a header.
Signed-off-by Mimi Zohar <[email protected]>
---
 fs/exec.c          | 28 ++++++++++++++--------------
 include/linux/fs.h | 17 +++++++++++------
 2 files changed, 25 insertions(+), 20 deletions(-)
fs/exec.c +1 ~~-10~~
index 05e71b6..e9b9b85 100644
@@ -819,25 +819,25 @@ struct file *open_exec(const char *name)
 }
 EXPORT_SYMBOL(open_exec);
 
+static char *kernel_read_file_str[READING_MAX_ID];
 const char *kernel_read_file_id_str(enum kernel_read_file_id id)
 {
-       switch (id) {
-       case READING_FIRMWARE:
-               return "firmware";
-       case READING_MODULE:
-               return "kernel-module";
-       case READING_KEXEC_IMAGE:
-               return "kexec-image";
-       case READING_KEXEC_INITRAMFS:
-               return "kexec-initramfs";
-       case READING_POLICY:

Quoted 8 lines

fs/exec.c struct file *open_exec(const char *name) · line 825
Quoted 5 lines
fs/exec.c +1 -4
-               return "security-policy";
-       default:
-               return "unknown";
-       }
+       return kernel_read_file_str[id];
(Whatever is decided, I'd still prefer an explicit bounds-check on the "id" argument here.)

Agreed.

Quoted 1 line

-Kees

Explicitly hard coding the strings, as you did, is clearer and easier to read. It would be nice to get a general agreement as to whether using macros in this case (and similar ones) is acceptable. (Cc'ing linux-fsdevel)

Mimi

Quoted 53 lines

Quoted 50 lines

fs/exec.c +12 -0

 }
 EXPORT_SYMBOL(kernel_read_file_id_str);
 
+void __init kernel_read_file_init()
+{
+       const char *kernel_read_file_upper_str[] = {
+                __kernel_read_file_id(__stringify_1)
+       };
+
+       for (i = 0; i < READING_MAX_ID; i++) {
+               kernel_read_file_str[i] = strdup(kernel_read_file_upper_str[i];
+               lower_case(kernel_read_file_str[i];
+       }
+}
+
 int kernel_read(struct file *file, loff_t offset,
                char *addr, unsigned long count)
 {

include/linux/fs.h +11 -7

index 23ea886..35ed80f 100644
@@ -2580,13 +2580,18 @@ static inline void i_readcount_inc(struct inode *inode)
 #endif
 extern int do_pipe_flags(int *, int);
 
+#define __kernel_read_file_id(id) \
+       id(UNKNOWN)             \
+       id(FIRMWARE)            \
+       id(MODULE)              \
+       id(KEXEC_IMAGE)         \
+       id(KEXEC_INITRAMFS)     \
+       id(POLICY)              \
+       id(MAX_ID)              \
+#define __enumify(ENUM) READING_ ## ENUM,
+
 enum kernel_read_file_id {
-       READING_FIRMWARE = 1,
-       READING_MODULE,
-       READING_KEXEC_IMAGE,
-       READING_KEXEC_INITRAMFS,
-       READING_POLICY,
-       READING_MAX_ID
+       __kernel_read_file_id(__enumify)
 };
 
 extern const char *kernel_read_file_id_str(enum kernel_read_file_id id);
--

2.1.0