[PATCH 00/12] FPGA Security Manager for 5.14

[PATCH 00/12] FPGA Security Manager for 5.14

[Moritz Fischer]

Hi Greg,

please consider these patches for inclusion into 5.14.

Russ' changes introduce a new framework that enables secure updates of
FPGA devices boot configuration by interacting with the secure HW
engine.

The use cases for the FPGA Security Manager which deals with the update
of the permanent image for the FPGA device do not overlap enough with
the FPGA Manage use case (runtime updates of the currently loaded
image) to warrant a tighter coupling.

Its first user is m10bmc-sec driver supporting the Intel MAX 10 BMC
device.

Thanks,
Moritz

Russ Weight (12):
  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
  fpga: m10bmc-sec: create max10 bmc secure update driver
  fpga: m10bmc-sec: expose max10 flash update count
  fpga: m10bmc-sec: expose max10 canceled keys in sysfs
  fpga: m10bmc-sec: add max10 secure update functions
  fpga: m10bmc-sec: add max10 get_hw_errinfo callback func

 .../ABI/testing/sysfs-class-fpga-sec-mgr      |  81 +++
 .../testing/sysfs-driver-intel-m10-bmc-secure |  61 ++
 Documentation/fpga/fpga-sec-mgr.rst           |  44 ++
 Documentation/fpga/index.rst                  |   1 +
 MAINTAINERS                                   |  11 +
 drivers/fpga/Kconfig                          |  20 +
 drivers/fpga/Makefile                         |   6 +
 drivers/fpga/fpga-sec-mgr.c                   | 648 ++++++++++++++++++
 drivers/fpga/intel-m10-bmc-secure.c           | 550 +++++++++++++++
 include/linux/fpga/fpga-sec-mgr.h             |  99 +++
 10 files changed, 1521 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
 create mode 100644 Documentation/fpga/fpga-sec-mgr.rst
 create mode 100644 drivers/fpga/fpga-sec-mgr.c
 create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
 create mode 100644 include/linux/fpga/fpga-sec-mgr.h

-- 
2.31.1

[PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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 bd7aff0c120f..ac81adcd8579 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7136,6 +7136,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.31.1

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

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Extend the FPGA Security Manager class driver to
include an update/filename sysfs node that can be used
to initiate a secure update.  The filename of a secure
update file (BMC image, FPGA image, Root Entry Hash image,
or Code Signing Key cancellation image) can be written to
this sysfs entry to cause a secure update to occur.

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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..bfdb01d2de57 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 || 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 all 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.31.1

[PATCH 03/12] fpga: sec-mgr: expose sec-mgr update status

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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 bfdb01d2de57..19f60048a965 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.31.1

[PATCH 04/12] fpga: sec-mgr: expose sec-mgr update errors

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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 19f60048a965..903385779a1f 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 fpga_sec_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;
+	fpga_sec_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;
+		fpga_sec_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;
+		fpga_sec_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.31.1

[PATCH 05/12] fpga: sec-mgr: expose sec-mgr update size

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 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 903385779a1f..bc6b35cc7237 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.31.1

[PATCH 06/12] fpga: sec-mgr: enable cancel of secure update

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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 bc6b35cc7237..48950843c2b4 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) {
+		fpga_sec_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.31.1

[PATCH 07/12] fpga: sec-mgr: expose hardware error info

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

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>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../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 48950843c2b4..cefe9182c3c3 100644
--- a/drivers/fpga/fpga-sec-mgr.c
+++ b/drivers/fpga/fpga-sec-mgr.c
@@ -36,10 +36,17 @@ static void fpga_sec_set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_
 	smgr->err_code = err_code;
 }
 
+static void fpga_sec_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)
 {
 	fpga_sec_set_error(smgr, err_code);
+	fpga_sec_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.31.1

[PATCH 08/12] fpga: m10bmc-sec: create max10 bmc secure update driver

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Create a platform driver that can be invoked as a sub
driver for the Intel MAX10 BMC in order to support
secure updates. This sub-driver will invoke an
instance of the FPGA Security Manager class driver
in order to expose sysfs interfaces for managing and
monitoring secure updates to FPGA and BMC images.

This patch creates the MAX10 BMC Secure Update driver and
provides sysfs files for displaying the current root entry hashes
for the FPGA static region, the FPGA PR region, and the MAX10
BMC.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../testing/sysfs-driver-intel-m10-bmc-secure |  29 ++++
 MAINTAINERS                                   |   2 +
 drivers/fpga/Kconfig                          |  11 ++
 drivers/fpga/Makefile                         |   3 +
 drivers/fpga/intel-m10-bmc-secure.c           | 135 ++++++++++++++++++
 5 files changed, 180 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
 create mode 100644 drivers/fpga/intel-m10-bmc-secure.c

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
new file mode 100644
index 000000000000..9a0abb147b28
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
@@ -0,0 +1,29 @@
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/sr_root_entry_hash
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns the root entry hash for the static
+		region if one is programmed, else it returns the
+		string: "hash not programmed".  This file is only
+		visible if the underlying device supports it.
+		Format: "0x%x".
+
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/pr_root_entry_hash
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns the root entry hash for the partial
+		reconfiguration region if one is programmed, else it
+		returns the string: "hash not programmed".  This file
+		is only visible if the underlying device supports it.
+		Format: "0x%x".
+
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/bmc_root_entry_hash
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns the root entry hash for the BMC image
+		if one is programmed, else it returns the string:
+		"hash not programmed".  This file is only visible if the
+		underlying device supports it.
+		Format: "0x%x".
diff --git a/MAINTAINERS b/MAINTAINERS
index ac81adcd8579..864ba65478bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7141,8 +7141,10 @@ 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/ABI/testing/sysfs-driver-intel-m10-bmc-secure
 F:	Documentation/fpga/fpga-sec-mgr.rst
 F:	drivers/fpga/fpga-sec-mgr.c
+F:	drivers/fpga/intel-m10-bmc-secure.c
 F:	include/linux/fpga/fpga-sec-mgr.h
 
 FPU EMULATOR
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 09a8d915db26..0f3bbebd8b08 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -243,4 +243,15 @@ config FPGA_SEC_MGR
 	  region and for the BMC. Select this option to enable
 	  updates for secure FPGA devices.
 
+config IFPGA_M10_BMC_SECURE
+	tristate "Intel MAX10 BMC Secure Update driver"
+	depends on MFD_INTEL_M10_BMC && FPGA_SEC_MGR
+	help
+	  Secure update support for the Intel MAX10 board management
+	  controller.
+
+	  This is a subdriver of the Intel MAX10 board management controller
+	  (BMC) and provides support for secure updates for the BMC image,
+	  the FPGA image, the Root Entry Hashes, etc.
+
 endif # FPGA
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 22576d1a3996..7259f1ab2531 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -24,6 +24,9 @@ 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 Secure Update Drivers
+obj-$(CONFIG_IFPGA_M10_BMC_SECURE)	+= intel-m10-bmc-secure.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/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
new file mode 100644
index 000000000000..5ac5f59b5731
--- /dev/null
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel Max10 Board Management Controller Secure Update Driver
+ *
+ * Copyright (C) 2019-2020 Intel Corporation. All rights reserved.
+ *
+ */
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/fpga/fpga-sec-mgr.h>
+#include <linux/mfd/intel-m10-bmc.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+struct m10bmc_sec {
+	struct device *dev;
+	struct intel_m10bmc *m10bmc;
+};
+
+/* Root Entry Hash (REH) support */
+#define REH_SHA256_SIZE		32
+#define REH_SHA384_SIZE		48
+#define REH_MAGIC		GENMASK(15, 0)
+#define REH_SHA_NUM_BYTES	GENMASK(31, 16)
+
+static ssize_t
+show_root_entry_hash(struct device *dev, u32 exp_magic,
+		     u32 prog_addr, u32 reh_addr, char *buf)
+{
+	struct m10bmc_sec *sec = dev_get_drvdata(dev);
+	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+	int sha_num_bytes, i, cnt, ret;
+	u8 hash[REH_SHA384_SIZE];
+	u32 magic;
+
+	ret = m10bmc_raw_read(sec->m10bmc, prog_addr, &magic);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "%s magic 0x%08x\n", __func__, magic);
+
+	if (FIELD_GET(REH_MAGIC, magic) != exp_magic)
+		return sysfs_emit(buf, "hash not programmed\n");
+
+	sha_num_bytes = FIELD_GET(REH_SHA_NUM_BYTES, magic) / 8;
+	if (sha_num_bytes != REH_SHA256_SIZE &&
+	    sha_num_bytes != REH_SHA384_SIZE)   {
+		dev_err(sec->dev, "%s bad sha num bytes %d\n", __func__,
+			sha_num_bytes);
+		return -EINVAL;
+	}
+
+	WARN_ON(sha_num_bytes % stride);
+	ret = regmap_bulk_read(sec->m10bmc->regmap, reh_addr,
+			       hash, sha_num_bytes / stride);
+	if (ret) {
+		dev_err(dev, "failed to read root entry hash: %x cnt %x: %d\n",
+			reh_addr, sha_num_bytes / stride, ret);
+		return ret;
+	}
+
+	cnt = sprintf(buf, "0x");
+	for (i = 0; i < sha_num_bytes; i++)
+		cnt += sprintf(buf + cnt, "%02x", hash[i]);
+	cnt += sprintf(buf + cnt, "\n");
+
+	return cnt;
+}
+
+#define DEVICE_ATTR_SEC_REH_RO(_name, _magic, _prog_addr, _reh_addr) \
+static ssize_t _name##_root_entry_hash_show(struct device *dev, \
+					    struct device_attribute *attr, \
+					    char *buf) \
+{ return show_root_entry_hash(dev, _magic, _prog_addr, _reh_addr, buf); } \
+static DEVICE_ATTR_RO(_name##_root_entry_hash)
+
+DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
+DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
+DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
+
+static struct attribute *m10bmc_security_attrs[] = {
+	&dev_attr_bmc_root_entry_hash.attr,
+	&dev_attr_sr_root_entry_hash.attr,
+	&dev_attr_pr_root_entry_hash.attr,
+	NULL,
+};
+
+static struct attribute_group m10bmc_security_attr_group = {
+	.name = "security",
+	.attrs = m10bmc_security_attrs,
+};
+
+static const struct attribute_group *m10bmc_sec_attr_groups[] = {
+	&m10bmc_security_attr_group,
+	NULL,
+};
+
+static const struct fpga_sec_mgr_ops m10bmc_sops = { };
+
+static int m10bmc_secure_probe(struct platform_device *pdev)
+{
+	struct fpga_sec_mgr *smgr;
+	struct m10bmc_sec *sec;
+
+	sec = devm_kzalloc(&pdev->dev, sizeof(*sec), GFP_KERNEL);
+	if (!sec)
+		return -ENOMEM;
+
+	sec->dev = &pdev->dev;
+	sec->m10bmc = dev_get_drvdata(pdev->dev.parent);
+	dev_set_drvdata(&pdev->dev, sec);
+
+	smgr = devm_fpga_sec_mgr_create(sec->dev, "Max10 BMC Secure Update",
+					&m10bmc_sops, sec);
+	if (!smgr) {
+		dev_err(sec->dev, "Security manager failed to start\n");
+		return -ENOMEM;
+	}
+
+	return devm_fpga_sec_mgr_register(sec->dev, smgr);
+}
+
+static struct platform_driver intel_m10bmc_secure_driver = {
+	.probe = m10bmc_secure_probe,
+	.driver = {
+		.name = "n3000bmc-secure",
+		.dev_groups = m10bmc_sec_attr_groups,
+	},
+};
+module_platform_driver(intel_m10bmc_secure_driver);
+
+MODULE_ALIAS("platform:n3000bmc-secure");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel MAX10 BMC Secure Update");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

[PATCH 09/12] fpga: m10bmc-sec: expose max10 flash update count

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Extend the MAX10 BMC Secure Update driver to provide a
sysfs file to expose the flash update count for the FPGA
user image.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../testing/sysfs-driver-intel-m10-bmc-secure |  8 ++++
 drivers/fpga/intel-m10-bmc-secure.c           | 37 +++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
index 9a0abb147b28..c805c25e776d 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
@@ -27,3 +27,11 @@ Description:	Read only. Returns the root entry hash for the BMC image
 		"hash not programmed".  This file is only visible if the
 		underlying device supports it.
 		Format: "0x%x".
+
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/flash_count
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns number of times the secure update
+		staging area has been flashed.
+		Format: "%u".
diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index 5ac5f59b5731..ecd63c13cb2d 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -11,6 +11,7 @@
 #include <linux/mfd/intel-m10-bmc.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/slab.h>
 
 struct m10bmc_sec {
 	struct device *dev;
@@ -78,7 +79,43 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
 DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
 DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
 
+#define FLASH_COUNT_SIZE 4096	/* count stored as inverted bit vector */
+
+static ssize_t flash_count_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct m10bmc_sec *sec = dev_get_drvdata(dev);
+	unsigned int stride, num_bits;
+	u8 *flash_buf;
+	int cnt, ret;
+
+	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+	num_bits = FLASH_COUNT_SIZE * 8;
+
+	flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
+	if (!flash_buf)
+		return -ENOMEM;
+
+	WARN_ON(FLASH_COUNT_SIZE % stride);
+	ret = regmap_bulk_read(sec->m10bmc->regmap, STAGING_FLASH_COUNT,
+			       flash_buf, FLASH_COUNT_SIZE / stride);
+	if (ret) {
+		dev_err(sec->dev,
+			"failed to read flash count: %x cnt %x: %d\n",
+			STAGING_FLASH_COUNT, FLASH_COUNT_SIZE / stride, ret);
+		goto exit_free;
+	}
+	cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
+
+exit_free:
+	kfree(flash_buf);
+
+	return ret ? : sysfs_emit(buf, "%u\n", cnt);
+}
+static DEVICE_ATTR_RO(flash_count);
+
 static struct attribute *m10bmc_security_attrs[] = {
+	&dev_attr_flash_count.attr,
 	&dev_attr_bmc_root_entry_hash.attr,
 	&dev_attr_sr_root_entry_hash.attr,
 	&dev_attr_pr_root_entry_hash.attr,
-- 
2.31.1

[PATCH 10/12] fpga: m10bmc-sec: expose max10 canceled keys in sysfs

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Extend the MAX10 BMC Secure Update driver to provide sysfs
files to expose the canceled code signing key (CSK) bit
vectors. These use the standard bitmap list format
(e.g. 1,2-6,9).

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 .../testing/sysfs-driver-intel-m10-bmc-secure | 24 ++++++++++
 drivers/fpga/intel-m10-bmc-secure.c           | 48 +++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
index c805c25e776d..798d33b625d8 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
+++ b/Documentation/ABI/testing/sysfs-driver-intel-m10-bmc-secure
@@ -28,6 +28,30 @@ Description:	Read only. Returns the root entry hash for the BMC image
 		underlying device supports it.
 		Format: "0x%x".
 
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/sr_canceled_csks
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns a list of indices for canceled code
+		signing keys for the static region. The standard bitmap
+		list format is used (e.g. "1,2-6,9").
+
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/pr_canceled_csks
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns a list of indices for canceled code
+		signing keys for the partial reconfiguration region. The
+		standard bitmap list format is used (e.g. "1,2-6,9").
+
+What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/bmc_canceled_csks
+Date:		June 2021
+KernelVersion:	5.14
+Contact:	Russ Weight <russell.h.weight@intel.com>
+Description:	Read only. Returns a list of indices for canceled code
+		signing keys for the BMC.  The standard bitmap list format
+		is used (e.g. "1,2-6,9").
+
 What:		/sys/bus/platform/drivers/intel-m10bmc-secure/.../security/flash_count
 Date:		June 2021
 KernelVersion:	5.14
diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index ecd63c13cb2d..87e16c146569 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -79,6 +79,51 @@ DEVICE_ATTR_SEC_REH_RO(bmc, BMC_PROG_MAGIC, BMC_PROG_ADDR, BMC_REH_ADDR);
 DEVICE_ATTR_SEC_REH_RO(sr, SR_PROG_MAGIC, SR_PROG_ADDR, SR_REH_ADDR);
 DEVICE_ATTR_SEC_REH_RO(pr, PR_PROG_MAGIC, PR_PROG_ADDR, PR_REH_ADDR);
 
+#define CSK_BIT_LEN		128U
+#define CSK_32ARRAY_SIZE	DIV_ROUND_UP(CSK_BIT_LEN, 32)
+
+static ssize_t
+show_canceled_csk(struct device *dev, u32 addr, char *buf)
+{
+	unsigned int i, stride, size = CSK_32ARRAY_SIZE * sizeof(u32);
+	struct m10bmc_sec *sec = dev_get_drvdata(dev);
+	DECLARE_BITMAP(csk_map, CSK_BIT_LEN);
+	__le32 csk_le32[CSK_32ARRAY_SIZE];
+	u32 csk32[CSK_32ARRAY_SIZE];
+	int ret;
+
+	stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+
+	WARN_ON(size % stride);
+	ret = regmap_bulk_read(sec->m10bmc->regmap, addr, csk_le32,
+			       size / stride);
+	if (ret) {
+		dev_err(sec->dev, "failed to read CSK vector: %x cnt %x: %d\n",
+			addr, size / stride, ret);
+		return ret;
+	}
+
+	for (i = 0; i < CSK_32ARRAY_SIZE; i++)
+		csk32[i] = le32_to_cpu(((csk_le32[i])));
+
+	bitmap_from_arr32(csk_map, csk32, CSK_BIT_LEN);
+	bitmap_complement(csk_map, csk_map, CSK_BIT_LEN);
+	return bitmap_print_to_pagebuf(1, buf, csk_map, CSK_BIT_LEN);
+}
+
+#define DEVICE_ATTR_SEC_CSK_RO(_name, _addr) \
+static ssize_t _name##_canceled_csks_show(struct device *dev, \
+					  struct device_attribute *attr, \
+					  char *buf) \
+{ return show_canceled_csk(dev, _addr, buf); } \
+static DEVICE_ATTR_RO(_name##_canceled_csks)
+
+#define CSK_VEC_OFFSET 0x34
+
+DEVICE_ATTR_SEC_CSK_RO(bmc, BMC_PROG_ADDR + CSK_VEC_OFFSET);
+DEVICE_ATTR_SEC_CSK_RO(sr, SR_PROG_ADDR + CSK_VEC_OFFSET);
+DEVICE_ATTR_SEC_CSK_RO(pr, PR_PROG_ADDR + CSK_VEC_OFFSET);
+
 #define FLASH_COUNT_SIZE 4096	/* count stored as inverted bit vector */
 
 static ssize_t flash_count_show(struct device *dev,
@@ -119,6 +164,9 @@ static struct attribute *m10bmc_security_attrs[] = {
 	&dev_attr_bmc_root_entry_hash.attr,
 	&dev_attr_sr_root_entry_hash.attr,
 	&dev_attr_pr_root_entry_hash.attr,
+	&dev_attr_sr_canceled_csks.attr,
+	&dev_attr_pr_canceled_csks.attr,
+	&dev_attr_bmc_canceled_csks.attr,
 	NULL,
 };
 
-- 
2.31.1

[PATCH 11/12] fpga: m10bmc-sec: add max10 secure update functions

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Extend the MAX10 BMC Secure Update driver to include
the functions that enable secure updates of BMC images,
FPGA images, etc.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/intel-m10-bmc-secure.c | 310 +++++++++++++++++++++++++++-
 1 file changed, 309 insertions(+), 1 deletion(-)

diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index 87e16c146569..9d45312001a3 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -180,7 +180,315 @@ static const struct attribute_group *m10bmc_sec_attr_groups[] = {
 	NULL,
 };
 
-static const struct fpga_sec_mgr_ops m10bmc_sops = { };
+static void log_error_regs(struct m10bmc_sec *sec, u32 doorbell)
+{
+	u32 auth_result;
+
+	dev_err(sec->dev, "RSU error status: 0x%08x\n", doorbell);
+
+	if (!m10bmc_sys_read(sec->m10bmc, M10BMC_AUTH_RESULT, &auth_result))
+		dev_err(sec->dev, "RSU auth result: 0x%08x\n", auth_result);
+}
+
+static enum fpga_sec_err rsu_check_idle(struct m10bmc_sec *sec)
+{
+	u32 doorbell;
+	int ret;
+
+	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	if (rsu_prog(doorbell) != RSU_PROG_IDLE &&
+	    rsu_prog(doorbell) != RSU_PROG_RSU_DONE) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_BUSY;
+	}
+
+	return FPGA_SEC_ERR_NONE;
+}
+
+static inline bool rsu_start_done(u32 doorbell)
+{
+	u32 status, progress;
+
+	if (doorbell & DRBL_RSU_REQUEST)
+		return false;
+
+	status = rsu_stat(doorbell);
+	if (status == RSU_STAT_ERASE_FAIL || status == RSU_STAT_WEAROUT)
+		return true;
+
+	progress = rsu_prog(doorbell);
+	if (progress != RSU_PROG_IDLE && progress != RSU_PROG_RSU_DONE)
+		return true;
+
+	return false;
+}
+
+static enum fpga_sec_err rsu_update_init(struct m10bmc_sec *sec)
+{
+	u32 doorbell, status;
+	int ret;
+
+	ret = regmap_update_bits(sec->m10bmc->regmap,
+				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 DRBL_RSU_REQUEST | DRBL_HOST_STATUS,
+				 DRBL_RSU_REQUEST |
+				 FIELD_PREP(DRBL_HOST_STATUS,
+					    HOST_STATUS_IDLE));
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
+				       M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				       doorbell,
+				       rsu_start_done(doorbell),
+				       NIOS_HANDSHAKE_INTERVAL_US,
+				       NIOS_HANDSHAKE_TIMEOUT_US);
+
+	if (ret == -ETIMEDOUT) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_TIMEOUT;
+	} else if (ret) {
+		return FPGA_SEC_ERR_RW_ERROR;
+	}
+
+	status = rsu_stat(doorbell);
+	if (status == RSU_STAT_WEAROUT) {
+		dev_warn(sec->dev, "Excessive flash update count detected\n");
+		return FPGA_SEC_ERR_WEAROUT;
+	} else if (status == RSU_STAT_ERASE_FAIL) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_HW_ERROR;
+	}
+
+	return FPGA_SEC_ERR_NONE;
+}
+
+static enum fpga_sec_err rsu_prog_ready(struct m10bmc_sec *sec)
+{
+	unsigned long poll_timeout;
+	u32 doorbell, progress;
+	int ret;
+
+	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	poll_timeout = jiffies + msecs_to_jiffies(RSU_PREP_TIMEOUT_MS);
+	while (rsu_prog(doorbell) == RSU_PROG_PREPARE) {
+		msleep(RSU_PREP_INTERVAL_MS);
+		if (time_after(jiffies, poll_timeout))
+			break;
+
+		ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+		if (ret)
+			return FPGA_SEC_ERR_RW_ERROR;
+	}
+
+	progress = rsu_prog(doorbell);
+	if (progress == RSU_PROG_PREPARE) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_TIMEOUT;
+	} else if (progress != RSU_PROG_READY) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_HW_ERROR;
+	}
+
+	return FPGA_SEC_ERR_NONE;
+}
+
+static enum fpga_sec_err rsu_send_data(struct m10bmc_sec *sec)
+{
+	u32 doorbell;
+	int ret;
+
+	ret = regmap_update_bits(sec->m10bmc->regmap,
+				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 DRBL_HOST_STATUS,
+				 FIELD_PREP(DRBL_HOST_STATUS,
+					    HOST_STATUS_WRITE_DONE));
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	ret = regmap_read_poll_timeout(sec->m10bmc->regmap,
+				       M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				       doorbell,
+				       rsu_prog(doorbell) != RSU_PROG_READY,
+				       NIOS_HANDSHAKE_INTERVAL_US,
+				       NIOS_HANDSHAKE_TIMEOUT_US);
+
+	if (ret == -ETIMEDOUT) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_TIMEOUT;
+	} else if (ret) {
+		return FPGA_SEC_ERR_RW_ERROR;
+	}
+
+	switch (rsu_stat(doorbell)) {
+	case RSU_STAT_NORMAL:
+	case RSU_STAT_NIOS_OK:
+	case RSU_STAT_USER_OK:
+	case RSU_STAT_FACTORY_OK:
+		break;
+	default:
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_HW_ERROR;
+	}
+
+	return FPGA_SEC_ERR_NONE;
+}
+
+static int rsu_check_complete(struct m10bmc_sec *sec, u32 *doorbell)
+{
+	if (m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, doorbell))
+		return -EIO;
+
+	switch (rsu_stat(*doorbell)) {
+	case RSU_STAT_NORMAL:
+	case RSU_STAT_NIOS_OK:
+	case RSU_STAT_USER_OK:
+	case RSU_STAT_FACTORY_OK:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	switch (rsu_prog(*doorbell)) {
+	case RSU_PROG_IDLE:
+	case RSU_PROG_RSU_DONE:
+		return 0;
+	case RSU_PROG_AUTHENTICATING:
+	case RSU_PROG_COPYING:
+	case RSU_PROG_UPDATE_CANCEL:
+	case RSU_PROG_PROGRAM_KEY_HASH:
+		return -EAGAIN;
+	default:
+		return -EINVAL;
+	}
+}
+
+static enum fpga_sec_err m10bmc_sec_prepare(struct fpga_sec_mgr *smgr)
+{
+	struct m10bmc_sec *sec = smgr->priv;
+	enum fpga_sec_err ret;
+
+	if (smgr->remaining_size > M10BMC_STAGING_SIZE)
+		return FPGA_SEC_ERR_INVALID_SIZE;
+
+	ret = rsu_check_idle(sec);
+	if (ret != FPGA_SEC_ERR_NONE)
+		return ret;
+
+	ret = rsu_update_init(sec);
+	if (ret != FPGA_SEC_ERR_NONE)
+		return ret;
+
+	return rsu_prog_ready(sec);
+}
+
+#define WRITE_BLOCK_SIZE 0x4000 /* Update remaining_size every 0x4000 bytes */
+
+static enum fpga_sec_err
+m10bmc_sec_write_blk(struct fpga_sec_mgr *smgr, u32 offset)
+{
+	struct m10bmc_sec *sec = smgr->priv;
+	unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+	u32 doorbell, blk_size;
+	int ret;
+
+	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	if (ret) {
+		return FPGA_SEC_ERR_RW_ERROR;
+	} else if (rsu_prog(doorbell) != RSU_PROG_READY) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_HW_ERROR;
+	}
+
+	blk_size = min_t(u32, smgr->remaining_size, WRITE_BLOCK_SIZE);
+	ret = regmap_bulk_write(sec->m10bmc->regmap,
+				M10BMC_STAGING_BASE + offset,
+				(void *)smgr->data + offset,
+				(blk_size + stride - 1) / stride);
+
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	smgr->remaining_size -= blk_size;
+	return FPGA_SEC_ERR_NONE;
+}
+
+/*
+ * m10bmc_sec_poll_complete() is called after handing things off to
+ * the BMC firmware. Depending on the type of update, it could be
+ * 30+ minutes before the BMC firmware completes the update. The
+ * smgr->driver_unload check allows the driver to be unloaded,
+ * but the BMC firmware will continue the update and no further
+ * secure updates can be started for this device until the update
+ * is complete.
+ */
+static enum fpga_sec_err m10bmc_sec_poll_complete(struct fpga_sec_mgr *smgr)
+{
+	struct m10bmc_sec *sec = smgr->priv;
+	unsigned long poll_timeout;
+	enum fpga_sec_err result;
+	u32 doorbell;
+	int ret;
+
+	result = rsu_send_data(sec);
+	if (result != FPGA_SEC_ERR_NONE)
+		return result;
+
+	poll_timeout = jiffies + msecs_to_jiffies(RSU_COMPLETE_TIMEOUT_MS);
+	do {
+		msleep(RSU_COMPLETE_INTERVAL_MS);
+		ret = rsu_check_complete(sec, &doorbell);
+		if (smgr->driver_unload)
+			return FPGA_SEC_ERR_CANCELED;
+	} while (ret == -EAGAIN && !time_after(jiffies, poll_timeout));
+
+	if (ret == -EAGAIN) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_TIMEOUT;
+	} else if (ret == -EIO) {
+		return FPGA_SEC_ERR_RW_ERROR;
+	} else if (ret) {
+		log_error_regs(sec, doorbell);
+		return FPGA_SEC_ERR_HW_ERROR;
+	}
+
+	return FPGA_SEC_ERR_NONE;
+}
+
+static enum fpga_sec_err m10bmc_sec_cancel(struct fpga_sec_mgr *smgr)
+{
+	struct m10bmc_sec *sec = smgr->priv;
+	u32 doorbell;
+	int ret;
+
+	ret = m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+	if (ret)
+		return FPGA_SEC_ERR_RW_ERROR;
+
+	if (rsu_prog(doorbell) != RSU_PROG_READY)
+		return FPGA_SEC_ERR_BUSY;
+
+	ret = regmap_update_bits(sec->m10bmc->regmap,
+				 M10BMC_SYS_BASE + M10BMC_DOORBELL,
+				 DRBL_HOST_STATUS,
+				 FIELD_PREP(DRBL_HOST_STATUS,
+					    HOST_STATUS_ABORT_RSU));
+
+	return ret ? FPGA_SEC_ERR_RW_ERROR : FPGA_SEC_ERR_NONE;
+}
+
+static const struct fpga_sec_mgr_ops m10bmc_sops = {
+	.prepare = m10bmc_sec_prepare,
+	.write_blk = m10bmc_sec_write_blk,
+	.poll_complete = m10bmc_sec_poll_complete,
+	.cancel = m10bmc_sec_cancel,
+};
 
 static int m10bmc_secure_probe(struct platform_device *pdev)
 {
-- 
2.31.1

[PATCH 12/12] fpga: m10bmc-sec: add max10 get_hw_errinfo callback func

[Moritz Fischer]

From: Russ Weight <russell.h.weight@intel.com>

Extend the MAX10 BMC Secure Update driver to include
a function that returns 64 bits of additional HW specific
data for errors that require additional information.
This callback function enables the hw_errinfo sysfs
node in the Intel Security Manager class driver.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Moritz Fischer <mdf@kernel.org>
---
 drivers/fpga/intel-m10-bmc-secure.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index 9d45312001a3..bdf87ec125fe 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -483,11 +483,33 @@ static enum fpga_sec_err m10bmc_sec_cancel(struct fpga_sec_mgr *smgr)
 	return ret ? FPGA_SEC_ERR_RW_ERROR : FPGA_SEC_ERR_NONE;
 }
 
+#define HW_ERRINFO_POISON	GENMASK(31, 0)
+static u64 m10bmc_sec_hw_errinfo(struct fpga_sec_mgr *smgr)
+{
+	struct m10bmc_sec *sec = smgr->priv;
+	u32 auth_result = HW_ERRINFO_POISON;
+	u32 doorbell = HW_ERRINFO_POISON;
+
+	switch (smgr->err_code) {
+	case FPGA_SEC_ERR_HW_ERROR:
+	case FPGA_SEC_ERR_TIMEOUT:
+	case FPGA_SEC_ERR_BUSY:
+	case FPGA_SEC_ERR_WEAROUT:
+		m10bmc_sys_read(sec->m10bmc, M10BMC_DOORBELL, &doorbell);
+		m10bmc_sys_read(sec->m10bmc, M10BMC_AUTH_RESULT, &auth_result);
+
+		return (u64)doorbell << 32 | (u64)auth_result;
+	default:
+		return 0;
+	}
+}
+
 static const struct fpga_sec_mgr_ops m10bmc_sops = {
 	.prepare = m10bmc_sec_prepare,
 	.write_blk = m10bmc_sec_write_blk,
 	.poll_complete = m10bmc_sec_poll_complete,
 	.cancel = m10bmc_sec_cancel,
+	.get_hw_errinfo = m10bmc_sec_hw_errinfo,
 };
 
 static int m10bmc_secure_probe(struct platform_device *pdev)
-- 
2.31.1

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Greg KH]

On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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>

Russ, you know the Intel rules here, why did you not get someone who has
knowledge of the kernel's driver model to review your patches before
sending them out?

Basic driver model review comments below, I'm stopping after reviewing
this one as there's some big failures here...

> +++ 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.

What year is it?  :(

> + */
> +
> +#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);

What is wrong with the name of the device?  Please just use that and do
not have a "second name" of the thing.


> +
> +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,
> +};

ATTRIBUTE_GROUPS()?

> +
> +/**
> + * 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,

Where is the "device" coming from here?  If it's a parent, call it
"parent".

> +		    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);

I think we have to only use xarray() calls now, no more new IDA/IDR
calls please.

> +	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);

There's your name :)

> +	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);

Why did you not register the device here.

> +
> +/**
> + * 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);

Oh look, memory leaks!  :(

More below...


> +
> +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);

Drivers, if working properly, are quiet.  Please remove all dev_info()
mess from here (and the series).

> +
> +	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);

Like this, not needed.

> +
> +	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)
> +{
> +}

There used to be some lovely documentation in the kernel that said I was
allowed to yell at anyone who did something like this.  But that's
removed, so I'll just be quiet and ask you to think about why you would
ever want to provide an empty function, just to make the kernel core "be
quiet".  Did you perhaps think you were smarter than the kobject core
and this was the proper solution to make it "shut up" with it's crazy
warning that some over-eager developer added?  Or perhaps, that warning
was there on purpose, lovingly hand-added to help provide a HUGE HINT
that not providing a REAL release function was wrong.

You are now required to go and fix this whole series, and get someone
from Intel with some real knowledge of the driver model to sign off on
it, before you are allowed to post the series in public again.

greg k-h

Re: [PATCH 08/12] fpga: m10bmc-sec: create max10 bmc secure update driver

[Greg KH]

On Sun, May 16, 2021 at 07:31:56PM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> Create a platform driver that can be invoked as a sub
> driver for the Intel MAX10 BMC in order to support
> secure updates. This sub-driver will invoke an
> instance of the FPGA Security Manager class driver
> in order to expose sysfs interfaces for managing and
> monitoring secure updates to FPGA and BMC images.

No, please NEVER create a platform device for something that is not
actually a platform device.  That's a huge abuse of the platform device
code.

Please use the proper api for this if you need it, hint, it's NOT the
platform device code.  Your Intel reviewer should have told you what
it is when they saw a changelog comment like this....

greg k-h

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

[Greg KH]

On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> Extend the FPGA Security Manager class driver to
> include an update/filename sysfs node that can be used
> to initiate a secure update.  The filename of a secure
> update file (BMC image, FPGA image, Root Entry Hash image,
> or Code Signing Key cancellation image) can be written to
> this sysfs entry to cause a secure update to occur.

Why is userspace responsible for triggering this?  Passing a "filename"
into the kernel and having it do something with it is ripe for major
problems, please do not.

Re: [PATCH 07/12] fpga: sec-mgr: expose hardware error info

[Greg KH]

On Sun, May 16, 2021 at 07:31:55PM -0700, Moritz Fischer wrote:
> From: Russ Weight <russell.h.weight@intel.com>
> 
> 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.

No, you don't provide "opaque" data to userspace, that's a sure way to
make it such that no one knows what this data is supposed to be, and so
it can not be maintained at all.


> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> Signed-off-by: Moritz Fischer <mdf@kernel.org>
> ---
>  .../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 48950843c2b4..cefe9182c3c3 100644
> --- a/drivers/fpga/fpga-sec-mgr.c
> +++ b/drivers/fpga/fpga-sec-mgr.c
> @@ -36,10 +36,17 @@ static void fpga_sec_set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_
>  	smgr->err_code = err_code;
>  }
>  
> +static void fpga_sec_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)
>  {
>  	fpga_sec_set_error(smgr, err_code);
> +	fpga_sec_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;

Why does the state matter here?

greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Russ Weight]

Hi Greg,

On 5/16/21 10:18 PM, Greg KH wrote:
> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> 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>
> Russ, you know the Intel rules here, why did you not get someone who has
> knowledge of the kernel's driver model to review your patches before
> sending them out?
>
> Basic driver model review comments below, I'm stopping after reviewing
> this one as there's some big failures here...
>
>> +++ 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.
> What year is it?  :(
Thanks - I'll fix the copyright dates.
>
>> + */
>> +
>> +#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);
> What is wrong with the name of the device?  Please just use that and do
> not have a "second name" of the thing.
The purpose was to display the name of the parent driver. Should I change
"name" to "parent"? Or drop this altogether?

Please note that in this and other cases, I have been conforming to
conventions already used in FPGA Manager class driver:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397

>
>
>> +
>> +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,
>> +};
> ATTRIBUTE_GROUPS()?
Yes - I'll fix this.
>
>> +
>> +/**
>> + * 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,
> Where is the "device" coming from here?  If it's a parent, call it
> "parent".
I'll change it to "parent".

>
>> +		    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);
> I think we have to only use xarray() calls now, no more new IDA/IDR
> calls please.
Thanks - I'll look into changing this to use xarray().
>
>> +	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);
> There's your name :)
>
>> +	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);
> Why did you not register the device here.
My original implementation created and registered the device in a single function:

https://marc.info/?l=linux-fpga&m=159926365226264&w=2

It was split up to conform to the conventions used by other class drivers in the FPGA
framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c

>
>> +
>> +/**
>> + * 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);
> Oh look, memory leaks!  :(
>
> More below...
>
>
>> +
>> +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);
> Drivers, if working properly, are quiet.  Please remove all dev_info()
> mess from here (and the series).
OK - I'll remove the dev_info calls.
>
>> +
>> +	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);
> Like this, not needed.
>
>> +
>> +	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)
>> +{
>> +}
> There used to be some lovely documentation in the kernel that said I was
> allowed to yell at anyone who did something like this.  But that's
> removed, so I'll just be quiet and ask you to think about why you would
> ever want to provide an empty function, just to make the kernel core "be
> quiet".  Did you perhaps think you were smarter than the kobject core
> and this was the proper solution to make it "shut up" with it's crazy
> warning that some over-eager developer added?  Or perhaps, that warning
> was there on purpose, lovingly hand-added to help provide a HUGE HINT
> that not providing a REAL release function was wrong.

In my original submission, this function was populated.

https://marc.info/?l=linux-fpga&m=159926365226264&w=2

Again, I was conforming to conventions used in the other class drivers in
the FPGA framework, all of which have an empty *_dev_release()
function:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317

- Russ

>
> You are now required to go and fix this whole series, and get someone
> from Intel with some real knowledge of the driver model to sign off on
> it, before you are allowed to post the series in public again.
>
> greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Greg KH]

On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
> Hi Greg,
> 
> On 5/16/21 10:18 PM, Greg KH wrote:
> > On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
> >> From: Russ Weight <russell.h.weight@intel.com>
> >>
> >> 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>
> > Russ, you know the Intel rules here, why did you not get someone who has
> > knowledge of the kernel's driver model to review your patches before
> > sending them out?
> >
> > Basic driver model review comments below, I'm stopping after reviewing
> > this one as there's some big failures here...
> >
> >> +++ 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.
> > What year is it?  :(
> Thanks - I'll fix the copyright dates.
> >
> >> + */
> >> +
> >> +#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);
> > What is wrong with the name of the device?  Please just use that and do
> > not have a "second name" of the thing.
> The purpose was to display the name of the parent driver. Should I change
> "name" to "parent"? Or drop this altogether?

How is "name" a "parent"?  To find the parent, just walk up the sysfs
tree.

> Please note that in this and other cases, I have been conforming to
> conventions already used in FPGA Manager class driver:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397

Maybe that needs to be fixed as well :)

But, why re-implement the same thing and not just use the existing class
framework and code?


> >> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
> > Why did you not register the device here.
> My original implementation created and registered the device in a single function:
> 
> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
> 
> It was split up to conform to the conventions used by other class drivers in the FPGA
> framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c

If you don't need things to be split, don't split it.  Or better yet,
use the existing code.

> > There used to be some lovely documentation in the kernel that said I was
> > allowed to yell at anyone who did something like this.  But that's
> > removed, so I'll just be quiet and ask you to think about why you would
> > ever want to provide an empty function, just to make the kernel core "be
> > quiet".  Did you perhaps think you were smarter than the kobject core
> > and this was the proper solution to make it "shut up" with it's crazy
> > warning that some over-eager developer added?  Or perhaps, that warning
> > was there on purpose, lovingly hand-added to help provide a HUGE HINT
> > that not providing a REAL release function was wrong.
> 
> In my original submission, this function was populated.
> 
> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
> 
> Again, I was conforming to conventions used in the other class drivers in
> the FPGA framework, all of which have an empty *_dev_release()
> function:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317

Oh wow, that's totally wrong and broken, thanks for pointing it out.
Please fix that up first.

thanks,

greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Russ Weight]

On 5/17/21 10:55 AM, Greg KH wrote:
> On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
>> Hi Greg,
>>
>> On 5/16/21 10:18 PM, Greg KH wrote:
>>> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>
>>>> 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>
>>> Russ, you know the Intel rules here, why did you not get someone who has
>>> knowledge of the kernel's driver model to review your patches before
>>> sending them out?
>>>
>>> Basic driver model review comments below, I'm stopping after reviewing
>>> this one as there's some big failures here...
>>>
>>>> +++ 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.
>>> What year is it?  :(
>> Thanks - I'll fix the copyright dates.
>>>> + */
>>>> +
>>>> +#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);
>>> What is wrong with the name of the device?  Please just use that and do
>>> not have a "second name" of the thing.
>> The purpose was to display the name of the parent driver. Should I change
>> "name" to "parent"? Or drop this altogether?
> How is "name" a "parent"?  To find the parent, just walk up the sysfs
> tree.
>
>> Please note that in this and other cases, I have been conforming to
>> conventions already used in FPGA Manager class driver:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
> Maybe that needs to be fixed as well :)
>
> But, why re-implement the same thing and not just use the existing class
> framework and code?

I did the exercise of trying to merge the new functionality into the
fpga-mgr.c code, but there was so little commonality that it was beginning
to look like a dual-personality driver. The only thing that could be shared
was the registration/unregistration of the driver. It seemed cleaner to
have it as a separate class driver.

- Russ

>
>
>>>> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
>>> Why did you not register the device here.
>> My original implementation created and registered the device in a single function:
>>
>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>
>> It was split up to conform to the conventions used by other class drivers in the FPGA
>> framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c
> If you don't need things to be split, don't split it.  Or better yet,
> use the existing code.
>
>>> There used to be some lovely documentation in the kernel that said I was
>>> allowed to yell at anyone who did something like this.  But that's
>>> removed, so I'll just be quiet and ask you to think about why you would
>>> ever want to provide an empty function, just to make the kernel core "be
>>> quiet".  Did you perhaps think you were smarter than the kobject core
>>> and this was the proper solution to make it "shut up" with it's crazy
>>> warning that some over-eager developer added?  Or perhaps, that warning
>>> was there on purpose, lovingly hand-added to help provide a HUGE HINT
>>> that not providing a REAL release function was wrong.
>> In my original submission, this function was populated.
>>
>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>
>> Again, I was conforming to conventions used in the other class drivers in
>> the FPGA framework, all of which have an empty *_dev_release()
>> function:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317
> Oh wow, that's totally wrong and broken, thanks for pointing it out.
> Please fix that up first.
>
> thanks,
>
> greg k-h

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

[Russ Weight]

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

Is this not an acceptable use for request_firmware?

- Russ

Re: [PATCH 07/12] fpga: sec-mgr: expose hardware error info

[Russ Weight]

On 5/17/21 12:10 AM, Greg KH wrote:
> On Sun, May 16, 2021 at 07:31:55PM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> 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.
> No, you don't provide "opaque" data to userspace, that's a sure way to
> make it such that no one knows what this data is supposed to be, and so
> it can not be maintained at all.
My intent was that the data be opaque to the class driver layer, but not
to the parent driver or to the user-space code (which could process the
data based on the device type).

If this is not appropriate, it is an optional feature and can be removed.

>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Reviewed-by: Tom Rix <trix@redhat.com>
>> Signed-off-by: Moritz Fischer <mdf@kernel.org>
>> ---
>>  .../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 48950843c2b4..cefe9182c3c3 100644
>> --- a/drivers/fpga/fpga-sec-mgr.c
>> +++ b/drivers/fpga/fpga-sec-mgr.c
>> @@ -36,10 +36,17 @@ static void fpga_sec_set_error(struct fpga_sec_mgr *smgr, enum fpga_sec_err err_
>>  	smgr->err_code = err_code;
>>  }
>>  
>> +static void fpga_sec_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)
>>  {
>>  	fpga_sec_set_error(smgr, err_code);
>> +	fpga_sec_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;
> Why does the state matter here?
The transfer and validation of image data can take up to 40 minutes for the N3000
acceleration card, so the transfer of data and the monitoring of the update are
happening in a worker thread. The hw_errinfo data is only valid when the update
engine is in the IDLE state. Returning EBUSY if the update is still in progress
simplifies the locking model.

- Russ

>
> greg k-h

Re: [PATCH 08/12] fpga: m10bmc-sec: create max10 bmc secure update driver

[Russ Weight]

On 5/16/21 10:30 PM, Greg KH wrote:
> On Sun, May 16, 2021 at 07:31:56PM -0700, Moritz Fischer wrote:
>> From: Russ Weight <russell.h.weight@intel.com>
>>
>> Create a platform driver that can be invoked as a sub
>> driver for the Intel MAX10 BMC in order to support
>> secure updates. This sub-driver will invoke an
>> instance of the FPGA Security Manager class driver
>> in order to expose sysfs interfaces for managing and
>> monitoring secure updates to FPGA and BMC images.
> No, please NEVER create a platform device for something that is not
> actually a platform device.  That's a huge abuse of the platform device
> code.
>
> Please use the proper api for this if you need it, hint, it's NOT the
> platform device code.  Your Intel reviewer should have told you what
> it is when they saw a changelog comment like this....

I was following the design of the n3000bmc-hwmon driver, which was recently
accepted upstream.

The MAX10 BMC driver lists sub-devices here, including my device
(n3000bmc-secure) and the n3000bmc-hwmon device:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/intel-m10-bmc.c#n25

The HWMON sub-driver is implemented as a platform driver here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hwmon/intel-m10-bmc-hwmon.c

Is the HWMON driver implemented incorrectly? Or is there something fundamentally
different in what I am trying to do? Can you point me in the right direction? What
type of device should this be?

Thanks,

- Russ

>
> greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Tom Rix]

On 5/17/21 11:25 AM, Russ Weight wrote:
>
> On 5/17/21 10:55 AM, Greg KH wrote:
>> On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
>>> Hi Greg,
>>>
>>> On 5/16/21 10:18 PM, Greg KH wrote:
>>>> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
>>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>>
>>>>> 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>
>>>> Russ, you know the Intel rules here, why did you not get someone who has
>>>> knowledge of the kernel's driver model to review your patches before
>>>> sending them out?
>>>>
>>>> Basic driver model review comments below, I'm stopping after reviewing
>>>> this one as there's some big failures here...
>>>>
>>>>> +++ 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.
>>>> What year is it?  :(
>>> Thanks - I'll fix the copyright dates.
>>>>> + */
>>>>> +
>>>>> +#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);
>>>> What is wrong with the name of the device?  Please just use that and do
>>>> not have a "second name" of the thing.
>>> The purpose was to display the name of the parent driver. Should I change
>>> "name" to "parent"? Or drop this altogether?
>> How is "name" a "parent"?  To find the parent, just walk up the sysfs
>> tree.
>>
>>> Please note that in this and other cases, I have been conforming to
>>> conventions already used in FPGA Manager class driver:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
>> Maybe that needs to be fixed as well :)
>>
>> But, why re-implement the same thing and not just use the existing class
>> framework and code?
> I did the exercise of trying to merge the new functionality into the
> fpga-mgr.c code, but there was so little commonality that it was beginning
> to look like a dual-personality driver. The only thing that could be shared
> was the registration/unregistration of the driver. It seemed cleaner to
> have it as a separate class driver.
>
> - Russ

I'll post a patch in a bit that does nothing new but refactor fpga-mgr's 
ops into 'partial update' and 'full update'

existing stuff in partial

security update stuff in full

Tom

>
>>
>>>>> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
>>>> Why did you not register the device here.
>>> My original implementation created and registered the device in a single function:
>>>
>>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>>
>>> It was split up to conform to the conventions used by other class drivers in the FPGA
>>> framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c
>> If you don't need things to be split, don't split it.  Or better yet,
>> use the existing code.
>>
>>>> There used to be some lovely documentation in the kernel that said I was
>>>> allowed to yell at anyone who did something like this.  But that's
>>>> removed, so I'll just be quiet and ask you to think about why you would
>>>> ever want to provide an empty function, just to make the kernel core "be
>>>> quiet".  Did you perhaps think you were smarter than the kobject core
>>>> and this was the proper solution to make it "shut up" with it's crazy
>>>> warning that some over-eager developer added?  Or perhaps, that warning
>>>> was there on purpose, lovingly hand-added to help provide a HUGE HINT
>>>> that not providing a REAL release function was wrong.
>>> In my original submission, this function was populated.
>>>
>>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>>
>>> Again, I was conforming to conventions used in the other class drivers in
>>> the FPGA framework, all of which have an empty *_dev_release()
>>> function:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317
>> Oh wow, that's totally wrong and broken, thanks for pointing it out.
>> Please fix that up first.
>>
>> thanks,
>>
>> greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Russ Weight]

On 5/19/21 1:42 PM, Tom Rix wrote:
>
> On 5/17/21 11:25 AM, Russ Weight wrote:
>>
>> On 5/17/21 10:55 AM, Greg KH wrote:
>>> On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
>>>> Hi Greg,
>>>>
>>>> On 5/16/21 10:18 PM, Greg KH wrote:
>>>>> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
>>>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>>>
>>>>>> 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>
>>>>> Russ, you know the Intel rules here, why did you not get someone who has
>>>>> knowledge of the kernel's driver model to review your patches before
>>>>> sending them out?
>>>>>
>>>>> Basic driver model review comments below, I'm stopping after reviewing
>>>>> this one as there's some big failures here...
>>>>>
>>>>>> +++ 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.
>>>>> What year is it?  :(
>>>> Thanks - I'll fix the copyright dates.
>>>>>> + */
>>>>>> +
>>>>>> +#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);
>>>>> What is wrong with the name of the device?  Please just use that and do
>>>>> not have a "second name" of the thing.
>>>> The purpose was to display the name of the parent driver. Should I change
>>>> "name" to "parent"? Or drop this altogether?
>>> How is "name" a "parent"?  To find the parent, just walk up the sysfs
>>> tree.
>>>
>>>> Please note that in this and other cases, I have been conforming to
>>>> conventions already used in FPGA Manager class driver:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
>>> Maybe that needs to be fixed as well :)
>>>
>>> But, why re-implement the same thing and not just use the existing class
>>> framework and code?
>> I did the exercise of trying to merge the new functionality into the
>> fpga-mgr.c code, but there was so little commonality that it was beginning
>> to look like a dual-personality driver. The only thing that could be shared
>> was the registration/unregistration of the driver. It seemed cleaner to
>> have it as a separate class driver.
>>
>> - Russ
>
> I'll post a patch in a bit that does nothing new but refactor fpga-mgr's ops into 'partial update' and 'full update'
>
> existing stuff in partial
>
> security update stuff in full
>
> Tom

FYI: I just posted patches that remove the managed resource functions and
populate the class dev_release functions for fpga_mgr.c, fpga_region.c,
and fpga_bridge.c.

https://marc.info/?l=linux-fpga&m=162155904229400&w=2
https://marc.info/?l=linux-fpga&m=162155904329404&w=2
https://marc.info/?l=linux-fpga&m=162155904529409&w=2
https://marc.info/?l=linux-fpga&m=162155904529412&w=2

- Russ

>
>>
>>>
>>>>>> +EXPORT_SYMBOL_GPL(fpga_sec_mgr_create);
>>>>> Why did you not register the device here.
>>>> My original implementation created and registered the device in a single function:
>>>>
>>>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>>>
>>>> It was split up to conform to the conventions used by other class drivers in the FPGA
>>>> framework: fpga-mgr.c, fpga-bridge.c, fpga-region.c
>>> If you don't need things to be split, don't split it.  Or better yet,
>>> use the existing code.
>>>
>>>>> There used to be some lovely documentation in the kernel that said I was
>>>>> allowed to yell at anyone who did something like this.  But that's
>>>>> removed, so I'll just be quiet and ask you to think about why you would
>>>>> ever want to provide an empty function, just to make the kernel core "be
>>>>> quiet".  Did you perhaps think you were smarter than the kobject core
>>>>> and this was the proper solution to make it "shut up" with it's crazy
>>>>> warning that some over-eager developer added?  Or perhaps, that warning
>>>>> was there on purpose, lovingly hand-added to help provide a HUGE HINT
>>>>> that not providing a REAL release function was wrong.
>>>> In my original submission, this function was populated.
>>>>
>>>> https://marc.info/?l=linux-fpga&m=159926365226264&w=2
>>>>
>>>> Again, I was conforming to conventions used in the other class drivers in
>>>> the FPGA framework, all of which have an empty *_dev_release()
>>>> function:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n782
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-bridge.c#n476
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-region.c#n317
>>> Oh wow, that's totally wrong and broken, thanks for pointing it out.
>>> Please fix that up first.
>>>
>>> thanks,
>>>
>>> greg k-h
>

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Greg KH]

On Thu, May 20, 2021 at 06:10:17PM -0700, Russ Weight wrote:
> 
> On 5/19/21 1:42 PM, Tom Rix wrote:
> >
> > On 5/17/21 11:25 AM, Russ Weight wrote:
> >>
> >> On 5/17/21 10:55 AM, Greg KH wrote:
> >>> On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
> >>>> Hi Greg,
> >>>>
> >>>> On 5/16/21 10:18 PM, Greg KH wrote:
> >>>>> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
> >>>>>> From: Russ Weight <russell.h.weight@intel.com>
> >>>>>>
> >>>>>> 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>
> >>>>> Russ, you know the Intel rules here, why did you not get someone who has
> >>>>> knowledge of the kernel's driver model to review your patches before
> >>>>> sending them out?
> >>>>>
> >>>>> Basic driver model review comments below, I'm stopping after reviewing
> >>>>> this one as there's some big failures here...
> >>>>>
> >>>>>> +++ 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.
> >>>>> What year is it?  :(
> >>>> Thanks - I'll fix the copyright dates.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#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);
> >>>>> What is wrong with the name of the device?  Please just use that and do
> >>>>> not have a "second name" of the thing.
> >>>> The purpose was to display the name of the parent driver. Should I change
> >>>> "name" to "parent"? Or drop this altogether?
> >>> How is "name" a "parent"?  To find the parent, just walk up the sysfs
> >>> tree.
> >>>
> >>>> Please note that in this and other cases, I have been conforming to
> >>>> conventions already used in FPGA Manager class driver:
> >>>>
> >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
> >>> Maybe that needs to be fixed as well :)
> >>>
> >>> But, why re-implement the same thing and not just use the existing class
> >>> framework and code?
> >> I did the exercise of trying to merge the new functionality into the
> >> fpga-mgr.c code, but there was so little commonality that it was beginning
> >> to look like a dual-personality driver. The only thing that could be shared
> >> was the registration/unregistration of the driver. It seemed cleaner to
> >> have it as a separate class driver.
> >>
> >> - Russ
> >
> > I'll post a patch in a bit that does nothing new but refactor fpga-mgr's ops into 'partial update' and 'full update'
> >
> > existing stuff in partial
> >
> > security update stuff in full
> >
> > Tom
> 
> FYI: I just posted patches that remove the managed resource functions and
> populate the class dev_release functions for fpga_mgr.c, fpga_region.c,
> and fpga_bridge.c.
> 
> https://marc.info/?l=linux-fpga&m=162155904229400&w=2
> https://marc.info/?l=linux-fpga&m=162155904329404&w=2
> https://marc.info/?l=linux-fpga&m=162155904529409&w=2
> https://marc.info/?l=linux-fpga&m=162155904529412&w=2

You forgot to cc: me :(

I guess you didn't want me to point out the fact that you forgot to go
through the proper internal Intel patch review process first, before
asking others to review your changes?

{sigh}

greg k-h

Re: [PATCH 01/12] fpga: sec-mgr: fpga security manager class driver

[Russ Weight]

On 5/20/21 9:58 PM, Greg KH wrote:
> On Thu, May 20, 2021 at 06:10:17PM -0700, Russ Weight wrote:
>> On 5/19/21 1:42 PM, Tom Rix wrote:
>>> On 5/17/21 11:25 AM, Russ Weight wrote:
>>>> On 5/17/21 10:55 AM, Greg KH wrote:
>>>>> On Mon, May 17, 2021 at 10:45:40AM -0700, Russ Weight wrote:
>>>>>> Hi Greg,
>>>>>>
>>>>>> On 5/16/21 10:18 PM, Greg KH wrote:
>>>>>>> On Sun, May 16, 2021 at 07:31:49PM -0700, Moritz Fischer wrote:
>>>>>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>>>>>
>>>>>>>> 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>
>>>>>>> Russ, you know the Intel rules here, why did you not get someone who has
>>>>>>> knowledge of the kernel's driver model to review your patches before
>>>>>>> sending them out?
>>>>>>>
>>>>>>> Basic driver model review comments below, I'm stopping after reviewing
>>>>>>> this one as there's some big failures here...
>>>>>>>
>>>>>>>> +++ 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.
>>>>>>> What year is it?  :(
>>>>>> Thanks - I'll fix the copyright dates.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#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);
>>>>>>> What is wrong with the name of the device?  Please just use that and do
>>>>>>> not have a "second name" of the thing.
>>>>>> The purpose was to display the name of the parent driver. Should I change
>>>>>> "name" to "parent"? Or drop this altogether?
>>>>> How is "name" a "parent"?  To find the parent, just walk up the sysfs
>>>>> tree.
>>>>>
>>>>>> Please note that in this and other cases, I have been conforming to
>>>>>> conventions already used in FPGA Manager class driver:
>>>>>>
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/fpga/fpga-mgr.c#n397
>>>>> Maybe that needs to be fixed as well :)
>>>>>
>>>>> But, why re-implement the same thing and not just use the existing class
>>>>> framework and code?
>>>> I did the exercise of trying to merge the new functionality into the
>>>> fpga-mgr.c code, but there was so little commonality that it was beginning
>>>> to look like a dual-personality driver. The only thing that could be shared
>>>> was the registration/unregistration of the driver. It seemed cleaner to
>>>> have it as a separate class driver.
>>>>
>>>> - Russ
>>> I'll post a patch in a bit that does nothing new but refactor fpga-mgr's ops into 'partial update' and 'full update'
>>>
>>> existing stuff in partial
>>>
>>> security update stuff in full
>>>
>>> Tom
>> FYI: I just posted patches that remove the managed resource functions and
>> populate the class dev_release functions for fpga_mgr.c, fpga_region.c,
>> and fpga_bridge.c.
>>
>> https://marc.info/?l=linux-fpga&m=162155904229400&w=2
>> https://marc.info/?l=linux-fpga&m=162155904329404&w=2
>> https://marc.info/?l=linux-fpga&m=162155904529409&w=2
>> https://marc.info/?l=linux-fpga&m=162155904529412&w=2
> You forgot to cc: me :(
>
> I guess you didn't want me to point out the fact that you forgot to go
> through the proper internal Intel patch review process first, before
> asking others to review your changes?
I didn't CC you because I thought you would want the patches to be
vetted on the linux-fpga mail list (which includes Intel folks) and approved
by Moritz before looking at them. I gave you the FYI here because you requested
the changes and I wanted to let you know that they are in progress.

>
> {sigh}
>
> greg k-h

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

[Russ Weight]

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

Hi Greg,

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

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

Example Output of Current Driver:

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


Assumptions:

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

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

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

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

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

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

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

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

I appreciate any feedback/direction you can give.

Thanks,
- Russ

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

[Greg KH]

On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
> 
> 
> On 5/17/21 12:37 PM, Russ Weight wrote:
> > On 5/16/21 10:32 PM, Greg KH wrote:
> >> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> >>> From: Russ Weight <russell.h.weight@intel.com>
> >>>
> >>> Extend the FPGA Security Manager class driver to
> >>> include an update/filename sysfs node that can be used
> >>> to initiate a secure update.  The filename of a secure
> >>> update file (BMC image, FPGA image, Root Entry Hash image,
> >>> or Code Signing Key cancellation image) can be written to
> >>> this sysfs entry to cause a secure update to occur.
> >> Why is userspace responsible for triggering this?  Passing a "filename"
> >> into the kernel and having it do something with it is ripe for major
> >> problems, please do not.
> >>
> > I am using the "request_firmware" framework, which accepts a filename
> > and finds the firmware file under /lib/firmware.
> >
> > Is this not an acceptable use for request_firmware?
> >
> > - Russ
> 
> Hi Greg,
> 
> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
> Region code are almost complete. I'm trying to get back to the FPGA
> security manager patch set. Your previous comments challenged some basic
> assumptions. If it is OK, I would like to get some clarity before I rework
> the patches.

Note, I do not have the time, nor the inclination, to help your company
out with design reviews at this point in time.  If you have questions
about this, please discuss it with the open source managers at Intel,
they know the current situation quite well.

I am glad to review patches that have gone through the proper internal
Intel patch review process and then sent out to the community.

That being said, I will make one comment on your questions below:

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

request_firmware() should always be used for requesting firmware for a
device.  Having an api where you write a random filename to a sysfs file
and have that loaded by the kernel seems ripe for disaster though, I can
not think of any other in-kernel user of the firmware api that does
this.  Or are there examples that I have just missed?

thanks,

greg k-h

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

[Russ Weight]

On 7/30/21 4:18 AM, Greg KH wrote:
> On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
>>
>> On 5/17/21 12:37 PM, Russ Weight wrote:
>>> On 5/16/21 10:32 PM, Greg KH wrote:
>>>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
>>>>> From: Russ Weight <russell.h.weight@intel.com>
>>>>>
>>>>> Extend the FPGA Security Manager class driver to
>>>>> include an update/filename sysfs node that can be used
>>>>> to initiate a secure update.  The filename of a secure
>>>>> update file (BMC image, FPGA image, Root Entry Hash image,
>>>>> or Code Signing Key cancellation image) can be written to
>>>>> this sysfs entry to cause a secure update to occur.
>>>> Why is userspace responsible for triggering this?  Passing a "filename"
>>>> into the kernel and having it do something with it is ripe for major
>>>> problems, please do not.
>>>>
>>> I am using the "request_firmware" framework, which accepts a filename
>>> and finds the firmware file under /lib/firmware.
>>>
>>> Is this not an acceptable use for request_firmware?
>>>
>>> - Russ
>> Hi Greg,
>>
>> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
>> Region code are almost complete. I'm trying to get back to the FPGA
>> security manager patch set. Your previous comments challenged some basic
>> assumptions. If it is OK, I would like to get some clarity before I rework
>> the patches.
> Note, I do not have the time, nor the inclination, to help your company
> out with design reviews at this point in time.  If you have questions
> about this, please discuss it with the open source managers at Intel,
> they know the current situation quite well.
>
> I am glad to review patches that have gone through the proper internal
> Intel patch review process and then sent out to the community.

For what it is worth, these patches _were_ reviewed internally before
they were submitted to the public list. The first public submission
(Sep 2020) included review tags. I was asked to remove them and let
them be added back during the public review:

https://marc.info/?l=linux-fpga&m=159926670526828&w=2

Unfortunately, Intel review tags were not volunteered during the
public review, and it didn't occur to me to solicit the tags before
the patches were forwarded on to you by Moritz.

Most notably, Yilun and Hao contributed to the review both internally
and publicly. They are both listed in the MAINTAINERS file:

Xu Yilun <yilun.xu@intel.com>
Wu Hao <hao.wu@intel.com>

All issues/comments were resolved before the patches were sent to you.

> That being said, I will make one comment on your questions below:
>
>> (1) request_firmware(). We had assumed that making use of the existing
>> request_firmware() would be preferred. This requires providing a filename
>> under /lib/firmware to the framework. You commented (above): "Passing a
>> 'filename' into the kernel and having it do something with it is ripe for
>> problems, please do not." Unless you have additional comments on this, I
>> will plan to NOT use the request_firmware framework.
> request_firmware() should always be used for requesting firmware for a
> device.  Having an api where you write a random filename to a sysfs file
> and have that loaded by the kernel seems ripe for disaster though, I can
> not think of any other in-kernel user of the firmware api that does
> this.  Or are there examples that I have just missed?

I found an instance of a driver that allows the firmware filename
(under /lib/firmware) to be changed via sysfs:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n107
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n117

I get why it seems dangerous to provide a /lib/firmware filename via
sysfs and have the system automatically transfer that data to a device.
I'm just trying to figure out if there is a preferred/less-dangerous
way to do what we are trying to do.

Our objective is to allow the user to provide a new, signed FPGA image
that can be loaded on the fly (think cloud environment). The PCIe FPGA
card authenticates the image data with encryption keys; the host-side
software is not trusted by the device. So, other than checking the data
size, host software just passes the data through.

I think our usage is firmware-like, but it doesn't exactly fit the
current usage of request_firmware(). The firmware filename isn't
hardwired into the driver and the image isn't loaded at probe time.

If the request_firmware() implementation is not acceptable, then would
you agree that an IOCTL implementation is our best option?

- Russ

>
> thanks,
>
> greg k-h

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

[Greg KH]

On Mon, Aug 02, 2021 at 11:31:47AM -0700, Russ Weight wrote:
> 
> 
> On 7/30/21 4:18 AM, Greg KH wrote:
> > On Thu, Jul 29, 2021 at 06:23:12PM -0700, Russ Weight wrote:
> >>
> >> On 5/17/21 12:37 PM, Russ Weight wrote:
> >>> On 5/16/21 10:32 PM, Greg KH wrote:
> >>>> On Sun, May 16, 2021 at 07:31:50PM -0700, Moritz Fischer wrote:
> >>>>> From: Russ Weight <russell.h.weight@intel.com>
> >>>>>
> >>>>> Extend the FPGA Security Manager class driver to
> >>>>> include an update/filename sysfs node that can be used
> >>>>> to initiate a secure update.  The filename of a secure
> >>>>> update file (BMC image, FPGA image, Root Entry Hash image,
> >>>>> or Code Signing Key cancellation image) can be written to
> >>>>> this sysfs entry to cause a secure update to occur.
> >>>> Why is userspace responsible for triggering this?  Passing a "filename"
> >>>> into the kernel and having it do something with it is ripe for major
> >>>> problems, please do not.
> >>>>
> >>> I am using the "request_firmware" framework, which accepts a filename
> >>> and finds the firmware file under /lib/firmware.
> >>>
> >>> Is this not an acceptable use for request_firmware?
> >>>
> >>> - Russ
> >> Hi Greg,
> >>
> >> The dev_release fixes that you asked for in the FPGA Manager, Bridge, and
> >> Region code are almost complete. I'm trying to get back to the FPGA
> >> security manager patch set. Your previous comments challenged some basic
> >> assumptions. If it is OK, I would like to get some clarity before I rework
> >> the patches.
> > Note, I do not have the time, nor the inclination, to help your company
> > out with design reviews at this point in time.  If you have questions
> > about this, please discuss it with the open source managers at Intel,
> > they know the current situation quite well.
> >
> > I am glad to review patches that have gone through the proper internal
> > Intel patch review process and then sent out to the community.
> 
> For what it is worth, these patches _were_ reviewed internally before
> they were submitted to the public list. The first public submission
> (Sep 2020) included review tags. I was asked to remove them and let
> them be added back during the public review:
> 
> https://marc.info/?l=linux-fpga&m=159926670526828&w=2
> 
> Unfortunately, Intel review tags were not volunteered during the
> public review, and it didn't occur to me to solicit the tags before
> the patches were forwarded on to you by Moritz.
> 
> Most notably, Yilun and Hao contributed to the review both internally
> and publicly. They are both listed in the MAINTAINERS file:
> 
> Xu Yilun <yilun.xu@intel.com>
> Wu Hao <hao.wu@intel.com>
> 
> All issues/comments were resolved before the patches were sent to you.

That's fine and wonderful, but not what I was talking about here at all.

Again, I am willing to review patches, but not take the time out to
comment on general design decisions like this for the reasons stated in
the past.

> > That being said, I will make one comment on your questions below:
> >
> >> (1) request_firmware(). We had assumed that making use of the existing
> >> request_firmware() would be preferred. This requires providing a filename
> >> under /lib/firmware to the framework. You commented (above): "Passing a
> >> 'filename' into the kernel and having it do something with it is ripe for
> >> problems, please do not." Unless you have additional comments on this, I
> >> will plan to NOT use the request_firmware framework.
> > request_firmware() should always be used for requesting firmware for a
> > device.  Having an api where you write a random filename to a sysfs file
> > and have that loaded by the kernel seems ripe for disaster though, I can
> > not think of any other in-kernel user of the firmware api that does
> > this.  Or are there examples that I have just missed?
> 
> I found an instance of a driver that allows the firmware filename
> (under /lib/firmware) to be changed via sysfs:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n107
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/ABI/testing/sysfs-class-mic#n117

Ah, another Intel driver wanting to do bad things, nice example :)

> I get why it seems dangerous to provide a /lib/firmware filename via
> sysfs and have the system automatically transfer that data to a device.
> I'm just trying to figure out if there is a preferred/less-dangerous
> way to do what we are trying to do.
> 
> Our objective is to allow the user to provide a new, signed FPGA image
> that can be loaded on the fly (think cloud environment). The PCIe FPGA
> card authenticates the image data with encryption keys; the host-side
> software is not trusted by the device. So, other than checking the data
> size, host software just passes the data through.
> 
> I think our usage is firmware-like, but it doesn't exactly fit the
> current usage of request_firmware(). The firmware filename isn't
> hardwired into the driver and the image isn't loaded at probe time.
> 
> If the request_firmware() implementation is not acceptable, then would
> you agree that an IOCTL implementation is our best option?

There is no difference in the end between using an ioctl, or a sysfs
file, to provide the filename of your firmware, don't get hung up on
that.

By providing a "filename", you are going around all of the namespace and
other "container" protection that the kernel provides, and allowing
processes to potentially load files that are normally outside of their
scope to the hardware.  If you are willing to allow that security
"escape", wonderful, but you better document the heck out of it and
explain why this is allowed for your special hardware and use case.

As you are expecting this to work "in the cloud", I do not think that
the operators of such hardware are really going to be all that happy to
see this type of interface given these reasons.

What is wrong with the current fpga firmware api that somehow is lacking
for your special hardware, that other devices do not have to worry
about?

thanks,

greg k-h

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

[Russ Weight]

On 8/2/21 10:49 PM, Greg KH wrote:
>> If the request_firmware() implementation is not acceptable, then would
>> you agree that an IOCTL implementation is our best option?
> There is no difference in the end between using an ioctl, or a sysfs
> file, to provide the filename of your firmware, don't get hung up on
> that.

I meant to suggest that passing file data (not a filename) through an
IOCTL might be better for this use case than trying to use request_firmware.
We have to, somehow, allow the user to point us to the desired image
data (which could be a root-entry-hash, or an FPGA image). We can't
really use a fixed filename modified by device version as many of
the devices do.

> By providing a "filename", you are going around all of the namespace and
> other "container" protection that the kernel provides, and allowing
> processes to potentially load files that are normally outside of their
> scope to the hardware.  If you are willing to allow that security
> "escape", wonderful, but you better document the heck out of it and
> explain why this is allowed for your special hardware and use case.
>
> As you are expecting this to work "in the cloud", I do not think that
> the operators of such hardware are really going to be all that happy to
> see this type of interface given these reasons.
>
> What is wrong with the current fpga firmware api that somehow is lacking
> for your special hardware, that other devices do not have to worry
> about?
The existing framework wants to update the live image in the FPGA,
whereas for this device, we are passing signed data to BMC firmware
which will store it in FLASH to be loaded on a subsequent boot of
the card.

The existing framework needs to manage FPGA state, whereas for this
device, it is just a transfer of signed data. We also have to handle
a total transfer/authentication time of up to 45 minutes, so we are
using a kernel worker thread for the update.

Perhaps the name, fpga security manager, is wrong? Maybe something
like fpga_sec_image_xfer is better?

- Russ
> thanks,
>
> greg k-h

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

[Greg KH]

On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> 
> 
> On 8/2/21 10:49 PM, Greg KH wrote:
> >> If the request_firmware() implementation is not acceptable, then would
> >> you agree that an IOCTL implementation is our best option?
> > There is no difference in the end between using an ioctl, or a sysfs
> > file, to provide the filename of your firmware, don't get hung up on
> > that.
> 
> I meant to suggest that passing file data (not a filename) through an
> IOCTL might be better for this use case than trying to use request_firmware.
> We have to, somehow, allow the user to point us to the desired image
> data (which could be a root-entry-hash, or an FPGA image). We can't
> really use a fixed filename modified by device version as many of
> the devices do.

Ah, yes, a "normal" write command might be best for this as that can be
properly containerized and controlled.

> > By providing a "filename", you are going around all of the namespace and
> > other "container" protection that the kernel provides, and allowing
> > processes to potentially load files that are normally outside of their
> > scope to the hardware.  If you are willing to allow that security
> > "escape", wonderful, but you better document the heck out of it and
> > explain why this is allowed for your special hardware and use case.
> >
> > As you are expecting this to work "in the cloud", I do not think that
> > the operators of such hardware are really going to be all that happy to
> > see this type of interface given these reasons.
> >
> > What is wrong with the current fpga firmware api that somehow is lacking
> > for your special hardware, that other devices do not have to worry
> > about?
> The existing framework wants to update the live image in the FPGA,
> whereas for this device, we are passing signed data to BMC firmware
> which will store it in FLASH to be loaded on a subsequent boot of
> the card.
> 
> The existing framework needs to manage FPGA state, whereas for this
> device, it is just a transfer of signed data. We also have to handle
> a total transfer/authentication time of up to 45 minutes, so we are
> using a kernel worker thread for the update.
> 
> Perhaps the name, fpga security manager, is wrong? Maybe something
> like fpga_sec_image_xfer is better?

It does not sound like this has anything to do with "security", and
rather is just a normal firmware upload, so "fpga_image_upload()"
perhaps?

naming is hard,

greg k-h

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

[Moritz Fischer]

On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
> On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> > 
> > 
> > On 8/2/21 10:49 PM, Greg KH wrote:
> > >> If the request_firmware() implementation is not acceptable, then would
> > >> you agree that an IOCTL implementation is our best option?
> > > There is no difference in the end between using an ioctl, or a sysfs
> > > file, to provide the filename of your firmware, don't get hung up on
> > > that.
> > 
> > I meant to suggest that passing file data (not a filename) through an
> > IOCTL might be better for this use case than trying to use request_firmware.
> > We have to, somehow, allow the user to point us to the desired image
> > data (which could be a root-entry-hash, or an FPGA image). We can't
> > really use a fixed filename modified by device version as many of
> > the devices do.
> 
> Ah, yes, a "normal" write command might be best for this as that can be
> properly containerized and controlled.
> 
> > > By providing a "filename", you are going around all of the namespace and
> > > other "container" protection that the kernel provides, and allowing
> > > processes to potentially load files that are normally outside of their
> > > scope to the hardware.  If you are willing to allow that security
> > > "escape", wonderful, but you better document the heck out of it and
> > > explain why this is allowed for your special hardware and use case.
> > >
> > > As you are expecting this to work "in the cloud", I do not think that
> > > the operators of such hardware are really going to be all that happy to
> > > see this type of interface given these reasons.
> > >
> > > What is wrong with the current fpga firmware api that somehow is lacking
> > > for your special hardware, that other devices do not have to worry
> > > about?
> > The existing framework wants to update the live image in the FPGA,
> > whereas for this device, we are passing signed data to BMC firmware
> > which will store it in FLASH to be loaded on a subsequent boot of
> > the card.
> > 
> > The existing framework needs to manage FPGA state, whereas for this
> > device, it is just a transfer of signed data. We also have to handle
> > a total transfer/authentication time of up to 45 minutes, so we are
> > using a kernel worker thread for the update.
> > 
> > Perhaps the name, fpga security manager, is wrong? Maybe something
> > like fpga_sec_image_xfer is better?
> 
> It does not sound like this has anything to do with "security", and
> rather is just a normal firmware upload, so "fpga_image_upload()"
> perhaps?

I had originally suggested 'load' and 'persist' or 'load' and 'update or
something of that sort.

Taking one step back, maybe the case could be made for a generic
'persistent firmware' update framework that addresses use-cases that
require updating firmware that may take extended periods of time.

A similar case that comes to mind would be writing firmware to an
external flash on a Renesas xHCI controller, or updating the BMC
firmware for a plug-in card with BMC.

- Moritz

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

[Greg KH]

On Wed, Aug 04, 2021 at 07:58:34AM -0700, Moritz Fischer wrote:
> On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
> > On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> > > 
> > > 
> > > On 8/2/21 10:49 PM, Greg KH wrote:
> > > >> If the request_firmware() implementation is not acceptable, then would
> > > >> you agree that an IOCTL implementation is our best option?
> > > > There is no difference in the end between using an ioctl, or a sysfs
> > > > file, to provide the filename of your firmware, don't get hung up on
> > > > that.
> > > 
> > > I meant to suggest that passing file data (not a filename) through an
> > > IOCTL might be better for this use case than trying to use request_firmware.
> > > We have to, somehow, allow the user to point us to the desired image
> > > data (which could be a root-entry-hash, or an FPGA image). We can't
> > > really use a fixed filename modified by device version as many of
> > > the devices do.
> > 
> > Ah, yes, a "normal" write command might be best for this as that can be
> > properly containerized and controlled.
> > 
> > > > By providing a "filename", you are going around all of the namespace and
> > > > other "container" protection that the kernel provides, and allowing
> > > > processes to potentially load files that are normally outside of their
> > > > scope to the hardware.  If you are willing to allow that security
> > > > "escape", wonderful, but you better document the heck out of it and
> > > > explain why this is allowed for your special hardware and use case.
> > > >
> > > > As you are expecting this to work "in the cloud", I do not think that
> > > > the operators of such hardware are really going to be all that happy to
> > > > see this type of interface given these reasons.
> > > >
> > > > What is wrong with the current fpga firmware api that somehow is lacking
> > > > for your special hardware, that other devices do not have to worry
> > > > about?
> > > The existing framework wants to update the live image in the FPGA,
> > > whereas for this device, we are passing signed data to BMC firmware
> > > which will store it in FLASH to be loaded on a subsequent boot of
> > > the card.
> > > 
> > > The existing framework needs to manage FPGA state, whereas for this
> > > device, it is just a transfer of signed data. We also have to handle
> > > a total transfer/authentication time of up to 45 minutes, so we are
> > > using a kernel worker thread for the update.
> > > 
> > > Perhaps the name, fpga security manager, is wrong? Maybe something
> > > like fpga_sec_image_xfer is better?
> > 
> > It does not sound like this has anything to do with "security", and
> > rather is just a normal firmware upload, so "fpga_image_upload()"
> > perhaps?
> 
> I had originally suggested 'load' and 'persist' or 'load' and 'update or
> something of that sort.
> 
> Taking one step back, maybe the case could be made for a generic
> 'persistent firmware' update framework that addresses use-cases that
> require updating firmware that may take extended periods of time.

There should not be a problem with using the existing firmware layer for
images that take long periods of time, as long as you are not wanting to
see any potential progress :)

So how about just adding anything missing to the existing firmware
subsystem.  It's attempting to handle all use cases already, if it is
missing one, no harm in adding more options there...

thanks,

greg k-h

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

[Moritz Fischer]

On Wed, Aug 04, 2021 at 05:12:41PM +0200, Greg KH wrote:
> On Wed, Aug 04, 2021 at 07:58:34AM -0700, Moritz Fischer wrote:
> > On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
> > > On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> > > > 
> > > > 
> > > > On 8/2/21 10:49 PM, Greg KH wrote:
> > > > >> If the request_firmware() implementation is not acceptable, then would
> > > > >> you agree that an IOCTL implementation is our best option?
> > > > > There is no difference in the end between using an ioctl, or a sysfs
> > > > > file, to provide the filename of your firmware, don't get hung up on
> > > > > that.
> > > > 
> > > > I meant to suggest that passing file data (not a filename) through an
> > > > IOCTL might be better for this use case than trying to use request_firmware.
> > > > We have to, somehow, allow the user to point us to the desired image
> > > > data (which could be a root-entry-hash, or an FPGA image). We can't
> > > > really use a fixed filename modified by device version as many of
> > > > the devices do.
> > > 
> > > Ah, yes, a "normal" write command might be best for this as that can be
> > > properly containerized and controlled.
> > > 
> > > > > By providing a "filename", you are going around all of the namespace and
> > > > > other "container" protection that the kernel provides, and allowing
> > > > > processes to potentially load files that are normally outside of their
> > > > > scope to the hardware.  If you are willing to allow that security
> > > > > "escape", wonderful, but you better document the heck out of it and
> > > > > explain why this is allowed for your special hardware and use case.
> > > > >
> > > > > As you are expecting this to work "in the cloud", I do not think that
> > > > > the operators of such hardware are really going to be all that happy to
> > > > > see this type of interface given these reasons.
> > > > >
> > > > > What is wrong with the current fpga firmware api that somehow is lacking
> > > > > for your special hardware, that other devices do not have to worry
> > > > > about?
> > > > The existing framework wants to update the live image in the FPGA,
> > > > whereas for this device, we are passing signed data to BMC firmware
> > > > which will store it in FLASH to be loaded on a subsequent boot of
> > > > the card.
> > > > 
> > > > The existing framework needs to manage FPGA state, whereas for this
> > > > device, it is just a transfer of signed data. We also have to handle
> > > > a total transfer/authentication time of up to 45 minutes, so we are
> > > > using a kernel worker thread for the update.
> > > > 
> > > > Perhaps the name, fpga security manager, is wrong? Maybe something
> > > > like fpga_sec_image_xfer is better?
> > > 
> > > It does not sound like this has anything to do with "security", and
> > > rather is just a normal firmware upload, so "fpga_image_upload()"
> > > perhaps?
> > 
> > I had originally suggested 'load' and 'persist' or 'load' and 'update or
> > something of that sort.
> > 
> > Taking one step back, maybe the case could be made for a generic
> > 'persistent firmware' update framework that addresses use-cases that
> > require updating firmware that may take extended periods of time.
> 
> There should not be a problem with using the existing firmware layer for
> images that take long periods of time, as long as you are not wanting to
> see any potential progress :)
> 
> So how about just adding anything missing to the existing firmware
> subsystem.  It's attempting to handle all use cases already, if it is
> missing one, no harm in adding more options there...

Even better if we can do that. It would have a limited overlap with
existing functionality, though.

- Moritz

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

[Russ Weight]

On 8/4/21 8:12 AM, Greg KH wrote:
> On Wed, Aug 04, 2021 at 07:58:34AM -0700, Moritz Fischer wrote:
>> On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
>>> On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
>>>>
>>>> On 8/2/21 10:49 PM, Greg KH wrote:
>>>>>> If the request_firmware() implementation is not acceptable, then would
>>>>>> you agree that an IOCTL implementation is our best option?
>>>>> There is no difference in the end between using an ioctl, or a sysfs
>>>>> file, to provide the filename of your firmware, don't get hung up on
>>>>> that.
>>>> I meant to suggest that passing file data (not a filename) through an
>>>> IOCTL might be better for this use case than trying to use request_firmware.
>>>> We have to, somehow, allow the user to point us to the desired image
>>>> data (which could be a root-entry-hash, or an FPGA image). We can't
>>>> really use a fixed filename modified by device version as many of
>>>> the devices do.
>>> Ah, yes, a "normal" write command might be best for this as that can be
>>> properly containerized and controlled.
>>>
>>>>> By providing a "filename", you are going around all of the namespace and
>>>>> other "container" protection that the kernel provides, and allowing
>>>>> processes to potentially load files that are normally outside of their
>>>>> scope to the hardware.  If you are willing to allow that security
>>>>> "escape", wonderful, but you better document the heck out of it and
>>>>> explain why this is allowed for your special hardware and use case.
>>>>>
>>>>> As you are expecting this to work "in the cloud", I do not think that
>>>>> the operators of such hardware are really going to be all that happy to
>>>>> see this type of interface given these reasons.
>>>>>
>>>>> What is wrong with the current fpga firmware api that somehow is lacking
>>>>> for your special hardware, that other devices do not have to worry
>>>>> about?
>>>> The existing framework wants to update the live image in the FPGA,
>>>> whereas for this device, we are passing signed data to BMC firmware
>>>> which will store it in FLASH to be loaded on a subsequent boot of
>>>> the card.
>>>>
>>>> The existing framework needs to manage FPGA state, whereas for this
>>>> device, it is just a transfer of signed data. We also have to handle
>>>> a total transfer/authentication time of up to 45 minutes, so we are
>>>> using a kernel worker thread for the update.
>>>>
>>>> Perhaps the name, fpga security manager, is wrong? Maybe something
>>>> like fpga_sec_image_xfer is better?
>>> It does not sound like this has anything to do with "security", and
>>> rather is just a normal firmware upload, so "fpga_image_upload()"
>>> perhaps?
>> I had originally suggested 'load' and 'persist' or 'load' and 'update or
>> something of that sort.
>>
>> Taking one step back, maybe the case could be made for a generic
>> 'persistent firmware' update framework that addresses use-cases that
>> require updating firmware that may take extended periods of time.
> There should not be a problem with using the existing firmware layer for
> images that take long periods of time, as long as you are not wanting to
> see any potential progress :)
>
> So how about just adding anything missing to the existing firmware
> subsystem.  It's attempting to handle all use cases already, if it is
> missing one, no harm in adding more options there...
Hi Greg,

We have had a lot of internal (to Intel) discussion about how to
organize the support for uploading FPGA images. It would be helpful
to know which of the following two options you find the least
disturbing :-)

Background: We are uploading signed, self-describing images that are
authenticated and dispositioned by the Card BMC. These could result
in FLASH updates for FPGA images, BMC images, firmware, or security
keys.  They could also result in a temporary authentication
certificate being loaded into RAM as part of a multi-step key
provisioning process.

Options:
(a) A single API that facilitates the upload of a data stream
without analyzing the stream contents, relying on the lower-level
driver and/or HW to accept or reject the data.

(b) Multiple, targeted APIs (e.g. IOCTL_FPGA_IMAGE_UPDATE,
IOCTL_BMC_IMAGE_UPDATE, IOCTL_KEY_UPDATE, IOCTL_KEY_CANCEL) that
each interpret the stream type and reject them if they don't
correspond to the API target.

Do you have a preference between (a) and (b)?

Thanks,
- Russ
> thanks,
>
> greg k-h

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

[Greg KH]

On Tue, Nov 02, 2021 at 09:25:10AM -0700, Russ Weight wrote:
> 
> 
> On 8/4/21 8:12 AM, Greg KH wrote:
> > On Wed, Aug 04, 2021 at 07:58:34AM -0700, Moritz Fischer wrote:
> >> On Wed, Aug 04, 2021 at 09:37:45AM +0200, Greg KH wrote:
> >>> On Tue, Aug 03, 2021 at 12:02:24PM -0700, Russ Weight wrote:
> >>>>
> >>>> On 8/2/21 10:49 PM, Greg KH wrote:
> >>>>>> If the request_firmware() implementation is not acceptable, then would
> >>>>>> you agree that an IOCTL implementation is our best option?
> >>>>> There is no difference in the end between using an ioctl, or a sysfs
> >>>>> file, to provide the filename of your firmware, don't get hung up on
> >>>>> that.
> >>>> I meant to suggest that passing file data (not a filename) through an
> >>>> IOCTL might be better for this use case than trying to use request_firmware.
> >>>> We have to, somehow, allow the user to point us to the desired image
> >>>> data (which could be a root-entry-hash, or an FPGA image). We can't
> >>>> really use a fixed filename modified by device version as many of
> >>>> the devices do.
> >>> Ah, yes, a "normal" write command might be best for this as that can be
> >>> properly containerized and controlled.
> >>>
> >>>>> By providing a "filename", you are going around all of the namespace and
> >>>>> other "container" protection that the kernel provides, and allowing
> >>>>> processes to potentially load files that are normally outside of their
> >>>>> scope to the hardware.  If you are willing to allow that security
> >>>>> "escape", wonderful, but you better document the heck out of it and
> >>>>> explain why this is allowed for your special hardware and use case.
> >>>>>
> >>>>> As you are expecting this to work "in the cloud", I do not think that
> >>>>> the operators of such hardware are really going to be all that happy to
> >>>>> see this type of interface given these reasons.
> >>>>>
> >>>>> What is wrong with the current fpga firmware api that somehow is lacking
> >>>>> for your special hardware, that other devices do not have to worry
> >>>>> about?
> >>>> The existing framework wants to update the live image in the FPGA,
> >>>> whereas for this device, we are passing signed data to BMC firmware
> >>>> which will store it in FLASH to be loaded on a subsequent boot of
> >>>> the card.
> >>>>
> >>>> The existing framework needs to manage FPGA state, whereas for this
> >>>> device, it is just a transfer of signed data. We also have to handle
> >>>> a total transfer/authentication time of up to 45 minutes, so we are
> >>>> using a kernel worker thread for the update.
> >>>>
> >>>> Perhaps the name, fpga security manager, is wrong? Maybe something
> >>>> like fpga_sec_image_xfer is better?
> >>> It does not sound like this has anything to do with "security", and
> >>> rather is just a normal firmware upload, so "fpga_image_upload()"
> >>> perhaps?
> >> I had originally suggested 'load' and 'persist' or 'load' and 'update or
> >> something of that sort.
> >>
> >> Taking one step back, maybe the case could be made for a generic
> >> 'persistent firmware' update framework that addresses use-cases that
> >> require updating firmware that may take extended periods of time.
> > There should not be a problem with using the existing firmware layer for
> > images that take long periods of time, as long as you are not wanting to
> > see any potential progress :)
> >
> > So how about just adding anything missing to the existing firmware
> > subsystem.  It's attempting to handle all use cases already, if it is
> > missing one, no harm in adding more options there...
> Hi Greg,
> 
> We have had a lot of internal (to Intel) discussion about how to
> organize the support for uploading FPGA images. It would be helpful
> to know which of the following two options you find the least
> disturbing :-)
> 
> Background: We are uploading signed, self-describing images that are
> authenticated and dispositioned by the Card BMC. These could result
> in FLASH updates for FPGA images, BMC images, firmware, or security
> keys.  They could also result in a temporary authentication
> certificate being loaded into RAM as part of a multi-step key
> provisioning process.
> 
> Options:
> (a) A single API that facilitates the upload of a data stream
> without analyzing the stream contents, relying on the lower-level
> driver and/or HW to accept or reject the data.

That is the firmware api we have today, please use that like all other
drivers should be using.

> (b) Multiple, targeted APIs (e.g. IOCTL_FPGA_IMAGE_UPDATE,
> IOCTL_BMC_IMAGE_UPDATE, IOCTL_KEY_UPDATE, IOCTL_KEY_CANCEL) that
> each interpret the stream type and reject them if they don't
> correspond to the API target.

Please no, do not make a zillion "custom" ioctls.  That way lies
madness.  Will you want to maintain them all for the next 30+ years?

thanks,

greg k-h