linux-fpga.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v12 0/7] FPGA Security Manager Class Driver
@ 2021-05-03 21:35 Russ Weight
  2021-05-03 21:35 ` [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver Russ Weight
                   ` (7 more replies)
  0 siblings, 8 replies; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

The FPGA Security Manager class driver provides a common
API for user-space tools to manage updates for secure FPGA
devices. Device drivers that instantiate the FPGA Security
Manager class driver will interact with a HW secure update
engine in order to transfer new FPGA and BMC images to FLASH so
that they will be automatically loaded when the FPGA card reboots.

A significant difference between the FPGA Manager and the FPGA 
Security Manager is that the FPGA Manager does a live update (Partial
Reconfiguration) to a device whereas the FPGA Security Manager
updates the FLASH images for the Static Region and the BMC so that
they will be loaded the next time the FPGA card boots. Security is
enforced by hardware and firmware. The security manager interacts
with the firmware to initiate an update, pass in the necessary data,
and collect status on the update.

The n3000bmc-secure driver is the first driver to use the FPGA
Security Manager. This driver was previously submitted in the same
patch set, but has been split out into a separate patch set starting
with V2. Future devices will also make use of this common API for
secure updates.

In addition to managing secure updates of the FPGA and BMC images,
the FPGA Security Manager update process may also be used to
program root entry hashes and cancellation keys for the FPGA static
region, the FPGA partial reconfiguration region, and the BMC.
The image files are self-describing, and contain a header describing
the image type.

Secure updates make use of the request_firmware framework, which
requires that image files are accessible under /lib/firmware. A request
for a secure update returns immediately, while the update itself
proceeds in the context of a kernel worker thread. Sysfs files provide
a means for monitoring the progress of a secure update and for
retrieving error information in the event of a failure.

The API includes a "name" sysfs file to export the name of the parent
driver. It also includes an "update" sub-directory containing files that
that can be used to instantiate and monitor a secure update.

Changelog v11 -> v12:
  - Updated Date and KernelVersion fields in ABI documentation
  - Removed size parameter from write_blk() op - it is now up to
    the lower-level driver to determine the appropriate size and
    to update smgr->remaining_size accordingly.
  - Changed syntax of sec_mgr_prog_str[] and sec_mgr_err_str array definitions
    from:
	"idle",			/* FPGA_SEC_PROG_IDLE */
    to:
	[FPGA_SEC_PROG_IDLE]	    = "idle",

Changelog v10 -> v11:
  - Fixed a spelling error in a comment
  - Initialize smgr->err_code and smgr->progress explicitly in
    fpga_sec_mgr_create() instead of accepting the default 0 value.

Changelog v9 -> v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation

Changelog v8 -> v9:
  - Rebased patches for 5.11-rc2
  - Updated Date and KernelVersion in ABI documentation

Changelog v7 -> v8:
  - Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst

Changelog v6 -> v7:
  - Changed dates in documentation file to December 2020
  - Changed filename_store() to use kmemdup_nul() instead of
    kstrndup() and changed the count to not assume a line-return.

Changelog v5 -> v6:
  - Removed sysfs support and documentation for the display of the
    flash count, root entry hashes, and code-signing-key cancelation
    vectors from the class driver. This information can vary by device
    and will instead be displayed by the device-specific parent driver.

Changelog v4 -> v5:
  - Added the devm_fpga_sec_mgr_unregister() function, following recent
    changes to the fpga_manager() implementation.
  - Changed most of the *_show() functions to use sysfs_emit()
    instead of sprintf(
  - When checking the return values for functions of type enum
    fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0

Changelog v3 -> v4:
  - This driver is generic enough that it could be used for non Intel
    FPGA devices. 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_
    Note that this also affects some filenames.

Changelog v2 -> v3:
  - Use dev_err() to report invalid progress in sec_progress()
  - Use dev_err() to report invalid error code in sec_error()
  - Modified sysfs handler check in check_sysfs_handler() to make
    it more readable.
  - Removed unnecessary "goto done"
  - Added a comment to explain imgr->driver_unload in
    ifpga_sec_mgr_unregister()

Changelog v1 -> v2:
  - Separated out the MAX10 BMC Security Engine to be submitted in
    a separate patch-set.
  - Bumped documentation dates and versions
  - Split ifpga_sec_mgr_register() into create() and register() functions
  - Added devm_ifpga_sec_mgr_create()
  - Added Documentation/fpga/ifpga-sec-mgr.rst 
  - Changed progress state "read_file" to "reading"
  - Added sec_error() function (similar to sec_progress())
  - Removed references to bmc_flash_count & smbus_flash_count (not supported)
  - Removed typedefs for imgr ops
  - Removed explicit value assignments in enums
  - Other minor code cleanup per review comments 

Russ Weight (7):
  fpga: sec-mgr: fpga security manager class driver
  fpga: sec-mgr: enable secure updates
  fpga: sec-mgr: expose sec-mgr update status
  fpga: sec-mgr: expose sec-mgr update errors
  fpga: sec-mgr: expose sec-mgr update size
  fpga: sec-mgr: enable cancel of secure update
  fpga: sec-mgr: expose hardware error info

 .../ABI/testing/sysfs-class-fpga-sec-mgr      |  81 +++
 Documentation/fpga/fpga-sec-mgr.rst           |  44 ++
 Documentation/fpga/index.rst                  |   1 +
 MAINTAINERS                                   |   9 +
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   3 +
 drivers/fpga/fpga-sec-mgr.c                   | 648 ++++++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  99 +++
 8 files changed, 894 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

-- 
2.25.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:28   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 2/7] fpga: sec-mgr: enable secure updates Russ Weight
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

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. The image type is encoded in the image file
and is decoded by the HW/FW secure update engine.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst
v7:
  - Changed Date in documentation file to December 2020
v6:
  - Removed sysfs support and documentation for the display of the
    flash count, root entry hashes, and code-signing-key cancelation
    vectors.
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      |   5 +
 Documentation/fpga/fpga-sec-mgr.rst           |  44 +++
 Documentation/fpga/index.rst                  |   1 +
 MAINTAINERS                                   |   9 +
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   3 +
 drivers/fpga/fpga-sec-mgr.c                   | 296 ++++++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  44 +++
 8 files changed, 411 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..2498aef0ac51
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -0,0 +1,5 @@
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/name
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Name of low level fpga security manager driver.
diff --git a/Documentation/fpga/fpga-sec-mgr.rst b/Documentation/fpga/fpga-sec-mgr.rst
new file mode 100644
index 000000000000..9f74c29fe63d
--- /dev/null
+++ b/Documentation/fpga/fpga-sec-mgr.rst
@@ -0,0 +1,44 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+========================================
+FPGA Security Manager Class Driver
+========================================
+
+The FPGA Security Manager class driver provides a common
+API for user-space tools to manage updates for secure FPGA
+devices. Device drivers that instantiate the Security
+Manager class driver will interact with a HW secure update
+engine in order to transfer new FPGA and BMC images to FLASH so
+that they will be automatically loaded when the FPGA card reboots.
+
+A significant difference between the FPGA Manager and the FPGA
+Security Manager is that the FPGA Manager does a live update (Partial
+Reconfiguration) to a device, whereas the FPGA Security Manager
+updates the FLASH images for the Static Region and the BMC so that
+they will be loaded the next time the FPGA card boots. Security is
+enforced by hardware and firmware. The security manager interacts
+with the firmware to initiate an update, pass in the necessary data,
+and collect status on the update.
+
+In addition to managing secure updates of the FPGA and BMC images,
+the FPGA Security Manager update process may also be used to
+program root entry hashes and cancellation keys for the FPGA static
+region, the FPGA partial reconfiguration region, and the BMC.
+
+Secure updates make use of the request_firmware framework, which
+requires that image files are accessible under /lib/firmware. A request
+for a secure update returns immediately, while the update itself
+proceeds in the context of a kernel worker thread. Sysfs files provide
+a means for monitoring the progress of a secure update and for
+retrieving error information in the event of a failure.
+
+Sysfs Attributes
+================
+
+The API includes a sysfs entry *name* to export the name of the parent
+driver. It also includes an *update* sub-directory that can be used to
+instantiate and monitor a secure update.
+
+See `<../ABI/testing/sysfs-class-fpga-sec-mgr>`__ for a full
+description of the sysfs attributes for the FPGA Security
+Manager.
diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
index f80f95667ca2..0b2f427042af 100644
--- a/Documentation/fpga/index.rst
+++ b/Documentation/fpga/index.rst
@@ -8,6 +8,7 @@ fpga
     :maxdepth: 1
 
     dfl
+    fpga-sec-mgr
 
 .. only::  subproject and html
 
diff --git a/MAINTAINERS b/MAINTAINERS
index 1783372a608a..3b1dc0376b52 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7146,6 +7146,15 @@ F:	Documentation/fpga/
 F:	drivers/fpga/
 F:	include/linux/fpga/
 
+FPGA SECURITY MANAGER DRIVERS
+M:	Russ Weight <russell.h.weight@intel.com>
+L:	linux-fpga@vger.kernel.org
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+F:	Documentation/fpga/fpga-sec-mgr.rst
+F:	drivers/fpga/fpga-sec-mgr.c
+F:	include/linux/fpga/fpga-sec-mgr.h
+
 FPU EMULATOR
 M:	Bill Metzenthen <billm@melbpc.org.au>
 S:	Maintained
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 33e15058d0dc..09a8d915db26 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -234,4 +234,13 @@ config FPGA_MGR_ZYNQMP_FPGA
 	  to configure the programmable logic(PL) through PS
 	  on ZynqMP SoC.
 
+config FPGA_SEC_MGR
+	tristate "FPGA Security Manager"
+	help
+	  The Security Manager class driver presents a common
+	  user API for managing secure updates for FPGA
+	  devices, including flash images for the FPGA static
+	  region and for the BMC. Select this option to enable
+	  updates for secure FPGA devices.
+
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885883a..22576d1a3996 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
 obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
 obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
 
+# FPGA Security Manager Framework
+obj-$(CONFIG_FPGA_SEC_MGR)		+= fpga-sec-mgr.o
+
 # FPGA Bridge Drivers
 obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
 obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
new file mode 100644
index 000000000000..468379e0c825
--- /dev/null
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * FPGA Security Manager
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ */
+
+#include <linux/fpga/fpga-sec-mgr.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.h>
+
+static DEFINE_IDA(fpga_sec_mgr_ida);
+static struct class *fpga_sec_mgr_class;
+
+struct fpga_sec_mgr_devres {
+	struct fpga_sec_mgr *smgr;
+};
+
+#define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
+
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+
+	return sysfs_emit(buf, "%s\n", smgr->name);
+}
+static DEVICE_ATTR_RO(name);
+
+static struct attribute *sec_mgr_attrs[] = {
+	&dev_attr_name.attr,
+	NULL,
+};
+
+static struct attribute_group sec_mgr_attr_group = {
+	.attrs = sec_mgr_attrs,
+};
+
+static const struct attribute_group *fpga_sec_mgr_attr_groups[] = {
+	&sec_mgr_attr_group,
+	NULL,
+};
+
+/**
+ * fpga_sec_mgr_create - create and initialize an FPGA
+ *			  security manager struct
+ *
+ * @dev:  fpga security manager device from pdev
+ * @name: fpga security manager name
+ * @sops: pointer to a structure of fpga callback functions
+ * @priv: fpga security manager private data
+ *
+ * The caller of this function is responsible for freeing the struct
+ * with ifpg_sec_mgr_free(). Using devm_fpga_sec_mgr_create() instead
+ * is recommended.
+ *
+ * Return: pointer to struct fpga_sec_mgr or NULL
+ */
+struct fpga_sec_mgr *
+fpga_sec_mgr_create(struct device *dev, const char *name,
+		    const struct fpga_sec_mgr_ops *sops, void *priv)
+{
+	struct fpga_sec_mgr *smgr;
+	int id, ret;
+
+	if (!name || !strlen(name)) {
+		dev_err(dev, "Attempt to register with no name!\n");
+		return NULL;
+	}
+
+	smgr = kzalloc(sizeof(*smgr), GFP_KERNEL);
+	if (!smgr)
+		return NULL;
+
+	id = ida_simple_get(&fpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		goto error_kfree;
+
+	mutex_init(&smgr->lock);
+
+	smgr->name = name;
+	smgr->priv = priv;
+	smgr->sops = sops;
+
+	device_initialize(&smgr->dev);
+	smgr->dev.class = fpga_sec_mgr_class;
+	smgr->dev.parent = dev;
+	smgr->dev.id = id;
+
+	ret = dev_set_name(&smgr->dev, "fpga_sec%d", id);
+	if (ret) {
+		dev_err(dev, "Failed to set device name: fpga_sec%d\n", id);
+		goto error_device;
+	}
+
+	return smgr;
+
+error_device:
+	ida_simple_remove(&fpga_sec_mgr_ida, id);
+
+error_kfree:
+	kfree(smgr);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
+
+/**
+ * fpga_sec_mgr_free - free an FPGA security manager created
+ *			with fpga_sec_mgr_create()
+ *
+ * @smgr:	FPGA security manager structure
+ */
+void fpga_sec_mgr_free(struct fpga_sec_mgr *smgr)
+{
+	ida_simple_remove(&fpga_sec_mgr_ida, smgr->dev.id);
+	kfree(smgr);
+}
+EXPORT_SYMBOL_GPL(fpga_sec_mgr_free);
+
+static void devm_fpga_sec_mgr_release(struct device *dev, void *res)
+{
+	struct fpga_sec_mgr_devres *dr = res;
+
+	fpga_sec_mgr_free(dr->smgr);
+}
+
+/**
+ * devm_fpga_sec_mgr_create - create and initialize an FPGA
+ *			       security manager struct
+ *
+ * @dev:  fpga security manager device from pdev
+ * @name: fpga security manager name
+ * @sops: pointer to a structure of fpga callback functions
+ * @priv: fpga security manager private data
+ *
+ * This function is intended for use in a FPGA Security manager
+ * driver's probe function.  After the security manager driver creates
+ * the fpga_sec_mgr struct with devm_fpga_sec_mgr_create(), it should
+ * register it with devm_fpga_sec_mgr_register().
+ * The fpga_sec_mgr struct allocated with this function will be freed
+ * automatically on driver detach.
+ *
+ * Return: pointer to struct fpga_sec_mgr or NULL
+ */
+struct fpga_sec_mgr *
+devm_fpga_sec_mgr_create(struct device *dev, const char *name,
+			 const struct fpga_sec_mgr_ops *sops, void *priv)
+{
+	struct fpga_sec_mgr_devres *dr;
+
+	dr = devres_alloc(devm_fpga_sec_mgr_release, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return NULL;
+
+	dr->smgr = fpga_sec_mgr_create(dev, name, sops, priv);
+	if (!dr->smgr) {
+		devres_free(dr);
+		return NULL;
+	}
+
+	devres_add(dev, dr);
+
+	return dr->smgr;
+}
+EXPORT_SYMBOL_GPL(devm_fpga_sec_mgr_create);
+
+/**
+ * fpga_sec_mgr_register - register an FPGA security manager
+ *
+ * @smgr: fpga security manager struct
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int fpga_sec_mgr_register(struct fpga_sec_mgr *smgr)
+{
+	int ret;
+
+	ret = device_add(&smgr->dev);
+	if (ret)
+		goto error_device;
+
+	dev_info(&smgr->dev, "%s registered\n", smgr->name);
+
+	return 0;
+
+error_device:
+	ida_simple_remove(&fpga_sec_mgr_ida, smgr->dev.id);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_sec_mgr_register);
+
+/**
+ * fpga_sec_mgr_unregister - unregister an FPGA security manager
+ *
+ * @mgr: fpga manager struct
+ *
+ * This function is intended for use in an FPGA security manager
+ * driver's remove() function.
+ */
+void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
+{
+	dev_info(&smgr->dev, "%s %s\n", __func__, smgr->name);
+
+	device_unregister(&smgr->dev);
+}
+EXPORT_SYMBOL_GPL(fpga_sec_mgr_unregister);
+
+static int fpga_sec_mgr_devres_match(struct device *dev, void *res,
+				     void *match_data)
+{
+	struct fpga_sec_mgr_devres *dr = res;
+
+	return match_data == dr->smgr;
+}
+
+static void devm_fpga_sec_mgr_unregister(struct device *dev, void *res)
+{
+	struct fpga_sec_mgr_devres *dr = res;
+
+	fpga_sec_mgr_unregister(dr->smgr);
+}
+
+/**
+ * devm_fpga_sec_mgr_register - resource managed variant of
+ *				fpga_sec_mgr_register()
+ *
+ * @dev: managing device for this FPGA security manager
+ * @smgr: fpga security manager struct
+ *
+ * This is the devres variant of fpga_sec_mgr_register() for which the
+ * unregister function will be called automatically when the managing
+ * device is detached.
+ */
+int devm_fpga_sec_mgr_register(struct device *dev, struct fpga_sec_mgr *smgr)
+{
+	struct fpga_sec_mgr_devres *dr;
+	int ret;
+
+	/*
+	 * Make sure that the struct fpga_sec_mgr * that is passed in is
+	 * managed itself.
+	 */
+	if (WARN_ON(!devres_find(dev, devm_fpga_sec_mgr_release,
+				 fpga_sec_mgr_devres_match, smgr)))
+		return -EINVAL;
+
+	dr = devres_alloc(devm_fpga_sec_mgr_unregister, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return -ENOMEM;
+
+	ret = fpga_sec_mgr_register(smgr);
+	if (ret) {
+		devres_free(dr);
+		return ret;
+	}
+
+	dr->smgr = smgr;
+	devres_add(dev, dr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_fpga_sec_mgr_register);
+
+static void fpga_sec_mgr_dev_release(struct device *dev)
+{
+}
+
+static int __init fpga_sec_mgr_class_init(void)
+{
+	pr_info("FPGA Security Manager\n");
+
+	fpga_sec_mgr_class = class_create(THIS_MODULE, "fpga_sec_mgr");
+	if (IS_ERR(fpga_sec_mgr_class))
+		return PTR_ERR(fpga_sec_mgr_class);
+
+	fpga_sec_mgr_class->dev_groups = fpga_sec_mgr_attr_groups;
+	fpga_sec_mgr_class->dev_release = fpga_sec_mgr_dev_release;
+
+	return 0;
+}
+
+static void __exit fpga_sec_mgr_class_exit(void)
+{
+	class_destroy(fpga_sec_mgr_class);
+	ida_destroy(&fpga_sec_mgr_ida);
+}
+
+MODULE_DESCRIPTION("FPGA Security Manager Driver");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_sec_mgr_class_init);
+module_exit(fpga_sec_mgr_class_exit)
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
new file mode 100644
index 000000000000..f85665b79b9d
--- /dev/null
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -0,0 +1,44 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for FPGA Security Manager
+ *
+ * Copyright (C) 2019-2020 Intel Corporation, Inc.
+ */
+#ifndef _LINUX_FPGA_SEC_MGR_H
+#define _LINUX_FPGA_SEC_MGR_H
+
+#include <linux/device.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+struct fpga_sec_mgr;
+
+/**
+ * struct fpga_sec_mgr_ops - device specific operations
+ */
+struct fpga_sec_mgr_ops {
+};
+
+struct fpga_sec_mgr {
+	const char *name;
+	struct device dev;
+	const struct fpga_sec_mgr_ops *sops;
+	struct mutex lock;		/* protect data structure contents */
+	void *priv;
+};
+
+struct fpga_sec_mgr *
+fpga_sec_mgr_create(struct device *dev, const char *name,
+		    const struct fpga_sec_mgr_ops *sops, void *priv);
+
+struct fpga_sec_mgr *
+devm_fpga_sec_mgr_create(struct device *dev, const char *name,
+			 const struct fpga_sec_mgr_ops *sops, void *priv);
+
+int fpga_sec_mgr_register(struct fpga_sec_mgr *smgr);
+int devm_fpga_sec_mgr_register(struct device *dev,
+			       struct fpga_sec_mgr *smgr);
+void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr);
+void fpga_sec_mgr_free(struct fpga_sec_mgr *smgr);
+
+#endif
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 2/7] fpga: sec-mgr: enable secure updates
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
  2021-05-03 21:35 ` [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:32   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status Russ Weight
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

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.

The write of the filename will return immediately, and the
update will begin in the context of a kernel worker thread.
This tool utilizes the request_firmware framework, which
requires that the image file reside under /lib/firmware.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
  - Removed size parameter from write_blk() op - it is now up to
    the lower-level driver to determine the appropriate size and
    to update smgr->remaining_size accordingly.
v11:
  - Fixed a spelling error in a comment
  - Initialize smgr->err_code and smgr->progress explicitly in
    fpga_sec_mgr_create() instead of accepting the default 0 value.
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
  - Changed filename_store() to use kmemdup_nul() instead of
    kstrndup() and changed the count to not assume a line-return.
v6:
  - Changed "security update" to "secure update" in commit message
v5:
  - When checking the return values for functions of type enum
    fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
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:
  - Removed unnecessary "goto done"
  - Added a comment to explain imgr->driver_unload in
    ifpga_sec_mgr_unregister()
v2:
  - Bumped documentation date and version
  - Removed explicit value assignments in enums
  - Other minor code cleanup per review comments 
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      |  13 ++
 drivers/fpga/fpga-sec-mgr.c                   | 160 ++++++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  48 ++++++
 3 files changed, 221 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index 2498aef0ac51..36d1b6ba8d76 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -3,3 +3,16 @@ Date:		June 2021
 KernelVersion:	5.14
 Contact:	Russ Weight <russell.h.weight@intel.com>
 Description:	Name of low level fpga security manager driver.
+
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/filename
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Write only. Write the filename of an image
+		file to this sysfs file to initiate a secure
+		update. The file must have an appropriate header
+		which, among other things, identifies the target
+		for the update. This mechanism is used to update
+		BMC images, BMC firmware, Static Region images,
+		and Root Entry Hashes, and to cancel Code Signing
+		Keys (CSK).
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index 468379e0c825..fe82feda6b3c 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -5,8 +5,11 @@
  * Copyright (C) 2019-2020 Intel Corporation, Inc.
  */
 
+#include <linux/delay.h>
+#include <linux/firmware.h>
 #include <linux/fpga/fpga-sec-mgr.h>
 #include <linux/idr.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
@@ -20,6 +23,132 @@ struct fpga_sec_mgr_devres {
 
 #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
 
+static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
+			       enum fpga_sec_err err_code)
+{
+	smgr->err_code = err_code;
+	smgr->sops->cancel(smgr);
+}
+
+static void progress_complete(struct fpga_sec_mgr *smgr)
+{
+	mutex_lock(&smgr->lock);
+	smgr->progress = FPGA_SEC_PROG_IDLE;
+	complete_all(&smgr->update_done);
+	mutex_unlock(&smgr->lock);
+}
+
+static void fpga_sec_mgr_update(struct work_struct *work)
+{
+	struct fpga_sec_mgr *smgr;
+	const struct firmware *fw;
+	enum fpga_sec_err ret;
+	u32 offset = 0;
+
+	smgr = container_of(work, struct fpga_sec_mgr, work);
+
+	get_device(&smgr->dev);
+	if (request_firmware(&fw, smgr->filename, &smgr->dev)) {
+		smgr->err_code = FPGA_SEC_ERR_FILE_READ;
+		goto idle_exit;
+	}
+
+	smgr->data = fw->data;
+	smgr->remaining_size = fw->size;
+
+	if (!try_module_get(smgr->dev.parent->driver->owner)) {
+		smgr->err_code = FPGA_SEC_ERR_BUSY;
+		goto release_fw_exit;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_PREPARING;
+	ret = smgr->sops->prepare(smgr);
+	if (ret != FPGA_SEC_ERR_NONE) {
+		fpga_sec_dev_error(smgr, ret);
+		goto modput_exit;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_WRITING;
+	while (smgr->remaining_size) {
+		ret = smgr->sops->write_blk(smgr, offset);
+		if (ret != FPGA_SEC_ERR_NONE) {
+			fpga_sec_dev_error(smgr, ret);
+			goto done;
+		}
+
+		offset = fw->size - smgr->remaining_size;
+	}
+
+	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
+	ret = smgr->sops->poll_complete(smgr);
+	if (ret != FPGA_SEC_ERR_NONE)
+		fpga_sec_dev_error(smgr, ret);
+
+done:
+	if (smgr->sops->cleanup)
+		smgr->sops->cleanup(smgr);
+
+modput_exit:
+	module_put(smgr->dev.parent->driver->owner);
+
+release_fw_exit:
+	smgr->data = NULL;
+	release_firmware(fw);
+
+idle_exit:
+	/*
+	 * Note: smgr->remaining_size is left unmodified here to
+	 * provide additional information on errors. It will be
+	 * reinitialized when the next secure update begins.
+	 */
+	kfree(smgr->filename);
+	smgr->filename = NULL;
+	put_device(&smgr->dev);
+	progress_complete(smgr);
+}
+
+static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	int ret = count;
+
+	if (count == 0 || count >= PATH_MAX)
+		return -EINVAL;
+
+	mutex_lock(&smgr->lock);
+	if (smgr->driver_unload || smgr->progress != FPGA_SEC_PROG_IDLE) {
+		ret = -EBUSY;
+		goto unlock_exit;
+	}
+
+	smgr->filename = kmemdup_nul(buf, count, GFP_KERNEL);
+	if (!smgr->filename) {
+		ret = -ENOMEM;
+		goto unlock_exit;
+	}
+
+	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->progress = FPGA_SEC_PROG_READING;
+	reinit_completion(&smgr->update_done);
+	schedule_work(&smgr->work);
+
+unlock_exit:
+	mutex_unlock(&smgr->lock);
+	return ret;
+}
+static DEVICE_ATTR_WO(filename);
+
+static struct attribute *sec_mgr_update_attrs[] = {
+	&dev_attr_filename.attr,
+	NULL,
+};
+
+static struct attribute_group sec_mgr_update_attr_group = {
+	.name = "update",
+	.attrs = sec_mgr_update_attrs,
+};
+
 static ssize_t name_show(struct device *dev,
 			 struct device_attribute *attr, char *buf)
 {
@@ -40,6 +169,7 @@ static struct attribute_group sec_mgr_attr_group = {
 
 static const struct attribute_group *fpga_sec_mgr_attr_groups[] = {
 	&sec_mgr_attr_group,
+	&sec_mgr_update_attr_group,
 	NULL,
 };
 
@@ -65,6 +195,12 @@ fpga_sec_mgr_create(struct device *dev, const char *name,
 	struct fpga_sec_mgr *smgr;
 	int id, ret;
 
+	if (!sops || !sops->cancel || !sops->prepare ||
+	    !sops->write_blk || !sops->poll_complete) {
+		dev_err(dev, "Attempt to register without required ops\n");
+		return NULL;
+	}
+
 	if (!name || !strlen(name)) {
 		dev_err(dev, "Attempt to register with no name!\n");
 		return NULL;
@@ -83,6 +219,10 @@ fpga_sec_mgr_create(struct device *dev, const char *name,
 	smgr->name = name;
 	smgr->priv = priv;
 	smgr->sops = sops;
+	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->progress = FPGA_SEC_PROG_IDLE;
+	init_completion(&smgr->update_done);
+	INIT_WORK(&smgr->work, fpga_sec_mgr_update);
 
 	device_initialize(&smgr->dev);
 	smgr->dev.class = fpga_sec_mgr_class;
@@ -200,11 +340,31 @@ EXPORT_SYMBOL_GPL(fpga_sec_mgr_register);
  *
  * This function is intended for use in an FPGA security manager
  * driver's remove() function.
+ *
+ * For some devices, once the secure update has begun authentication
+ * the hardware cannot be signaled to stop, and the driver will not
+ * exit until the hardware signals completion.  This could be 30+
+ * minutes of waiting. The driver_unload flag enables a force-unload
+ * of the driver (e.g. modprobe -r) by signaling the parent driver to
+ * exit even if the hardware update is incomplete. The driver_unload
+ * flag also prevents new updates from starting once the unregister
+ * process has begun.
  */
 void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
 {
 	dev_info(&smgr->dev, "%s %s\n", __func__, smgr->name);
 
+	mutex_lock(&smgr->lock);
+	smgr->driver_unload = true;
+	if (smgr->progress == FPGA_SEC_PROG_IDLE) {
+		mutex_unlock(&smgr->lock);
+		goto unregister;
+	}
+
+	mutex_unlock(&smgr->lock);
+	wait_for_completion(&smgr->update_done);
+
+unregister:
 	device_unregister(&smgr->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_sec_mgr_unregister);
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
index f85665b79b9d..978ab98ffac5 100644
--- a/include/linux/fpga/fpga-sec-mgr.h
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -7,16 +7,56 @@
 #ifndef _LINUX_FPGA_SEC_MGR_H
 #define _LINUX_FPGA_SEC_MGR_H
 
+#include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
 #include <linux/types.h>
 
 struct fpga_sec_mgr;
 
+enum fpga_sec_err {
+	FPGA_SEC_ERR_NONE,
+	FPGA_SEC_ERR_HW_ERROR,
+	FPGA_SEC_ERR_TIMEOUT,
+	FPGA_SEC_ERR_CANCELED,
+	FPGA_SEC_ERR_BUSY,
+	FPGA_SEC_ERR_INVALID_SIZE,
+	FPGA_SEC_ERR_RW_ERROR,
+	FPGA_SEC_ERR_WEAROUT,
+	FPGA_SEC_ERR_FILE_READ,
+	FPGA_SEC_ERR_MAX
+};
+
 /**
  * struct fpga_sec_mgr_ops - device specific operations
+ * @prepare:		    Required: Prepare secure update
+ * @write_blk:		    Required: Write a block of data
+ * @poll_complete:	    Required: Check for the completion of the
+ *			    HW authentication/programming process. This
+ *			    function should check for smgr->driver_unload
+ *			    and abort with FPGA_SEC_ERR_CANCELED when true.
+ * @cancel:		    Required: Signal HW to cancel update
+ * @cleanup:		    Optional: Complements the prepare()
+ *			    function and is called at the completion
+ *			    of the update, whether success or failure,
+ *			    if the prepare function succeeded.
  */
 struct fpga_sec_mgr_ops {
+	enum fpga_sec_err (*prepare)(struct fpga_sec_mgr *smgr);
+	enum fpga_sec_err (*write_blk)(struct fpga_sec_mgr *smgr, u32 offset);
+	enum fpga_sec_err (*poll_complete)(struct fpga_sec_mgr *smgr);
+	enum fpga_sec_err (*cancel)(struct fpga_sec_mgr *smgr);
+	void (*cleanup)(struct fpga_sec_mgr *smgr);
+};
+
+/* Update progress codes */
+enum fpga_sec_prog {
+	FPGA_SEC_PROG_IDLE,
+	FPGA_SEC_PROG_READING,
+	FPGA_SEC_PROG_PREPARING,
+	FPGA_SEC_PROG_WRITING,
+	FPGA_SEC_PROG_PROGRAMMING,
+	FPGA_SEC_PROG_MAX
 };
 
 struct fpga_sec_mgr {
@@ -24,6 +64,14 @@ struct fpga_sec_mgr {
 	struct device dev;
 	const struct fpga_sec_mgr_ops *sops;
 	struct mutex lock;		/* protect data structure contents */
+	struct work_struct work;
+	struct completion update_done;
+	char *filename;
+	const u8 *data;			/* pointer to update data */
+	u32 remaining_size;		/* size remaining to transfer */
+	enum fpga_sec_prog progress;
+	enum fpga_sec_err err_code;	/* security manager error code */
+	bool driver_unload;
 	void *priv;
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
  2021-05-03 21:35 ` [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver Russ Weight
  2021-05-03 21:35 ` [PATCH v12 2/7] fpga: sec-mgr: enable secure updates Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:36   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors Russ Weight
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

Extend the FPGA Security Manager class driver to
include an update/status sysfs node that can be polled
and read to monitor the progress of an ongoing secure
update. Sysfs_notify() is used to signal transitions
between different phases of the update process.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
  - Changed syntax of sec_mgr_prog_str[] array definition from:
	"idle",			/* FPGA_SEC_PROG_IDLE */
    to:
	[FPGA_SEC_PROG_IDLE]	    = "idle",
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
v6:
  - No change
v5:
  - Use new function sysfs_emit() in the status_show() function
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:
  - Use a local variable to read progress once in status_show()
  - Use dev_err to report invalid progress status
v2:
  - Bumped documentation date and version
  - Changed progress state "read_file" to "reading"
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      | 11 +++++
 drivers/fpga/fpga-sec-mgr.c                   | 42 +++++++++++++++++--
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index 36d1b6ba8d76..b962ad2cf18d 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -16,3 +16,14 @@ Description:	Write only. Write the filename of an image
 		BMC images, BMC firmware, Static Region images,
 		and Root Entry Hashes, and to cancel Code Signing
 		Keys (CSK).
+
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/status
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read-only. Returns a string describing the current
+		status of an update. The string will be one of the
+		following: idle, reading, preparing, writing,
+		programming. Userspace code can poll on this file,
+		as it will be signaled by sysfs_notify() on each
+		state change.
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index fe82feda6b3c..dec52c68fe16 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -23,6 +23,13 @@ struct fpga_sec_mgr_devres {
 
 #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
 
+static void update_progress(struct fpga_sec_mgr *smgr,
+			    enum fpga_sec_prog new_progress)
+{
+	smgr->progress = new_progress;
+	sysfs_notify(&smgr->dev.kobj, "update", "status");
+}
+
 static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
 			       enum fpga_sec_err err_code)
 {
@@ -33,7 +40,7 @@ static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
 static void progress_complete(struct fpga_sec_mgr *smgr)
 {
 	mutex_lock(&smgr->lock);
-	smgr->progress = FPGA_SEC_PROG_IDLE;
+	update_progress(smgr, FPGA_SEC_PROG_IDLE);
 	complete_all(&smgr->update_done);
 	mutex_unlock(&smgr->lock);
 }
@@ -61,14 +68,14 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 		goto release_fw_exit;
 	}
 
-	smgr->progress = FPGA_SEC_PROG_PREPARING;
+	update_progress(smgr, FPGA_SEC_PROG_PREPARING);
 	ret = smgr->sops->prepare(smgr);
 	if (ret != FPGA_SEC_ERR_NONE) {
 		fpga_sec_dev_error(smgr, ret);
 		goto modput_exit;
 	}
 
-	smgr->progress = FPGA_SEC_PROG_WRITING;
+	update_progress(smgr, FPGA_SEC_PROG_WRITING);
 	while (smgr->remaining_size) {
 		ret = smgr->sops->write_blk(smgr, offset);
 		if (ret != FPGA_SEC_ERR_NONE) {
@@ -79,7 +86,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 		offset = fw->size - smgr->remaining_size;
 	}
 
-	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
+	update_progress(smgr, FPGA_SEC_PROG_PROGRAMMING);
 	ret = smgr->sops->poll_complete(smgr);
 	if (ret != FPGA_SEC_ERR_NONE)
 		fpga_sec_dev_error(smgr, ret);
@@ -107,6 +114,32 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 	progress_complete(smgr);
 }
 
+static const char * const sec_mgr_prog_str[] = {
+	[FPGA_SEC_PROG_IDLE]	    = "idle",
+	[FPGA_SEC_PROG_READING]	    = "reading",
+	[FPGA_SEC_PROG_PREPARING]   = "preparing",
+	[FPGA_SEC_PROG_WRITING]	    = "writing",
+	[FPGA_SEC_PROG_PROGRAMMING] = "programming"
+};
+
+static ssize_t
+status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	const char *status = "unknown-status";
+	enum fpga_sec_prog progress;
+
+	progress = smgr->progress;
+	if (progress < FPGA_SEC_PROG_MAX)
+		status = sec_mgr_prog_str[progress];
+	else
+		dev_err(dev, "Invalid status during secure update: %d\n",
+			progress);
+
+	return sysfs_emit(buf, "%s\n", status);
+}
+static DEVICE_ATTR_RO(status);
+
 static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -141,6 +174,7 @@ static DEVICE_ATTR_WO(filename);
 
 static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
+	&dev_attr_status.attr,
 	NULL,
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
                   ` (2 preceding siblings ...)
  2021-05-03 21:35 ` [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:39   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size Russ Weight
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

Extend the FPGA Security Manager class driver to include
an update/error sysfs node that can be read for error
information when a secure update fails.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
  - Changed syntax of sec_mgr_err_str[] array definition from:
	"none",			/* FPGA_SEC_ERR_NONE */
    to:
	[FPGA_SEC_ERR_NONE]	    = "none",
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
v6:
  - No change
v5:
  - Use new function sysfs_emit() in the error_show() function
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:
  - Use dev_err() for invalid error code in sec_error()
v2:
  - Bumped documentation date and version
  - Added warning to sec_progress() for invalid progress status
  - Added sec_error() function (similar to sec_progress())
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      | 17 ++++
 drivers/fpga/fpga-sec-mgr.c                   | 83 ++++++++++++++++---
 include/linux/fpga/fpga-sec-mgr.h             |  1 +
 3 files changed, 89 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index b962ad2cf18d..24890d04521f 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -27,3 +27,20 @@ Description:	Read-only. Returns a string describing the current
 		programming. Userspace code can poll on this file,
 		as it will be signaled by sysfs_notify() on each
 		state change.
+
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/error
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read-only. Returns a string describing the failure
+		of a secure update. This string will be in the form
+		of <STATUS>:<ERROR>, where <STATUS> will be one of
+		the status strings described for the status sysfs
+		file and <ERROR> will be one of the following:
+		hw-error, timeout, user-abort, device-busy,
+		invalid-file-size, read-write-error, flash-wearout,
+		file-read-error.  The error sysfs file is only
+		meaningful when the secure update engine is in the
+		idle state. If this file is read while a secure
+		update is in progress, then the read will fail with
+		EBUSY.
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index dec52c68fe16..e43fa2797d27 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -30,10 +30,16 @@ static void update_progress(struct fpga_sec_mgr *smgr,
 	sysfs_notify(&smgr->dev.kobj, "update", "status");
 }
 
+static void set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_code)
+{
+	smgr->err_state = smgr->progress;
+	smgr->err_code = err_code;
+}
+
 static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
 			       enum fpga_sec_err err_code)
 {
-	smgr->err_code = err_code;
+	set_error(smgr, err_code);
 	smgr->sops->cancel(smgr);
 }
 
@@ -56,7 +62,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 
 	get_device(&smgr->dev);
 	if (request_firmware(&fw, smgr->filename, &smgr->dev)) {
-		smgr->err_code = FPGA_SEC_ERR_FILE_READ;
+		set_error(smgr, FPGA_SEC_ERR_FILE_READ);
 		goto idle_exit;
 	}
 
@@ -64,7 +70,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 	smgr->remaining_size = fw->size;
 
 	if (!try_module_get(smgr->dev.parent->driver->owner)) {
-		smgr->err_code = FPGA_SEC_ERR_BUSY;
+		set_error(smgr, FPGA_SEC_ERR_BUSY);
 		goto release_fw_exit;
 	}
 
@@ -122,24 +128,76 @@ static const char * const sec_mgr_prog_str[] = {
 	[FPGA_SEC_PROG_PROGRAMMING] = "programming"
 };
 
-static ssize_t
-status_show(struct device *dev, struct device_attribute *attr, char *buf)
+static const char * const sec_mgr_err_str[] = {
+	[FPGA_SEC_ERR_NONE]	    = "none",
+	[FPGA_SEC_ERR_HW_ERROR]	    = "hw-error",
+	[FPGA_SEC_ERR_TIMEOUT]	    = "timeout",
+	[FPGA_SEC_ERR_CANCELED]	    = "user-abort",
+	[FPGA_SEC_ERR_BUSY]	    = "device-busy",
+	[FPGA_SEC_ERR_INVALID_SIZE] = "invalid-file-size",
+	[FPGA_SEC_ERR_RW_ERROR]	    = "read-write-error",
+	[FPGA_SEC_ERR_WEAROUT]	    = "flash-wearout",
+	[FPGA_SEC_ERR_FILE_READ]    = "file-read-error"
+};
+
+static const char *sec_progress(struct device *dev, enum fpga_sec_prog prog)
 {
-	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
 	const char *status = "unknown-status";
-	enum fpga_sec_prog progress;
 
-	progress = smgr->progress;
-	if (progress < FPGA_SEC_PROG_MAX)
-		status = sec_mgr_prog_str[progress];
+	if (prog < FPGA_SEC_PROG_MAX)
+		status = sec_mgr_prog_str[prog];
 	else
 		dev_err(dev, "Invalid status during secure update: %d\n",
-			progress);
+			prog);
+
+	return status;
+}
+
+static const char *sec_error(struct device *dev, enum fpga_sec_err err_code)
+{
+	const char *error = "unknown-error";
+
+	if (err_code < FPGA_SEC_ERR_MAX)
+		error = sec_mgr_err_str[err_code];
+	else
+		dev_err(dev, "Invalid error code during secure update: %d\n",
+			err_code);
+
+	return error;
+}
+
+static ssize_t
+status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
 
-	return sysfs_emit(buf, "%s\n", status);
+	return sysfs_emit(buf, "%s\n", sec_progress(dev, smgr->progress));
 }
 static DEVICE_ATTR_RO(status);
 
+static ssize_t
+error_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	int ret;
+
+	mutex_lock(&smgr->lock);
+
+	if (smgr->progress != FPGA_SEC_PROG_IDLE)
+		ret = -EBUSY;
+	else if (!smgr->err_code)
+		ret = 0;
+	else
+		ret = sysfs_emit(buf, "%s:%s\n",
+				 sec_progress(dev, smgr->err_state),
+				 sec_error(dev, smgr->err_code));
+
+	mutex_unlock(&smgr->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(error);
+
 static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -175,6 +233,7 @@ static DEVICE_ATTR_WO(filename);
 static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
 	&dev_attr_status.attr,
+	&dev_attr_error.attr,
 	NULL,
 };
 
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
index 978ab98ffac5..6b7b8a3d6aac 100644
--- a/include/linux/fpga/fpga-sec-mgr.h
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -70,6 +70,7 @@ struct fpga_sec_mgr {
 	const u8 *data;			/* pointer to update data */
 	u32 remaining_size;		/* size remaining to transfer */
 	enum fpga_sec_prog progress;
+	enum fpga_sec_prog err_state;	/* progress state at time of failure */
 	enum fpga_sec_err err_code;	/* security manager error code */
 	bool driver_unload;
 	void *priv;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
                   ` (3 preceding siblings ...)
  2021-05-03 21:35 ` [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:39   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update Russ Weight
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

Extend the FPGA Security Manager class driver to include
an update/remaining_size sysfs node that can be read to
determine how much data remains to be transferred to the
secure update engine. This file can be used to monitor
progress during the "writing" phase of an update.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
v6:
  - No change
v5:
  - Use new function sysfs_emit() in the remaining_size_show() function
v4:
  - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
    and removed unnecessary references to "Intel".
  - Changed: imgr -> smgr, ifpga_ to fpga_
v3:
  - No change
v2:
  - Bumped documentation date and version
---
 Documentation/ABI/testing/sysfs-class-fpga-sec-mgr | 11 +++++++++++
 drivers/fpga/fpga-sec-mgr.c                        | 10 ++++++++++
 2 files changed, 21 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index 24890d04521f..c5d0b9d7c7e4 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -28,6 +28,17 @@ Description:	Read-only. Returns a string describing the current
 		as it will be signaled by sysfs_notify() on each
 		state change.
 
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/remaining_size
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read-only. Returns the size of data that remains to
+		be written to the secure update engine. The size
+		value is initialized to the full size of the file
+		image and the value is updated periodically during
+		the "writing" phase of the update.
+		Format: "%u".
+
 What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/error
 Date:		June 2021
 KernelVersion:	5.14
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index e43fa2797d27..2487042ace82 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -198,6 +198,15 @@ error_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(error);
 
+static ssize_t remaining_size_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+
+	return sysfs_emit(buf, "%u\n", smgr->remaining_size);
+}
+static DEVICE_ATTR_RO(remaining_size);
+
 static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 			      const char *buf, size_t count)
 {
@@ -234,6 +243,7 @@ static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
 	&dev_attr_status.attr,
 	&dev_attr_error.attr,
+	&dev_attr_remaining_size.attr,
 	NULL,
 };
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
                   ` (4 preceding siblings ...)
  2021-05-03 21:35 ` [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:40   ` Moritz Fischer
  2021-05-03 21:35 ` [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info Russ Weight
  2021-05-10 14:12 ` [PATCH v12 0/7] FPGA Security Manager Class Driver Tom Rix
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

Extend the FPGA Security Manager class driver to include
an update/cancel sysfs file that can be written to request
that an update be canceled. The write may return EBUSY if
the update has progressed to the point that it cannot be
canceled by software or ENODEV if there is no update in
progress.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
v6:
  - No change
v5:
  - No change
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:
  - No change
v2:
  - Bumped documentation date and version
  - Minor code cleanup per review comments 
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      | 10 ++++
 drivers/fpga/fpga-sec-mgr.c                   | 59 +++++++++++++++++--
 include/linux/fpga/fpga-sec-mgr.h             |  1 +
 3 files changed, 66 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index c5d0b9d7c7e4..749f2d4c78d3 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -17,6 +17,16 @@ Description:	Write only. Write the filename of an image
 		and Root Entry Hashes, and to cancel Code Signing
 		Keys (CSK).
 
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/cancel
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Write-only. Write a "1" to this file to request
+		that a current update be canceled. This request
+		will be rejected (EBUSY) if the programming phase
+		has already started or (ENODEV) if there is no
+		update in progress.
+
 What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/status
 Date:		June 2021
 KernelVersion:	5.14
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index 2487042ace82..35bd419bd3b9 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -43,6 +43,23 @@ static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
 	smgr->sops->cancel(smgr);
 }
 
+static int progress_transition(struct fpga_sec_mgr *smgr,
+			       enum fpga_sec_prog new_progress)
+{
+	int ret = 0;
+
+	mutex_lock(&smgr->lock);
+	if (smgr->request_cancel) {
+		set_error(smgr, FPGA_SEC_ERR_CANCELED);
+		smgr->sops->cancel(smgr);
+		ret = -ECANCELED;
+	} else {
+		update_progress(smgr, new_progress);
+	}
+	mutex_unlock(&smgr->lock);
+	return ret;
+}
+
 static void progress_complete(struct fpga_sec_mgr *smgr)
 {
 	mutex_lock(&smgr->lock);
@@ -74,15 +91,19 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 		goto release_fw_exit;
 	}
 
-	update_progress(smgr, FPGA_SEC_PROG_PREPARING);
+	if (progress_transition(smgr, FPGA_SEC_PROG_PREPARING))
+		goto modput_exit;
+
 	ret = smgr->sops->prepare(smgr);
 	if (ret != FPGA_SEC_ERR_NONE) {
 		fpga_sec_dev_error(smgr, ret);
 		goto modput_exit;
 	}
 
-	update_progress(smgr, FPGA_SEC_PROG_WRITING);
-	while (smgr->remaining_size) {
+	if (progress_transition(smgr, FPGA_SEC_PROG_WRITING))
+		goto done;
+
+	while (smgr->remaining_size && !smgr->request_cancel) {
 		ret = smgr->sops->write_blk(smgr, offset);
 		if (ret != FPGA_SEC_ERR_NONE) {
 			fpga_sec_dev_error(smgr, ret);
@@ -92,7 +113,9 @@ static void fpga_sec_mgr_update(struct work_struct *work)
 		offset = fw->size - smgr->remaining_size;
 	}
 
-	update_progress(smgr, FPGA_SEC_PROG_PROGRAMMING);
+	if (progress_transition(smgr, FPGA_SEC_PROG_PROGRAMMING))
+		goto done;
+
 	ret = smgr->sops->poll_complete(smgr);
 	if (ret != FPGA_SEC_ERR_NONE)
 		fpga_sec_dev_error(smgr, ret);
@@ -229,6 +252,7 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 	}
 
 	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->request_cancel = false;
 	smgr->progress = FPGA_SEC_PROG_READING;
 	reinit_completion(&smgr->update_done);
 	schedule_work(&smgr->work);
@@ -239,8 +263,32 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(filename);
 
+static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	bool cancel;
+	int ret = count;
+
+	if (kstrtobool(buf, &cancel) || !cancel)
+		return -EINVAL;
+
+	mutex_lock(&smgr->lock);
+	if (smgr->progress == FPGA_SEC_PROG_PROGRAMMING)
+		ret = -EBUSY;
+	else if (smgr->progress == FPGA_SEC_PROG_IDLE)
+		ret = -ENODEV;
+	else
+		smgr->request_cancel = true;
+	mutex_unlock(&smgr->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_WO(cancel);
+
 static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
+	&dev_attr_cancel.attr,
 	&dev_attr_status.attr,
 	&dev_attr_error.attr,
 	&dev_attr_remaining_size.attr,
@@ -464,6 +512,9 @@ void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
 		goto unregister;
 	}
 
+	if (smgr->progress != FPGA_SEC_PROG_PROGRAMMING)
+		smgr->request_cancel = true;
+
 	mutex_unlock(&smgr->lock);
 	wait_for_completion(&smgr->update_done);
 
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
index 6b7b8a3d6aac..0e1f50434024 100644
--- a/include/linux/fpga/fpga-sec-mgr.h
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -72,6 +72,7 @@ struct fpga_sec_mgr {
 	enum fpga_sec_prog progress;
 	enum fpga_sec_prog err_state;	/* progress state at time of failure */
 	enum fpga_sec_err err_code;	/* security manager error code */
+	bool request_cancel;
 	bool driver_unload;
 	void *priv;
 };
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
                   ` (5 preceding siblings ...)
  2021-05-03 21:35 ` [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update Russ Weight
@ 2021-05-03 21:35 ` Russ Weight
  2021-05-10 17:41   ` Moritz Fischer
  2021-05-10 14:12 ` [PATCH v12 0/7] FPGA Security Manager Class Driver Tom Rix
  7 siblings, 1 reply; 19+ messages in thread
From: Russ Weight @ 2021-05-03 21:35 UTC (permalink / raw)
  To: mdf, linux-fpga, linux-kernel
  Cc: trix, lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong,
	Russ Weight

Extend the FPGA Security Manager class driver to include
an optional update/hw_errinfo sysfs node that can be used
to retrieve 64 bits of device specific error information
following a secure update failure.

The underlying driver must provide a get_hw_errinfo() callback
function to enable this feature. This data is treated as
opaque by the class driver. It is left to user-space software
or support personnel to interpret this data.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v12:
  - Updated Date and KernelVersion fields in ABI documentation
v11:
  - No change
v10:
  - Rebased to 5.12-rc2 next
  - Updated Date and KernelVersion in ABI documentation
v9:
  - Updated Date and KernelVersion in ABI documentation
v8:
  - No change
v7:
  - Changed Date in documentation file to December 2020
v6:
  - No change
v5:
  - No change
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:
  - No change
v2:
  - Bumped documentation date and version
---
 .../ABI/testing/sysfs-class-fpga-sec-mgr      | 14 +++++++
 drivers/fpga/fpga-sec-mgr.c                   | 38 +++++++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  5 +++
 3 files changed, 57 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
index 749f2d4c78d3..f1881ce39c63 100644
--- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
@@ -65,3 +65,17 @@ Description:	Read-only. Returns a string describing the failure
 		idle state. If this file is read while a secure
 		update is in progress, then the read will fail with
 		EBUSY.
+
+What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/hw_errinfo
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read-only. Returns a 64 bit error value providing
+		hardware specific information that may be useful in
+		debugging errors that occur during FPGA image updates.
+		This file is only visible if the underlying device
+		supports it. The hw_errinfo value is only accessible
+		when the secure update engine is in the idle state.
+		If this file is read while a secure update is in
+		progress, then the read will fail with EBUSY.
+		Format: "0x%llx".
diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
index 35bd419bd3b9..3c59b142291d 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -36,10 +36,17 @@ static void set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_code)
 	smgr->err_code = err_code;
 }
 
+static void set_hw_errinfo(struct fpga_sec_mgr *smgr)
+{
+	if (smgr->sops->get_hw_errinfo)
+		smgr->hw_errinfo = smgr->sops->get_hw_errinfo(smgr);
+}
+
 static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
 			       enum fpga_sec_err err_code)
 {
 	set_error(smgr, err_code);
+	set_hw_errinfo(smgr);
 	smgr->sops->cancel(smgr);
 }
 
@@ -221,6 +228,23 @@ error_show(struct device *dev, struct device_attribute *attr, char *buf)
 }
 static DEVICE_ATTR_RO(error);
 
+static ssize_t
+hw_errinfo_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
+	int ret;
+
+	mutex_lock(&smgr->lock);
+	if (smgr->progress != FPGA_SEC_PROG_IDLE)
+		ret = -EBUSY;
+	else
+		ret = sysfs_emit(buf, "0x%llx\n", smgr->hw_errinfo);
+	mutex_unlock(&smgr->lock);
+
+	return ret;
+}
+static DEVICE_ATTR_RO(hw_errinfo);
+
 static ssize_t remaining_size_show(struct device *dev,
 				   struct device_attribute *attr, char *buf)
 {
@@ -252,6 +276,7 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
 	}
 
 	smgr->err_code = FPGA_SEC_ERR_NONE;
+	smgr->hw_errinfo = 0;
 	smgr->request_cancel = false;
 	smgr->progress = FPGA_SEC_PROG_READING;
 	reinit_completion(&smgr->update_done);
@@ -286,18 +311,31 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_WO(cancel);
 
+static umode_t
+sec_mgr_update_visible(struct kobject *kobj, struct attribute *attr, int n)
+{
+	struct fpga_sec_mgr *smgr = to_sec_mgr(kobj_to_dev(kobj));
+
+	if (attr == &dev_attr_hw_errinfo.attr && !smgr->sops->get_hw_errinfo)
+		return 0;
+
+	return attr->mode;
+}
+
 static struct attribute *sec_mgr_update_attrs[] = {
 	&dev_attr_filename.attr,
 	&dev_attr_cancel.attr,
 	&dev_attr_status.attr,
 	&dev_attr_error.attr,
 	&dev_attr_remaining_size.attr,
+	&dev_attr_hw_errinfo.attr,
 	NULL,
 };
 
 static struct attribute_group sec_mgr_update_attr_group = {
 	.name = "update",
 	.attrs = sec_mgr_update_attrs,
+	.is_visible = sec_mgr_update_visible,
 };
 
 static ssize_t name_show(struct device *dev,
diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
index 0e1f50434024..a99bfd28f38c 100644
--- a/include/linux/fpga/fpga-sec-mgr.h
+++ b/include/linux/fpga/fpga-sec-mgr.h
@@ -40,6 +40,9 @@ enum fpga_sec_err {
  *			    function and is called at the completion
  *			    of the update, whether success or failure,
  *			    if the prepare function succeeded.
+ * @get_hw_errinfo:	    Optional: Return u64 hw specific error info.
+ *			    The software err_code may used to determine
+ *			    whether the hw error info is applicable.
  */
 struct fpga_sec_mgr_ops {
 	enum fpga_sec_err (*prepare)(struct fpga_sec_mgr *smgr);
@@ -47,6 +50,7 @@ struct fpga_sec_mgr_ops {
 	enum fpga_sec_err (*poll_complete)(struct fpga_sec_mgr *smgr);
 	enum fpga_sec_err (*cancel)(struct fpga_sec_mgr *smgr);
 	void (*cleanup)(struct fpga_sec_mgr *smgr);
+	u64 (*get_hw_errinfo)(struct fpga_sec_mgr *smgr);
 };
 
 /* Update progress codes */
@@ -72,6 +76,7 @@ struct fpga_sec_mgr {
 	enum fpga_sec_prog progress;
 	enum fpga_sec_prog err_state;	/* progress state at time of failure */
 	enum fpga_sec_err err_code;	/* security manager error code */
+	u64 hw_errinfo;			/* 64 bits of HW specific error info */
 	bool request_cancel;
 	bool driver_unload;
 	void *priv;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 0/7] FPGA Security Manager Class Driver
  2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
                   ` (6 preceding siblings ...)
  2021-05-03 21:35 ` [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info Russ Weight
@ 2021-05-10 14:12 ` Tom Rix
  2021-05-10 17:37   ` Moritz Fischer
  7 siblings, 1 reply; 19+ messages in thread
From: Tom Rix @ 2021-05-10 14:12 UTC (permalink / raw)
  To: Russ Weight, mdf, linux-fpga, linux-kernel
  Cc: lgoncalv, yilun.xu, hao.wu, matthew.gerlach, richard.gong


On 5/3/21 2:35 PM, Russ Weight wrote:
> The FPGA Security Manager class driver provides a common
> API for user-space tools to manage updates for secure FPGA
> devices. Device drivers that instantiate the FPGA Security
> Manager class driver will interact with a HW secure update
> engine in order to transfer new FPGA and BMC images to FLASH so
> that they will be automatically loaded when the FPGA card reboots.

Russ,

These have my Reviewed-by, but since it has been a while, I am looking 
these over again.

If you do not hear anything from me in the next couple of days, please 
assume everything is fine.

Tom


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver
  2021-05-03 21:35 ` [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver Russ Weight
@ 2021-05-10 17:28   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:28 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

Hi,

On Mon, May 03, 2021 at 02:35:40PM -0700, Russ Weight wrote:
> 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. The image type is encoded in the image file
> and is decoded by the HW/FW secure update engine.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - Fixed grammatical error in Documentation/fpga/fpga-sec-mgr.rst
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - Removed sysfs support and documentation for the display of the
>     flash count, root entry hashes, and code-signing-key cancelation
>     vectors.
> 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      |   5 +
>  Documentation/fpga/fpga-sec-mgr.rst           |  44 +++
>  Documentation/fpga/index.rst                  |   1 +
>  MAINTAINERS                                   |   9 +
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   3 +
>  drivers/fpga/fpga-sec-mgr.c                   | 296 ++++++++++++++++++
>  include/linux/fpga/fpga-sec-mgr.h             |  44 +++
>  8 files changed, 411 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..2498aef0ac51
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -0,0 +1,5 @@
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/name
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Name of low level fpga security manager driver.
> diff --git a/Documentation/fpga/fpga-sec-mgr.rst b/Documentation/fpga/fpga-sec-mgr.rst
> new file mode 100644
> index 000000000000..9f74c29fe63d
> --- /dev/null
> +++ b/Documentation/fpga/fpga-sec-mgr.rst
> @@ -0,0 +1,44 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +========================================
> +FPGA Security Manager Class Driver
> +========================================
> +
> +The FPGA Security Manager class driver provides a common
> +API for user-space tools to manage updates for secure FPGA
> +devices. Device drivers that instantiate the Security
> +Manager class driver will interact with a HW secure update
> +engine in order to transfer new FPGA and BMC images to FLASH so
> +that they will be automatically loaded when the FPGA card reboots.
> +
> +A significant difference between the FPGA Manager and the FPGA
> +Security Manager is that the FPGA Manager does a live update (Partial
> +Reconfiguration) to a device, whereas the FPGA Security Manager
> +updates the FLASH images for the Static Region and the BMC so that
> +they will be loaded the next time the FPGA card boots. Security is
> +enforced by hardware and firmware. The security manager interacts
> +with the firmware to initiate an update, pass in the necessary data,
> +and collect status on the update.
> +
> +In addition to managing secure updates of the FPGA and BMC images,
> +the FPGA Security Manager update process may also be used to
> +program root entry hashes and cancellation keys for the FPGA static
> +region, the FPGA partial reconfiguration region, and the BMC.
> +
> +Secure updates make use of the request_firmware framework, which
> +requires that image files are accessible under /lib/firmware. A request
> +for a secure update returns immediately, while the update itself
> +proceeds in the context of a kernel worker thread. Sysfs files provide
> +a means for monitoring the progress of a secure update and for
> +retrieving error information in the event of a failure.
> +
> +Sysfs Attributes
> +================
> +
> +The API includes a sysfs entry *name* to export the name of the parent
> +driver. It also includes an *update* sub-directory that can be used to
> +instantiate and monitor a secure update.
> +
> +See `<../ABI/testing/sysfs-class-fpga-sec-mgr>`__ for a full
> +description of the sysfs attributes for the FPGA Security
> +Manager.
> diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst
> index f80f95667ca2..0b2f427042af 100644
> --- a/Documentation/fpga/index.rst
> +++ b/Documentation/fpga/index.rst
> @@ -8,6 +8,7 @@ fpga
>      :maxdepth: 1
>  
>      dfl
> +    fpga-sec-mgr
>  
>  .. only::  subproject and html
>  
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1783372a608a..3b1dc0376b52 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7146,6 +7146,15 @@ F:	Documentation/fpga/
>  F:	drivers/fpga/
>  F:	include/linux/fpga/
>  
> +FPGA SECURITY MANAGER DRIVERS
> +M:	Russ Weight <russell.h.weight@intel.com>
> +L:	linux-fpga@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +F:	Documentation/fpga/fpga-sec-mgr.rst
> +F:	drivers/fpga/fpga-sec-mgr.c
> +F:	include/linux/fpga/fpga-sec-mgr.h
> +
>  FPU EMULATOR
>  M:	Bill Metzenthen <billm@melbpc.org.au>
>  S:	Maintained
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 33e15058d0dc..09a8d915db26 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -234,4 +234,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>  	  to configure the programmable logic(PL) through PS
>  	  on ZynqMP SoC.
>  
> +config FPGA_SEC_MGR
> +	tristate "FPGA Security Manager"
> +	help
> +	  The Security Manager class driver presents a common
> +	  user API for managing secure updates for FPGA
> +	  devices, including flash images for the FPGA static
> +	  region and for the BMC. Select this option to enable
> +	  updates for secure FPGA devices.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 18dc9885883a..22576d1a3996 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  
> +# FPGA Security Manager Framework
> +obj-$(CONFIG_FPGA_SEC_MGR)		+= fpga-sec-mgr.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> new file mode 100644
> index 000000000000..468379e0c825
> --- /dev/null
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -0,0 +1,296 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Security Manager
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +
> +#include <linux/fpga/fpga-sec-mgr.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +
> +static DEFINE_IDA(fpga_sec_mgr_ida);
> +static struct class *fpga_sec_mgr_class;
> +
> +struct fpga_sec_mgr_devres {
> +	struct fpga_sec_mgr *smgr;
> +};
> +
> +#define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +
> +	return sysfs_emit(buf, "%s\n", smgr->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *sec_mgr_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_attr_group = {
> +	.attrs = sec_mgr_attrs,
> +};
> +
> +static const struct attribute_group *fpga_sec_mgr_attr_groups[] = {
> +	&sec_mgr_attr_group,
> +	NULL,
> +};
> +
> +/**
> + * fpga_sec_mgr_create - create and initialize an FPGA
> + *			  security manager struct
> + *
> + * @dev:  fpga security manager device from pdev
> + * @name: fpga security manager name
> + * @sops: pointer to a structure of fpga callback functions
> + * @priv: fpga security manager private data
> + *
> + * The caller of this function is responsible for freeing the struct
> + * with ifpg_sec_mgr_free(). Using devm_fpga_sec_mgr_create() instead
> + * is recommended.
> + *
> + * Return: pointer to struct fpga_sec_mgr or NULL
> + */
> +struct fpga_sec_mgr *
> +fpga_sec_mgr_create(struct device *dev, const char *name,
> +		    const struct fpga_sec_mgr_ops *sops, void *priv)
> +{
> +	struct fpga_sec_mgr *smgr;
> +	int id, ret;
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(dev, "Attempt to register with no name!\n");
> +		return NULL;
> +	}
> +
> +	smgr = kzalloc(sizeof(*smgr), GFP_KERNEL);
> +	if (!smgr)
> +		return NULL;
> +
> +	id = ida_simple_get(&fpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0)
> +		goto error_kfree;
> +
> +	mutex_init(&smgr->lock);
> +
> +	smgr->name = name;
> +	smgr->priv = priv;
> +	smgr->sops = sops;
> +
> +	device_initialize(&smgr->dev);
> +	smgr->dev.class = fpga_sec_mgr_class;
> +	smgr->dev.parent = dev;
> +	smgr->dev.id = id;
> +
> +	ret = dev_set_name(&smgr->dev, "fpga_sec%d", id);
> +	if (ret) {
> +		dev_err(dev, "Failed to set device name: fpga_sec%d\n", id);
> +		goto error_device;
> +	}
> +
> +	return smgr;
> +
> +error_device:
> +	ida_simple_remove(&fpga_sec_mgr_ida, id);
> +
> +error_kfree:
> +	kfree(smgr);
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
> +
> +/**
> + * fpga_sec_mgr_free - free an FPGA security manager created
> + *			with fpga_sec_mgr_create()
> + *
> + * @smgr:	FPGA security manager structure
> + */
> +void fpga_sec_mgr_free(struct fpga_sec_mgr *smgr)
> +{
> +	ida_simple_remove(&fpga_sec_mgr_ida, smgr->dev.id);
> +	kfree(smgr);
> +}
> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_free);
> +
> +static void devm_fpga_sec_mgr_release(struct device *dev, void *res)
> +{
> +	struct fpga_sec_mgr_devres *dr = res;
> +
> +	fpga_sec_mgr_free(dr->smgr);
> +}
> +
> +/**
> + * devm_fpga_sec_mgr_create - create and initialize an FPGA
> + *			       security manager struct
> + *
> + * @dev:  fpga security manager device from pdev
> + * @name: fpga security manager name
> + * @sops: pointer to a structure of fpga callback functions
> + * @priv: fpga security manager private data
> + *
> + * This function is intended for use in a FPGA Security manager
> + * driver's probe function.  After the security manager driver creates
> + * the fpga_sec_mgr struct with devm_fpga_sec_mgr_create(), it should
> + * register it with devm_fpga_sec_mgr_register().
> + * The fpga_sec_mgr struct allocated with this function will be freed
> + * automatically on driver detach.
> + *
> + * Return: pointer to struct fpga_sec_mgr or NULL
> + */
> +struct fpga_sec_mgr *
> +devm_fpga_sec_mgr_create(struct device *dev, const char *name,
> +			 const struct fpga_sec_mgr_ops *sops, void *priv)
> +{
> +	struct fpga_sec_mgr_devres *dr;
> +
> +	dr = devres_alloc(devm_fpga_sec_mgr_release, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return NULL;
> +
> +	dr->smgr = fpga_sec_mgr_create(dev, name, sops, priv);
> +	if (!dr->smgr) {
> +		devres_free(dr);
> +		return NULL;
> +	}
> +
> +	devres_add(dev, dr);
> +
> +	return dr->smgr;
> +}
> +EXPORT_SYMBOL_GPL(devm_fpga_sec_mgr_create);
> +
> +/**
> + * fpga_sec_mgr_register - register an FPGA security manager
> + *
> + * @smgr: fpga security manager struct
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int fpga_sec_mgr_register(struct fpga_sec_mgr *smgr)
> +{
> +	int ret;
> +
> +	ret = device_add(&smgr->dev);
> +	if (ret)
> +		goto error_device;
> +
> +	dev_info(&smgr->dev, "%s registered\n", smgr->name);
> +
> +	return 0;
> +
> +error_device:
> +	ida_simple_remove(&fpga_sec_mgr_ida, smgr->dev.id);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_register);
> +
> +/**
> + * fpga_sec_mgr_unregister - unregister an FPGA security manager
> + *
> + * @mgr: fpga manager struct
> + *
> + * This function is intended for use in an FPGA security manager
> + * driver's remove() function.
> + */
> +void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
> +{
> +	dev_info(&smgr->dev, "%s %s\n", __func__, smgr->name);
> +
> +	device_unregister(&smgr->dev);
> +}
> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_unregister);
> +
> +static int fpga_sec_mgr_devres_match(struct device *dev, void *res,
> +				     void *match_data)
> +{
> +	struct fpga_sec_mgr_devres *dr = res;
> +
> +	return match_data == dr->smgr;
> +}
> +
> +static void devm_fpga_sec_mgr_unregister(struct device *dev, void *res)
> +{
> +	struct fpga_sec_mgr_devres *dr = res;
> +
> +	fpga_sec_mgr_unregister(dr->smgr);
> +}
> +
> +/**
> + * devm_fpga_sec_mgr_register - resource managed variant of
> + *				fpga_sec_mgr_register()
> + *
> + * @dev: managing device for this FPGA security manager
> + * @smgr: fpga security manager struct
> + *
> + * This is the devres variant of fpga_sec_mgr_register() for which the
> + * unregister function will be called automatically when the managing
> + * device is detached.
> + */
> +int devm_fpga_sec_mgr_register(struct device *dev, struct fpga_sec_mgr *smgr)
> +{
> +	struct fpga_sec_mgr_devres *dr;
> +	int ret;
> +
> +	/*
> +	 * Make sure that the struct fpga_sec_mgr * that is passed in is
> +	 * managed itself.
> +	 */
> +	if (WARN_ON(!devres_find(dev, devm_fpga_sec_mgr_release,
> +				 fpga_sec_mgr_devres_match, smgr)))
> +		return -EINVAL;
> +
> +	dr = devres_alloc(devm_fpga_sec_mgr_unregister, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	ret = fpga_sec_mgr_register(smgr);
> +	if (ret) {
> +		devres_free(dr);
> +		return ret;
> +	}
> +
> +	dr->smgr = smgr;
> +	devres_add(dev, dr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_fpga_sec_mgr_register);
> +
> +static void fpga_sec_mgr_dev_release(struct device *dev)
> +{
> +}
> +
> +static int __init fpga_sec_mgr_class_init(void)
> +{
> +	pr_info("FPGA Security Manager\n");
> +
> +	fpga_sec_mgr_class = class_create(THIS_MODULE, "fpga_sec_mgr");
> +	if (IS_ERR(fpga_sec_mgr_class))
> +		return PTR_ERR(fpga_sec_mgr_class);
> +
> +	fpga_sec_mgr_class->dev_groups = fpga_sec_mgr_attr_groups;
> +	fpga_sec_mgr_class->dev_release = fpga_sec_mgr_dev_release;
> +
> +	return 0;
> +}
> +
> +static void __exit fpga_sec_mgr_class_exit(void)
> +{
> +	class_destroy(fpga_sec_mgr_class);
> +	ida_destroy(&fpga_sec_mgr_ida);
> +}
> +
> +MODULE_DESCRIPTION("FPGA Security Manager Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +subsys_initcall(fpga_sec_mgr_class_init);
> +module_exit(fpga_sec_mgr_class_exit)
> diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
> new file mode 100644
> index 000000000000..f85665b79b9d
> --- /dev/null
> +++ b/include/linux/fpga/fpga-sec-mgr.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for FPGA Security Manager
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +#ifndef _LINUX_FPGA_SEC_MGR_H
> +#define _LINUX_FPGA_SEC_MGR_H
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct fpga_sec_mgr;
> +
> +/**
> + * struct fpga_sec_mgr_ops - device specific operations
> + */
> +struct fpga_sec_mgr_ops {
> +};
> +
> +struct fpga_sec_mgr {
> +	const char *name;
> +	struct device dev;
> +	const struct fpga_sec_mgr_ops *sops;
> +	struct mutex lock;		/* protect data structure contents */
> +	void *priv;
> +};
> +
> +struct fpga_sec_mgr *
> +fpga_sec_mgr_create(struct device *dev, const char *name,
> +		    const struct fpga_sec_mgr_ops *sops, void *priv);
> +
> +struct fpga_sec_mgr *
> +devm_fpga_sec_mgr_create(struct device *dev, const char *name,
> +			 const struct fpga_sec_mgr_ops *sops, void *priv);
> +
> +int fpga_sec_mgr_register(struct fpga_sec_mgr *smgr);
> +int devm_fpga_sec_mgr_register(struct device *dev,
> +			       struct fpga_sec_mgr *smgr);
> +void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr);
> +void fpga_sec_mgr_free(struct fpga_sec_mgr *smgr);
> +
> +#endif
> -- 
> 2.25.1
> 

Looks good to me.

- Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 2/7] fpga: sec-mgr: enable secure updates
  2021-05-03 21:35 ` [PATCH v12 2/7] fpga: sec-mgr: enable secure updates Russ Weight
@ 2021-05-10 17:32   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:32 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:41PM -0700, Russ Weight wrote:
> 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.
> 
> The write of the filename will return immediately, and the
> update will begin in the context of a kernel worker thread.
> This tool utilizes the request_firmware framework, which
> requires that the image file reside under /lib/firmware.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
>   - Removed size parameter from write_blk() op - it is now up to
>     the lower-level driver to determine the appropriate size and
>     to update smgr->remaining_size accordingly.
> v11:
>   - Fixed a spelling error in a comment
>   - Initialize smgr->err_code and smgr->progress explicitly in
>     fpga_sec_mgr_create() instead of accepting the default 0 value.
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
>   - Changed filename_store() to use kmemdup_nul() instead of
>     kstrndup() and changed the count to not assume a line-return.
> v6:
>   - Changed "security update" to "secure update" in commit message
> v5:
>   - When checking the return values for functions of type enum
>     fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
> 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:
>   - Removed unnecessary "goto done"
>   - Added a comment to explain imgr->driver_unload in
>     ifpga_sec_mgr_unregister()
> v2:
>   - Bumped documentation date and version
>   - Removed explicit value assignments in enums
>   - Other minor code cleanup per review comments 
> ---
>  .../ABI/testing/sysfs-class-fpga-sec-mgr      |  13 ++
>  drivers/fpga/fpga-sec-mgr.c                   | 160 ++++++++++++++++++
>  include/linux/fpga/fpga-sec-mgr.h             |  48 ++++++
>  3 files changed, 221 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index 2498aef0ac51..36d1b6ba8d76 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -3,3 +3,16 @@ Date:		June 2021
>  KernelVersion:	5.14
>  Contact:	Russ Weight <russell.h.weight@intel.com>
>  Description:	Name of low level fpga security manager driver.
> +
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/filename
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Write only. Write the filename of an image
> +		file to this sysfs file to initiate a secure
> +		update. The file must have an appropriate header
> +		which, among other things, identifies the target
> +		for the update. This mechanism is used to update
> +		BMC images, BMC firmware, Static Region images,
> +		and Root Entry Hashes, and to cancel Code Signing
> +		Keys (CSK).
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index 468379e0c825..fe82feda6b3c 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -5,8 +5,11 @@
>   * Copyright (C) 2019-2020 Intel Corporation, Inc.
>   */
>  
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
>  #include <linux/fpga/fpga-sec-mgr.h>
>  #include <linux/idr.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
> @@ -20,6 +23,132 @@ struct fpga_sec_mgr_devres {
>  
>  #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
>  
> +static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
> +			       enum fpga_sec_err err_code)
> +{
> +	smgr->err_code = err_code;
> +	smgr->sops->cancel(smgr);
> +}
> +
> +static void progress_complete(struct fpga_sec_mgr *smgr)
> +{
> +	mutex_lock(&smgr->lock);
> +	smgr->progress = FPGA_SEC_PROG_IDLE;
> +	complete_all(&smgr->update_done);
> +	mutex_unlock(&smgr->lock);
> +}
> +
> +static void fpga_sec_mgr_update(struct work_struct *work)
> +{
> +	struct fpga_sec_mgr *smgr;
> +	const struct firmware *fw;
> +	enum fpga_sec_err ret;
> +	u32 offset = 0;
> +
> +	smgr = container_of(work, struct fpga_sec_mgr, work);
> +
> +	get_device(&smgr->dev);
> +	if (request_firmware(&fw, smgr->filename, &smgr->dev)) {
> +		smgr->err_code = FPGA_SEC_ERR_FILE_READ;
> +		goto idle_exit;
> +	}
> +
> +	smgr->data = fw->data;
> +	smgr->remaining_size = fw->size;
> +
> +	if (!try_module_get(smgr->dev.parent->driver->owner)) {
> +		smgr->err_code = FPGA_SEC_ERR_BUSY;
> +		goto release_fw_exit;
> +	}
> +
> +	smgr->progress = FPGA_SEC_PROG_PREPARING;
> +	ret = smgr->sops->prepare(smgr);
> +	if (ret != FPGA_SEC_ERR_NONE) {
> +		fpga_sec_dev_error(smgr, ret);
> +		goto modput_exit;
> +	}
> +
> +	smgr->progress = FPGA_SEC_PROG_WRITING;
> +	while (smgr->remaining_size) {
> +		ret = smgr->sops->write_blk(smgr, offset);
> +		if (ret != FPGA_SEC_ERR_NONE) {
> +			fpga_sec_dev_error(smgr, ret);
> +			goto done;
> +		}
> +
> +		offset = fw->size - smgr->remaining_size;
> +	}
> +
> +	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
> +	ret = smgr->sops->poll_complete(smgr);
> +	if (ret != FPGA_SEC_ERR_NONE)
> +		fpga_sec_dev_error(smgr, ret);
> +
> +done:
> +	if (smgr->sops->cleanup)
> +		smgr->sops->cleanup(smgr);
> +
> +modput_exit:
> +	module_put(smgr->dev.parent->driver->owner);
> +
> +release_fw_exit:
> +	smgr->data = NULL;
> +	release_firmware(fw);
> +
> +idle_exit:
> +	/*
> +	 * Note: smgr->remaining_size is left unmodified here to
> +	 * provide additional information on errors. It will be
> +	 * reinitialized when the next secure update begins.
> +	 */
> +	kfree(smgr->filename);
> +	smgr->filename = NULL;
> +	put_device(&smgr->dev);
> +	progress_complete(smgr);
> +}
> +
> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	int ret = count;
> +
> +	if (count == 0 || count >= PATH_MAX)
> +		return -EINVAL;

Nit, consider if (!count || count >= PATH_MAX)
> +
> +	mutex_lock(&smgr->lock);
> +	if (smgr->driver_unload || smgr->progress != FPGA_SEC_PROG_IDLE) {
> +		ret = -EBUSY;
> +		goto unlock_exit;
> +	}
> +
> +	smgr->filename = kmemdup_nul(buf, count, GFP_KERNEL);
> +	if (!smgr->filename) {
> +		ret = -ENOMEM;
> +		goto unlock_exit;
> +	}
> +
> +	smgr->err_code = FPGA_SEC_ERR_NONE;
> +	smgr->progress = FPGA_SEC_PROG_READING;
> +	reinit_completion(&smgr->update_done);
> +	schedule_work(&smgr->work);
> +
> +unlock_exit:
> +	mutex_unlock(&smgr->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(filename);
> +
> +static struct attribute *sec_mgr_update_attrs[] = {
> +	&dev_attr_filename.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_update_attr_group = {
> +	.name = "update",
> +	.attrs = sec_mgr_update_attrs,
> +};
> +
>  static ssize_t name_show(struct device *dev,
>  			 struct device_attribute *attr, char *buf)
>  {
> @@ -40,6 +169,7 @@ static struct attribute_group sec_mgr_attr_group = {
>  
>  static const struct attribute_group *fpga_sec_mgr_attr_groups[] = {
>  	&sec_mgr_attr_group,
> +	&sec_mgr_update_attr_group,
>  	NULL,
>  };
>  
> @@ -65,6 +195,12 @@ fpga_sec_mgr_create(struct device *dev, const char *name,
>  	struct fpga_sec_mgr *smgr;
>  	int id, ret;
>  
> +	if (!sops || !sops->cancel || !sops->prepare ||
> +	    !sops->write_blk || !sops->poll_complete) {
> +		dev_err(dev, "Attempt to register without required ops\n");
Nit: Consider "Attempt to register without all required ops"
> +		return NULL;
> +	}
> +
>  	if (!name || !strlen(name)) {
>  		dev_err(dev, "Attempt to register with no name!\n");
>  		return NULL;
> @@ -83,6 +219,10 @@ fpga_sec_mgr_create(struct device *dev, const char *name,
>  	smgr->name = name;
>  	smgr->priv = priv;
>  	smgr->sops = sops;
> +	smgr->err_code = FPGA_SEC_ERR_NONE;
> +	smgr->progress = FPGA_SEC_PROG_IDLE;
> +	init_completion(&smgr->update_done);
> +	INIT_WORK(&smgr->work, fpga_sec_mgr_update);
>  
>  	device_initialize(&smgr->dev);
>  	smgr->dev.class = fpga_sec_mgr_class;
> @@ -200,11 +340,31 @@ EXPORT_SYMBOL_GPL(fpga_sec_mgr_register);
>   *
>   * This function is intended for use in an FPGA security manager
>   * driver's remove() function.
> + *
> + * For some devices, once the secure update has begun authentication
> + * the hardware cannot be signaled to stop, and the driver will not
> + * exit until the hardware signals completion.  This could be 30+
> + * minutes of waiting. The driver_unload flag enables a force-unload
> + * of the driver (e.g. modprobe -r) by signaling the parent driver to
> + * exit even if the hardware update is incomplete. The driver_unload
> + * flag also prevents new updates from starting once the unregister
> + * process has begun.
>   */
>  void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
>  {
>  	dev_info(&smgr->dev, "%s %s\n", __func__, smgr->name);
>  
> +	mutex_lock(&smgr->lock);
> +	smgr->driver_unload = true;
> +	if (smgr->progress == FPGA_SEC_PROG_IDLE) {
> +		mutex_unlock(&smgr->lock);
> +		goto unregister;
> +	}
> +
> +	mutex_unlock(&smgr->lock);
> +	wait_for_completion(&smgr->update_done);
> +
> +unregister:
>  	device_unregister(&smgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_sec_mgr_unregister);
> diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
> index f85665b79b9d..978ab98ffac5 100644
> --- a/include/linux/fpga/fpga-sec-mgr.h
> +++ b/include/linux/fpga/fpga-sec-mgr.h
> @@ -7,16 +7,56 @@
>  #ifndef _LINUX_FPGA_SEC_MGR_H
>  #define _LINUX_FPGA_SEC_MGR_H
>  
> +#include <linux/completion.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/types.h>
>  
>  struct fpga_sec_mgr;
>  
> +enum fpga_sec_err {
> +	FPGA_SEC_ERR_NONE,
> +	FPGA_SEC_ERR_HW_ERROR,
> +	FPGA_SEC_ERR_TIMEOUT,
> +	FPGA_SEC_ERR_CANCELED,
> +	FPGA_SEC_ERR_BUSY,
> +	FPGA_SEC_ERR_INVALID_SIZE,
> +	FPGA_SEC_ERR_RW_ERROR,
> +	FPGA_SEC_ERR_WEAROUT,
> +	FPGA_SEC_ERR_FILE_READ,
> +	FPGA_SEC_ERR_MAX
> +};
> +
>  /**
>   * struct fpga_sec_mgr_ops - device specific operations
> + * @prepare:		    Required: Prepare secure update
> + * @write_blk:		    Required: Write a block of data
> + * @poll_complete:	    Required: Check for the completion of the
> + *			    HW authentication/programming process. This
> + *			    function should check for smgr->driver_unload
> + *			    and abort with FPGA_SEC_ERR_CANCELED when true.
> + * @cancel:		    Required: Signal HW to cancel update
> + * @cleanup:		    Optional: Complements the prepare()
> + *			    function and is called at the completion
> + *			    of the update, whether success or failure,
> + *			    if the prepare function succeeded.
>   */
>  struct fpga_sec_mgr_ops {
> +	enum fpga_sec_err (*prepare)(struct fpga_sec_mgr *smgr);
> +	enum fpga_sec_err (*write_blk)(struct fpga_sec_mgr *smgr, u32 offset);
> +	enum fpga_sec_err (*poll_complete)(struct fpga_sec_mgr *smgr);
> +	enum fpga_sec_err (*cancel)(struct fpga_sec_mgr *smgr);
> +	void (*cleanup)(struct fpga_sec_mgr *smgr);
> +};
> +
> +/* Update progress codes */
> +enum fpga_sec_prog {
> +	FPGA_SEC_PROG_IDLE,
> +	FPGA_SEC_PROG_READING,
> +	FPGA_SEC_PROG_PREPARING,
> +	FPGA_SEC_PROG_WRITING,
> +	FPGA_SEC_PROG_PROGRAMMING,
> +	FPGA_SEC_PROG_MAX
>  };
>  
>  struct fpga_sec_mgr {
> @@ -24,6 +64,14 @@ struct fpga_sec_mgr {
>  	struct device dev;
>  	const struct fpga_sec_mgr_ops *sops;
>  	struct mutex lock;		/* protect data structure contents */
> +	struct work_struct work;
> +	struct completion update_done;
> +	char *filename;
> +	const u8 *data;			/* pointer to update data */
> +	u32 remaining_size;		/* size remaining to transfer */
> +	enum fpga_sec_prog progress;
> +	enum fpga_sec_err err_code;	/* security manager error code */
> +	bool driver_unload;
>  	void *priv;
>  };
>  
> -- 
> 2.25.1
> 

Looks good to me,
- Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status
  2021-05-03 21:35 ` [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status Russ Weight
@ 2021-05-10 17:36   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:36 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:42PM -0700, Russ Weight wrote:
> Extend the FPGA Security Manager class driver to
> include an update/status sysfs node that can be polled
> and read to monitor the progress of an ongoing secure
> update. Sysfs_notify() is used to signal transitions
> between different phases of the update process.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
>   - Changed syntax of sec_mgr_prog_str[] array definition from:
> 	"idle",			/* FPGA_SEC_PROG_IDLE */
>     to:
> 	[FPGA_SEC_PROG_IDLE]	    = "idle",
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - No change
> v5:
>   - Use new function sysfs_emit() in the status_show() function
> 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:
>   - Use a local variable to read progress once in status_show()
>   - Use dev_err to report invalid progress status
> v2:
>   - Bumped documentation date and version
>   - Changed progress state "read_file" to "reading"
> ---
>  .../ABI/testing/sysfs-class-fpga-sec-mgr      | 11 +++++
>  drivers/fpga/fpga-sec-mgr.c                   | 42 +++++++++++++++++--
>  2 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index 36d1b6ba8d76..b962ad2cf18d 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -16,3 +16,14 @@ Description:	Write only. Write the filename of an image
>  		BMC images, BMC firmware, Static Region images,
>  		and Root Entry Hashes, and to cancel Code Signing
>  		Keys (CSK).
> +
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/status
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns a string describing the current
> +		status of an update. The string will be one of the
> +		following: idle, reading, preparing, writing,
> +		programming. Userspace code can poll on this file,
> +		as it will be signaled by sysfs_notify() on each
> +		state change.
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index fe82feda6b3c..dec52c68fe16 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -23,6 +23,13 @@ struct fpga_sec_mgr_devres {
>  
>  #define to_sec_mgr(d) container_of(d, struct fpga_sec_mgr, dev)
>  
> +static void update_progress(struct fpga_sec_mgr *smgr,
> +			    enum fpga_sec_prog new_progress)
> +{
> +	smgr->progress = new_progress;
> +	sysfs_notify(&smgr->dev.kobj, "update", "status");
> +}
> +
>  static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>  			       enum fpga_sec_err err_code)
>  {
> @@ -33,7 +40,7 @@ static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>  static void progress_complete(struct fpga_sec_mgr *smgr)
>  {
>  	mutex_lock(&smgr->lock);
> -	smgr->progress = FPGA_SEC_PROG_IDLE;
> +	update_progress(smgr, FPGA_SEC_PROG_IDLE);
>  	complete_all(&smgr->update_done);
>  	mutex_unlock(&smgr->lock);
>  }
> @@ -61,14 +68,14 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  		goto release_fw_exit;
>  	}
>  
> -	smgr->progress = FPGA_SEC_PROG_PREPARING;
> +	update_progress(smgr, FPGA_SEC_PROG_PREPARING);
>  	ret = smgr->sops->prepare(smgr);
>  	if (ret != FPGA_SEC_ERR_NONE) {
>  		fpga_sec_dev_error(smgr, ret);
>  		goto modput_exit;
>  	}
>  
> -	smgr->progress = FPGA_SEC_PROG_WRITING;
> +	update_progress(smgr, FPGA_SEC_PROG_WRITING);
>  	while (smgr->remaining_size) {
>  		ret = smgr->sops->write_blk(smgr, offset);
>  		if (ret != FPGA_SEC_ERR_NONE) {
> @@ -79,7 +86,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  		offset = fw->size - smgr->remaining_size;
>  	}
>  
> -	smgr->progress = FPGA_SEC_PROG_PROGRAMMING;
> +	update_progress(smgr, FPGA_SEC_PROG_PROGRAMMING);
>  	ret = smgr->sops->poll_complete(smgr);
>  	if (ret != FPGA_SEC_ERR_NONE)
>  		fpga_sec_dev_error(smgr, ret);
> @@ -107,6 +114,32 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  	progress_complete(smgr);
>  }
>  
> +static const char * const sec_mgr_prog_str[] = {
> +	[FPGA_SEC_PROG_IDLE]	    = "idle",
> +	[FPGA_SEC_PROG_READING]	    = "reading",
> +	[FPGA_SEC_PROG_PREPARING]   = "preparing",
> +	[FPGA_SEC_PROG_WRITING]	    = "writing",
> +	[FPGA_SEC_PROG_PROGRAMMING] = "programming"
> +};
> +
> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	const char *status = "unknown-status";
> +	enum fpga_sec_prog progress;
> +
> +	progress = smgr->progress;
> +	if (progress < FPGA_SEC_PROG_MAX)
> +		status = sec_mgr_prog_str[progress];
> +	else
> +		dev_err(dev, "Invalid status during secure update: %d\n",
> +			progress);
> +
> +	return sysfs_emit(buf, "%s\n", status);
> +}
> +static DEVICE_ATTR_RO(status);
> +
>  static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -141,6 +174,7 @@ static DEVICE_ATTR_WO(filename);
>  
>  static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
> +	&dev_attr_status.attr,
>  	NULL,
>  };
>  
> -- 
> 2.25.1
> 
Looks good to me,

- Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 0/7] FPGA Security Manager Class Driver
  2021-05-10 14:12 ` [PATCH v12 0/7] FPGA Security Manager Class Driver Tom Rix
@ 2021-05-10 17:37   ` Moritz Fischer
  2021-05-10 18:52     ` Tom Rix
  0 siblings, 1 reply; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:37 UTC (permalink / raw)
  To: Tom Rix
  Cc: Russ Weight, mdf, linux-fpga, linux-kernel, lgoncalv, yilun.xu,
	hao.wu, matthew.gerlach, richard.gong

On Mon, May 10, 2021 at 07:12:57AM -0700, Tom Rix wrote:
> 
> On 5/3/21 2:35 PM, Russ Weight wrote:
> > The FPGA Security Manager class driver provides a common
> > API for user-space tools to manage updates for secure FPGA
> > devices. Device drivers that instantiate the FPGA Security
> > Manager class driver will interact with a HW secure update
> > engine in order to transfer new FPGA and BMC images to FLASH so
> > that they will be automatically loaded when the FPGA card reboots.
> 
> Russ,
> 
> These have my Reviewed-by, but since it has been a while, I am looking these
> over again.
> 
> If you do not hear anything from me in the next couple of days, please
> assume everything is fine.
> 
> Tom
> 

I'll do one more one-over, if it looks good will apply end of the week,

- Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors
  2021-05-03 21:35 ` [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors Russ Weight
@ 2021-05-10 17:39   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:39 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:43PM -0700, Russ Weight wrote:
> Extend the FPGA Security Manager class driver to include
> an update/error sysfs node that can be read for error
> information when a secure update fails.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
>   - Changed syntax of sec_mgr_err_str[] array definition from:
> 	"none",			/* FPGA_SEC_ERR_NONE */
>     to:
> 	[FPGA_SEC_ERR_NONE]	    = "none",
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - No change
> v5:
>   - Use new function sysfs_emit() in the error_show() function
> 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:
>   - Use dev_err() for invalid error code in sec_error()
> v2:
>   - Bumped documentation date and version
>   - Added warning to sec_progress() for invalid progress status
>   - Added sec_error() function (similar to sec_progress())
> ---
>  .../ABI/testing/sysfs-class-fpga-sec-mgr      | 17 ++++
>  drivers/fpga/fpga-sec-mgr.c                   | 83 ++++++++++++++++---
>  include/linux/fpga/fpga-sec-mgr.h             |  1 +
>  3 files changed, 89 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index b962ad2cf18d..24890d04521f 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -27,3 +27,20 @@ Description:	Read-only. Returns a string describing the current
>  		programming. Userspace code can poll on this file,
>  		as it will be signaled by sysfs_notify() on each
>  		state change.
> +
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/error
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns a string describing the failure
> +		of a secure update. This string will be in the form
> +		of <STATUS>:<ERROR>, where <STATUS> will be one of
> +		the status strings described for the status sysfs
> +		file and <ERROR> will be one of the following:
> +		hw-error, timeout, user-abort, device-busy,
> +		invalid-file-size, read-write-error, flash-wearout,
> +		file-read-error.  The error sysfs file is only
> +		meaningful when the secure update engine is in the
> +		idle state. If this file is read while a secure
> +		update is in progress, then the read will fail with
> +		EBUSY.
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index dec52c68fe16..e43fa2797d27 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -30,10 +30,16 @@ static void update_progress(struct fpga_sec_mgr *smgr,
>  	sysfs_notify(&smgr->dev.kobj, "update", "status");
>  }
>  
> +static void set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_code)
> +{
> +	smgr->err_state = smgr->progress;
> +	smgr->err_code = err_code;
> +}

Nit: Could this be fpga_sec_set_error() ?
> +
>  static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>  			       enum fpga_sec_err err_code)
>  {
> -	smgr->err_code = err_code;
> +	set_error(smgr, err_code);
>  	smgr->sops->cancel(smgr);
>  }
>  
> @@ -56,7 +62,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  
>  	get_device(&smgr->dev);
>  	if (request_firmware(&fw, smgr->filename, &smgr->dev)) {
> -		smgr->err_code = FPGA_SEC_ERR_FILE_READ;
> +		set_error(smgr, FPGA_SEC_ERR_FILE_READ);
>  		goto idle_exit;
>  	}
>  
> @@ -64,7 +70,7 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  	smgr->remaining_size = fw->size;
>  
>  	if (!try_module_get(smgr->dev.parent->driver->owner)) {
> -		smgr->err_code = FPGA_SEC_ERR_BUSY;
> +		set_error(smgr, FPGA_SEC_ERR_BUSY);
>  		goto release_fw_exit;
>  	}
>  
> @@ -122,24 +128,76 @@ static const char * const sec_mgr_prog_str[] = {
>  	[FPGA_SEC_PROG_PROGRAMMING] = "programming"
>  };
>  
> -static ssize_t
> -status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +static const char * const sec_mgr_err_str[] = {
> +	[FPGA_SEC_ERR_NONE]	    = "none",
> +	[FPGA_SEC_ERR_HW_ERROR]	    = "hw-error",
> +	[FPGA_SEC_ERR_TIMEOUT]	    = "timeout",
> +	[FPGA_SEC_ERR_CANCELED]	    = "user-abort",
> +	[FPGA_SEC_ERR_BUSY]	    = "device-busy",
> +	[FPGA_SEC_ERR_INVALID_SIZE] = "invalid-file-size",
> +	[FPGA_SEC_ERR_RW_ERROR]	    = "read-write-error",
> +	[FPGA_SEC_ERR_WEAROUT]	    = "flash-wearout",
> +	[FPGA_SEC_ERR_FILE_READ]    = "file-read-error"
> +};
> +
> +static const char *sec_progress(struct device *dev, enum fpga_sec_prog prog)
>  {
> -	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
>  	const char *status = "unknown-status";
> -	enum fpga_sec_prog progress;
>  
> -	progress = smgr->progress;
> -	if (progress < FPGA_SEC_PROG_MAX)
> -		status = sec_mgr_prog_str[progress];
> +	if (prog < FPGA_SEC_PROG_MAX)
> +		status = sec_mgr_prog_str[prog];
>  	else
>  		dev_err(dev, "Invalid status during secure update: %d\n",
> -			progress);
> +			prog);
> +
> +	return status;
> +}
> +
> +static const char *sec_error(struct device *dev, enum fpga_sec_err err_code)
> +{
> +	const char *error = "unknown-error";
> +
> +	if (err_code < FPGA_SEC_ERR_MAX)
> +		error = sec_mgr_err_str[err_code];
> +	else
> +		dev_err(dev, "Invalid error code during secure update: %d\n",
> +			err_code);
> +
> +	return error;
> +}
> +
> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
>  
> -	return sysfs_emit(buf, "%s\n", status);
> +	return sysfs_emit(buf, "%s\n", sec_progress(dev, smgr->progress));
>  }
>  static DEVICE_ATTR_RO(status);
>  
> +static ssize_t
> +error_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	int ret;
> +
> +	mutex_lock(&smgr->lock);
> +
> +	if (smgr->progress != FPGA_SEC_PROG_IDLE)
> +		ret = -EBUSY;
> +	else if (!smgr->err_code)
> +		ret = 0;
> +	else
> +		ret = sysfs_emit(buf, "%s:%s\n",
> +				 sec_progress(dev, smgr->err_state),
> +				 sec_error(dev, smgr->err_code));
> +
> +	mutex_unlock(&smgr->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(error);
> +
>  static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -175,6 +233,7 @@ static DEVICE_ATTR_WO(filename);
>  static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
>  	&dev_attr_status.attr,
> +	&dev_attr_error.attr,
>  	NULL,
>  };
>  
> diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
> index 978ab98ffac5..6b7b8a3d6aac 100644
> --- a/include/linux/fpga/fpga-sec-mgr.h
> +++ b/include/linux/fpga/fpga-sec-mgr.h
> @@ -70,6 +70,7 @@ struct fpga_sec_mgr {
>  	const u8 *data;			/* pointer to update data */
>  	u32 remaining_size;		/* size remaining to transfer */
>  	enum fpga_sec_prog progress;
> +	enum fpga_sec_prog err_state;	/* progress state at time of failure */
>  	enum fpga_sec_err err_code;	/* security manager error code */
>  	bool driver_unload;
>  	void *priv;
> -- 
> 2.25.1
> 

Looks good,
Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size
  2021-05-03 21:35 ` [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size Russ Weight
@ 2021-05-10 17:39   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:39 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:44PM -0700, Russ Weight wrote:
> Extend the FPGA Security Manager class driver to include
> an update/remaining_size sysfs node that can be read to
> determine how much data remains to be transferred to the
> secure update engine. This file can be used to monitor
> progress during the "writing" phase of an update.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - No change
> v5:
>   - Use new function sysfs_emit() in the remaining_size_show() function
> v4:
>   - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
>     and removed unnecessary references to "Intel".
>   - Changed: imgr -> smgr, ifpga_ to fpga_
> v3:
>   - No change
> v2:
>   - Bumped documentation date and version
> ---
>  Documentation/ABI/testing/sysfs-class-fpga-sec-mgr | 11 +++++++++++
>  drivers/fpga/fpga-sec-mgr.c                        | 10 ++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index 24890d04521f..c5d0b9d7c7e4 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -28,6 +28,17 @@ Description:	Read-only. Returns a string describing the current
>  		as it will be signaled by sysfs_notify() on each
>  		state change.
>  
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/remaining_size
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns the size of data that remains to
> +		be written to the secure update engine. The size
> +		value is initialized to the full size of the file
> +		image and the value is updated periodically during
> +		the "writing" phase of the update.
> +		Format: "%u".
> +
>  What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/error
>  Date:		June 2021
>  KernelVersion:	5.14
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index e43fa2797d27..2487042ace82 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -198,6 +198,15 @@ error_show(struct device *dev, struct device_attribute *attr, char *buf)
>  }
>  static DEVICE_ATTR_RO(error);
>  
> +static ssize_t remaining_size_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +
> +	return sysfs_emit(buf, "%u\n", smgr->remaining_size);
> +}
> +static DEVICE_ATTR_RO(remaining_size);
> +
>  static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  			      const char *buf, size_t count)
>  {
> @@ -234,6 +243,7 @@ static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
>  	&dev_attr_status.attr,
>  	&dev_attr_error.attr,
> +	&dev_attr_remaining_size.attr,
>  	NULL,
>  };
>  
> -- 
> 2.25.1
> 

Looks good,
Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update
  2021-05-03 21:35 ` [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update Russ Weight
@ 2021-05-10 17:40   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:40 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:45PM -0700, Russ Weight wrote:
> Extend the FPGA Security Manager class driver to include
> an update/cancel sysfs file that can be written to request
> that an update be canceled. The write may return EBUSY if
> the update has progressed to the point that it cannot be
> canceled by software or ENODEV if there is no update in
> progress.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - No change
> v5:
>   - No change
> 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:
>   - No change
> v2:
>   - Bumped documentation date and version
>   - Minor code cleanup per review comments 
> ---
>  .../ABI/testing/sysfs-class-fpga-sec-mgr      | 10 ++++
>  drivers/fpga/fpga-sec-mgr.c                   | 59 +++++++++++++++++--
>  include/linux/fpga/fpga-sec-mgr.h             |  1 +
>  3 files changed, 66 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index c5d0b9d7c7e4..749f2d4c78d3 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -17,6 +17,16 @@ Description:	Write only. Write the filename of an image
>  		and Root Entry Hashes, and to cancel Code Signing
>  		Keys (CSK).
>  
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/cancel
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Write-only. Write a "1" to this file to request
> +		that a current update be canceled. This request
> +		will be rejected (EBUSY) if the programming phase
> +		has already started or (ENODEV) if there is no
> +		update in progress.
> +
>  What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/status
>  Date:		June 2021
>  KernelVersion:	5.14
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index 2487042ace82..35bd419bd3b9 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -43,6 +43,23 @@ static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>  	smgr->sops->cancel(smgr);
>  }
>  
> +static int progress_transition(struct fpga_sec_mgr *smgr,
> +			       enum fpga_sec_prog new_progress)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&smgr->lock);
> +	if (smgr->request_cancel) {
> +		set_error(smgr, FPGA_SEC_ERR_CANCELED);
> +		smgr->sops->cancel(smgr);
> +		ret = -ECANCELED;
> +	} else {
> +		update_progress(smgr, new_progress);
> +	}
> +	mutex_unlock(&smgr->lock);
> +	return ret;
> +}
> +
>  static void progress_complete(struct fpga_sec_mgr *smgr)
>  {
>  	mutex_lock(&smgr->lock);
> @@ -74,15 +91,19 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  		goto release_fw_exit;
>  	}
>  
> -	update_progress(smgr, FPGA_SEC_PROG_PREPARING);
> +	if (progress_transition(smgr, FPGA_SEC_PROG_PREPARING))
> +		goto modput_exit;
> +
>  	ret = smgr->sops->prepare(smgr);
>  	if (ret != FPGA_SEC_ERR_NONE) {
>  		fpga_sec_dev_error(smgr, ret);
>  		goto modput_exit;
>  	}
>  
> -	update_progress(smgr, FPGA_SEC_PROG_WRITING);
> -	while (smgr->remaining_size) {
> +	if (progress_transition(smgr, FPGA_SEC_PROG_WRITING))
> +		goto done;
> +
> +	while (smgr->remaining_size && !smgr->request_cancel) {
>  		ret = smgr->sops->write_blk(smgr, offset);
>  		if (ret != FPGA_SEC_ERR_NONE) {
>  			fpga_sec_dev_error(smgr, ret);
> @@ -92,7 +113,9 @@ static void fpga_sec_mgr_update(struct work_struct *work)
>  		offset = fw->size - smgr->remaining_size;
>  	}
>  
> -	update_progress(smgr, FPGA_SEC_PROG_PROGRAMMING);
> +	if (progress_transition(smgr, FPGA_SEC_PROG_PROGRAMMING))
> +		goto done;
> +
>  	ret = smgr->sops->poll_complete(smgr);
>  	if (ret != FPGA_SEC_ERR_NONE)
>  		fpga_sec_dev_error(smgr, ret);
> @@ -229,6 +252,7 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  	}
>  
>  	smgr->err_code = FPGA_SEC_ERR_NONE;
> +	smgr->request_cancel = false;
>  	smgr->progress = FPGA_SEC_PROG_READING;
>  	reinit_completion(&smgr->update_done);
>  	schedule_work(&smgr->work);
> @@ -239,8 +263,32 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(filename);
>  
> +static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	bool cancel;
> +	int ret = count;
> +
> +	if (kstrtobool(buf, &cancel) || !cancel)
> +		return -EINVAL;
> +
> +	mutex_lock(&smgr->lock);
> +	if (smgr->progress == FPGA_SEC_PROG_PROGRAMMING)
> +		ret = -EBUSY;
> +	else if (smgr->progress == FPGA_SEC_PROG_IDLE)
> +		ret = -ENODEV;
> +	else
> +		smgr->request_cancel = true;
> +	mutex_unlock(&smgr->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(cancel);
> +
>  static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
> +	&dev_attr_cancel.attr,
>  	&dev_attr_status.attr,
>  	&dev_attr_error.attr,
>  	&dev_attr_remaining_size.attr,
> @@ -464,6 +512,9 @@ void fpga_sec_mgr_unregister(struct fpga_sec_mgr *smgr)
>  		goto unregister;
>  	}
>  
> +	if (smgr->progress != FPGA_SEC_PROG_PROGRAMMING)
> +		smgr->request_cancel = true;
> +
>  	mutex_unlock(&smgr->lock);
>  	wait_for_completion(&smgr->update_done);
>  
> diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
> index 6b7b8a3d6aac..0e1f50434024 100644
> --- a/include/linux/fpga/fpga-sec-mgr.h
> +++ b/include/linux/fpga/fpga-sec-mgr.h
> @@ -72,6 +72,7 @@ struct fpga_sec_mgr {
>  	enum fpga_sec_prog progress;
>  	enum fpga_sec_prog err_state;	/* progress state at time of failure */
>  	enum fpga_sec_err err_code;	/* security manager error code */
> +	bool request_cancel;
>  	bool driver_unload;
>  	void *priv;
>  };
> -- 
> 2.25.1
> 

Looks good,
Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info
  2021-05-03 21:35 ` [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info Russ Weight
@ 2021-05-10 17:41   ` Moritz Fischer
  0 siblings, 0 replies; 19+ messages in thread
From: Moritz Fischer @ 2021-05-10 17:41 UTC (permalink / raw)
  To: Russ Weight
  Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

On Mon, May 03, 2021 at 02:35:46PM -0700, Russ Weight wrote:
> Extend the FPGA Security Manager class driver to include
> an optional update/hw_errinfo sysfs node that can be used
> to retrieve 64 bits of device specific error information
> following a secure update failure.
> 
> The underlying driver must provide a get_hw_errinfo() callback
> function to enable this feature. This data is treated as
> opaque by the class driver. It is left to user-space software
> or support personnel to interpret this data.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v12:
>   - Updated Date and KernelVersion fields in ABI documentation
> v11:
>   - No change
> v10:
>   - Rebased to 5.12-rc2 next
>   - Updated Date and KernelVersion in ABI documentation
> v9:
>   - Updated Date and KernelVersion in ABI documentation
> v8:
>   - No change
> v7:
>   - Changed Date in documentation file to December 2020
> v6:
>   - No change
> v5:
>   - No change
> 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:
>   - No change
> v2:
>   - Bumped documentation date and version
> ---
>  .../ABI/testing/sysfs-class-fpga-sec-mgr      | 14 +++++++
>  drivers/fpga/fpga-sec-mgr.c                   | 38 +++++++++++++++++++
>  include/linux/fpga/fpga-sec-mgr.h             |  5 +++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> index 749f2d4c78d3..f1881ce39c63 100644
> --- a/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> @@ -65,3 +65,17 @@ Description:	Read-only. Returns a string describing the failure
>  		idle state. If this file is read while a secure
>  		update is in progress, then the read will fail with
>  		EBUSY.
> +
> +What: 		/sys/class/fpga_sec_mgr/fpga_secX/update/hw_errinfo
> +Date:		June 2021
> +KernelVersion:	5.14
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read-only. Returns a 64 bit error value providing
> +		hardware specific information that may be useful in
> +		debugging errors that occur during FPGA image updates.
> +		This file is only visible if the underlying device
> +		supports it. The hw_errinfo value is only accessible
> +		when the secure update engine is in the idle state.
> +		If this file is read while a secure update is in
> +		progress, then the read will fail with EBUSY.
> +		Format: "0x%llx".
> diff --git a/drivers/fpga/fpga-sec-mgr.c b/drivers/fpga/fpga-sec-mgr.c
> index 35bd419bd3b9..3c59b142291d 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -36,10 +36,17 @@ static void set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_code)
>  	smgr->err_code = err_code;
>  }
>  
> +static void set_hw_errinfo(struct fpga_sec_mgr *smgr)
> +{
> +	if (smgr->sops->get_hw_errinfo)
> +		smgr->hw_errinfo = smgr->sops->get_hw_errinfo(smgr);
> +}

Nit: fpga_sec_set_hw_errinfo() maybe?
> +
>  static void fpga_sec_dev_error(struct fpga_sec_mgr *smgr,
>  			       enum fpga_sec_err err_code)
>  {
>  	set_error(smgr, err_code);
> +	set_hw_errinfo(smgr);
>  	smgr->sops->cancel(smgr);
>  }
>  
> @@ -221,6 +228,23 @@ error_show(struct device *dev, struct device_attribute *attr, char *buf)
>  }
>  static DEVICE_ATTR_RO(error);
>  
> +static ssize_t
> +hw_errinfo_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(dev);
> +	int ret;
> +
> +	mutex_lock(&smgr->lock);
> +	if (smgr->progress != FPGA_SEC_PROG_IDLE)
> +		ret = -EBUSY;
> +	else
> +		ret = sysfs_emit(buf, "0x%llx\n", smgr->hw_errinfo);
> +	mutex_unlock(&smgr->lock);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR_RO(hw_errinfo);
> +
>  static ssize_t remaining_size_show(struct device *dev,
>  				   struct device_attribute *attr, char *buf)
>  {
> @@ -252,6 +276,7 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>  	}
>  
>  	smgr->err_code = FPGA_SEC_ERR_NONE;
> +	smgr->hw_errinfo = 0;
>  	smgr->request_cancel = false;
>  	smgr->progress = FPGA_SEC_PROG_READING;
>  	reinit_completion(&smgr->update_done);
> @@ -286,18 +311,31 @@ static ssize_t cancel_store(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_WO(cancel);
>  
> +static umode_t
> +sec_mgr_update_visible(struct kobject *kobj, struct attribute *attr, int n)
> +{
> +	struct fpga_sec_mgr *smgr = to_sec_mgr(kobj_to_dev(kobj));
> +
> +	if (attr == &dev_attr_hw_errinfo.attr && !smgr->sops->get_hw_errinfo)
> +		return 0;
> +
> +	return attr->mode;
> +}
> +
>  static struct attribute *sec_mgr_update_attrs[] = {
>  	&dev_attr_filename.attr,
>  	&dev_attr_cancel.attr,
>  	&dev_attr_status.attr,
>  	&dev_attr_error.attr,
>  	&dev_attr_remaining_size.attr,
> +	&dev_attr_hw_errinfo.attr,
>  	NULL,
>  };
>  
>  static struct attribute_group sec_mgr_update_attr_group = {
>  	.name = "update",
>  	.attrs = sec_mgr_update_attrs,
> +	.is_visible = sec_mgr_update_visible,
>  };
>  
>  static ssize_t name_show(struct device *dev,
> diff --git a/include/linux/fpga/fpga-sec-mgr.h b/include/linux/fpga/fpga-sec-mgr.h
> index 0e1f50434024..a99bfd28f38c 100644
> --- a/include/linux/fpga/fpga-sec-mgr.h
> +++ b/include/linux/fpga/fpga-sec-mgr.h
> @@ -40,6 +40,9 @@ enum fpga_sec_err {
>   *			    function and is called at the completion
>   *			    of the update, whether success or failure,
>   *			    if the prepare function succeeded.
> + * @get_hw_errinfo:	    Optional: Return u64 hw specific error info.
> + *			    The software err_code may used to determine
> + *			    whether the hw error info is applicable.
>   */
>  struct fpga_sec_mgr_ops {
>  	enum fpga_sec_err (*prepare)(struct fpga_sec_mgr *smgr);
> @@ -47,6 +50,7 @@ struct fpga_sec_mgr_ops {
>  	enum fpga_sec_err (*poll_complete)(struct fpga_sec_mgr *smgr);
>  	enum fpga_sec_err (*cancel)(struct fpga_sec_mgr *smgr);
>  	void (*cleanup)(struct fpga_sec_mgr *smgr);
> +	u64 (*get_hw_errinfo)(struct fpga_sec_mgr *smgr);
>  };
>  
>  /* Update progress codes */
> @@ -72,6 +76,7 @@ struct fpga_sec_mgr {
>  	enum fpga_sec_prog progress;
>  	enum fpga_sec_prog err_state;	/* progress state at time of failure */
>  	enum fpga_sec_err err_code;	/* security manager error code */
> +	u64 hw_errinfo;			/* 64 bits of HW specific error info */
>  	bool request_cancel;
>  	bool driver_unload;
>  	void *priv;
> -- 
> 2.25.1
> 

Thanks,
Moritz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 0/7] FPGA Security Manager Class Driver
  2021-05-10 17:37   ` Moritz Fischer
@ 2021-05-10 18:52     ` Tom Rix
  2021-05-14 16:33       ` Russ Weight
  0 siblings, 1 reply; 19+ messages in thread
From: Tom Rix @ 2021-05-10 18:52 UTC (permalink / raw)
  To: Moritz Fischer
  Cc: Russ Weight, linux-fpga, linux-kernel, lgoncalv, yilun.xu,
	hao.wu, matthew.gerlach, richard.gong


On 5/10/21 10:37 AM, Moritz Fischer wrote:
> On Mon, May 10, 2021 at 07:12:57AM -0700, Tom Rix wrote:
>> On 5/3/21 2:35 PM, Russ Weight wrote:
>>> The FPGA Security Manager class driver provides a common
>>> API for user-space tools to manage updates for secure FPGA
>>> devices. Device drivers that instantiate the FPGA Security
>>> Manager class driver will interact with a HW secure update
>>> engine in order to transfer new FPGA and BMC images to FLASH so
>>> that they will be automatically loaded when the FPGA card reboots.
>> Russ,
>>
>> These have my Reviewed-by, but since it has been a while, I am looking these
>> over again.
>>
>> If you do not hear anything from me in the next couple of days, please
>> assume everything is fine.
>>
>> Tom
>>
> I'll do one more one-over, if it looks good will apply end of the week,

FWIW, my pass was fine.

Thanks,

Tom

>
> - Moritz
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v12 0/7] FPGA Security Manager Class Driver
  2021-05-10 18:52     ` Tom Rix
@ 2021-05-14 16:33       ` Russ Weight
  0 siblings, 0 replies; 19+ messages in thread
From: Russ Weight @ 2021-05-14 16:33 UTC (permalink / raw)
  To: Tom Rix, Moritz Fischer
  Cc: linux-fpga, linux-kernel, lgoncalv, yilun.xu, hao.wu,
	matthew.gerlach, richard.gong

Moritz,

On 5/10/21 11:52 AM, Tom Rix wrote:
>
> On 5/10/21 10:37 AM, Moritz Fischer wrote:
>> On Mon, May 10, 2021 at 07:12:57AM -0700, Tom Rix wrote:
>>> On 5/3/21 2:35 PM, Russ Weight wrote:
>>>> The FPGA Security Manager class driver provides a common
>>>> API for user-space tools to manage updates for secure FPGA
>>>> devices. Device drivers that instantiate the FPGA Security
>>>> Manager class driver will interact with a HW secure update
>>>> engine in order to transfer new FPGA and BMC images to FLASH so
>>>> that they will be automatically loaded when the FPGA card reboots.
>>> Russ,
>>>
>>> These have my Reviewed-by, but since it has been a while, I am looking these
>>> over again.
>>>
>>> If you do not hear anything from me in the next couple of days, please
>>> assume everything is fine.
>>>
>>> Tom
>>>
>> I'll do one more one-over, if it looks good will apply end of the week,

I've been out of town for the Monday through Thursday of this week. Should I
address the few comments you had and resubmit before you take these? Or were
you thinking of making the changes yourself?

I'll submit an updated patch set before the end of the day today unless I
hear otherwise...

Thanks,
- Russ

>
> FWIW, my pass was fine.
>
> Thanks,
>
> Tom
>
>>
>> - Moritz
>>
>


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-05-14 16:33 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-03 21:35 [PATCH v12 0/7] FPGA Security Manager Class Driver Russ Weight
2021-05-03 21:35 ` [PATCH v12 1/7] fpga: sec-mgr: fpga security manager class driver Russ Weight
2021-05-10 17:28   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 2/7] fpga: sec-mgr: enable secure updates Russ Weight
2021-05-10 17:32   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 3/7] fpga: sec-mgr: expose sec-mgr update status Russ Weight
2021-05-10 17:36   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 4/7] fpga: sec-mgr: expose sec-mgr update errors Russ Weight
2021-05-10 17:39   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 5/7] fpga: sec-mgr: expose sec-mgr update size Russ Weight
2021-05-10 17:39   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 6/7] fpga: sec-mgr: enable cancel of secure update Russ Weight
2021-05-10 17:40   ` Moritz Fischer
2021-05-03 21:35 ` [PATCH v12 7/7] fpga: sec-mgr: expose hardware error info Russ Weight
2021-05-10 17:41   ` Moritz Fischer
2021-05-10 14:12 ` [PATCH v12 0/7] FPGA Security Manager Class Driver Tom Rix
2021-05-10 17:37   ` Moritz Fischer
2021-05-10 18:52     ` Tom Rix
2021-05-14 16:33       ` Russ Weight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).