lkml.org 
[lkml]   [2020]   [Sep]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v1 10/12] fpga: enable sec-mgr update cancel
    From
    Date

    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.weight@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.weight@intel.com>
    > +Description: Write-only. Write a "1" to this file to request
    > + that a current update be canceled. This request
    > + will be rejected (EBUSY) if the programming phase
    > + has already started or (ENODEV) if there is no
    > + update in progress.
    > +
    > What: /sys/class/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 ?

    should request_cancel be cleared ?

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

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

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

    \
     
     \ /
      Last update: 2020-09-06 19:01    [W:4.108 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site