Re: RFC: Integrating secure update functions into the FPGA Manager

[Moritz Fischer <mdf@kernel.org>]

Hi Russ,

On Wed, Apr 07, 2021 at 04:56:29PM -0700, Russ Weight wrote:
> Hi Moritz,
> 
> Please see below my analysis of the effort to integrate secure functions
> into the FPGA Manager. I have recommendations interspersed below. The
> bottom line is that it seems like there is very little opportunity to
> share code. I could try to rewrite the fpga_mgr_load() related functions
> to accommodate worker threads, cancellation, and progress updates, but I'm
> not sure there is enough value-add to justify the the additional complexity,
> especially for the scatter-gather versions. The other options is to just
> port the secure-manager functions into the FPGA Manager.
> 
> - Russ
> 
> =======================================================================
> 
> This is a comparison of the FPGA Security Manager patch-set with the
> current FPGA Manager. Recommendations are provided on how to the extend
> the FPGA Manager to support the Security Manager functions.
> 
> Purpose of the FPGA Security Manager Class Driver
> =================================================
> 
> The security manager provides a common user API (via sysfs) for transferring
> an opaque image to the card BMC for validation and disposition with a
> completion time of up to 40 minutes. A separate trigger function (image_load)
> can be used to activate a previously transferred image (e.g. FPGA Static
> Region or BMC image).
> 
> Note that secure updates have no notion of Regions, Bridges or FPGA
> running state.

Not needed.
> 
> A successful merge of the Security Manager with the FPGA Manager would include
> an extension of the FPGA Manager API to provide sysfs nodes to support secure
> updates.

Yes.
> 
> The FPGA Security Manager API
> =============================
> 
> Image Update (transfer data to Card BMC for validation & storage)
>   WO: filename, cancel
>   RO: name, status, error, hw_errinfo, remaining_size
> 
> Image Load (trigger BMC, FPGA, or FW load from FLASH)
>   RO: available_images  # Images that can be loaded/activated
>   WO: image_load:       # Trigger an image load
> 
> 
> Merging Security Manager and FPGA Manager functions (organized by sysfs-node)
> =============================================================================
> 
> The "name" sysfs node has essentially the same purpose, so no change is
> required.
> 
> The sec-mgr "status" is similar to fpga-mgr "state"
> The sec-mgr "error" is simliar to fpga-mgr "status"
> 
> All others are unique to the security manager.
> 
> * RECOMMENDATION: The secure-update sysfs files should be grouped together
> * in "secure-update" sysfs directory to clearly associate them with the
> * secure update process and separate them from the other driver functions.

SGTM.
> 
> The sec-mgr sysfs status file has some similarity to the fpga-mgr sysfs
> state file:
> 
>     Sec-Mgr status             FPGA-Mgr state
>     --------------             --------------
>                             FPGA_MGR_STATE_UNKNOWN
>                             FPGA_MGR_STATE_POWER_OFF
>                             FPGA_MGR_STATE_POWER_UP
>                             FPGA_MGR_STATE_RESET
> FPGA_SEC_PROG_IDLE
> FPGA_SEC_PROG_READING       FPGA_MGR_STATE_FIRMWARE_REQ
>                             FPGA_MGR_STATE_FIRMWARE_REQ_ERR
> FPGA_SEC_PROG_PREPARING     FPGA_MGR_STATE_WRITE_INIT
>                             FPGA_MGR_STATE_WRITE_INIT_ERR
> FPGA_SEC_PROG_WRITING       FPGA_MGR_STATE_WRITE
>                             FPGA_MGR_STATE_WRITE_ERR
> FPGA_SEC_PROG_PROGRAMMING   FPGA_MGR_STATE_WRITE_COMPLETE
>                             FPGA_MGR_STATE_WRITE_COMPLETE_ERR
>                             FPGA_MGR_STATE_OPERATING
> FPGA_SEC_PROG_IDLE
> 
> The sec-mgr sysfs error file seems to be similar in purpose to the
> fpga-mgr sysfs status file, but there is no overlap in the actual
> error codes.
> 
> * RECOMMENDATION: there is not enough similarity between the status/state and
> * error/status nodes to try to share them. If all of the other secure-update
> * related nodes are grouped together in a secure-update directory, it would
> * probably create confusion to try to share the state and status files that
> * are in a different location.

Yes.
> 
> sysfs: filename
> ===============
> 
> In the current security manager patches, writing the name of an image file
> to "filename" is roughly equivalent to creating a worker thread and having it
> call fpga_mgr_load() with info->firmware_name set to the image file name.
> 
> The update sequences are similar:
> 
>     Sec-Mgr ops flow        FPGA-Mgr ops flow
>     ================        =================
>     prepare()               write_init()
>     # Loop on 16KB blocks   write()
>       write_blk()
>     poll_complete()         write_complete()
> 
> In the worst case, we have seen n3000 FPGA image updates take up to 40
> minutes. The remaining_size is updated between each 16KB block to
> allow users to monitor the progress of the data transfer. The sec-mgr
> also checks for a cancel flag between each 16KB write and between
> each stage of the update.
> 
> The fpga_mgr has a "buffer" flow and a "scatter-gather" flow. The sec-mgr
> flow is always started via sysfs and uses request_firmware(), so it only
> needs one (buffer) flow.
> 
> * RECOMMENDATION: keep the update functions separate. Although they are
> * similar, the fpga-mgr updates are simpler and more readable as they
> * are. The sec-mgr update is done via kernel worker thread and is a little
> * more complicated with support for maintaining the remaining size and
> * with support for cancellation. There currently no need for a scatter-gather
> * secure update implementation since secure updates use request_firmware.
> 
> Note that the sec-mgr supports additional ops:
>     cancel()         : send cancel-request to lower-level driver
>     cleanup()        : optional: called if and only if the prepare op succeeds
>     get_hw_errinfo() : optional: provides device-specific 64-bit error info
> 
> sysfs: cancel, status, error, hw_errinfo, remaining_size
> ========================================================
> 
> * RECOMMENDATION: There are no equivalent features in fpga-mgr. Port
> * these each from the security manager to the FPGA Manager.

SGTM.
> 
> sysfs: available_images, load_image
> ===================================
> 
>   available_images: RO: list device-specific keywords that can be used
>                     to trigger an image to be loaded/activated. eg.
>                     fpga_factory, fpga_user1, fpga_user2, bmc_factory.
> 
>   load_image:       WO: trigger the load/activation of an image from FLASH
> 
> * RECOMMENDATION: Nothing similar in fpga-mgr. Port functionality to the
> * FPGA Manager
> 
> Conclusion
> ==========
> 
> The purpose of the secure-update support is to provide a common user API
> for loading an opaque image to a device with completion times of up to 40
> minutes. The FPGA Manager can be extended to include the sysfs nodes
> required for a secure update. There is very little opportunity for shared
> code between the current FPGA Manager and the secure update support.  For
> the most part, this would be an addition of new functionality.

Sounds good to me overall, sorry for the late response.

- Moritz