Re: [PATCH v2 3/7] fpga: sec-mgr: expose sec-mgr update status

[Russ Weight]

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 <russell.h.weight@xxxxxxxxx>
>> ---
>> 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 <russell.h.weight@xxxxxxxxx>
>> +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