These patches depend on the patchset: "add regmap-spi-avmm & Intel
Max10 BMC chip support" which is currently under review.
--------------------------------------------------
This patchset introduces the Intel Security Manager class driver
for managing secure updates on Intel FPGA Cards. It also provides
the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
is implemented using the Intel Security Manager class driver.
The Intel Security Manager class driver provides a common API for
user-space tools to manage updates for Secure FPGA devices. Device
drivers that instantiate the Intel Security Manager class driver will
interact with the 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.
The API consists of sysfs nodes and supports the following functions:
(1) Instantiate and monitor a secure update
(2) Display security information including: Root Entry Hashes (REH),
Cancelled Code Signing Keys (CSK), and flash update counts for
both BMC and FPGA images.
Secure updates make use of the request_firmware framework, which
requires that image files are accessible under /lib/firmware. A request
for a secure update returns immediately, while the update itself
proceeds in the context of a kernel worker thread. Sysfs files provide
a means for monitoring the progress of a secure update and for
retrieving error information in the event of a failure.
The n3000bmc-secure driver instantiates the Intel Security Manager
class driver and provides the callback functions required to support
secure updates on Intel n3000 PAC devices.
Russ Weight (12):
fpga: fpga security manager class driver
fpga: create intel max10 bmc security engine
fpga: expose max10 flash update counts in sysfs
fpga: expose max10 canceled keys in sysfs
fpga: enable secure updates
fpga: add max10 secure update functions
fpga: expose sec-mgr update status
fpga: expose sec-mgr update errors
fpga: expose sec-mgr update size
fpga: enable sec-mgr update cancel
fpga: expose hardware error info in sysfs
fpga: add max10 get_hw_errinfo callback func
.../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
MAINTAINERS | 8 +
drivers/fpga/Kconfig | 20 +
drivers/fpga/Makefile | 6 +
drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
include/linux/mfd/intel-m10-bmc.h | 116 +++
8 files changed, 1728 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
create mode 100644 drivers/fpga/ifpga-sec-mgr.c
create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
--
2.17.1
Extend the MAX10 BMC Security Engine driver to provide a
handler 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 <[email protected]>
Reviewed-by: Wu Hao <[email protected]>
---
drivers/fpga/intel-m10-bmc-secure.c | 60 +++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index b824790e43aa..46cd49a08be0 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -130,14 +130,74 @@ static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
return ret ? : cnt;
}
+#define CSK_BIT_LEN 128U
+#define CSK_32ARRAY_SIZE(_nbits) DIV_ROUND_UP(_nbits, 32)
+
+#define SYSFS_GET_CSK_CANCEL_NBITS(_name) \
+static int get_##_name##_csk_cancel_nbits(struct ifpga_sec_mgr *imgr) \
+{ \
+ return (int)CSK_BIT_LEN; \
+}
+
+SYSFS_GET_CSK_CANCEL_NBITS(bmc)
+SYSFS_GET_CSK_CANCEL_NBITS(sr)
+SYSFS_GET_CSK_CANCEL_NBITS(pr)
+
+static int get_csk_vector(struct ifpga_sec_mgr *imgr, u32 addr,
+ unsigned long *csk_map, unsigned int nbits)
+{
+ unsigned int i, arr_size = CSK_32ARRAY_SIZE(nbits);
+ struct m10bmc_sec *sec = imgr->priv;
+ u32 *csk32;
+ int ret;
+
+ csk32 = vmalloc(arr_size);
+ if (!csk32)
+ return -ENOMEM;
+
+ ret = m10bmc_raw_bulk_read(sec->m10bmc, addr, csk32, arr_size);
+ if (ret) {
+ dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
+ goto vfree_exit;
+ }
+
+ for (i = 0; i < arr_size; i++)
+ csk32[i] = le32_to_cpu(csk32[i]);
+
+ bitmap_from_arr32(csk_map, csk32, nbits);
+ bitmap_complement(csk_map, csk_map, nbits);
+
+vfree_exit:
+ vfree(csk32);
+ return ret;
+}
+
+#define SYSFS_GET_CSK_VEC(_name, _addr) \
+static int get_##_name##_canceled_csks(struct ifpga_sec_mgr *imgr, \
+ unsigned long *csk_map, \
+ unsigned int nbits) \
+{ return get_csk_vector(imgr, _addr, csk_map, nbits); }
+
+#define CSK_VEC_OFFSET 0x34
+
+SYSFS_GET_CSK_VEC(bmc, BMC_PROG_ADDR + CSK_VEC_OFFSET)
+SYSFS_GET_CSK_VEC(sr, SR_PROG_ADDR + CSK_VEC_OFFSET)
+SYSFS_GET_CSK_VEC(pr, PR_PROG_ADDR + CSK_VEC_OFFSET)
+
static const struct ifpga_sec_mgr_ops m10bmc_iops = {
.user_flash_count = get_qspi_flash_count,
.bmc_root_entry_hash = get_bmc_root_entry_hash,
.sr_root_entry_hash = get_sr_root_entry_hash,
.pr_root_entry_hash = get_pr_root_entry_hash,
+ .bmc_canceled_csks = get_bmc_canceled_csks,
+ .sr_canceled_csks = get_sr_canceled_csks,
+ .pr_canceled_csks = get_pr_canceled_csks,
.bmc_reh_size = get_bmc_reh_size,
.sr_reh_size = get_sr_reh_size,
.pr_reh_size = get_pr_reh_size,
+ .bmc_canceled_csk_nbits = get_bmc_csk_cancel_nbits,
+ .sr_canceled_csk_nbits = get_sr_csk_cancel_nbits,
+ .pr_canceled_csk_nbits = get_pr_csk_cancel_nbits
};
static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
--
2.17.1
Extend the MAX10 BMC Security Engine driver to provide a
handler to expose the flash update count for the FPGA user
image.
Signed-off-by: Russ Weight <[email protected]>
Reviewed-by: Wu Hao <[email protected]>
---
drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
index 1f86bfb694b4..b824790e43aa 100644
--- a/drivers/fpga/intel-m10-bmc-secure.c
+++ b/drivers/fpga/intel-m10-bmc-secure.c
@@ -10,6 +10,7 @@
#include <linux/mfd/intel-m10-bmc.h>
#include <linux/module.h>
#include <linux/platform_device.h>
+#include <linux/slab.h>
#include <linux/vmalloc.h>
struct m10bmc_sec {
@@ -99,7 +100,38 @@ SYSFS_GET_REH(bmc, BMC_REH_ADDR)
SYSFS_GET_REH(sr, SR_REH_ADDR)
SYSFS_GET_REH(pr, PR_REH_ADDR)
+#define FLASH_COUNT_SIZE 4096
+#define USER_FLASH_COUNT 0x17ffb000
+
+static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
+{
+ struct m10bmc_sec *sec = imgr->priv;
+ unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
+ unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
+ u8 *flash_buf;
+ int ret;
+
+ flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
+ if (!flash_buf)
+ return -ENOMEM;
+
+ ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
+ FLASH_COUNT_SIZE / stride);
+ if (ret) {
+ dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
+ goto exit_free;
+ }
+
+ cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
+
+exit_free:
+ kfree(flash_buf);
+
+ return ret ? : cnt;
+}
+
static const struct ifpga_sec_mgr_ops m10bmc_iops = {
+ .user_flash_count = get_qspi_flash_count,
.bmc_root_entry_hash = get_bmc_root_entry_hash,
.sr_root_entry_hash = get_sr_root_entry_hash,
.pr_root_entry_hash = get_pr_root_entry_hash,
--
2.17.1
Extend the FPGA Intel Security Manager class driver to
include an update/filename sysfs node that can be used
to initiate a security 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 <[email protected]>
---
.../ABI/testing/sysfs-class-ifpga-sec-mgr | 13 ++
drivers/fpga/ifpga-sec-mgr.c | 155 ++++++++++++++++++
include/linux/fpga/ifpga-sec-mgr.h | 49 ++++++
3 files changed, 217 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
index 86f8992559bf..a476504b7ae9 100644
--- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
@@ -73,3 +73,16 @@ Contact: Russ Weight <[email protected]>
Description: Read only. Returns number of times the BMC image has been
flashed.
Format: "%d".
+
+What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/filename
+Date: Sep 2020
+KernelVersion: 5.10
+Contact: Russ Weight <[email protected]>
+Description: Write only. Write the filename of an Intel 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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
index 97bf80277ed2..73173badbe96 100644
--- a/drivers/fpga/ifpga-sec-mgr.c
+++ b/drivers/fpga/ifpga-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/ifpga-sec-mgr.h>
#include <linux/idr.h>
+#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/vmalloc.h>
@@ -14,6 +17,8 @@
static DEFINE_IDA(ifpga_sec_mgr_ida);
static struct class *ifpga_sec_mgr_class;
+#define WRITE_BLOCK_SIZE 0x4000
+
static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
sysfs_csk_hndlr_t get_csk,
sysfs_csk_nbits_t get_csk_nbits,
@@ -134,6 +139,91 @@ static struct attribute *sec_mgr_security_attrs[] = {
NULL,
};
+static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
+ enum ifpga_sec_err err_code)
+{
+ imgr->err_code = err_code;
+ imgr->iops->cancel(imgr);
+}
+
+static void progress_complete(struct ifpga_sec_mgr *imgr)
+{
+ mutex_lock(&imgr->lock);
+ imgr->progress = IFPGA_SEC_PROG_IDLE;
+ complete_all(&imgr->update_done);
+ mutex_unlock(&imgr->lock);
+}
+
+static void ifpga_sec_mgr_update(struct work_struct *work)
+{
+ u32 size, blk_size, offset = 0;
+ struct ifpga_sec_mgr *imgr;
+ const struct firmware *fw;
+ enum ifpga_sec_err ret;
+
+ imgr = container_of(work, struct ifpga_sec_mgr, work);
+
+ get_device(&imgr->dev);
+ if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
+ imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
+ goto idle_exit;
+ }
+
+ imgr->data = fw->data;
+ imgr->remaining_size = fw->size;
+
+ if (!try_module_get(imgr->dev.parent->driver->owner)) {
+ imgr->err_code = IFPGA_SEC_ERR_BUSY;
+ goto release_fw_exit;
+ }
+
+ imgr->progress = IFPGA_SEC_PROG_PREPARING;
+ ret = imgr->iops->prepare(imgr);
+ if (ret) {
+ ifpga_sec_dev_error(imgr, ret);
+ goto modput_exit;
+ }
+
+ imgr->progress = IFPGA_SEC_PROG_WRITING;
+ size = imgr->remaining_size;
+ while (size) {
+ blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
+ size -= blk_size;
+ ret = imgr->iops->write_blk(imgr, offset, blk_size);
+ if (ret) {
+ ifpga_sec_dev_error(imgr, ret);
+ goto done;
+ }
+
+ imgr->remaining_size = size;
+ offset += blk_size;
+ }
+
+ imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
+ ret = imgr->iops->poll_complete(imgr);
+ if (ret) {
+ ifpga_sec_dev_error(imgr, ret);
+ goto done;
+ }
+
+done:
+ if (imgr->iops->cleanup)
+ imgr->iops->cleanup(imgr);
+
+modput_exit:
+ module_put(imgr->dev.parent->driver->owner);
+
+release_fw_exit:
+ imgr->data = NULL;
+ release_firmware(fw);
+
+idle_exit:
+ kfree(imgr->filename);
+ imgr->filename = NULL;
+ put_device(&imgr->dev);
+ progress_complete(imgr);
+}
+
#define check_attr(attribute, _name) \
((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
@@ -161,6 +251,51 @@ static struct attribute_group sec_mgr_security_attr_group = {
.is_visible = sec_mgr_visible,
};
+static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
+ int ret = 0;
+
+ if (count == 0 || count >= PATH_MAX)
+ return -EINVAL;
+
+ mutex_lock(&imgr->lock);
+ if (imgr->driver_unload || imgr->progress != IFPGA_SEC_PROG_IDLE) {
+ ret = -EBUSY;
+ goto unlock_exit;
+ }
+
+ imgr->filename = kstrndup(buf, PATH_MAX - 1, GFP_KERNEL);
+ if (!imgr->filename) {
+ ret = -ENOMEM;
+ goto unlock_exit;
+ }
+
+ if (imgr->filename[strlen(imgr->filename) - 1] == '\n')
+ imgr->filename[strlen(imgr->filename) - 1] = '\0';
+
+ imgr->err_code = IFPGA_SEC_ERR_NONE;
+ imgr->progress = IFPGA_SEC_PROG_READ_FILE;
+ reinit_completion(&imgr->update_done);
+ schedule_work(&imgr->work);
+
+unlock_exit:
+ mutex_unlock(&imgr->lock);
+ return ret ? : count;
+}
+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)
{
@@ -182,6 +317,7 @@ static struct attribute_group sec_mgr_attr_group = {
static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
&sec_mgr_attr_group,
&sec_mgr_security_attr_group,
+ &sec_mgr_update_attr_group,
NULL,
};
@@ -233,6 +369,12 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
struct ifpga_sec_mgr *imgr;
int id, ret;
+ if (!iops || !iops->cancel || !iops->prepare ||
+ !iops->write_blk || !iops->poll_complete) {
+ dev_err(dev, "Attempt to register without ifpga_sec_mgr_ops\n");
+ return NULL;
+ }
+
if (!check_reh_handler(dev, iops, bmc) ||
!check_reh_handler(dev, iops, sr) ||
!check_reh_handler(dev, iops, pr) ||
@@ -254,6 +396,8 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
imgr->name = name;
imgr->priv = priv;
imgr->iops = iops;
+ init_completion(&imgr->update_done);
+ INIT_WORK(&imgr->work, ifpga_sec_mgr_update);
mutex_init(&imgr->lock);
id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
@@ -299,6 +443,17 @@ void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
{
dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
+ mutex_lock(&imgr->lock);
+ imgr->driver_unload = true;
+ if (imgr->progress == IFPGA_SEC_PROG_IDLE) {
+ mutex_unlock(&imgr->lock);
+ goto unregister;
+ }
+
+ mutex_unlock(&imgr->lock);
+ wait_for_completion(&imgr->update_done);
+
+unregister:
device_unregister(&imgr->dev);
}
EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
index e391b0c8f448..4da2864e251c 100644
--- a/include/linux/fpga/ifpga-sec-mgr.h
+++ b/include/linux/fpga/ifpga-sec-mgr.h
@@ -7,6 +7,7 @@
#ifndef _LINUX_IFPGA_SEC_MGR_H
#define _LINUX_IFPGA_SEC_MGR_H
+#include <linux/completion.h>
#include <linux/device.h>
#include <linux/mutex.h>
#include <linux/types.h>
@@ -86,6 +87,19 @@ typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
unsigned long *csk_map, unsigned int nbits);
+enum ifpga_sec_err {
+ IFPGA_SEC_ERR_NONE = 0x0,
+ IFPGA_SEC_ERR_HW_ERROR = 0x1,
+ IFPGA_SEC_ERR_TIMEOUT = 0x2,
+ IFPGA_SEC_ERR_CANCELED = 0x3,
+ IFPGA_SEC_ERR_BUSY = 0x4,
+ IFPGA_SEC_ERR_INVALID_SIZE = 0x5,
+ IFPGA_SEC_ERR_RW_ERROR = 0x6,
+ IFPGA_SEC_ERR_WEAROUT = 0x7,
+ IFPGA_SEC_ERR_FILE_READ = 0x8,
+ IFPGA_SEC_ERR_MAX = 0x9
+};
+
/**
* struct ifpga_sec_mgr_ops - device specific operations
* @user_flash_count: Optional: Return sysfs string output for FPGA
@@ -110,6 +124,17 @@ typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
* @bmc_reh_size: Optional: Return byte size for BMC root entry hash
* @sr_reh_size: Optional: Return byte size for SR root entry hash
* @pr_reh_size: Optional: Return byte size for PR root entry hash
+ * @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 imgr->driver_unload
+ * and abort with IFPGA_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 ifpga_sec_mgr_ops {
sysfs_cnt_hndlr_t user_flash_count;
@@ -127,6 +152,22 @@ struct ifpga_sec_mgr_ops {
sysfs_csk_nbits_t bmc_canceled_csk_nbits;
sysfs_csk_nbits_t sr_canceled_csk_nbits;
sysfs_csk_nbits_t pr_canceled_csk_nbits;
+ enum ifpga_sec_err (*prepare)(struct ifpga_sec_mgr *imgr);
+ enum ifpga_sec_err (*write_blk)(struct ifpga_sec_mgr *imgr,
+ u32 offset, u32 size);
+ enum ifpga_sec_err (*poll_complete)(struct ifpga_sec_mgr *imgr);
+ void (*cleanup)(struct ifpga_sec_mgr *imgr);
+ enum ifpga_sec_err (*cancel)(struct ifpga_sec_mgr *imgr);
+};
+
+/* Update progress codes */
+enum ifpga_sec_prog {
+ IFPGA_SEC_PROG_IDLE = 0x0,
+ IFPGA_SEC_PROG_READ_FILE = 0x1,
+ IFPGA_SEC_PROG_PREPARING = 0x2,
+ IFPGA_SEC_PROG_WRITING = 0x3,
+ IFPGA_SEC_PROG_PROGRAMMING = 0x4,
+ IFPGA_SEC_PROG_MAX = 0x5
};
struct ifpga_sec_mgr {
@@ -134,6 +175,14 @@ struct ifpga_sec_mgr {
struct device dev;
const struct ifpga_sec_mgr_ops *iops;
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 ifpga_sec_prog progress;
+ enum ifpga_sec_err err_code; /* security manager error code */
+ bool driver_unload;
void *priv;
};
--
2.17.1
> Subject: [PATCH v1 00/12] Intel FPGA Security Manager Class Driver
>
>
> These patches depend on the patchset: "add regmap-spi-avmm & Intel
> Max10 BMC chip support" which is currently under review.
>
> --------------------------------------------------
>
> This patchset introduces the Intel Security Manager class driver
> for managing secure updates on Intel FPGA Cards. It also provides
> the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
> Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
> is implemented using the Intel Security Manager class driver.
So this patchset contains two parts
(1) adding a new class driver for Intel FPGA secure update.
(2) a new driver which uses (1) to implement secure update for n3000 PAC.
And only part (2) depends on "Intel MAX10 BMC chip support" patchset.
(Maybe you can provide a link to that thread).
Is my understanding correct? If yes, is it possible to reorder these patches?
At least there is no dependency on the class driver patches, right?
>
> The Intel Security Manager class driver provides a common API for
> user-space tools to manage updates for Secure FPGA devices. Device
> drivers that instantiate the Intel Security Manager class driver will
> interact with the 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.
>
> The API consists of sysfs nodes and supports the following functions:
>
> (1) Instantiate and monitor a secure update
> (2) Display security information including: Root Entry Hashes (REH),
> Cancelled Code Signing Keys (CSK), and flash update counts for
> both BMC and FPGA images.
>
> 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.
Maybe you can explain a little more on why we need to have this done
via a class driver not just some internal code in max10 driver? This class
driver will be reused in different cases? And why adding a new class
driver not just reuse or extend fpga manager (existing fpga mgr is used
to update fpga too).
>
> The n3000bmc-secure driver instantiates the Intel Security Manager
> class driver and provides the callback functions required to support
> secure updates on Intel n3000 PAC devices.
>
> Russ Weight (12):
> fpga: fpga security manager class driver
Intel FPGA Security Manager?
> fpga: create intel max10 bmc security engine
> fpga: expose max10 flash update counts in sysfs
> fpga: expose max10 canceled keys in sysfs
> fpga: enable secure updates
> fpga: add max10 secure update functions
> fpga: expose sec-mgr update status
> fpga: expose sec-mgr update errors
> fpga: expose sec-mgr update size
> fpga: enable sec-mgr update cancel
> fpga: expose hardware error info in sysfs
For these patches, is it possible to have a better title for these patches.
Then it will be easier to know which component this patch is going to modify.
e.g. fpga: ifpga-sec-mgr: xxxxxx
Thanks
Hao
> fpga: add max10 get_hw_errinfo callback func
>
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
> MAINTAINERS | 8 +
> drivers/fpga/Kconfig | 20 +
> drivers/fpga/Makefile | 6 +
> drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
> drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
> include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
> include/linux/mfd/intel-m10-bmc.h | 116 +++
> 8 files changed, 1728 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
> create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>
> --
> 2.17.1
On 9/4/20 4:52 PM, Russ Weight wrote:
> These patches depend on the patchset: "add regmap-spi-avmm & Intel
> Max10 BMC chip support" which is currently under review.
https://marc.info/?l=linux-kernel&m=159782274232229&w=2
regmap-spi-avmm is in linux-next.
max10 is not. however applying it does not resolve resolve git am conflicts with yesterday's linux-next. I normally build the larger patchsets as a test.
> --------------------------------------------------
>
> This patchset introduces the Intel Security Manager class driver
> for managing secure updates on Intel FPGA Cards. It also provides
> the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
> Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
> is implemented using the Intel Security Manager class driver.
>
> The Intel Security Manager class driver provides a common API for
> user-space tools to manage updates for Secure FPGA devices. Device
> drivers that instantiate the Intel Security Manager class driver will
> interact with the 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.
>
> The API consists of sysfs nodes and supports the following functions:
>
> (1) Instantiate and monitor a secure update
> (2) Display security information including: Root Entry Hashes (REH),
> Cancelled Code Signing Keys (CSK), and flash update counts for
> both BMC and FPGA images.
>
> Secure updates make use of the request_firmware framework, which
> requires that image files are accessible under /lib/firmware. A request
> for a secure update returns immediately, while the update itself
> proceeds in the context of a kernel worker thread. Sysfs files provide
> a means for monitoring the progress of a secure update and for
> retrieving error information in the event of a failure.
>
> The n3000bmc-secure driver instantiates the Intel Security Manager
> class driver and provides the callback functions required to support
> secure updates on Intel n3000 PAC devices.
This is a good description. Because security manager is a new interface, there should be a Documentation/fpga/ifpga-sec-mgr.rst to collect this description.
How will these devices be discovered ? n3000 is a dfl device, will there be a dfl feature id for it at some point ?
Can you describe if/how the security manager would live outside of dfl ? I am wondering why this shouldn't be dfl-sec-mgr.
I did not see any version handling. How would this sw adapt to a newer or older version of the bmc interface?
Tom
>
> Russ Weight (12):
> fpga: fpga security manager class driver
> fpga: create intel max10 bmc security engine
> fpga: expose max10 flash update counts in sysfs
> fpga: expose max10 canceled keys in sysfs
> fpga: enable secure updates
> fpga: add max10 secure update functions
> fpga: expose sec-mgr update status
> fpga: expose sec-mgr update errors
> fpga: expose sec-mgr update size
> fpga: enable sec-mgr update cancel
> fpga: expose hardware error info in sysfs
> fpga: add max10 get_hw_errinfo callback func
>
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
> MAINTAINERS | 8 +
> drivers/fpga/Kconfig | 20 +
> drivers/fpga/Makefile | 6 +
> drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
> drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
> include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
> include/linux/mfd/intel-m10-bmc.h | 116 +++
> 8 files changed, 1728 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
> create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>
resending.
sorry for blowing past 80 chars.
On 9/4/20 4:52 PM, Russ Weight wrote:
> These patches depend on the patchset: "add regmap-spi-avmm & Intel
> Max10 BMC chip support" which is currently under review.
https://marc.info/?l=linux-kernel&m=159782274232229&w=2
regmap-spi-avmm is in linux-next.
max10 is not. however applying it does not resolve resolve
git am conflicts with yesterday's linux-next.
I normally build the larger patchsets as a test.
>
> --------------------------------------------------
>
> This patchset introduces the Intel Security Manager class driver
> for managing secure updates on Intel FPGA Cards. It also provides
> the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
> Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
> is implemented using the Intel Security Manager class driver.
>
> The Intel Security Manager class driver provides a common API for
> user-space tools to manage updates for Secure FPGA devices. Device
> drivers that instantiate the Intel Security Manager class driver will
> interact with the 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.
>
> The API consists of sysfs nodes and supports the following functions:
>
> (1) Instantiate and monitor a secure update
> (2) Display security information including: Root Entry Hashes (REH),
> Cancelled Code Signing Keys (CSK), and flash update counts for
> both BMC and FPGA images.
>
> Secure updates make use of the request_firmware framework, which
> requires that image files are accessible under /lib/firmware. A request
> for a secure update returns immediately, while the update itself
> proceeds in the context of a kernel worker thread. Sysfs files provide
> a means for monitoring the progress of a secure update and for
> retrieving error information in the event of a failure.
>
> The n3000bmc-secure driver instantiates the Intel Security Manager
> class driver and provides the callback functions required to support
> secure updates on Intel n3000 PAC devices.
This is a good description. Because security manager is a new
interface, there should be a Documentation/fpga/ifpga-sec-mgr.rst
to collect this description.
How will these devices be discovered ? n3000 is a dfl device,
will there be a dfl feature id for it at some point ?
Can you describe if/how the security manager would live outside
of dfl ? I am wondering why this shouldn't be dfl-sec-mgr.
I did not see any version handling. How would this sw adapt
to a newer or older version of the bmc interface?
Tom
>
> Russ Weight (12):
> fpga: fpga security manager class driver
> fpga: create intel max10 bmc security engine
> fpga: expose max10 flash update counts in sysfs
> fpga: expose max10 canceled keys in sysfs
> fpga: enable secure updates
> fpga: add max10 secure update functions
> fpga: expose sec-mgr update status
> fpga: expose sec-mgr update errors
> fpga: expose sec-mgr update size
> fpga: enable sec-mgr update cancel
> fpga: expose hardware error info in sysfs
> fpga: add max10 get_hw_errinfo callback func
>
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
> MAINTAINERS | 8 +
> drivers/fpga/Kconfig | 20 +
> drivers/fpga/Makefile | 6 +
> drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
> drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
> include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
> include/linux/mfd/intel-m10-bmc.h | 116 +++
> 8 files changed, 1728 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
> create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>
On 9/4/20 4:52 PM, Russ Weight wrote:
> Extend the MAX10 BMC Security Engine driver to provide a
> handler to expose the flash update count for the FPGA user
> image.
>
> Signed-off-by: Russ Weight <[email protected]>
> Reviewed-by: Wu Hao <[email protected]>
> ---
> drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> index 1f86bfb694b4..b824790e43aa 100644
> --- a/drivers/fpga/intel-m10-bmc-secure.c
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -10,6 +10,7 @@
> #include <linux/mfd/intel-m10-bmc.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/slab.h>
> #include <linux/vmalloc.h>
>
> struct m10bmc_sec {
> @@ -99,7 +100,38 @@ SYSFS_GET_REH(bmc, BMC_REH_ADDR)
> SYSFS_GET_REH(sr, SR_REH_ADDR)
> SYSFS_GET_REH(pr, PR_REH_ADDR)
>
> +#define FLASH_COUNT_SIZE 4096
This seems too high at most it should be 64.
> +#define USER_FLASH_COUNT 0x17ffb000
Why shouldn't this be in intel-m10-bmc.h ?
> +
> +static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
what does 'qspi' mean ?
unless there are going to be several *flash_count's consider
removing this substring.
> +{
> + struct m10bmc_sec *sec = imgr->priv;
> + unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
> + unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
> + u8 *flash_buf;
> + int ret;
> +
> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
> + if (!flash_buf)
> + return -ENOMEM;
> +
> + ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
> + FLASH_COUNT_SIZE / stride);
> + if (ret) {
> + dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
> + goto exit_free;
> + }
> +
> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
Simplify ret = num_bits...
> +
> +exit_free:
> + kfree(flash_buf);
> +
> + return ret ? : cnt;
Then simplify
return ret;
Tom
> +}
> +
> static const struct ifpga_sec_mgr_ops m10bmc_iops = {
> + .user_flash_count = get_qspi_flash_count,
> .bmc_root_entry_hash = get_bmc_root_entry_hash,
> .sr_root_entry_hash = get_sr_root_entry_hash,
> .pr_root_entry_hash = get_pr_root_entry_hash,
On 9/4/20 4:52 PM, Russ Weight wrote:
> Extend the MAX10 BMC Security Engine driver to provide a
> handler 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 <[email protected]>
> Reviewed-by: Wu Hao <[email protected]>
> ---
> drivers/fpga/intel-m10-bmc-secure.c | 60 +++++++++++++++++++++++++++++
> 1 file changed, 60 insertions(+)
>
> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
> index b824790e43aa..46cd49a08be0 100644
> --- a/drivers/fpga/intel-m10-bmc-secure.c
> +++ b/drivers/fpga/intel-m10-bmc-secure.c
> @@ -130,14 +130,74 @@ static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
> return ret ? : cnt;
> }
>
> +#define CSK_BIT_LEN 128U
> +#define CSK_32ARRAY_SIZE(_nbits) DIV_ROUND_UP(_nbits, 32)
> +
> +#define SYSFS_GET_CSK_CANCEL_NBITS(_name) \
> +static int get_##_name##_csk_cancel_nbits(struct ifpga_sec_mgr *imgr) \
> +{ \
> + return (int)CSK_BIT_LEN; \
> +}
> +
> +SYSFS_GET_CSK_CANCEL_NBITS(bmc)
> +SYSFS_GET_CSK_CANCEL_NBITS(sr)
> +SYSFS_GET_CSK_CANCEL_NBITS(pr)
> +
> +static int get_csk_vector(struct ifpga_sec_mgr *imgr, u32 addr,
> + unsigned long *csk_map, unsigned int nbits)
> +{
> + unsigned int i, arr_size = CSK_32ARRAY_SIZE(nbits);
> + struct m10bmc_sec *sec = imgr->priv;
> + u32 *csk32;
> + int ret;
> +
> + csk32 = vmalloc(arr_size);
> + if (!csk32)
> + return -ENOMEM;
> +
> + ret = m10bmc_raw_bulk_read(sec->m10bmc, addr, csk32, arr_size);
Is this correct ? other similar bulk read used the
regmap stride.
> + if (ret) {
> + dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
> + goto vfree_exit;
> + }
> +
> + for (i = 0; i < arr_size; i++)
> + csk32[i] = le32_to_cpu(csk32[i]);
> +
> + bitmap_from_arr32(csk_map, csk32, nbits);
> + bitmap_complement(csk_map, csk_map, nbits);
> +
> +vfree_exit:
> + vfree(csk32);
> + return ret;
> +}
> +
> +#define SYSFS_GET_CSK_VEC(_name, _addr) \
> +static int get_##_name##_canceled_csks(struct ifpga_sec_mgr *imgr, \
> + unsigned long *csk_map, \
> + unsigned int nbits) \
> +{ return get_csk_vector(imgr, _addr, csk_map, nbits); }
> +
> +#define CSK_VEC_OFFSET 0x34
> +
> +SYSFS_GET_CSK_VEC(bmc, BMC_PROG_ADDR + CSK_VEC_OFFSET)
> +SYSFS_GET_CSK_VEC(sr, SR_PROG_ADDR + CSK_VEC_OFFSET)
> +SYSFS_GET_CSK_VEC(pr, PR_PROG_ADDR + CSK_VEC_OFFSET)
Issues similar with earlier patches.
> +
> static const struct ifpga_sec_mgr_ops m10bmc_iops = {
> .user_flash_count = get_qspi_flash_count,
> .bmc_root_entry_hash = get_bmc_root_entry_hash,
> .sr_root_entry_hash = get_sr_root_entry_hash,
> .pr_root_entry_hash = get_pr_root_entry_hash,
> + .bmc_canceled_csks = get_bmc_canceled_csks,
> + .sr_canceled_csks = get_sr_canceled_csks,
> + .pr_canceled_csks = get_pr_canceled_csks,
> .bmc_reh_size = get_bmc_reh_size,
> .sr_reh_size = get_sr_reh_size,
> .pr_reh_size = get_pr_reh_size,
> + .bmc_canceled_csk_nbits = get_bmc_csk_cancel_nbits,
> + .sr_canceled_csk_nbits = get_sr_csk_cancel_nbits,
> + .pr_canceled_csk_nbits = get_pr_csk_cancel_nbits
These are copies the same function, replace with a
common function.
Tom
> };
>
> static void ifpga_sec_mgr_uinit(struct m10bmc_sec *sec)
On 9/4/20 4:52 PM, Russ Weight wrote:
> Extend the FPGA Intel Security Manager class driver to
> include an update/filename sysfs node that can be used
> to initiate a security 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 <[email protected]>
> ---
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 13 ++
> drivers/fpga/ifpga-sec-mgr.c | 155 ++++++++++++++++++
> include/linux/fpga/ifpga-sec-mgr.h | 49 ++++++
> 3 files changed, 217 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> index 86f8992559bf..a476504b7ae9 100644
> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -73,3 +73,16 @@ Contact: Russ Weight <[email protected]>
> Description: Read only. Returns number of times the BMC image has been
> flashed.
> Format: "%d".
> +
> +What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/filename
> +Date: Sep 2020
> +KernelVersion: 5.10
> +Contact: Russ Weight <[email protected]>
> +Description: Write only. Write the filename of an Intel 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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> index 97bf80277ed2..73173badbe96 100644
> --- a/drivers/fpga/ifpga-sec-mgr.c
> +++ b/drivers/fpga/ifpga-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/ifpga-sec-mgr.h>
> #include <linux/idr.h>
> +#include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/vmalloc.h>
> @@ -14,6 +17,8 @@
> static DEFINE_IDA(ifpga_sec_mgr_ida);
> static struct class *ifpga_sec_mgr_class;
>
> +#define WRITE_BLOCK_SIZE 0x4000
> +
> static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
> sysfs_csk_hndlr_t get_csk,
> sysfs_csk_nbits_t get_csk_nbits,
> @@ -134,6 +139,91 @@ static struct attribute *sec_mgr_security_attrs[] = {
> NULL,
> };
>
> +static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
> + enum ifpga_sec_err err_code)
> +{
> + imgr->err_code = err_code;
> + imgr->iops->cancel(imgr);
> +}
> +
> +static void progress_complete(struct ifpga_sec_mgr *imgr)
> +{
> + mutex_lock(&imgr->lock);
> + imgr->progress = IFPGA_SEC_PROG_IDLE;
> + complete_all(&imgr->update_done);
> + mutex_unlock(&imgr->lock);
> +}
> +
> +static void ifpga_sec_mgr_update(struct work_struct *work)
> +{
> + u32 size, blk_size, offset = 0;
> + struct ifpga_sec_mgr *imgr;
> + const struct firmware *fw;
> + enum ifpga_sec_err ret;
> +
> + imgr = container_of(work, struct ifpga_sec_mgr, work);
Why not lock here ? It seems like filename and other
state could be changed out from under the work func.
> +
> + get_device(&imgr->dev);
> + if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
> + imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
> + goto idle_exit;
> + }
> +
> + imgr->data = fw->data;
> + imgr->remaining_size = fw->size;
> +
> + if (!try_module_get(imgr->dev.parent->driver->owner)) {
> + imgr->err_code = IFPGA_SEC_ERR_BUSY;
> + goto release_fw_exit;
> + }
> +
> + imgr->progress = IFPGA_SEC_PROG_PREPARING;
> + ret = imgr->iops->prepare(imgr);
> + if (ret) {
> + ifpga_sec_dev_error(imgr, ret);
> + goto modput_exit;
> + }
> +
> + imgr->progress = IFPGA_SEC_PROG_WRITING;
> + size = imgr->remaining_size;
> + while (size) {
> + blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
> + size -= blk_size;
> + ret = imgr->iops->write_blk(imgr, offset, blk_size);
Check for function pointer later, good.
Could writing a short block be handled like libc's write()
by passing back the bytes written ?
> + if (ret) {
> + ifpga_sec_dev_error(imgr, ret);
> + goto done;
> + }
> +
> + imgr->remaining_size = size;
> + offset += blk_size;
> + }
> +
> + imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
> + ret = imgr->iops->poll_complete(imgr);
> + if (ret) {
> + ifpga_sec_dev_error(imgr, ret);
> + goto done;
> + }
Add a paranoid crc check the flash is what was written ?
> +
> +done:
> + if (imgr->iops->cleanup)
> + imgr->iops->cleanup(imgr);
> +
> +modput_exit:
> + module_put(imgr->dev.parent->driver->owner);
> +
> +release_fw_exit:
> + imgr->data = NULL;
clear remaining_size ?
> + release_firmware(fw);
> +
> +idle_exit:
> + kfree(imgr->filename);
> + imgr->filename = NULL;
> + put_device(&imgr->dev);
> + progress_complete(imgr);
> +}
> +
> #define check_attr(attribute, _name) \
> ((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>
> @@ -161,6 +251,51 @@ static struct attribute_group sec_mgr_security_attr_group = {
> .is_visible = sec_mgr_visible,
> };
>
> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> + int ret = 0;
> +
> + if (count == 0 || count >= PATH_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&imgr->lock);
> + if (imgr->driver_unload || imgr->progress != IFPGA_SEC_PROG_IDLE) {
> + ret = -EBUSY;
> + goto unlock_exit;
> + }
> +
> + imgr->filename = kstrndup(buf, PATH_MAX - 1, GFP_KERNEL);
shouldn't this be 'count - 1' ?
> + if (!imgr->filename) {
> + ret = -ENOMEM;
> + goto unlock_exit;
> + }
> +
> + if (imgr->filename[strlen(imgr->filename) - 1] == '\n')
> + imgr->filename[strlen(imgr->filename) - 1] = '\0';
If you are catching the '\n' is a more general striping of
whitespace needed ?
Could a file exists check be done before kicking off the worker?
> +
> + imgr->err_code = IFPGA_SEC_ERR_NONE;
> + imgr->progress = IFPGA_SEC_PROG_READ_FILE;
> + reinit_completion(&imgr->update_done);
> + schedule_work(&imgr->work);
Skip the if-check at the end
ret = count.
> +
> +unlock_exit:
> + mutex_unlock(&imgr->lock);
> + return ret ? : count;
> +}
> +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)
> {
> @@ -182,6 +317,7 @@ static struct attribute_group sec_mgr_attr_group = {
> static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
> &sec_mgr_attr_group,
> &sec_mgr_security_attr_group,
> + &sec_mgr_update_attr_group,
> NULL,
> };
>
> @@ -233,6 +369,12 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
> struct ifpga_sec_mgr *imgr;
> int id, ret;
>
> + if (!iops || !iops->cancel || !iops->prepare ||
> + !iops->write_blk || !iops->poll_complete) {
Comments in ifpga-sec-mgr.h say 'Required: ' good.
> + dev_err(dev, "Attempt to register without ifpga_sec_mgr_ops\n");
without required ifpga_sec_mgr_ops
> + return NULL;
> + }
> +
> if (!check_reh_handler(dev, iops, bmc) ||
> !check_reh_handler(dev, iops, sr) ||
> !check_reh_handler(dev, iops, pr) ||
> @@ -254,6 +396,8 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
> imgr->name = name;
> imgr->priv = priv;
> imgr->iops = iops;
> + init_completion(&imgr->update_done);
> + INIT_WORK(&imgr->work, ifpga_sec_mgr_update);
> mutex_init(&imgr->lock);
>
> id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> @@ -299,6 +443,17 @@ void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
> {
> dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>
> + mutex_lock(&imgr->lock);
> + imgr->driver_unload = true;
> + if (imgr->progress == IFPGA_SEC_PROG_IDLE) {
> + mutex_unlock(&imgr->lock);
> + goto unregister;
> + }
> +
> + mutex_unlock(&imgr->lock);
> + wait_for_completion(&imgr->update_done);
> +
> +unregister:
> device_unregister(&imgr->dev);
> }
> EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
> index e391b0c8f448..4da2864e251c 100644
> --- a/include/linux/fpga/ifpga-sec-mgr.h
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -7,6 +7,7 @@
> #ifndef _LINUX_IFPGA_SEC_MGR_H
> #define _LINUX_IFPGA_SEC_MGR_H
>
> +#include <linux/completion.h>
> #include <linux/device.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> @@ -86,6 +87,19 @@ typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
> typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> unsigned long *csk_map, unsigned int nbits);
>
> +enum ifpga_sec_err {
> + IFPGA_SEC_ERR_NONE = 0x0,
> + IFPGA_SEC_ERR_HW_ERROR = 0x1,
> + IFPGA_SEC_ERR_TIMEOUT = 0x2,
> + IFPGA_SEC_ERR_CANCELED = 0x3,
> + IFPGA_SEC_ERR_BUSY = 0x4,
> + IFPGA_SEC_ERR_INVALID_SIZE = 0x5,
> + IFPGA_SEC_ERR_RW_ERROR = 0x6,
> + IFPGA_SEC_ERR_WEAROUT = 0x7,
> + IFPGA_SEC_ERR_FILE_READ = 0x8,
> + IFPGA_SEC_ERR_MAX = 0x9
The initializers are redundant.
> +};
> +
> /**
> * struct ifpga_sec_mgr_ops - device specific operations
> * @user_flash_count: Optional: Return sysfs string output for FPGA
> @@ -110,6 +124,17 @@ typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> * @bmc_reh_size: Optional: Return byte size for BMC root entry hash
> * @sr_reh_size: Optional: Return byte size for SR root entry hash
> * @pr_reh_size: Optional: Return byte size for PR root entry hash
> + * @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 imgr->driver_unload
> + * and abort with IFPGA_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 ifpga_sec_mgr_ops {
> sysfs_cnt_hndlr_t user_flash_count;
> @@ -127,6 +152,22 @@ struct ifpga_sec_mgr_ops {
> sysfs_csk_nbits_t bmc_canceled_csk_nbits;
> sysfs_csk_nbits_t sr_canceled_csk_nbits;
> sysfs_csk_nbits_t pr_canceled_csk_nbits;
> + enum ifpga_sec_err (*prepare)(struct ifpga_sec_mgr *imgr);
> + enum ifpga_sec_err (*write_blk)(struct ifpga_sec_mgr *imgr,
> + u32 offset, u32 size);
> + enum ifpga_sec_err (*poll_complete)(struct ifpga_sec_mgr *imgr);
> + void (*cleanup)(struct ifpga_sec_mgr *imgr);
> + enum ifpga_sec_err (*cancel)(struct ifpga_sec_mgr *imgr);
> +};
> +
> +/* Update progress codes */
> +enum ifpga_sec_prog {
> + IFPGA_SEC_PROG_IDLE = 0x0,
> + IFPGA_SEC_PROG_READ_FILE = 0x1,
> + IFPGA_SEC_PROG_PREPARING = 0x2,
> + IFPGA_SEC_PROG_WRITING = 0x3,
> + IFPGA_SEC_PROG_PROGRAMMING = 0x4,
> + IFPGA_SEC_PROG_MAX = 0x5
ditto
Tom
> };
>
> struct ifpga_sec_mgr {
> @@ -134,6 +175,14 @@ struct ifpga_sec_mgr {
> struct device dev;
> const struct ifpga_sec_mgr_ops *iops;
> 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 ifpga_sec_prog progress;
> + enum ifpga_sec_err err_code; /* security manager error code */
> + bool driver_unload;
> void *priv;
> };
>
On 9/5/20 1:39 PM, Tom Rix wrote:
> On 9/4/20 4:52 PM, Russ Weight wrote:
>> Extend the MAX10 BMC Security Engine driver to provide a
>> handler to expose the flash update count for the FPGA user
>> image.
>>
>> Signed-off-by: Russ Weight <[email protected]>
>> Reviewed-by: Wu Hao <[email protected]>
>> ---
>> drivers/fpga/intel-m10-bmc-secure.c | 32 +++++++++++++++++++++++++++++
>> 1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/fpga/intel-m10-bmc-secure.c b/drivers/fpga/intel-m10-bmc-secure.c
>> index 1f86bfb694b4..b824790e43aa 100644
>> --- a/drivers/fpga/intel-m10-bmc-secure.c
>> +++ b/drivers/fpga/intel-m10-bmc-secure.c
>> @@ -10,6 +10,7 @@
>> #include <linux/mfd/intel-m10-bmc.h>
>> #include <linux/module.h>
>> #include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> #include <linux/vmalloc.h>
>>
>> struct m10bmc_sec {
>> @@ -99,7 +100,38 @@ SYSFS_GET_REH(bmc, BMC_REH_ADDR)
>> SYSFS_GET_REH(sr, SR_REH_ADDR)
>> SYSFS_GET_REH(pr, PR_REH_ADDR)
>>
>> +#define FLASH_COUNT_SIZE 4096
> This seems too high at most it should be 64.
The flash count size represents the size of the flash memory that stores the
count. The count is represented in flash as an inverted bit vector.
I suppose a comment would be helpful here...
>> +#define USER_FLASH_COUNT 0x17ffb000
> Why shouldn't this be in intel-m10-bmc.h ?
Sure - I'll move it there with the other addresses.
>> +
>> +static int get_qspi_flash_count(struct ifpga_sec_mgr *imgr)
> what does 'qspi' mean ?
It means Quad-SPI, the controller type that connects to the FLASH. This term
does seem out of place here. There is also a BMC flash count. I'll change this to
"user".
>
> unless there are going to be several *flash_count's consider
>
> removing this substring.
>
>> +{
>> + struct m10bmc_sec *sec = imgr->priv;
>> + unsigned int stride = regmap_get_reg_stride(sec->m10bmc->regmap);
>> + unsigned int cnt, num_bits = FLASH_COUNT_SIZE * 8;
>> + u8 *flash_buf;
>> + int ret;
>> +
>> + flash_buf = kmalloc(FLASH_COUNT_SIZE, GFP_KERNEL);
>> + if (!flash_buf)
>> + return -ENOMEM;
>> +
>> + ret = m10bmc_raw_bulk_read(sec->m10bmc, USER_FLASH_COUNT, flash_buf,
>> + FLASH_COUNT_SIZE / stride);
>> + if (ret) {
>> + dev_err(sec->dev, "%s failed to read %d\n", __func__, ret);
>> + goto exit_free;
>> + }
>> +
>> + cnt = num_bits - bitmap_weight((unsigned long *)flash_buf, num_bits);
> Simplify ret = num_bits...
yes - will do.
Thanks!
- Russ
>> +
>> +exit_free:
>> + kfree(flash_buf);
>> +
>> + return ret ? : cnt;
> Then simplify
>
> return ret;
>
> Tom
>
>> +}
>> +
>> static const struct ifpga_sec_mgr_ops m10bmc_iops = {
>> + .user_flash_count = get_qspi_flash_count,
>> .bmc_root_entry_hash = get_bmc_root_entry_hash,
>> .sr_root_entry_hash = get_sr_root_entry_hash,
>> .pr_root_entry_hash = get_pr_root_entry_hash,
On 9/18/20 6:10 PM, Russ Weight wrote:
>
> On 9/5/20 3:04 PM, Tom Rix wrote:
>> On 9/4/20 4:52 PM, Russ Weight wrote:
>>> Extend the FPGA Intel Security Manager class driver to
>>> include an update/filename sysfs node that can be used
>>> to initiate a security 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 <[email protected]>
>>> ---
>>> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 13 ++
>>> drivers/fpga/ifpga-sec-mgr.c | 155 ++++++++++++++++++
>>> include/linux/fpga/ifpga-sec-mgr.h | 49 ++++++
>>> 3 files changed, 217 insertions(+)
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> index 86f8992559bf..a476504b7ae9 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> @@ -73,3 +73,16 @@ Contact: Russ Weight <[email protected]>
>>> Description: Read only. Returns number of times the BMC image has been
>>> flashed.
>>> Format: "%d".
>>> +
>>> +What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/filename
>>> +Date: Sep 2020
>>> +KernelVersion: 5.10
>>> +Contact: Russ Weight <[email protected]>
>>> +Description: Write only. Write the filename of an Intel 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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>>> index 97bf80277ed2..73173badbe96 100644
>>> --- a/drivers/fpga/ifpga-sec-mgr.c
>>> +++ b/drivers/fpga/ifpga-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/ifpga-sec-mgr.h>
>>> #include <linux/idr.h>
>>> +#include <linux/kernel.h>
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/vmalloc.h>
>>> @@ -14,6 +17,8 @@
>>> static DEFINE_IDA(ifpga_sec_mgr_ida);
>>> static struct class *ifpga_sec_mgr_class;
>>>
>>> +#define WRITE_BLOCK_SIZE 0x4000
>>> +
>>> static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
>>> sysfs_csk_hndlr_t get_csk,
>>> sysfs_csk_nbits_t get_csk_nbits,
>>> @@ -134,6 +139,91 @@ static struct attribute *sec_mgr_security_attrs[] = {
>>> NULL,
>>> };
>>>
>>> +static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
>>> + enum ifpga_sec_err err_code)
>>> +{
>>> + imgr->err_code = err_code;
>>> + imgr->iops->cancel(imgr);
>>> +}
>>> +
>>> +static void progress_complete(struct ifpga_sec_mgr *imgr)
>>> +{
>>> + mutex_lock(&imgr->lock);
>>> + imgr->progress = IFPGA_SEC_PROG_IDLE;
>>> + complete_all(&imgr->update_done);
>>> + mutex_unlock(&imgr->lock);
>>> +}
>>> +
>>> +static void ifpga_sec_mgr_update(struct work_struct *work)
>>> +{
>>> + u32 size, blk_size, offset = 0;
>>> + struct ifpga_sec_mgr *imgr;
>>> + const struct firmware *fw;
>>> + enum ifpga_sec_err ret;
>>> +
>>> + imgr = container_of(work, struct ifpga_sec_mgr, work);
>> Why not lock here ? It seems like filename and other
>>
>> state could be changed out from under the work func.
> Filename_store() uses imgr->progress, imgr->driver_unload, and imgr->lock
> to ensure that only one worker thread is running at a time. In some of the later
> patches there is some lock protection around error, progress, and cancel accesses.
> At this point in the patchset, the synchronization in filename_store() should be
> sufficient.
ok
>>> +
>>> + get_device(&imgr->dev);
>>> + if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
>>> + imgr->err_code = IFPGA_SEC_ERR_FILE_READ;
>>> + goto idle_exit;
>>> + }
>>> +
>>> + imgr->data = fw->data;
>>> + imgr->remaining_size = fw->size;
>>> +
>>> + if (!try_module_get(imgr->dev.parent->driver->owner)) {
>>> + imgr->err_code = IFPGA_SEC_ERR_BUSY;
>>> + goto release_fw_exit;
>>> + }
>>> +
>>> + imgr->progress = IFPGA_SEC_PROG_PREPARING;
>>> + ret = imgr->iops->prepare(imgr);
>>> + if (ret) {
>>> + ifpga_sec_dev_error(imgr, ret);
>>> + goto modput_exit;
>>> + }
>>> +
>>> + imgr->progress = IFPGA_SEC_PROG_WRITING;
>>> + size = imgr->remaining_size;
>>> + while (size) {
>>> + blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>>> + size -= blk_size;
>>> + ret = imgr->iops->write_blk(imgr, offset, blk_size);
>> Check for function pointer later, good.
>>
>> Could writing a short block be handled like libc's write()
>>
>> by passing back the bytes written ?
> It could be done, multiplexing the size into the return code. Do you think it
> would add value?
A toss up. I like consistent interfaces.
>>> + if (ret) {
>>> + ifpga_sec_dev_error(imgr, ret);
>>> + goto done;
>>> + }
>>> +
>>> + imgr->remaining_size = size;
>>> + offset += blk_size;
>>> + }
>>> +
>>> + imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
>>> + ret = imgr->iops->poll_complete(imgr);
>>> + if (ret) {
>>> + ifpga_sec_dev_error(imgr, ret);
>>> + goto done;
>>> + }
>> Add a paranoid crc check the flash is what was written ?
> In the secure update implementation, the host driver transfers the data
> to a staging area in flash, and then it is up to the BMC firmware to
> actually do the programming and validation, so it is unnecessary for
> the host driver to check the flash. Also, on the n3000, the updates
> take more than 30 minutes, so adding additional checks would add
> to an already long wait.
ok
>>> +
>>> +done:
>>> + if (imgr->iops->cleanup)
>>> + imgr->iops->cleanup(imgr);
>>> +
>>> +modput_exit:
>>> + module_put(imgr->dev.parent->driver->owner);
>>> +
>>> +release_fw_exit:
>>> + imgr->data = NULL;
>> clear remaining_size ?
> Remaining size is left unchanged intentionally. On success it will be zero.
> In the case of an error it can be helpful/interesting to see how much data
> has transferred (using the remaining_size sysfs file). Remaining_size will
> be reinitialized at the start of the next instance of a secure update.
Is there a way to tell if the transfer is stuck/in the middle or a failure ?
Maybe add a comment that this is intentional so someone does not remove this later.
>>> + release_firmware(fw);
>>> +
>>> +idle_exit:
>>> + kfree(imgr->filename);
>>> + imgr->filename = NULL;
>>> + put_device(&imgr->dev);
>>> + progress_complete(imgr);
>>> +}
>>> +
>>> #define check_attr(attribute, _name) \
>>> ((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>>>
>>> @@ -161,6 +251,51 @@ static struct attribute_group sec_mgr_security_attr_group = {
>>> .is_visible = sec_mgr_visible,
>>> };
>>>
>>> +static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> +{
>>> + struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>> + int ret = 0;
>>> +
>>> + if (count == 0 || count >= PATH_MAX)
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&imgr->lock);
>>> + if (imgr->driver_unload || imgr->progress != IFPGA_SEC_PROG_IDLE) {
>>> + ret = -EBUSY;
>>> + goto unlock_exit;
>>> + }
>>> +
>>> + imgr->filename = kstrndup(buf, PATH_MAX - 1, GFP_KERNEL);
>> shouldn't this be 'count - 1' ?
> Yes - you are right. I'll make that change. Thanks!
>>> + if (!imgr->filename) {
>>> + ret = -ENOMEM;
>>> + goto unlock_exit;
>>> + }
>>> +
>>> + if (imgr->filename[strlen(imgr->filename) - 1] == '\n')
>>> + imgr->filename[strlen(imgr->filename) - 1] = '\0';
>> If you are catching the '\n' is a more general striping of
>>
>> whitespace needed ?
> My intent was to take care of the common case, but now that you mention it,
> I don't think the '\n' case is really required. Perhaps this check could be
> removed?
>
> Or do you think I should add more checks? Currently, if a quoted string with
> whitespace is provided for the filename, then the update will fail with
> "file-error". But it seems like an unlikely case.
Perhaps i am being paranoid, i think user inputs need extra checking.
see more comment below.
>> Could a file exists check be done before kicking off the worker?
> It could be done. I didn't include them because I thought it would be redundant
> with functionality already provided by request_firmware(). In the current
> implementation, the error sysfs file would indicate "file-error". It may be
> possible to give a more descriptive error using errno if it was detected in
> this context.
yes this would be redundant, but you would know early if filename was invalid.
toss up, change if you want.
Tom
>>> +
>>> + imgr->err_code = IFPGA_SEC_ERR_NONE;
>>> + imgr->progress = IFPGA_SEC_PROG_READ_FILE;
>>> + reinit_completion(&imgr->update_done);
>>> + schedule_work(&imgr->work);
>> Skip the if-check at the end
>>
>> ret = count.
> OK. It looks like I could just initialize ret to count.
>>> +
>>> +unlock_exit:
>>> + mutex_unlock(&imgr->lock);
>>> + return ret ? : count;
>>> +}
>>> +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)
>>> {
>>> @@ -182,6 +317,7 @@ static struct attribute_group sec_mgr_attr_group = {
>>> static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
>>> &sec_mgr_attr_group,
>>> &sec_mgr_security_attr_group,
>>> + &sec_mgr_update_attr_group,
>>> NULL,
>>> };
>>>
>>> @@ -233,6 +369,12 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
>>> struct ifpga_sec_mgr *imgr;
>>> int id, ret;
>>>
>>> + if (!iops || !iops->cancel || !iops->prepare ||
>>> + !iops->write_blk || !iops->poll_complete) {
>> Comments in ifpga-sec-mgr.h say 'Required: ' good.
>>> + dev_err(dev, "Attempt to register without ifpga_sec_mgr_ops\n");
>> without required ifpga_sec_mgr_ops
> Yes - I'll make the change.
>>> + return NULL;
>>> + }
>>> +
>>> if (!check_reh_handler(dev, iops, bmc) ||
>>> !check_reh_handler(dev, iops, sr) ||
>>> !check_reh_handler(dev, iops, pr) ||
>>> @@ -254,6 +396,8 @@ ifpga_sec_mgr_register(struct device *dev, const char *name,
>>> imgr->name = name;
>>> imgr->priv = priv;
>>> imgr->iops = iops;
>>> + init_completion(&imgr->update_done);
>>> + INIT_WORK(&imgr->work, ifpga_sec_mgr_update);
>>> mutex_init(&imgr->lock);
>>>
>>> id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
>>> @@ -299,6 +443,17 @@ void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
>>> {
>>> dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>>>
>>> + mutex_lock(&imgr->lock);
>>> + imgr->driver_unload = true;
>>> + if (imgr->progress == IFPGA_SEC_PROG_IDLE) {
>>> + mutex_unlock(&imgr->lock);
>>> + goto unregister;
>>> + }
>>> +
>>> + mutex_unlock(&imgr->lock);
>>> + wait_for_completion(&imgr->update_done);
>>> +
>>> +unregister:
>>> device_unregister(&imgr->dev);
>>> }
>>> EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
>>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
>>> index e391b0c8f448..4da2864e251c 100644
>>> --- a/include/linux/fpga/ifpga-sec-mgr.h
>>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>>> @@ -7,6 +7,7 @@
>>> #ifndef _LINUX_IFPGA_SEC_MGR_H
>>> #define _LINUX_IFPGA_SEC_MGR_H
>>>
>>> +#include <linux/completion.h>
>>> #include <linux/device.h>
>>> #include <linux/mutex.h>
>>> #include <linux/types.h>
>>> @@ -86,6 +87,19 @@ typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
>>> typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>>> unsigned long *csk_map, unsigned int nbits);
>>>
>>> +enum ifpga_sec_err {
>>> + IFPGA_SEC_ERR_NONE = 0x0,
>>> + IFPGA_SEC_ERR_HW_ERROR = 0x1,
>>> + IFPGA_SEC_ERR_TIMEOUT = 0x2,
>>> + IFPGA_SEC_ERR_CANCELED = 0x3,
>>> + IFPGA_SEC_ERR_BUSY = 0x4,
>>> + IFPGA_SEC_ERR_INVALID_SIZE = 0x5,
>>> + IFPGA_SEC_ERR_RW_ERROR = 0x6,
>>> + IFPGA_SEC_ERR_WEAROUT = 0x7,
>>> + IFPGA_SEC_ERR_FILE_READ = 0x8,
>>> + IFPGA_SEC_ERR_MAX = 0x9
>> The initializers are redundant.
> OK - I'll remove them.
>>> +};
>>> +
>>> /**
>>> * struct ifpga_sec_mgr_ops - device specific operations
>>> * @user_flash_count: Optional: Return sysfs string output for FPGA
>>> @@ -110,6 +124,17 @@ typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>>> * @bmc_reh_size: Optional: Return byte size for BMC root entry hash
>>> * @sr_reh_size: Optional: Return byte size for SR root entry hash
>>> * @pr_reh_size: Optional: Return byte size for PR root entry hash
>>> + * @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 imgr->driver_unload
>>> + * and abort with IFPGA_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 ifpga_sec_mgr_ops {
>>> sysfs_cnt_hndlr_t user_flash_count;
>>> @@ -127,6 +152,22 @@ struct ifpga_sec_mgr_ops {
>>> sysfs_csk_nbits_t bmc_canceled_csk_nbits;
>>> sysfs_csk_nbits_t sr_canceled_csk_nbits;
>>> sysfs_csk_nbits_t pr_canceled_csk_nbits;
>>> + enum ifpga_sec_err (*prepare)(struct ifpga_sec_mgr *imgr);
>>> + enum ifpga_sec_err (*write_blk)(struct ifpga_sec_mgr *imgr,
>>> + u32 offset, u32 size);
>>> + enum ifpga_sec_err (*poll_complete)(struct ifpga_sec_mgr *imgr);
>>> + void (*cleanup)(struct ifpga_sec_mgr *imgr);
>>> + enum ifpga_sec_err (*cancel)(struct ifpga_sec_mgr *imgr);
>>> +};
>>> +
>>> +/* Update progress codes */
>>> +enum ifpga_sec_prog {
>>> + IFPGA_SEC_PROG_IDLE = 0x0,
>>> + IFPGA_SEC_PROG_READ_FILE = 0x1,
>>> + IFPGA_SEC_PROG_PREPARING = 0x2,
>>> + IFPGA_SEC_PROG_WRITING = 0x3,
>>> + IFPGA_SEC_PROG_PROGRAMMING = 0x4,
>>> + IFPGA_SEC_PROG_MAX = 0x5
>> ditto
> Yes. Thanks for the comments!
>
> - Russ
>> Tom
>>
>>> };
>>>
>>> struct ifpga_sec_mgr {
>>> @@ -134,6 +175,14 @@ struct ifpga_sec_mgr {
>>> struct device dev;
>>> const struct ifpga_sec_mgr_ops *iops;
>>> 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 ifpga_sec_prog progress;
>>> + enum ifpga_sec_err err_code; /* security manager error code */
>>> + bool driver_unload;
>>> void *priv;
>>> };
>>>
On 9/5/20 10:16 AM, Tom Rix wrote:
> resending.
> sorry for blowing past 80 chars.
>
> On 9/4/20 4:52 PM, Russ Weight wrote:
>> These patches depend on the patchset: "add regmap-spi-avmm & Intel
>> Max10 BMC chip support" which is currently under review.
> https://marc.info/?l=linux-kernel&m=159782274232229&w=2
>
> regmap-spi-avmm is in linux-next.
>
> max10 is not. however applying it does not resolve resolve
> git am conflicts with yesterday's linux-next.
> I normally build the larger patchsets as a test.
I have rebased to a more recent version of linux-next. I'll make sure to
send dependency information with the next patch set. I'll also split it
into two patch sets, since the class driver has no dependencies on patches
that are in flight.
>
>> --------------------------------------------------
>>
>> This patchset introduces the Intel Security Manager class driver
>> for managing secure updates on Intel FPGA Cards. It also provides
>> the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
>> Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
>> is implemented using the Intel Security Manager class driver.
>>
>> The Intel Security Manager class driver provides a common API for
>> user-space tools to manage updates for Secure FPGA devices. Device
>> drivers that instantiate the Intel Security Manager class driver will
>> interact with the 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.
>>
>> The API consists of sysfs nodes and supports the following functions:
>>
>> (1) Instantiate and monitor a secure update
>> (2) Display security information including: Root Entry Hashes (REH),
>> Cancelled Code Signing Keys (CSK), and flash update counts for
>> both BMC and FPGA images.
>>
>> Secure updates make use of the request_firmware framework, which
>> requires that image files are accessible under /lib/firmware. A request
>> for a secure update returns immediately, while the update itself
>> proceeds in the context of a kernel worker thread. Sysfs files provide
>> a means for monitoring the progress of a secure update and for
>> retrieving error information in the event of a failure.
>>
>> The n3000bmc-secure driver instantiates the Intel Security Manager
>> class driver and provides the callback functions required to support
>> secure updates on Intel n3000 PAC devices.
> This is a good description. Because security manager is a new
> interface, there should be a Documentation/fpga/ifpga-sec-mgr.rst
> to collect this description.
Sure - I'll create the documentation file.
>
> How will these devices be discovered ? n3000 is a dfl device,
> will there be a dfl feature id for it at some point ?
The n3000 implementation of the MAX10 BMC, and eventually the d5005
implementation of the MAX10 BMC, both instantiate the MAX10 BMC Secure
Engine as a sub device. There is a dfl feature ID for the SPI interface
to the BMC, but no dfl feature ID for the secure engine itself.
Given that the security manager is dependent on other hardware and
firmware to manage the root entry hashes and to verify and program
the images, I think it is safe to say that implementations of the
security manager will always be implemented as sub devices. I can't
think of a case for giving it it's own dfl feature ID.
The class driver, of course, is an abstraction above the MAX10 BMC
implementation.
> Can you describe if/how the security manager would live outside
> of dfl ? I am wondering why this shouldn't be dfl-sec-mgr.
The Intel FPGA Security Manager could be used by Intel FPGA devices that
are implemented without a Device Feature List.
> I did not see any version handling. How would this sw adapt
> to a newer or older version of the bmc interface?
There are some slight differences in the implementation of the MAX10 BMC
Secure Engine between the n3000 and d5005 implementations. The d5005
support is not in the current patch set, but when it is included there
will be a different device name to indicate which code variations should
be active. Also, different platform data can be passed into the secure
engine by the parent MAX10 device if needed for new versions of the BMC.
Thanks,
- Russ
>
> Tom
>
>> Russ Weight (12):
>> fpga: fpga security manager class driver
>> fpga: create intel max10 bmc security engine
>> fpga: expose max10 flash update counts in sysfs
>> fpga: expose max10 canceled keys in sysfs
>> fpga: enable secure updates
>> fpga: add max10 secure update functions
>> fpga: expose sec-mgr update status
>> fpga: expose sec-mgr update errors
>> fpga: expose sec-mgr update size
>> fpga: enable sec-mgr update cancel
>> fpga: expose hardware error info in sysfs
>> fpga: add max10 get_hw_errinfo callback func
>>
>> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
>> MAINTAINERS | 8 +
>> drivers/fpga/Kconfig | 20 +
>> drivers/fpga/Makefile | 6 +
>> drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
>> drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
>> include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
>> include/linux/mfd/intel-m10-bmc.h | 116 +++
>> 8 files changed, 1728 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>> create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>
On 9/5/20 7:13 AM, Wu, Hao wrote:
>> Subject: [PATCH v1 00/12] Intel FPGA Security Manager Class Driver
>>
>>
>> These patches depend on the patchset: "add regmap-spi-avmm & Intel
>> Max10 BMC chip support" which is currently under review.
>>
>> --------------------------------------------------
>>
>> This patchset introduces the Intel Security Manager class driver
>> for managing secure updates on Intel FPGA Cards. It also provides
>> the n3000bmc-secure mfd sub-driver for the MAX10 BMC for the n3000
>> Programmable Acceleration Cards (PAC). The n3000bmc-secure driver
>> is implemented using the Intel Security Manager class driver.
> So this patchset contains two parts
> (1) adding a new class driver for Intel FPGA secure update.
> (2) a new driver which uses (1) to implement secure update for n3000 PAC.
Yes - that is correct
>
> And only part (2) depends on "Intel MAX10 BMC chip support" patchset.
> (Maybe you can provide a link to that thread).
>
> Is my understanding correct? If yes, is it possible to reorder these patches?
> At least there is no dependency on the class driver patches, right?
Yes - I'm splitting the patch set, and I'll provide links for the dependencies
on the MAX10 BMC Secure Engine patch set.
>
>> The Intel Security Manager class driver provides a common API for
>> user-space tools to manage updates for Secure FPGA devices. Device
>> drivers that instantiate the Intel Security Manager class driver will
>> interact with the 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.
>>
>> The API consists of sysfs nodes and supports the following functions:
>>
>> (1) Instantiate and monitor a secure update
>> (2) Display security information including: Root Entry Hashes (REH),
>> Cancelled Code Signing Keys (CSK), and flash update counts for
>> both BMC and FPGA images.
>>
>> 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.
> Maybe you can explain a little more on why we need to have this done
> via a class driver not just some internal code in max10 driver? This class
> driver will be reused in different cases? And why adding a new class
> driver not just reuse or extend fpga manager (existing fpga mgr is used
> to update fpga too).
Yes - I'll so that in the next patch set.
>
>> The n3000bmc-secure driver instantiates the Intel Security Manager
>> class driver and provides the callback functions required to support
>> secure updates on Intel n3000 PAC devices.
>>
>> Russ Weight (12):
>> fpga: fpga security manager class driver
> Intel FPGA Security Manager?
Yes - I'll make that change
>
>> fpga: create intel max10 bmc security engine
>> fpga: expose max10 flash update counts in sysfs
>> fpga: expose max10 canceled keys in sysfs
>> fpga: enable secure updates
>> fpga: add max10 secure update functions
>> fpga: expose sec-mgr update status
>> fpga: expose sec-mgr update errors
>> fpga: expose sec-mgr update size
>> fpga: enable sec-mgr update cancel
>> fpga: expose hardware error info in sysfs
> For these patches, is it possible to have a better title for these patches.
> Then it will be easier to know which component this patch is going to modify.
> e.g. fpga: ifpga-sec-mgr: xxxxxx
Yes. Thanks for the comments.
- Russ
>
> Thanks
> Hao
>
>> fpga: add max10 get_hw_errinfo callback func
>>
>> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 151 ++++
>> MAINTAINERS | 8 +
>> drivers/fpga/Kconfig | 20 +
>> drivers/fpga/Makefile | 6 +
>> drivers/fpga/ifpga-sec-mgr.c | 669 ++++++++++++++++++
>> drivers/fpga/intel-m10-bmc-secure.c | 557 +++++++++++++++
>> include/linux/fpga/ifpga-sec-mgr.h | 201 ++++++
>> include/linux/mfd/intel-m10-bmc.h | 116 +++
>> 8 files changed, 1728 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>> create mode 100644 drivers/fpga/intel-m10-bmc-secure.c
>> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>
>> --
>> 2.17.1