Re: [RFC PATCH 1/5] vfs: define a generic function to read a file from the kernel

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

On Fri, 2016-01-08 at 12:24 -0800, Kees Cook wrote:
> On Fri, Jan 8, 2016 at 11:22 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > In order to measure and appraise files being read by the kernel,
> > new module and kexec syscalls were defined which include a file
> > descriptor.  Other places in the kernel (eg. firmware, IMA,
> > sound) also read files.
> >
> > This patch introduces a common function for reading files from
> > the kernel with the corresponding security post-read hook and
> > function.
> >
> > Changelog:
> > - Add missing <linux/vmalloc.h>
> >
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > ---
> >  fs/exec.c                 | 56 +++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/fs.h        |  1 +
> >  include/linux/lsm_hooks.h | 11 ++++++++++
> >  include/linux/security.h  |  9 ++++++++
> >  security/security.c       | 16 ++++++++++++++
> >  5 files changed, 93 insertions(+)
> >
> > diff --git a/fs/exec.c b/fs/exec.c
> > index b06623a..3c48a19 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -56,6 +56,7 @@
> >  #include <linux/pipe_fs_i.h>
> >  #include <linux/oom.h>
> >  #include <linux/compat.h>
> > +#include <linux/vmalloc.h>
> >
> >  #include <asm/uaccess.h>
> >  #include <asm/mmu_context.h>
> > @@ -831,6 +832,61 @@ int kernel_read(struct file *file, loff_t offset,
> >
> >  EXPORT_SYMBOL(kernel_read);
> >
> > +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > +                    loff_t max_size, int policy_id)
> > +{
> > +       loff_t i_size, pos;
> > +       ssize_t bytes = 0;
> > +       int ret;
> > +
> > +       if (!S_ISREG(file_inode(file)->i_mode))
> > +               return -EINVAL;
> > +
> > +       i_size = i_size_read(file_inode(file));
> > +       if (max_size > 0 && i_size > max_size)
> > +               return -EFBIG;
> > +       if (i_size == 0)
> > +               return -EINVAL;
> > +
> > +       *buf = vmalloc(i_size);
> 
> This could get very large -- what risks do we have to system stability
> here? Having userspace able to trigger such a massive allocation could
> be a problem. The firmware loader was limited to MAX_INT...

The different callers allowed different sizes.  Instead of hard coding
the max size for all callers, the third parameter of kernel_file_read is
the caller max_size.
  
Mimi