Message-ID: <X7dhkWZmsfTXfpWd@epycbox.lan>
Date:   Thu, 19 Nov 2020 22:26:25 -0800
From:   Moritz Fischer <mdf@...nel.org>
To:     Russ Weight <russell.h.weight@...el.com>
Cc:     Moritz Fischer <mdf@...nel.org>, linux-fpga@...r.kernel.org,
        linux-kernel@...r.kernel.org, trix@...hat.com, lgoncalv@...hat.com,
        yilun.xu@...el.com, hao.wu@...el.com, matthew.gerlach@...el.com
Subject: Re: [PATCH v6 1/7] fpga: sec-mgr: fpga security manager class driver

Hi Russ,

On Thu, Nov 19, 2020 at 06:39:44PM -0800, Russ Weight wrote:
> 
> 
> On 11/15/20 3:03 PM, Moritz Fischer wrote:
> > Hi Russ,
> >
> > On Thu, Nov 05, 2020 at 05:08:59PM -0800, Russ Weight wrote:
> >> Create the FPGA Security Manager class driver. The security
> >> manager provides interfaces to manage secure updates for the
> >> FPGA and BMC images that are stored in FLASH. The driver can
> >> also be used to update root entry hashes and to cancel code
> >> signing keys. The image type is encoded in the image file
> >> and is decoded by the HW/FW secure update engine.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> >> Signed-off-by: Xu Yilun <yilun.xu@...el.com>
> >> Reviewed-by: Tom Rix <trix@...hat.com>
> >> ---
> >> v6:
> >>   - Removed sysfs support and documentation for the display of the
> >>     flash count, root entry hashes, and code-signing-key cancelation
> >>     vectors.
> >> v5:
> >>   - Added the devm_fpga_sec_mgr_unregister() function, following recent
> >>     changes to the fpga_manager() implementation.
> >>   - Changed some *_show() functions to use sysfs_emit() instead of sprintf(
> >> v4:
> >>   - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
> >>     and removed unnecessary references to "Intel".
> >>   - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> >> v3:
> >>   - Modified sysfs handler check in check_sysfs_handler() to make
> >>     it more readable.
> >> v2:
> >>   - Bumped documentation dates and versions
> >>   - Added Documentation/fpga/ifpga-sec-mgr.rst 
> >>   - Removed references to bmc_flash_count & smbus_flash_count (not supported)
> >>   - Split ifpga_sec_mgr_register() into create() and register() functions
> >>   - Added devm_ifpga_sec_mgr_create()
> >>   - Removed typedefs for imgr ops
> >> ---
> >>  .../ABI/testing/sysfs-class-fpga-sec-mgr      |   5 +
> >>  Documentation/fpga/fpga-sec-mgr.rst           |  44 +++
> >>  Documentation/fpga/index.rst                  |   1 +
> >>  MAINTAINERS                                   |   9 +
> >>  drivers/fpga/Kconfig                          |   9 +
> >>  drivers/fpga/Makefile                         |   3 +
> >>  drivers/fpga/fpga-sec-mgr.c                   | 296 ++++++++++++++++++
> >>  include/linux/fpga/fpga-sec-mgr.h             |  44 +++
> >>  8 files changed, 411 insertions(+)
> >>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> >>  create mode 100644 Documentation/fpga/fpga-sec-mgr.rst
> >>  create mode 100644 drivers/fpga/fpga-sec-mgr.c
> >>  create mode 100644 include/linux/fpga/fpga-sec-mgr.h
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> >> new file mode 100644
> >> index 000000000000..ecda22a3ff3b
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> >> @@ -0,0 +1,5 @@
> >> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/name
> >> +Date:		Oct 2020
> >> +KernelVersion:  5.11
> >> +Contact:	Russ Weight <russell.h.weight@...el.com>
> >> +Description:	Name of low level fpga security manager driver.
> >> diff --git a/Documentation/fpga/fpga-sec-mgr.rst b/Documentation/fpga/fpga-sec-mgr.rst
> >> new file mode 100644
> >> index 000000000000..26dac599ead7
> >> --- /dev/null
> >> +++ b/Documentation/fpga/fpga-sec-mgr.rst
> >> @@ -0,0 +1,44 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +========================================
> >> +FPGA Security Manager Class Driver
> >> +========================================
> >> +
> >> +The FPGA Security Manager class driver provides a common
> >> +API for user-space tools to manage updates for secure FPGA
> >> +devices. Device drivers that instantiate the Security
> >> +Manager class driver will interact with a HW secure update
> >> +engine in order to transfer new FPGA and BMC images to FLASH so
> >> +that they will be automatically loaded when the FPGA card reboots.
> >> +
> >> +A significant difference between the FPGA Manager and the FPGA
> >> +Security Manager is that the FPGA Manager does a live update (Partial
> >> +Reconfiguration) to a device, whereas the FPGA Security Manager
> >> +updates the FLASH images for the Static Region and the BMC so that
> >> +they will be loaded the next time the FPGA card boots. Security is
> >> +enforced by hardware and firmware. The security manager interacts
> >> +with the firmware to initiate an update, pass in the necessary data,
> >> +and collect status on the update.
> > I've always wondered if we could've made this a functionality of an FPGA
> > manager 'non-volatile' node or something.
> >
> > I guess there might be cases where you can only do either of them, i.e.
> > only update flash or only update at runtime.
> 
> Today, in light of Richard Gong's recent patch set, I took another look at
> the fpga manager, trying to determine what changes would need to be made in
> the fpga manager order to support secure updates. These are my observations:
> 
> (1) For the devices that I am working on, the lower-level drivers are
>     completely different for PR vs image updates to flash. As a result,
>     if we used the fpga-mgr, we would need to create different instances
>     of the fpga-mgr, one for PR and one for secure updates - each supported
>     by a different low-level driver.

I was mostly thinking about adding a somewhat similar API to the FPGA
manager (close to what you're doing), but as I said it was a suggestion.
> 
> (2) For secure updates, our worst case time is 40 minutes. I doubt that it
>     will ever be longer than that, but we need to support that case. For this
>     length of time, we feel that it is important to show some indication
>     of progress to the user during the update. To handle this, we
>     are using a write_blk() function to break up the writes so that the
>     class driver can provide updates during the data transfer (e.g. this
>     much is left to transfer). We have also "backgrounded" the kernel
>     process by spawning a kernel worker thread to do the update. The user,
>     or user-space code, can monitor the progress by polling for status
>     through sysfs.
> 
> (3) Also, because of the long updates, it seems necessary to provide a way
>     to cancel the update. For example, if the user accidentally specifies the
>     wrong image file, 40 minutes is too long to wait before they are able
>     to try again. We have provided a way to signal the worker thread to
>     abort when possible.
> 
> (4) Another observation is that we are using the same secure update mechanism
>     to program new root-entry-hashes and to cancel code-signing keys. The
>     image type is encoded in the file header. The payload is opaque to host
>     software, so this isn't an issue - just an observation.
>    
> So is it worth adding these additional features to the fpga-mgr? Or is it
> better to keep them separate? To me they seem different enough, that I think
> it would be cleaner to keep them separate.

Yeah, I think that's fine. Thanks for taking another look, though.

Cheers,
Moritz

Powered by blists - more mailing lists