Re: [PATCH 02/12] fpga: sec-mgr: enable secure updates

[Russ Weight <russell.h.weight@intel.com>]

On 5/17/21 12:37 PM, Russ Weight wrote:
> On 5/16/21 10:32 PM, Greg KH wrote:
>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
>>> From: Russ Weight <russell.h.weight@intel.com>
>>>
>>> Extend the FPGA Security Manager class driver to
>>> include an update/filename sysfs node that can be used
>>> to initiate a secure update.  The filename of a secure
>>> update file (BMC image, FPGA image, Root Entry Hash image,
>>> or Code Signing Key cancellation image) can be written to
>>> this sysfs entry to cause a secure update to occur.
>> Why is userspace responsible for triggering this?  Passing a "filename"
>> into the kernel and having it do something with it is ripe for major
>> problems, please do not.
>>
> I am using the "request_firmware" framework, which accepts a filename
> and finds the firmware file under /lib/firmware.
>
> Is this not an acceptable use for request_firmware?
>
> - Russ

Hi Greg,

The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
Region code are almost complete. I'm trying to get back to the FPGA
security manager patch set. Your previous comments challenged some basic
assumptions. If it is OK, I would like to get some clarity before I rework
the patches.

Overview: The goal is to pass signed data to the PCIe FPGA Card's BMC. BMC
firmware will authenticate the data and disposition it. In our case, FPGA
image data, root entry hashes, and cancellation keys are authenticated
and then stored in FLASH memory. The patchset contains both a class
driver and a platform driver.

Example Output of Current Driver:

        [root@psera2-dell24 update]# echo -n intel/dcp_2_0_page0_0x2020002000000237_signed.bin > filename
        [root@psera2-dell24 update]# while :; do cat status remaining_size ; sleep 3; done
        preparing
        8094720
        <- snip ->
        writing
        8012800
        <- snip ->
        programming
        0
        <- snip ->
        programming
        0
        <- snip ->
        idle
        0
        ^C
        [root@psera2-dell24 update]# cat error
        [root@psera2-dell24 update]#


Assumptions:

(1) request_firmware(). We had assumed that making use of the existing
request_firmware() would be preferred. This requires providing a filename
under /lib/firmware to the framework. You commented (above): "Passing a
'filename' into the kernel and having it do something with it is ripe for
problems, please do not." Unless you have additional comments on this, I
will plan to NOT use the request_firmware framework.

(2) sysfs interface. We had assumed that a sysfs interface would
be preferred. If I am not passing a filename, then I think my only option
with sysfs is to use a binary attribute and cat the data in. Is that
acceptable, or would it be better to use IOCTLs to pass the data?

(3) Platform Driver. This driver is for the BMC on the FPGA Card.
I think that is similar to the SOC model. This is actually a sub-driver
for the MAX10 BMC driver. Other platform drivers (e.g. hwmon) have already
been accepted as subdrivers for the BMC. Is the platform driver the
right approach? If not, can you please point me in the right direction?

(4) Kernel worker thread: The worst case update is currently about 45
minutes (newer implementations are shorter). We chose to do the data
transfer in a kernel worker thread and then make it possible for
userspace to monitor the progress (currently via sysfs). Any concerns
about doing the transfer in a background thread?

(5) New vs modified driver: Perhaps "FPGA Security Manager" is not
a good name. Simply put, the driver passes signed data from the host
to the Card BMC for authentication and disposition. I looked at
merging the class driver with the FPGA Manager, but:

a) Secure updates make no use of the existing FPGA Manager code (FPGA
state management, etc). The only similarity is in the ops data structure.

b) Because of the kernel worker thread the driver remove functionality
adds complexity that is not helpful to the FPGA Manager.

I can, of course, combine the drivers anyway if you think that is better.

I appreciate any feedback/direction you can give.

Thanks,
- Russ