Re: [RFC PATCH] fs: define a string representation of the kernel_read_file_id enumeration

[Kees Cook]

On Thu, Apr 14, 2016 at 1:49 PM, Mimi Zohar <zohar@xxxxxxxxxxxxxxxxxx> wrote:
> (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 <zohar@xxxxxxxxxxxxxxxxxx>
> ---
>  fs/exec.c          | 28 ++++++++++++++--------------
>  include/linux/fs.h | 17 +++++++++++------
>  2 files changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 05e71b6..e9b9b85 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -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];

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

-Kees

>  }
>  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)
>  {
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 23ea886..35ed80f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -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
>



-- 
Kees Cook
Chrome OS & Brillo Security