Hi Russ, On Wed, Oct 28, 2020 at 11:37:46AM -0700, Russ Weight wrote: > > > On 10/27/20 7:41 PM, Wu, Hao wrote: > >> On 10/25/20 7:29 PM, Wu, Hao wrote: > >>>> Subject: [PATCH v5 1/7] fpga: sec-mgr: intel fpga security manager class > >>>> driver > >>>> > >>>> 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. > >>>> > >>>> This patch creates the class driver and provides sysfs > >>>> interfaces for displaying root entry hashes, canceled code > >>>> signing keys and flash counts. > >>>> > >>>> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx> > >>>> Signed-off-by: Xu Yilun <yilun.xu@xxxxxxxxx> > >>>> Reviewed-by: Tom Rix <trix@xxxxxxxxxx> > >>>> --- > >>>> 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 | 67 +++ > >>>> Documentation/fpga/fpga-sec-mgr.rst | 50 ++ > >>>> Documentation/fpga/index.rst | 1 + > >>>> MAINTAINERS | 9 + > >>>> drivers/fpga/Kconfig | 9 + > >>>> drivers/fpga/Makefile | 3 + > >>>> drivers/fpga/fpga-sec-mgr.c | 487 ++++++++++++++++++ > >>>> include/linux/fpga/fpga-sec-mgr.h | 83 +++ > >>>> 8 files changed, 709 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..843f0b58f171 > >>>> --- /dev/null > >>>> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr > >>>> @@ -0,0 +1,67 @@ > >>>> +What: /sys/class/fpga_sec_mgr/fpga_secX/name > >>>> +Date:Oct 2020 > >>>> +KernelVersion: 5.11 > >>>> +Contact:Russ Weight <russell.h.weight@xxxxxxxxx> > >>>> +Description:Name of low level fpga security manager driver. > >>>> + > >>>> +What: > >>>> /sys/class/fpga_sec_mgr/fpga_secX/security/sr_root_entry_hash > >>>> +Date:Oct 2020 > >>>> +KernelVersion: 5.11 > >>>> +Contact:Russ Weight <russell.h.weight@xxxxxxxxx> > >>>> +Description:Read only. Returns the root entry hash for the static > >>>> +region if one is programmed, else it returns the > >>>> +string: "hash not programmed". This file is only > >>>> +visible if the underlying device supports it. > >>>> +Format: "0x%x". > >>>> + > >>> If we plan to make this class driver a common one for everybody, then > >>> these sysfs defined here sounds a little device-specific? This is just my > >>> personal feeling, Moritz and Tom, how do you guys think about these ones? > >> Yes - this occurred to me as well. The data in the security subdirectory > >> could be different for different vendors. I'm not sure if this is a problem > >> or not. One thing to note is that these sysfs entries are only present if there > >> is a handler for them. Additional, optional, file types could be added by other > >> vendors. > > But if other vendors want to have their own ones, they follow the same method. > > that means every time we need to add more handlers and sysfs entries to this > > common class driver. This is not what we really want. If we consider this common > > class driver is one abstraction layer, we would like to have the real common items > > in such driver. Vendor specific items could be handled in drivers provided by > > the different vendors but not in the common file, at least we don't want to touch > > it every time. How do you think? > It isn't a generic interface if every vendor has to extend the number of sysfs > entries supported by the class device - but if the total number of vendors is > two, then maybe it isn't an issue? I'm really not sure what the number would be... > > Looking through sysfs, I can also see a number stats files (e.g. > /sys/fs/xfs/stats/stats) that contain multiple lines of information. There are > also meminfo files (e.g. /sys/devices/system/node/node1/meminfo) that contain > multiple lines of information. > > All of the information in the security/ directory is read-only information. Would > it be reasonable to replace the security directory with a single secinfo file that > contains name/value pairs that describe the data? For example: > > bmc_canceled_csks: > bmc_root_entry_hash: 0x16609930bf6e65ee0d929a87884c37826a731bb317a11f4feb47b3cb328b9b0c > pr_canceled_csks: > pr_root_entry_hash: hash not programmed > sr_canceled_csks: > sr_root_entry_hash: 0xa74b1b31b6b010e94be4a3a043f9af3c5b81258893fbe40cd91d8441fb1cb827 > user_flash_count: 113 > > If we did that, then the format could be enforced as an array of name/value string > pairs, which could vary by device/vendor. It would be human readable. Any parser > would have to know what device and format it was dealing with. > > I think the options we have are: > (1) Extend the support sysfs entries as needed (the current implementation) > (2) Provide a single secinfo (or security_info) file that enforces name/value pairs > (3) Move the security information out of the class driver. The scope of the class > driver would be the update only. > > If (2) is acceptable, then I like that best. I think (3) is also OK. I am uncertain > about (1). I'm not a fan of (1). Not sure about (2) in terms of what people do with sysfs, if it's just readonly it might be fine, (3) seems reasonable to me. Cheers, Moritz
