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@xxxxxxxxx> >>> >>> 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
