On 9/22/20 5:55 PM, Russ Weight wrote:
>
> On 9/6/20 10:00 AM, Tom Rix wrote:
>> On 9/4/20 4:53 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 <russell.h.wei...@intel.com>
>>> ---
>>> .../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 cf1967f1b3e3..762a7dee9453 100644
>>> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> @@ -87,6 +87,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: Sep 2020
>>> +KernelVersion: 5.10
>>> +Contact: Russ Weight <russell.h.wei...@intel.com>
>>> +Description: Write-only. Write a "1" to this file to request
>>> + that a current update be canceled. This request
>>> + will be rejected (EBUSY) if the programming phase
>>> + has already started or (ENODEV) if there is no
>>> + update in progress.
>>> +
>>> What: /sys/class/ifpga_sec_mgr/ifpga_secX/update/status
>>> Date: Sep 2020
>>> KernelVersion: 5.10
>>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>>> index 4ca5d13e5656..afd97c135ebe 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);
>> check cancel() for double error ?
> Meaning - what if the cancel function returns an error? I have been of the
> opinion that the first event (in this case, the cancel) is the most important
> one to report. In the unlikely event that an error occurred during the
> cancel, if it was a persistent error, it would be reported on the next secure
> update. Do you think this is a problem? Do you think it would be worth adding
> logic to report both errors? One thought would be to add a flag to the
> ifpga_sec_mgr structure to indicate that the error being reported occurred
> while canceling. And then the error_show() logic could append two error
> strings (something like: "user-abort+read-write-error"). In this case we
> could also enable hw_errinfo. What do you think? Would it be better to make
> this change?
Ok, we will let the next secure update catch the problem.
>> should request_cancel be cleared ?
> I don't think it needs to be cleared here, as we are exiting on error/abort
> and
> we initialize request_cancel at the beginning of a new secure update.
ok
>>> + 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);
>>> @@ -359,6 +382,7 @@ static ssize_t filename_store(struct device *dev,
>>> struct device_attribute *attr,
>>> imgr->filename[strlen(imgr->filename) - 1] = '\0';
>>>
>>> imgr->err_code = IFPGA_SEC_ERR_NONE;
>>> + imgr->request_cancel = false;
>>> imgr->progress = IFPGA_SEC_PROG_READ_FILE;
>>> reinit_completion(&imgr->update_done);
>>> schedule_work(&imgr->work);
>>> @@ -369,8 +393,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 = 0;
>> int ret = count;
> OK
>>> +
>>> + if (kstrtobool(buf, &cancel) || !cancel)
>> This does not match your description in the testing section.
>>
>> kstrtobool has many other valid inputs.
>>
>> maybe check if count is 1 and buf[0] == '1'
> The documentation is not really incorrect though, is it? I see several other
> instances
> of *_store() functions that use krstrtobool for input and document that a 1
> or a 0
> should be written as input.
>
> However, I'm willing to change it if you think it needs to be changed.
I am being pedantic.
This is ok as-is, testing would work.
Tom
>
>>> + 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 ? : count;
>> return ret;
> Yes - I'll change this.
>> Tom
>>
>>> +}
>>> +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,
>>> @@ -536,6 +584,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 f04bf9e30c67..f51ed663a723 100644
>>> --- a/include/linux/fpga/ifpga-sec-mgr.h
>>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>>> @@ -183,6 +183,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;
>>> };
