Message-ID: <1461155557.2769.51.camel@linux.vnet.ibm.com>
Subject: Re: [RFC/PATCHv2 v2 0/4] request_firmware() on memory constrained
 devices
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
To: Stephen Boyd <stephen.boyd@linaro.org>
Cc: linux-kernel@vger.kernel.org, linux-arm@lists.infradead.org,
        Robin Murphy <robin.murphy@arm.com>, Laura Abbott <labbott@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mark Brown <broonie@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will.deacon@arm.com>, Ming Lei <ming.lei@canonical.com>
Date: Wed, 20 Apr 2016 08:32:37 -0400
In-Reply-To: <1461114269-13718-1-git-send-email-stephen.boyd@linaro.org>
References: <1461114269-13718-1-git-send-email-stephen.boyd@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3247
Lines: 72

Hi Stefan,

On Tue, 2016-04-19 at 18:04 -0700, Stephen Boyd wrote:
> I'm sending this again to solicit feedback on if this is even the right
> approach. After Mimi's patches that change where firmware loading code
> is done, I've had to modify fs/exec.c and add a struct to linux/fs.h,
> and that feels wrong. If that is OK, then my only other concern is
> doing the security checks a page at at time vs. all at once on the
> whole buffer. If there isn't any opposition to doing that I'll start
> working on the necessary changes.

Reading the file into memory, and then using it to calculate the file
hash, was an optimization to read the file only once.  All other hooks,
pre-read the file a buffer at a time, calculating the file hash.  If
you're ok with this pre-reading, you could define a new hook named
READING_FIRMWARE_DMA, or something similar.  The hash could be
calculated on the pre read hook (security_kernel_read_file), not on the
post read hook (security_kernel_post_read_file).  Validating the
firmware signature on the pre-read hook, would eliminate the possibility
of giving the driver unverified firmware.

Mimi

> 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.
> This patch sets adds support to the request firmware and DMA APIs
> to map DMA buffers a page at a time and load the firmware directly
> into those pages, skipping the intermediate copying step and
> alleviating memory pressure during firmware loading. The drawback
> is that we can't use the firmware caching feature because the
> memory for the firmware cache is never allocated.
> 
> Patches based on v4.6-rc1.
> 
> Changes since v1:
>  * Rebased onto v4.6-rc1 (large conflicts due to movement of code from Mimi)
>  * Added some CONFIG_HAS_DMA ifdefs around code that's using DMA ops
> 
> TODO:
>  * Performance metrics for DMA vs. non-DMA based loading
>  * Test on tiny memory parts with big firmwares
>  * Integrate/test with IMA/security checks
> 
> Laura Abbott (1):
>   dma-mapping: Add dma_remap() APIs
> 
> Stephen Boyd (2):
>   ARM64: dma: Add support for NO_KERNEL_MAPPING attribute
>   firmware: Support requesting firmware directly into DMA memory
> 
> Vikram Mulukutla (1):
>   firmware_class: Provide infrastructure to make fw caching optional
> 
>  arch/arm64/mm/dma-mapping.c     |  78 ++++++++++++++--
>  drivers/base/firmware_class.c   | 192 +++++++++++++++++++++++++++++-----------
>  fs/exec.c                       |  95 +++++++++++++++-----
>  include/linux/dma-mapping.h     |  35 ++++++++
>  include/linux/firmware.h        |  13 +++
>  include/linux/fs.h              |  14 ++-
>  security/integrity/ima/ima_fs.c |   3 +-
>  7 files changed, 347 insertions(+), 83 deletions(-)
>