Message-ID: <1460502257.3256.50.camel@linux.vnet.ibm.com>
Date:	Tue, 12 Apr 2016 19:04:17 -0400
From:	Mimi Zohar <zohar@...ux.vnet.ibm.com>
To:	Stephen Boyd <stephen.boyd@...aro.org>
Cc:	linux-kernel@...r.kernel.org, linux-arm@...ts.infradead.org,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Robin Murphy <robin.murphy@....com>,
	Laura Abbott <labbott@...hat.com>,
	Arnd Bergmann <arnd@...db.de>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Vikram Mulukutla <markivx@...eaurora.org>,
	linux-security-module <linux-security-module@...r.kernel.org>
Subject: Re: [RFC/PATCH 4/4] firmware: Support requesting firmware directly
 into DMA memory

On Tue, 2016-04-12 at 10:27 -0700, Stephen Boyd wrote:
> Quoting Mimi Zohar (2016-03-08 03:58:41)
> > Hi Stephen,
> > 
> > [CC''ing linux-security-module]
> > 
> > On Tue, 2016-03-08 at 16:22 +0700, Stephen Boyd wrote:
> > > Some systems are memory constrained but they need to load very
> > > large firmwares. The firmware subsystem allows drivers to request
> > > this firmware be loaded from the filesystem, but this requires
> > > that the entire firmware be loaded into kernel memory first
> > > before it's provided to the driver. This can lead to a situation
> > > where we map the firmware twice, once to load the firmware into
> > > kernel memory and once to copy the firmware into the final
> > > resting place.
> > > 
> > > This design creates needless memory pressure and delays loading
> > > because we have to copy from kernel memory to somewhere else.
> > > Let's add a request_firmware_dma() API that allows drivers to
> > > request firmware be loaded directly into a DMA buffer that's been
> > > pre-allocated. This skips the intermediate step of allocating a
> > > buffer in kernel memory to hold the firmware image while it's
> > > read from the filesystem and copying it to another location. It
> > > also requires that drivers know how much memory they'll require
> > > before requesting the firmware and negates any benefits of
> > > firmware caching.
> > 
> > Prior to the kernel_read_file() family of functions, IMA/IMA-appraisal,
> > when enabled, pre-read the firmware a page at a time calculating the
> > file hash and then verified the file signature, before the firmware was
> > re-read into memory.   With the kernel_read_file() family of functions
> > the firmware is read into memory once, the signature verified, before
> > the driver is given access.
> > 
> > This patch set wants to read the file piecemeal, giving the driver
> > direct access to the pieces read, without first verifying the firmware
> > signature.  This breaks the concepts of measuring (trusted boot) or
> > appraising (secure boot) a file before usage.
> 
> I'm not sure I follow. This patch is allowing drivers to choose where
> the firmware is loaded. The assumption is that the device isn't actively
> running or using the firmware when it's being loaded into the DMA
> buffer. I hope that we can give the IMA/IMA-appraisal code a way to
> calculate the file hash a page at a time on the region of memory that
> the caller has chosen to use. Once the entire file is read into the DMA
> buffer, the signature can be verified and then the caller will know if
> the firmware is trusted or not.

In the original version of the patches (Dec 2015), IMA read the file a
buffer at a time to calculate the file hash.  Based on David Young's
comment, instead of calling IMA to read the file, the file was read
inline and then called IMA to calculate the hash based on the buffer
data.

> I suppose the driver could use the firmware data even if the
> request_firmware() call fails due to a bad IMA/IMA-appraisal, but is
> that important? Wouldn't it be a bug in the driver because it isn't
> checking the return value of request_firmware_dma()?

True, it would be a bug, but the main point of the patch set was to
close this class of measurement/appraisal gaps.

> >    
> > > This is based on a patch from Vikram Mulukutla on
> > > codeaurora.org[1].
> > > 
> > > [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm-3.18/commit/drivers/base/firmware_class.c?h=rel/msm-3.18&id=0a328c5f6cd999f5c591f172216835636f39bcb5
> > > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > > Cc: Vikram Mulukutla <markivx@...eaurora.org>
> > > Cc: Mimi Zohar <zohar@...ux.vnet.ibm.com>
> > > Signed-off-by: Stephen Boyd <stephen.boyd@...aro.org>
> > > ---
> > > 
> > > TODO:
> > >  * Handle security_kernel_fw_from_file() in the userhelper fallback case
> > >  * Rework for pending patches from Mimi Zohar that change the way files
> > >    are read in kernel space
> > > 
> > >  drivers/base/firmware_class.c | 256 ++++++++++++++++++++++++++++++++----------
> > >  include/linux/firmware.h      |  13 +++
> > >  2 files changed, 207 insertions(+), 62 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 9a616cf7ac9f..8ab7a418f1d7 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -29,6 +29,8 @@
> > >  #include <linux/syscore_ops.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/security.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/dma-mapping.h>
> > > 
> > >  #include <generated/utsrelease.h>
> > > 
> > > @@ -154,6 +156,15 @@ struct firmware_buf {
> > >       const char *fw_id;
> > >  };
> > > 
> > > +struct firmware_dma_desc {
> > > +     struct device *dev;
> > > +     void *cpu_addr;
> > > +     dma_addr_t dma_addr;
> > > +     unsigned long offset;
> > > +     size_t size;
> > > +     struct dma_attrs *attrs;
> > > +};
> > > +
> > >  struct fw_cache_entry {
> > >       struct list_head list;
> > >       const char *name;
> > > @@ -292,39 +303,83 @@ static const char * const fw_path[] = {
> > >  module_param_string(path, fw_path_para, sizeof(fw_path_para), 0644);
> > >  MODULE_PARM_DESC(path, "customized firmware image search path with a higher priority than default path");
> > > 
> > > -static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> > > +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf,
> > > +                              struct firmware_dma_desc *dma)
> > >  {
> > > -     int size;
> > > +     int size, rsize, remaining;
> > >       char *buf;
> > >       int rc;
> > > +     loff_t offset;
> > > +     unsigned long dma_offset;
> > > 
> > >       if (!S_ISREG(file_inode(file)->i_mode))
> > >               return -EINVAL;
> > >       size = i_size_read(file_inode(file));
> > >       if (size <= 0)
> > >               return -EINVAL;
> > > -     buf = vmalloc(size);
> > > -     if (!buf)
> > > -             return -ENOMEM;
> > > -     rc = kernel_read(file, 0, buf, size);
> > > -     if (rc != size) {
> > > -             if (rc > 0)
> > > -                     rc = -EIO;
> > > -             goto fail;
> > > +
> > > +     if (dma) {
> > > +             if (size + dma->offset > dma->size)
> > > +                     return -EINVAL;
> > > +
> > > +             dma_offset = dma->offset;
> > > +             offset = 0;
> > > +             remaining = size;
> > > +
> > > +             while (remaining > 0) {
> > > +                     rsize = min_t(int, remaining, PAGE_SIZE);
> > > +
> > > +                     buf = dma_remap(dma->dev, dma->cpu_addr, dma->dma_addr,
> > > +                                     rsize, dma_offset, dma->attrs);
> > > +                     if (!buf)
> > > +                             return -ENOMEM;
> > > +
> > > +                     rc = kernel_read(file, offset, buf, rsize);
> > > +                     if (rc != rsize) {
> > > +                             if (rc > 0)
> > > +                                     rc = -EIO;
> > > +                             goto fail_dma;
> > > +                     }
> > > +                     rc = security_kernel_fw_from_file(file, buf, rsize); 
> > 
> > The security_kernel_fw_from_file hook was removed and replaced with the
> > pre and post hooks named security_kernel_read_file() hook and
> > security_kernel_post_read_file() respectively.  The pre security hook
> > gives the LSMs and IMA-appraisal opportunity to deny the read.   In
> > addition to giving the LSMs the opportunity to fail the firmware
> > request, the post security hook allows IMA/IMA-appraisal to measure the
> > file and appraise the file signature.
> > 
> 
> Thanks for the information. Would it be possible to move to appraising
> the file signature a page at a time in the case of DMA firmware loading?
> I see that this is all sort of moved away from the firmware code, so I'm
> trying to figure out how to make this work with the security checks now.

The signature is based on the hash of the entire file.

Mimi 

Powered by blists - more mailing lists