Re: [PATCH v5 1/7] fpga: sec-mgr: intel fpga security manager class driver

[Moritz Fischer]

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