| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 Oct 2021 11:13:22 -0700
From: Russ Weight <russell.h.weight@...el.com>
To: <mdf@...nel.org>, <linux-fpga@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
CC: <trix@...hat.com>, <lgoncalv@...hat.com>, <yilun.xu@...el.com>,
<hao.wu@...el.com>, <matthew.gerlach@...el.com>
Subject: Re: [PATCH v17 2/5] fpga: image-load: enable image uploads
On 9/29/21 4:00 PM, Russ Weight wrote:
> Extend the FPGA Image Load framework to include IOCTL support
> (FPGA_IMAGE_LOAD_WRITE) for initiating an upload of an image to a device.
> The IOCTL will return immediately, and the update will begin in the
> context of a kernel worker thread.
>
> Signed-off-by: Russ Weight <russell.h.weight@...el.com>
> ---
> v17:
> - Minor documentation cleanup and references to cancellation are moved
> to a later patch.
> - We no longer send a "just in case" cancel on HW errors. The cleanup()
> op can and should be used to manage the HW state on an error.
> - Removed the FPGA_IMAGE_ERR_NONE macro in favor of returning zero. The
> constant name made sense when we were using enum data types, but is no
> longer necessary.
> - Changed the syntax of the write_blk() function and renamed it to write().
> It now returns a positive size or a negative error number.
> - Changed the prepare() and write() ops to pass in elements of the
> fpga_image_load data structure instead of having the op functions access
> them directly via imgld.
> - Removed the default block size. The lower-level driver must choose a
> block size.
> - Changed work queue from system_unbound_wq to system_long_wq.
> - Removed calls to cond_resched(). The lower-level driver must manage
> any scheduler issues.
> - Use the cdev_set_parent() macro.
> v16:
> - Shift from "Driver" terminology to "Framework" in comments and
> documentation
> - Rename lops to ops for structure member and local variables
> - Change the write_blk() definition to pass in *blk_size (a pointer to
> a default block size of WRITE_BLOCK_SIZE=0x4000) and max_size (the
> the maximum block size to stay within the limit of the data buffer).
> The write_blk() op may use the default *blk_size or modify it to a
> more optimal number for the given device, subject to the max_size limit.
> - All enum values for progress and errors are changed to macros, because
> they are included in the uapi header. This is done to maintain consistency
> amongst the DFL related IOCTL header files. All references to the enum
> types are changed to u32.
> - Bail out early in fpga_image_do_load() if imgld->driver_unload is true.
> - Add a call to cond_resched() in the write_blk() loop to ensure that
> we yield to higher priority tasks during long data transfers.
> - Switch to the system_unbound_wq to enable calls to cond_resched().
> - Switch from test_and_set_bit() to atomic_cmpxchg() to manage
> imgld->opened.
> - Change fpga_image_load_release() to block until the image upload is
> complete.
> - Remove the completion object, imgld->update_done, in favor of calling
> flush_work(&imgld->work);
> v15:
> - Compare to previous patch:
> [PATCH v14 2/6] fpga: sec-mgr: enable secure updates
> - Changed file, symbol, and config names to reflect the new driver name
> - Removed update/filename sysfs file and added the FPGA_IMAGE_LOAD_WRITE
> IOCTL for writing the image data to the FPGA card. The driver no longer
> uses the request_firmware framework.
> - Fixed some error return values in fpga_image_load_register()
> - Removed signed-off/reviewed-by tags
> v14:
> - Added MAINTAINERS reference for
> Documentation/ABI/testing/sysfs-class-fpga-sec-mgr
> - Updated ABI documentation date and kernel version
> - Updated copyright to 2021
> v13:
> - Change "if (count == 0 || " to "if (!count || "
> - Improve error message: "Attempt to register without all required ops\n"
> v12:
> - Updated Date and KernelVersion fields in ABI documentation
> - Removed size parameter from write_blk() op - it is now up to
> the lower-level driver to determine the appropriate size and
> to update smgr->remaining_size accordingly.
> v11:
> - Fixed a spelling error in a comment
> - Initialize smgr->err_code and smgr->progress explicitly in
> fpga_sec_mgr_create() instead of accepting the default 0 value.
> v10:
> - Rebased to 5.12-rc2 next
> - Updated Date and KernelVersion in ABI documentation
> v9:
> - Updated Date and KernelVersion in ABI documentation
> v8:
> - No change
> v7:
> - Changed Date in documentation file to December 2020
> - Changed filename_store() to use kmemdup_nul() instead of
> kstrndup() and changed the count to not assume a line-return.
> v6:
> - Changed "security update" to "secure update" in commit message
> v5:
> - When checking the return values for functions of type enum
> fpga_sec_err err_code, test for FPGA_SEC_ERR_NONE instead of 0
> v4:
> - Changed from "Intel FPGA Security Manager" to FPGA Security Manager"
> and removed unnecessary references to "Intel".
> - Changed: iops -> sops, imgr -> smgr, IFPGA_ -> FPGA_, ifpga_ to fpga_
> v3:
> - Removed unnecessary "goto done"
> - Added a comment to explain imgr->driver_unload in
> ifpga_sec_mgr_unregister()
> v2:
> - Bumped documentation date and version
> - Removed explicit value assignments in enums
> - Other minor code cleanup per review comments
> ---
> Documentation/fpga/fpga-image-load.rst | 25 ++-
> MAINTAINERS | 1 +
> drivers/fpga/fpga-image-load.c | 217 ++++++++++++++++++++++++-
> include/linux/fpga/fpga-image-load.h | 27 +++
> include/uapi/linux/fpga-image-load.h | 53 ++++++
> 5 files changed, 321 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/fpga-image-load.h
>
> diff --git a/Documentation/fpga/fpga-image-load.rst b/Documentation/fpga/fpga-image-load.rst
> index dda9283ef1c7..f6b0e1624a05 100644
> --- a/Documentation/fpga/fpga-image-load.rst
> +++ b/Documentation/fpga/fpga-image-load.rst
> @@ -7,4 +7,27 @@ FPGA Image Load Framework
> The FPGA Image Load framework provides a common API for user-space
> tools to manage image uploads to FPGA devices. Device drivers that
> instantiate the FPGA Image Load framework will interact with the
> -target device to transfer and authenticate the image data.
> +target device to transfer and authenticate the image data. Image uploads
> +are processed in the context of a kernel worker thread.
> +
> +User API
> +========
> +
> +open
> +----
> +
> +An fpga_image_load device is opened exclusively to control an image upload.
> +The device must remain open throughout the duration of the image upload.
> +An attempt to close the device while an upload is in progress will block
> +until the image upload is complete.
> +
> +ioctl
> +-----
> +
> +FPGA_IMAGE_LOAD_WRITE:
> +
> +Start an image upload with the provided image buffer. This IOCTL returns
> +immediately after starting a kernel worker thread to process the image
> +upload which could take as long as 40 minutes depending on the actual
> +device being updated. This is an exclusive operation; an attempt to start
> +concurrent image uploads for the same device will fail with EBUSY.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a99622eebbff..e3b5c555ecbd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7415,6 +7415,7 @@ S: Maintained
> F: Documentation/fpga/fpga-image-load.rst
> F: drivers/fpga/fpga-image-load.c
> F: include/linux/fpga/fpga-image-load.h
> +F: include/uapi/linux/fpga-image-load.h
>
> FPU EMULATOR
> M: Bill Metzenthen <billm@...bpc.org.au>
> diff --git a/drivers/fpga/fpga-image-load.c b/drivers/fpga/fpga-image-load.c
> index 799d18444f7c..4c2571e3b26f 100644
> --- a/drivers/fpga/fpga-image-load.c
> +++ b/drivers/fpga/fpga-image-load.c
> @@ -5,18 +5,190 @@
> * Copyright (C) 2019-2021 Intel Corporation, Inc.
> */
>
> +#include <linux/delay.h>
> #include <linux/fpga/fpga-image-load.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/slab.h>
> +#include <linux/uaccess.h>
> #include <linux/vmalloc.h>
>
> #define IMAGE_LOAD_XA_LIMIT XA_LIMIT(0, INT_MAX)
> static DEFINE_XARRAY_ALLOC(fpga_image_load_xa);
>
> static struct class *fpga_image_load_class;
> +static dev_t fpga_image_devt;
>
> #define to_image_load(d) container_of(d, struct fpga_image_load, dev)
>
> +static void fpga_image_prog_complete(struct fpga_image_load *imgld)
> +{
> + mutex_lock(&imgld->lock);
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + mutex_unlock(&imgld->lock);
> +}
> +
> +static void fpga_image_do_load(struct work_struct *work)
> +{
> + struct fpga_image_load *imgld;
> + s32 ret, offset = 0;
> +
> + imgld = container_of(work, struct fpga_image_load, work);
> +
> + if (imgld->driver_unload) {
> + imgld->err_code = FPGA_IMAGE_ERR_CANCELED;
> + goto idle_exit;
> + }
> +
> + get_device(&imgld->dev);
> + if (!try_module_get(imgld->dev.parent->driver->owner)) {
> + imgld->err_code = FPGA_IMAGE_ERR_BUSY;
> + goto putdev_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PREPARING;
> + ret = imgld->ops->prepare(imgld, imgld->data, imgld->remaining_size);
> + if (ret) {
> + imgld->err_code = ret;
> + goto modput_exit;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_WRITING;
> + while (imgld->remaining_size) {
> + ret = imgld->ops->write(imgld, imgld->data, offset,
> + imgld->remaining_size);
> + if (ret <= 0) {
> + if (!ret) {
> + dev_warn(&imgld->dev,
> + "write-op wrote zero data\n");
> + ret = -FPGA_IMAGE_ERR_RW_ERROR;
> + }
> + imgld->err_code = -ret;
> + goto done;
> + }
> +
> + imgld->remaining_size -= ret;
> + offset += ret;
> + }
> +
> + imgld->progress = FPGA_IMAGE_PROG_PROGRAMMING;
> + ret = imgld->ops->poll_complete(imgld);
> + if (ret)
> + imgld->err_code = ret;
> +
> +done:
> + if (imgld->ops->cleanup)
> + imgld->ops->cleanup(imgld);
> +
> +modput_exit:
> + module_put(imgld->dev.parent->driver->owner);
> +
> +putdev_exit:
> + put_device(&imgld->dev);
> +
> +idle_exit:
> + /*
> + * Note: imgld->remaining_size is left unmodified here to provide
> + * additional information on errors. It will be reinitialized when
> + * the next image load begins.
> + */
> + vfree(imgld->data);
> + imgld->data = NULL;
> + fpga_image_prog_complete(imgld);
> +}
> +
> +static int fpga_image_load_ioctl_write(struct fpga_image_load *imgld,
> + unsigned long arg)
> +{
> + struct fpga_image_write wb;
> + unsigned long minsz;
> + u8 *buf;
> +
> + if (imgld->driver_unload || imgld->progress != FPGA_IMAGE_PROG_IDLE)
> + return -EBUSY;
> +
> + minsz = offsetofend(struct fpga_image_write, buf);
> + if (copy_from_user(&wb, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (wb.flags)
> + return -EINVAL;
FYI: Static analysis flagged that wb.size is not being validated
before being used by vzalloc. It is a 32-bit unsigned value, so
size is probably not a concern. If there are size or alignment
constraints, they belong to the lower-level driver and should
be checked there. I propose to add a null check here:
+ if (!wb.size)
+ return -EINVAL;
+
I'll await more comments before I re-roll the patch set.
- Russ
> +
> + buf = vzalloc(wb.size);
> + if (!buf)
> + return -ENOMEM;
> +
> + if (copy_from_user(buf, u64_to_user_ptr(wb.buf), wb.size)) {
> + vfree(buf);
> + return -EFAULT;
> + }
> +
> + imgld->data = buf;
> + imgld->remaining_size = wb.size;
> + imgld->err_code = 0;
> + imgld->progress = FPGA_IMAGE_PROG_STARTING;
> + queue_work(system_long_wq, &imgld->work);
> +
> + return 0;
> +}
> +
> +static long fpga_image_load_ioctl(struct file *filp, unsigned int cmd,
> + unsigned long arg)
> +{
> + struct fpga_image_load *imgld = filp->private_data;
> + int ret = -ENOTTY;
> +
> + switch (cmd) {
> + case FPGA_IMAGE_LOAD_WRITE:
> + mutex_lock(&imgld->lock);
> + ret = fpga_image_load_ioctl_write(imgld, arg);
> + mutex_unlock(&imgld->lock);
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int fpga_image_load_open(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = container_of(inode->i_cdev,
> + struct fpga_image_load, cdev);
> +
> + if (atomic_cmpxchg(&imgld->opened, 0, 1))
> + return -EBUSY;
> +
> + filp->private_data = imgld;
> +
> + return 0;
> +}
> +
> +static int fpga_image_load_release(struct inode *inode, struct file *filp)
> +{
> + struct fpga_image_load *imgld = filp->private_data;
> +
> + mutex_lock(&imgld->lock);
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto close_exit;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +close_exit:
> + atomic_set(&imgld->opened, 0);
> +
> + return 0;
> +}
> +
> +static const struct file_operations fpga_image_load_fops = {
> + .owner = THIS_MODULE,
> + .open = fpga_image_load_open,
> + .release = fpga_image_load_release,
> + .unlocked_ioctl = fpga_image_load_ioctl,
> +};
> +
> /**
> * fpga_image_load_register - create and register an FPGA Image Load Device
> *
> @@ -35,6 +207,11 @@ fpga_image_load_register(struct device *parent,
> struct fpga_image_load *imgld;
> int ret;
>
> + if (!ops || !ops->prepare || !ops->write || !ops->poll_complete) {
> + dev_err(parent, "Attempt to register without all required ops\n");
> + return ERR_PTR(-ENOMEM);
> + }
> +
> imgld = kzalloc(sizeof(*imgld), GFP_KERNEL);
> if (!imgld)
> return ERR_PTR(-ENOMEM);
> @@ -48,9 +225,13 @@ fpga_image_load_register(struct device *parent,
>
> imgld->priv = priv;
> imgld->ops = ops;
> + imgld->err_code = 0;
> + imgld->progress = FPGA_IMAGE_PROG_IDLE;
> + INIT_WORK(&imgld->work, fpga_image_do_load);
>
> imgld->dev.class = fpga_image_load_class;
> imgld->dev.parent = parent;
> + imgld->dev.devt = MKDEV(MAJOR(fpga_image_devt), imgld->dev.id);
>
> ret = dev_set_name(&imgld->dev, "fpga_image_load%d", imgld->dev.id);
> if (ret) {
> @@ -65,6 +246,16 @@ fpga_image_load_register(struct device *parent,
> return ERR_PTR(ret);
> }
>
> + cdev_init(&imgld->cdev, &fpga_image_load_fops);
> + imgld->cdev.owner = parent->driver->owner;
> + cdev_set_parent(&imgld->cdev, &imgld->dev.kobj);
> +
> + ret = cdev_add(&imgld->cdev, imgld->dev.devt, 1);
> + if (ret) {
> + put_device(&imgld->dev);
> + return ERR_PTR(ret);
> + }
> +
> return imgld;
>
> error_device:
> @@ -83,10 +274,23 @@ EXPORT_SYMBOL_GPL(fpga_image_load_register);
> * @imgld: pointer to struct fpga_image_load
> *
> * This function is intended for use in the parent driver's remove()
> - * function.
> + * function. The driver_unload flag prevents new updates from starting
> + * once the unregister process has begun.
> */
> void fpga_image_load_unregister(struct fpga_image_load *imgld)
> {
> + mutex_lock(&imgld->lock);
> + imgld->driver_unload = true;
> + if (imgld->progress == FPGA_IMAGE_PROG_IDLE) {
> + mutex_unlock(&imgld->lock);
> + goto unregister;
> + }
> +
> + mutex_unlock(&imgld->lock);
> + flush_work(&imgld->work);
> +
> +unregister:
> + cdev_del(&imgld->cdev);
> device_unregister(&imgld->dev);
> }
> EXPORT_SYMBOL_GPL(fpga_image_load_unregister);
> @@ -101,19 +305,30 @@ static void fpga_image_load_dev_release(struct device *dev)
>
> static int __init fpga_image_load_class_init(void)
> {
> + int ret;
> pr_info("FPGA Image Load Framework\n");
>
> fpga_image_load_class = class_create(THIS_MODULE, "fpga_image_load");
> if (IS_ERR(fpga_image_load_class))
> return PTR_ERR(fpga_image_load_class);
>
> + ret = alloc_chrdev_region(&fpga_image_devt, 0, MINORMASK,
> + "fpga_image_load");
> + if (ret)
> + goto exit_destroy_class;
> +
> fpga_image_load_class->dev_release = fpga_image_load_dev_release;
>
> return 0;
> +
> +exit_destroy_class:
> + class_destroy(fpga_image_load_class);
> + return ret;
> }
>
> static void __exit fpga_image_load_class_exit(void)
> {
> + unregister_chrdev_region(fpga_image_devt, MINORMASK);
> class_destroy(fpga_image_load_class);
> WARN_ON(!xa_empty(&fpga_image_load_xa));
> }
> diff --git a/include/linux/fpga/fpga-image-load.h b/include/linux/fpga/fpga-image-load.h
> index 8b051c82ef5f..7b445f489644 100644
> --- a/include/linux/fpga/fpga-image-load.h
> +++ b/include/linux/fpga/fpga-image-load.h
> @@ -7,22 +7,49 @@
> #ifndef _LINUX_FPGA_IMAGE_LOAD_H
> #define _LINUX_FPGA_IMAGE_LOAD_H
>
> +#include <linux/cdev.h>
> #include <linux/device.h>
> #include <linux/mutex.h>
> #include <linux/types.h>
> +#include <uapi/linux/fpga-image-load.h>
>
> struct fpga_image_load;
>
> /**
> * struct fpga_image_load_ops - device specific operations
> + * @prepare: Required: Prepare secure update
> + * @write: Required: The write() op receives the remaining
> + * size to be written and must return the actual
> + * size written or a negative error code. The write()
> + * op will be called repeatedly until all data is
> + * written.
> + * @poll_complete: Required: Check for the completion of the
> + * HW authentication/programming process.
> + * @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 fpga_image_load_ops {
> + u32 (*prepare)(struct fpga_image_load *imgld, const u8 *data, u32 size);
> + s32 (*write)(struct fpga_image_load *imgld, const u8 *data,
> + u32 offset, u32 size);
> + u32 (*poll_complete)(struct fpga_image_load *imgld);
> + void (*cleanup)(struct fpga_image_load *imgld);
> };
>
> struct fpga_image_load {
> struct device dev;
> + struct cdev cdev;
> const struct fpga_image_load_ops *ops;
> struct mutex lock; /* protect data structure contents */
> + atomic_t opened;
> + struct work_struct work;
> + const u8 *data; /* pointer to update data */
> + u32 remaining_size; /* size remaining to transfer */
> + u32 progress;
> + u32 err_code; /* image load error code */
> + bool driver_unload;
> void *priv;
> };
>
> diff --git a/include/uapi/linux/fpga-image-load.h b/include/uapi/linux/fpga-image-load.h
> new file mode 100644
> index 000000000000..20eae3bb10d8
> --- /dev/null
> +++ b/include/uapi/linux/fpga-image-load.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header File for FPGA Image Load User API
> + *
> + * Copyright (C) 2019-2021 Intel Corporation, Inc.
> + *
> + */
> +
> +#ifndef _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +#define _UAPI_LINUX_FPGA_IMAGE_LOAD_H
> +
> +#include <linux/types.h>
> +#include <linux/ioctl.h>
> +
> +#define FPGA_IMAGE_LOAD_MAGIC 0xB9
> +
> +/* Image load progress codes */
> +#define FPGA_IMAGE_PROG_IDLE 0
> +#define FPGA_IMAGE_PROG_STARTING 1
> +#define FPGA_IMAGE_PROG_PREPARING 2
> +#define FPGA_IMAGE_PROG_WRITING 3
> +#define FPGA_IMAGE_PROG_PROGRAMMING 4
> +#define FPGA_IMAGE_PROG_MAX 5
> +
> +/* Image error progress codes */
> +#define FPGA_IMAGE_ERR_HW_ERROR 1
> +#define FPGA_IMAGE_ERR_TIMEOUT 2
> +#define FPGA_IMAGE_ERR_CANCELED 3
> +#define FPGA_IMAGE_ERR_BUSY 4
> +#define FPGA_IMAGE_ERR_INVALID_SIZE 5
> +#define FPGA_IMAGE_ERR_RW_ERROR 6
> +#define FPGA_IMAGE_ERR_WEAROUT 7
> +#define FPGA_IMAGE_ERR_MAX 8
> +
> +/**
> + * FPGA_IMAGE_LOAD_WRITE - _IOW(FPGA_IMAGE_LOAD_MAGIC, 0,
> + * struct fpga_image_write)
> + *
> + * Upload a data buffer to the target device. The user must provide the
> + * data buffer and size.
> + *
> + * Return: 0 on success, -errno on failure.
> + */
> +struct fpga_image_write {
> + /* Input */
> + __u32 flags; /* Zero for now */
> + __u32 size; /* Data size (in bytes) to be written */
> + __u64 buf; /* User space address of source data */
> +};
> +
> +#define FPGA_IMAGE_LOAD_WRITE _IOW(FPGA_IMAGE_LOAD_MAGIC, 0, struct fpga_image_write)
> +
> +#endif /* _UAPI_LINUX_FPGA_IMAGE_LOAD_H */
Powered by blists - more mailing lists