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

[Mimi Zohar]

On Thu, 2016-04-14 at 15:46 -0700, Kees Cook wrote:
> 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.)

Agreed.

> -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

> >  }
> >  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
> >
> 
> 
>