The Intel FPGA Security Manager class driver provides a common
API for user-space tools to manage updates for secure Intel FPGA
devices. Device drivers that instantiate the Intel 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 Intel FPGA
Security Manager is that the FPGA Manager does a live update (Partial
Reconfiguration) to a device whereas the Intel FPGA Security Manager
updates the FLASH images for the Static Region and the BMC so that
they will be loaded the next time the FPGA card boots. Security is
enforced by hardware and firmware. The security manager interacts
with the firmware to initiate an update, pass in the necessary data,
and collect status on the update.
The n3000bmc-secure driver is the first driver to use the Intel FPG
Security Manager. This driver was previously submittied in the same
patch set, but has been split out in to a separate patch set for V2.
Follow-on Intel devices will also make use of this common API for
secure updates.
In addition to managing secure updates of the FPGA and BMC images,
the Intel FPGA Security Manager update process may also 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.
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.
Changelog v1 -> v2:
- Separated out the MAX10 BMC Security Engine to be submitted in
a separate patch-set.
- Bumped documentation dates and versions
- Split ifpga_sec_mgr_register() into create() and register() functions
- Added devm_ifpga_sec_mgr_create()
- Added Documentation/fpga/ifpga-sec-mgr.rst
- Changed progress state "read_file" to "reading"
- Added sec_error() function (similar to sec_progress())
- Removed references to bmc_flash_count & smbus_flash_count (not supported)
- Removed typedefs for imgr ops
- Removed explicit value assignments in enums
- Other minor code cleanup per review comments
Russ Weight (7):
fpga: sec-mgr: intel fpga security manager class driver
fpga: sec-mgr: enable secure updates
fpga: sec-mgr: expose sec-mgr update status
fpga: sec-mgr: expose sec-mgr update errors
fpga: sec-mgr: expose sec-mgr update size
fpga: sec-mgr: enable cancel of secure update
fpga: sec-mgr: expose hardware error info
.../ABI/testing/sysfs-class-ifpga-sec-mgr | 143 ++++
Documentation/fpga/ifpga-sec-mgr.rst | 50 ++
Documentation/fpga/index.rst | 1 +
MAINTAINERS | 9 +
drivers/fpga/Kconfig | 9 +
drivers/fpga/Makefile | 3 +
drivers/fpga/ifpga-sec-mgr.c | 781 ++++++++++++++++++
include/linux/fpga/ifpga-sec-mgr.h | 137 +++
8 files changed, 1133 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
create mode 100644 Documentation/fpga/ifpga-sec-mgr.rst
create mode 100644 drivers/fpga/ifpga-sec-mgr.c
create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
--
2.17.1
Extend the Intel 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 <[email protected]>
Reviewed-by: Tom Rix <[email protected]>
---
v2:
- Bumped documentation date and version
---
Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr | 11 +++++++++++
drivers/fpga/ifpga-sec-mgr.c | 10 ++++++++++
2 files changed, 21 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
index 1f9f2c215e0c..ec51135fcb6a 100644
--- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
@@ -90,6 +90,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/ifpga_sec_mgr/ifpga_secX/update/remaining_size
+Date: Oct 2020
+KernelVersion: 5.11
+Contact: Russ Weight <[email protected]>
+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/ifpga_sec_mgr/ifpga_secX/update/error
Date: Oct 2020
KernelVersion: 5.11
diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
index 456ea0b71e3d..d8ac863c1159 100644
--- a/drivers/fpga/ifpga-sec-mgr.c
+++ b/drivers/fpga/ifpga-sec-mgr.c
@@ -350,6 +350,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 ifpga_sec_mgr *imgr = to_sec_mgr(dev);
+
+ return sprintf(buf, "%u\n", imgr->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)
{
@@ -386,6 +395,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.17.1
Extend the Intel 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 <[email protected]>
---
v2:
- Bumped documentation date and version
- Minor code cleanup per review comments
---
.../ABI/testing/sysfs-class-ifpga-sec-mgr | 10 ++++
drivers/fpga/ifpga-sec-mgr.c | 59 +++++++++++++++++--
include/linux/fpga/ifpga-sec-mgr.h | 1 +
3 files changed, 66 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
index ec51135fcb6a..caafe7eb7670 100644
--- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
@@ -79,6 +79,16 @@ Description: Write only. Write the filename of an Intel image
and Root Entry Hashes, and to cancel Code Signing
Keys (CSK).
+What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/cancel
+Date: Oct 2020
+KernelVersion: 5.11
+Contact: Russ Weight <[email protected]>
+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/ifpga_sec_mgr/ifpga_secX/update/status
Date: Oct 2020
KernelVersion: 5.11
diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
index d8ac863c1159..6267ac3a0780 100644
--- a/drivers/fpga/ifpga-sec-mgr.c
+++ b/drivers/fpga/ifpga-sec-mgr.c
@@ -159,6 +159,23 @@ static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
imgr->iops->cancel(imgr);
}
+static int progress_transition(struct ifpga_sec_mgr *imgr,
+ enum ifpga_sec_prog new_progress)
+{
+ int ret = 0;
+
+ mutex_lock(&imgr->lock);
+ if (imgr->request_cancel) {
+ set_error(imgr, IFPGA_SEC_ERR_CANCELED);
+ imgr->iops->cancel(imgr);
+ ret = -ECANCELED;
+ } else {
+ update_progress(imgr, new_progress);
+ }
+ mutex_unlock(&imgr->lock);
+ return ret;
+}
+
static void progress_complete(struct ifpga_sec_mgr *imgr)
{
mutex_lock(&imgr->lock);
@@ -190,16 +207,20 @@ static void ifpga_sec_mgr_update(struct work_struct *work)
goto release_fw_exit;
}
- update_progress(imgr, IFPGA_SEC_PROG_PREPARING);
+ if (progress_transition(imgr, IFPGA_SEC_PROG_PREPARING))
+ goto modput_exit;
+
ret = imgr->iops->prepare(imgr);
if (ret) {
ifpga_sec_dev_error(imgr, ret);
goto modput_exit;
}
- update_progress(imgr, IFPGA_SEC_PROG_WRITING);
+ if (progress_transition(imgr, IFPGA_SEC_PROG_WRITING))
+ goto done;
+
size = imgr->remaining_size;
- while (size) {
+ while (size && !imgr->request_cancel) {
blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
size -= blk_size;
ret = imgr->iops->write_blk(imgr, offset, blk_size);
@@ -212,7 +233,9 @@ static void ifpga_sec_mgr_update(struct work_struct *work)
offset += blk_size;
}
- update_progress(imgr, IFPGA_SEC_PROG_PROGRAMMING);
+ if (progress_transition(imgr, IFPGA_SEC_PROG_PROGRAMMING))
+ goto done;
+
ret = imgr->iops->poll_complete(imgr);
if (ret) {
ifpga_sec_dev_error(imgr, ret);
@@ -381,6 +404,7 @@ static ssize_t filename_store(struct device *dev, struct device_attribute *attr,
}
imgr->err_code = IFPGA_SEC_ERR_NONE;
+ imgr->request_cancel = false;
imgr->progress = IFPGA_SEC_PROG_READING;
reinit_completion(&imgr->update_done);
schedule_work(&imgr->work);
@@ -391,8 +415,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 ifpga_sec_mgr *imgr = to_sec_mgr(dev);
+ bool cancel;
+ int ret = count;
+
+ if (kstrtobool(buf, &cancel) || !cancel)
+ return -EINVAL;
+
+ mutex_lock(&imgr->lock);
+ if (imgr->progress == IFPGA_SEC_PROG_PROGRAMMING)
+ ret = -EBUSY;
+ else if (imgr->progress == IFPGA_SEC_PROG_IDLE)
+ ret = -ENODEV;
+ else
+ imgr->request_cancel = true;
+ mutex_unlock(&imgr->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,
@@ -653,6 +701,9 @@ void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
goto unregister;
}
+ if (imgr->progress != IFPGA_SEC_PROG_PROGRAMMING)
+ imgr->request_cancel = true;
+
mutex_unlock(&imgr->lock);
wait_for_completion(&imgr->update_done);
diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
index 246e3d452c59..890be0800b05 100644
--- a/include/linux/fpga/ifpga-sec-mgr.h
+++ b/include/linux/fpga/ifpga-sec-mgr.h
@@ -112,6 +112,7 @@ struct ifpga_sec_mgr {
enum ifpga_sec_prog progress;
enum ifpga_sec_prog err_state; /* progress state at time of failure */
enum ifpga_sec_err err_code; /* security manager error code */
+ bool request_cancel;
bool driver_unload;
void *priv;
};
--
2.17.1
Extend the Intel 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 <[email protected]>
---
v2:
- Bumped documentation date and version
- Changed progress state "read_file" to "reading"
---
.../ABI/testing/sysfs-class-ifpga-sec-mgr | 11 +++++
drivers/fpga/ifpga-sec-mgr.c | 40 +++++++++++++++++--
2 files changed, 47 insertions(+), 4 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
index 4f375f132c34..73a5246fea1b 100644
--- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
+++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
@@ -78,3 +78,14 @@ Description: Write only. Write the filename of an Intel image
BMC images, BMC firmware, Static Region images,
and Root Entry Hashes, and to cancel Code Signing
Keys (CSK).
+
+What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/status
+Date: Oct 2020
+KernelVersion: 5.11
+Contact: Russ Weight <[email protected]>
+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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
index 7d5a4979554b..ad918fb42dc2 100644
--- a/drivers/fpga/ifpga-sec-mgr.c
+++ b/drivers/fpga/ifpga-sec-mgr.c
@@ -139,6 +139,13 @@ static struct attribute *sec_mgr_security_attrs[] = {
NULL,
};
+static void update_progress(struct ifpga_sec_mgr *imgr,
+ enum ifpga_sec_prog new_progress)
+{
+ imgr->progress = new_progress;
+ sysfs_notify(&imgr->dev.kobj, "update", "status");
+}
+
static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
enum ifpga_sec_err err_code)
{
@@ -149,7 +156,7 @@ static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
static void progress_complete(struct ifpga_sec_mgr *imgr)
{
mutex_lock(&imgr->lock);
- imgr->progress = IFPGA_SEC_PROG_IDLE;
+ update_progress(imgr, IFPGA_SEC_PROG_IDLE);
complete_all(&imgr->update_done);
mutex_unlock(&imgr->lock);
}
@@ -177,14 +184,14 @@ static void ifpga_sec_mgr_update(struct work_struct *work)
goto release_fw_exit;
}
- imgr->progress = IFPGA_SEC_PROG_PREPARING;
+ update_progress(imgr, 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;
+ update_progress(imgr, IFPGA_SEC_PROG_WRITING);
size = imgr->remaining_size;
while (size) {
blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
@@ -199,7 +206,7 @@ static void ifpga_sec_mgr_update(struct work_struct *work)
offset += blk_size;
}
- imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
+ update_progress(imgr, IFPGA_SEC_PROG_PROGRAMMING);
ret = imgr->iops->poll_complete(imgr);
if (ret) {
ifpga_sec_dev_error(imgr, ret);
@@ -259,6 +266,30 @@ static struct attribute_group sec_mgr_security_attr_group = {
.is_visible = sec_mgr_visible,
};
+static const char * const sec_mgr_prog_str[] = {
+ "idle", /* IFPGA_SEC_PROG_IDLE */
+ "reading", /* IFPGA_SEC_PROG_READING */
+ "preparing", /* IFPGA_SEC_PROG_PREPARING */
+ "writing", /* IFPGA_SEC_PROG_WRITING */
+ "programming" /* IFPGA_SEC_PROG_PROGRAMMING */
+};
+
+static ssize_t
+status_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
+ const char *status = "unknown-status";
+
+ if (imgr->progress < IFPGA_SEC_PROG_MAX)
+ status = sec_mgr_prog_str[imgr->progress];
+ else
+ dev_warn(dev, "Invalid status during secure update: %d\n",
+ imgr->progress);
+
+ return sprintf(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)
{
@@ -293,6 +324,7 @@ static DEVICE_ATTR_WO(filename);
static struct attribute *sec_mgr_update_attrs[] = {
&dev_attr_filename.attr,
+ &dev_attr_status.attr,
NULL,
};
--
2.17.1
On 10/2/20 3:36 PM, Russ Weight wrote:
> Extend the Intel 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 <[email protected]>
> ---
> v2:
> - Bumped documentation date and version
> - Changed progress state "read_file" to "reading"
> ---
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 11 +++++
> drivers/fpga/ifpga-sec-mgr.c | 40 +++++++++++++++++--
> 2 files changed, 47 insertions(+), 4 deletions(-)
This was 07/12 in the old patchset.
Also looks fine.
Reviewed-by: Tom Rix <[email protected]>
On 10/2/20 3:37 PM, Russ Weight wrote:
> Extend the Intel 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 <[email protected]>
> ---
> v2:
> - Bumped documentation date and version
> - Minor code cleanup per review comments
> ---
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 10 ++++
> drivers/fpga/ifpga-sec-mgr.c | 59 +++++++++++++++++--
> include/linux/fpga/ifpga-sec-mgr.h | 1 +
> 3 files changed, 66 insertions(+), 4 deletions(-)
This is 10/12 of the original patch set.
Discussions covered most of my issues, the others are changed here.
Reviewed-by: Tom Rix <[email protected]>
On 10/2/20 3:36 PM, Russ Weight wrote:
> The Intel FPGA Security Manager class driver provides a common
> API for user-space tools to manage updates for secure Intel FPGA
> devices. Device drivers that instantiate the Intel 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 Intel FPGA
> Security Manager is that the FPGA Manager does a live update (Partial
> Reconfiguration) to a device whereas the Intel FPGA Security Manager
> updates the FLASH images for the Static Region and the BMC so that
> they will be loaded the next time the FPGA card boots. Security is
> enforced by hardware and firmware. The security manager interacts
> with the firmware to initiate an update, pass in the necessary data,
> and collect status on the update.
>
> The n3000bmc-secure driver is the first driver to use the Intel FPG
> Security Manager. This driver was previously submittied in the same
> patch set, but has been split out in to a separate patch set for V2.
> Follow-on Intel devices will also make use of this common API for
> secure updates.
>
> In addition to managing secure updates of the FPGA and BMC images,
> the Intel FPGA Security Manager update process may also 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.
>
> 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.
>
> Changelog v1 -> v2:
> - Separated out the MAX10 BMC Security Engine to be submitted in
> a separate patch-set.
> - Bumped documentation dates and versions
> - Split ifpga_sec_mgr_register() into create() and register() functions
> - Added devm_ifpga_sec_mgr_create()
> - Added Documentation/fpga/ifpga-sec-mgr.rst
> - Changed progress state "read_file" to "reading"
> - Added sec_error() function (similar to sec_progress())
> - Removed references to bmc_flash_count & smbus_flash_count (not supported)
> - Removed typedefs for imgr ops
> - Removed explicit value assignments in enums
> - Other minor code cleanup per review comments
>
> Russ Weight (7):
> fpga: sec-mgr: intel fpga security manager class driver
> fpga: sec-mgr: enable secure updates
> fpga: sec-mgr: expose sec-mgr update status
> fpga: sec-mgr: expose sec-mgr update errors
> fpga: sec-mgr: expose sec-mgr update size
> fpga: sec-mgr: enable cancel of secure update
> fpga: sec-mgr: expose hardware error info
>
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 143 ++++
> Documentation/fpga/ifpga-sec-mgr.rst | 50 ++
> Documentation/fpga/index.rst | 1 +
> MAINTAINERS | 9 +
> drivers/fpga/Kconfig | 9 +
> drivers/fpga/Makefile | 3 +
> drivers/fpga/ifpga-sec-mgr.c | 781 ++++++++++++++++++
> include/linux/fpga/ifpga-sec-mgr.h | 137 +++
> 8 files changed, 1133 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> create mode 100644 Documentation/fpga/ifpga-sec-mgr.rst
> create mode 100644 drivers/fpga/ifpga-sec-mgr.c
> create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
Russ,
This set has all the changes I was looking for.
Thanks,
Tom
> Subject: [PATCH v2 3/7] fpga: sec-mgr: expose sec-mgr update status
>
> Extend the Intel 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 <[email protected]>
> ---
> v2:
> - Bumped documentation date and version
> - Changed progress state "read_file" to "reading"
> ---
> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 11 +++++
> drivers/fpga/ifpga-sec-mgr.c | 40 +++++++++++++++++--
> 2 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> index 4f375f132c34..73a5246fea1b 100644
> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -78,3 +78,14 @@ Description: Write only. Write the filename of an
> Intel image
> BMC images, BMC firmware, Static Region images,
> and Root Entry Hashes, and to cancel Code Signing
> Keys (CSK).
> +
> +What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/status
> +Date: Oct 2020
> +KernelVersion: 5.11
> +Contact: Russ Weight <[email protected]>
> +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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> index 7d5a4979554b..ad918fb42dc2 100644
> --- a/drivers/fpga/ifpga-sec-mgr.c
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -139,6 +139,13 @@ static struct attribute *sec_mgr_security_attrs[] = {
> NULL,
> };
>
> +static void update_progress(struct ifpga_sec_mgr *imgr,
> + enum ifpga_sec_prog new_progress)
> +{
> + imgr->progress = new_progress;
> + sysfs_notify(&imgr->dev.kobj, "update", "status");
> +}
> +
> static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
> enum ifpga_sec_err err_code)
> {
> @@ -149,7 +156,7 @@ static void ifpga_sec_dev_error(struct ifpga_sec_mgr
> *imgr,
> static void progress_complete(struct ifpga_sec_mgr *imgr)
> {
> mutex_lock(&imgr->lock);
> - imgr->progress = IFPGA_SEC_PROG_IDLE;
> + update_progress(imgr, IFPGA_SEC_PROG_IDLE);
> complete_all(&imgr->update_done);
> mutex_unlock(&imgr->lock);
> }
> @@ -177,14 +184,14 @@ static void ifpga_sec_mgr_update(struct
> work_struct *work)
> goto release_fw_exit;
> }
>
> - imgr->progress = IFPGA_SEC_PROG_PREPARING;
> + update_progress(imgr, 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;
> + update_progress(imgr, IFPGA_SEC_PROG_WRITING);
> size = imgr->remaining_size;
> while (size) {
> blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
> @@ -199,7 +206,7 @@ static void ifpga_sec_mgr_update(struct work_struct
> *work)
> offset += blk_size;
> }
>
> - imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
> + update_progress(imgr, IFPGA_SEC_PROG_PROGRAMMING);
> ret = imgr->iops->poll_complete(imgr);
> if (ret) {
> ifpga_sec_dev_error(imgr, ret);
> @@ -259,6 +266,30 @@ static struct attribute_group
> sec_mgr_security_attr_group = {
> .is_visible = sec_mgr_visible,
> };
>
> +static const char * const sec_mgr_prog_str[] = {
> + "idle", /* IFPGA_SEC_PROG_IDLE */
> + "reading", /* IFPGA_SEC_PROG_READING */
> + "preparing", /* IFPGA_SEC_PROG_PREPARING */
> + "writing", /* IFPGA_SEC_PROG_WRITING */
> + "programming" /* IFPGA_SEC_PROG_PROGRAMMING */
> +};
> +
> +static ssize_t
> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> + struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> + const char *status = "unknown-status";
> +
> + if (imgr->progress < IFPGA_SEC_PROG_MAX)
> + status = sec_mgr_prog_str[imgr->progress];
I am not sure if this would be a problem that as there is no lock protection for
the progress value. If someone changes imgr->progress into a bad value just
after the first check imgr->progress < IFPGA_SEC_PROG_MAX passed.
> + else
> + dev_warn(dev, "Invalid status during secure update: %d\n",
> + imgr->progress);
One minor thing, dev_err or even WARN_ON should be better, and I think
if it hits this, that will be a critical issue in the driver, isn't it?
Thanks
Hao
> +
> + return sprintf(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)
> {
> @@ -293,6 +324,7 @@ static DEVICE_ATTR_WO(filename);
>
> static struct attribute *sec_mgr_update_attrs[] = {
> &dev_attr_filename.attr,
> + &dev_attr_status.attr,
> NULL,
> };
>
> --
> 2.17.1
On 10/5/20 1:41 AM, Wu, Hao wrote:
>> Subject: [PATCH v2 3/7] fpga: sec-mgr: expose sec-mgr update status
>>
>> Extend the Intel 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 <[email protected]>
>> ---
>> v2:
>> - Bumped documentation date and version
>> - Changed progress state "read_file" to "reading"
>> ---
>> .../ABI/testing/sysfs-class-ifpga-sec-mgr | 11 +++++
>> drivers/fpga/ifpga-sec-mgr.c | 40 +++++++++++++++++--
>> 2 files changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> index 4f375f132c34..73a5246fea1b 100644
>> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> @@ -78,3 +78,14 @@ Description:Write only. Write the filename of an
>> Intel image
>> BMC images, BMC firmware, Static Region images,
>> and Root Entry Hashes, and to cancel Code Signing
>> Keys (CSK).
>> +
>> +What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/status
>> +Date:Oct 2020
>> +KernelVersion: 5.11
>> +Contact:Russ Weight <[email protected]>
>> +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/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>> index 7d5a4979554b..ad918fb42dc2 100644
>> --- a/drivers/fpga/ifpga-sec-mgr.c
>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>> @@ -139,6 +139,13 @@ static struct attribute *sec_mgr_security_attrs[] = {
>> NULL,
>> };
>>
>> +static void update_progress(struct ifpga_sec_mgr *imgr,
>> + enum ifpga_sec_prog new_progress)
>> +{
>> +imgr->progress = new_progress;
>> +sysfs_notify(&imgr->dev.kobj, "update", "status");
>> +}
>> +
>> static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
>> enum ifpga_sec_err err_code)
>> {
>> @@ -149,7 +156,7 @@ static void ifpga_sec_dev_error(struct ifpga_sec_mgr
>> *imgr,
>> static void progress_complete(struct ifpga_sec_mgr *imgr)
>> {
>> mutex_lock(&imgr->lock);
>> -imgr->progress = IFPGA_SEC_PROG_IDLE;
>> +update_progress(imgr, IFPGA_SEC_PROG_IDLE);
>> complete_all(&imgr->update_done);
>> mutex_unlock(&imgr->lock);
>> }
>> @@ -177,14 +184,14 @@ static void ifpga_sec_mgr_update(struct
>> work_struct *work)
>> goto release_fw_exit;
>> }
>>
>> -imgr->progress = IFPGA_SEC_PROG_PREPARING;
>> +update_progress(imgr, 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;
>> +update_progress(imgr, IFPGA_SEC_PROG_WRITING);
>> size = imgr->remaining_size;
>> while (size) {
>> blk_size = min_t(u32, size, WRITE_BLOCK_SIZE);
>> @@ -199,7 +206,7 @@ static void ifpga_sec_mgr_update(struct work_struct
>> *work)
>> offset += blk_size;
>> }
>>
>> -imgr->progress = IFPGA_SEC_PROG_PROGRAMMING;
>> +update_progress(imgr, IFPGA_SEC_PROG_PROGRAMMING);
>> ret = imgr->iops->poll_complete(imgr);
>> if (ret) {
>> ifpga_sec_dev_error(imgr, ret);
>> @@ -259,6 +266,30 @@ static struct attribute_group
>> sec_mgr_security_attr_group = {
>> .is_visible = sec_mgr_visible,
>> };
>>
>> +static const char * const sec_mgr_prog_str[] = {
>> +"idle",/* IFPGA_SEC_PROG_IDLE */
>> +"reading",/* IFPGA_SEC_PROG_READING */
>> +"preparing",/* IFPGA_SEC_PROG_PREPARING */
>> +"writing",/* IFPGA_SEC_PROG_WRITING */
>> +"programming"/* IFPGA_SEC_PROG_PROGRAMMING */
>> +};
>> +
>> +static ssize_t
>> +status_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> +struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +const char *status = "unknown-status";
>> +
>> +if (imgr->progress < IFPGA_SEC_PROG_MAX)
>> +status = sec_mgr_prog_str[imgr->progress];
> I am not sure if this would be a problem that as there is no lock protection for
> the progress value. If someone changes imgr->progress into a bad value just
> after the first check imgr->progress < IFPGA_SEC_PROG_MAX passed.
I'll read imgr->progress into a local variable and operate off of that so that I
am using a consistent value.
We really should never see an invalid value. These values are set explicitly (not
incremented or decremented) during the update process. If we do see an invalid
value, it would indicate a driver bug.
>
>> +else
>> +dev_warn(dev, "Invalid status during secure update: %d\n",
>> + imgr->progress);
> One minor thing, dev_err or even WARN_ON should be better, and I think
> if it hits this, that will be a critical issue in the driver, isn't it?
Yes. I'll switch to dev_err(). A stack trace probably wouldn't be useful
since the error would be happening in a different kernel thread.
>
> Thanks
> Hao
>
>> +
>> +return sprintf(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)
>> {
>> @@ -293,6 +324,7 @@ static DEVICE_ATTR_WO(filename);
>>
>> static struct attribute *sec_mgr_update_attrs[] = {
>> &dev_attr_filename.attr,
>> +&dev_attr_status.attr,
>> NULL,
>> };
>>
>> --
>> 2.17.1