Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: lqo+QCBrIi4sURiKp6r2xcw6AFNBFaFhchQ0BEaZzQ6nSf52HzEQuN0sMLHV+yVvcaZUdcGwJE
 b8H0AoKz4NzQ==
IronPort-SDR: 8357v1KC0QBbtzDFA2oc3ml7z4P22SytG7RDXiK7WaKgj0QnRNZkCeNQrkR+z7Fx26ocQw1Iqd
 obD8qiDJtfBg==
From:   "Wu, Hao" <hao.wu@intel.com>
To:     "Weight, Russell H" <russell.h.weight@intel.com>,
        "mdf@kernel.org" <mdf@kernel.org>,
        "linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "trix@redhat.com" <trix@redhat.com>,
        "lgoncalv@redhat.com" <lgoncalv@redhat.com>,
        "Xu, Yilun" <yilun.xu@intel.com>,
        "Gerlach, Matthew" <matthew.gerlach@intel.com>,
        "Weight, Russell H" <russell.h.weight@intel.com>
Subject: RE: [PATCH v2 2/7] fpga: sec-mgr: enable secure updates
Thread-Topic: [PATCH v2 2/7] fpga: sec-mgr: enable secure updates
Thread-Index: AQHWmQyebhDx7niYFEi5VTkGtD0RFamIo4Bw
Date:   Mon, 5 Oct 2020 08:19:13 +0000
Message-ID: <DM6PR11MB38195987C3B51752D4912577850C0@DM6PR11MB3819.namprd11.prod.outlook.com>
References: <20201002223701.1317-1-russell.h.weight@intel.com>
 <20201002223701.1317-3-russell.h.weight@intel.com>
In-Reply-To: <20201002223701.1317-3-russell.h.weight@intel.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: d5c6dc97-d5c3-4367-ddd2-08d86907580f
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Oct 2020 08:19:13.4898
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: tn3s2JZCanxjKQcKDZF4pC7C1JKx4KQHQNPFc5vd+Bzg3sGHjRdBA7LF6RR3Rm5DgH723vaQQftbWaIpQ30Wqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4594
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> -----Original Message-----
> From: Russ Weight <russell.h.weight@intel.com>
> Sent: Saturday, October 3, 2020 6:37 AM
> To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> Wu, Hao <hao.wu@intel.com>; Gerlach, Matthew
> <matthew.gerlach@intel.com>; Weight, Russell H
> <russell.h.weight@intel.com>
> Subject: [PATCH v2 2/7] fpga: sec-mgr: enable secure updates
>=20
> Extend the FPGA Intel Security Manager class driver to
> include an update/filename sysfs node that can be used
> to initiate a security update.  The filename of a secure
> update file (BMC image, FPGA image, Root Entry Hash image,
> or Code Signing Key cancellation image) can be written to
> this sysfs entry to cause a secure update to occur.
>=20
> The write of the filename will return immediately, and the
> update will begin in the context of a kernel worker thread.
> This tool utilizes the request_firmware framework, which
> requires that the image file reside under /lib/firmware.
>=20
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
> v2:
>   - Bumped documentation date and version
>   - Removed explicit value assignments in enums
>   - Other minor code cleanup per review comments
> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  13 ++
>  drivers/fpga/ifpga-sec-mgr.c                  | 157 ++++++++++++++++++
>  include/linux/fpga/ifpga-sec-mgr.h            |  49 ++++++
>  3 files changed, 219 insertions(+)
>=20
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> index 707958971bcb..4f375f132c34 100644
> --- a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -65,3 +65,16 @@ Contact:	Russ Weight <russell.h.weight@intel.com>
>  Description:	Read only. Returns number of times the user image for the
>  		static region has been flashed.
>  		Format: "%u".
> +
> +What:
> 	/sys/class/ifpga_sec_mgr/ifpga_secX/update/filename
> +Date:		Oct 2020
> +KernelVersion:  5.11
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Write only. Write the filename of an Intel image
> +		file to this sysfs file to initiate a secure
> +		update. The file must have an appropriate header
> +		which, among other things, identifies the target
> +		for the update. This mechanism is used to update
> +		BMC images, BMC firmware, Static Region images,
> +		and Root Entry Hashes, and to cancel Code Signing
> +		Keys (CSK).
> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> index f1caa4602ab3..7d5a4979554b 100644
> --- a/drivers/fpga/ifpga-sec-mgr.c
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -5,8 +5,11 @@
>   * Copyright (C) 2019-2020 Intel Corporation, Inc.
>   */
>=20
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
>  #include <linux/fpga/ifpga-sec-mgr.h>
>  #include <linux/idr.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/vmalloc.h>
> @@ -14,6 +17,8 @@
>  static DEFINE_IDA(ifpga_sec_mgr_ida);
>  static struct class *ifpga_sec_mgr_class;
>=20
> +#define WRITE_BLOCK_SIZE	0x4000

Maybe some comments here for this value or should this be a parameter
of ifpga sec mgr, provided from low level driver during initialization?

> +
>  #define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
>=20
>  static ssize_t
> @@ -134,6 +139,96 @@ static struct attribute *sec_mgr_security_attrs[] =
=3D {
>  	NULL,
>  };
>=20
> +static void ifpga_sec_dev_error(struct ifpga_sec_mgr *imgr,
> +				enum ifpga_sec_err err_code)
> +{
> +	imgr->err_code =3D err_code;
> +	imgr->iops->cancel(imgr);
> +}
> +
> +static void progress_complete(struct ifpga_sec_mgr *imgr)
> +{
> +	mutex_lock(&imgr->lock);
> +	imgr->progress =3D IFPGA_SEC_PROG_IDLE;
> +	complete_all(&imgr->update_done);
> +	mutex_unlock(&imgr->lock);
> +}
> +
> +static void ifpga_sec_mgr_update(struct work_struct *work)
> +{
> +	u32 size, blk_size, offset =3D 0;
> +	struct ifpga_sec_mgr *imgr;
> +	const struct firmware *fw;
> +	enum ifpga_sec_err ret;
> +
> +	imgr =3D container_of(work, struct ifpga_sec_mgr, work);
> +
> +	get_device(&imgr->dev);
> +	if (request_firmware(&fw, imgr->filename, &imgr->dev)) {
> +		imgr->err_code =3D IFPGA_SEC_ERR_FILE_READ;
> +		goto idle_exit;
> +	}
> +
> +	imgr->data =3D fw->data;
> +	imgr->remaining_size =3D fw->size;
> +
> +	if (!try_module_get(imgr->dev.parent->driver->owner)) {
> +		imgr->err_code =3D IFPGA_SEC_ERR_BUSY;
> +		goto release_fw_exit;
> +	}
> +
> +	imgr->progress =3D IFPGA_SEC_PROG_PREPARING;
> +	ret =3D imgr->iops->prepare(imgr);
> +	if (ret) {
> +		ifpga_sec_dev_error(imgr, ret);
> +		goto modput_exit;
> +	}
> +
> +	imgr->progress =3D IFPGA_SEC_PROG_WRITING;
> +	size =3D imgr->remaining_size;
> +	while (size) {
> +		blk_size =3D min_t(u32, size, WRITE_BLOCK_SIZE);
> +		size -=3D blk_size;
> +		ret =3D imgr->iops->write_blk(imgr, offset, blk_size);
> +		if (ret) {
> +			ifpga_sec_dev_error(imgr, ret);
> +			goto done;
> +		}
> +
> +		imgr->remaining_size =3D size;
> +		offset +=3D blk_size;
> +	}

Looks like we can remove size and just use remaining_size here?

> +
> +	imgr->progress =3D IFPGA_SEC_PROG_PROGRAMMING;
> +	ret =3D imgr->iops->poll_complete(imgr);
> +	if (ret) {
> +		ifpga_sec_dev_error(imgr, ret);
> +		goto done;

Looks like no need for this goto done.

> +	}
> +
> +done:
> +	if (imgr->iops->cleanup)
> +		imgr->iops->cleanup(imgr);
> +
> +modput_exit:
> +	module_put(imgr->dev.parent->driver->owner);
> +
> +release_fw_exit:
> +	imgr->data =3D NULL;
> +	release_firmware(fw);
> +
> +idle_exit:
> +	/*
> +	 * Note: imgr->remaining_size is left unmodified here to
> +	 * provide additional information on errors. It will be
> +	 * reinitialized when the next secure update begins.
> +	 */
> +	kfree(imgr->filename);
> +	imgr->filename =3D NULL;
> +	put_device(&imgr->dev);
> +	progress_complete(imgr);

Should it call this function progress complete even in failure case?
A little confusing.

> +}
> +
>  #define check_attr(attribute, _name) \
>  	((attribute) =3D=3D &dev_attr_##_name.attr && imgr->iops->_name)
>=20
> @@ -164,6 +259,48 @@ static struct attribute_group
> sec_mgr_security_attr_group =3D {
>  	.is_visible =3D sec_mgr_visible,
>  };
>=20
> +static ssize_t filename_store(struct device *dev, struct device_attribut=
e *attr,
> +			      const char *buf, size_t count)
> +{
> +	struct ifpga_sec_mgr *imgr =3D to_sec_mgr(dev);
> +	int ret =3D count;
> +
> +	if (count =3D=3D 0 || count >=3D PATH_MAX)
> +		return -EINVAL;
> +
> +	mutex_lock(&imgr->lock);
> +	if (imgr->driver_unload || imgr->progress !=3D IFPGA_SEC_PROG_IDLE)
> {
> +		ret =3D -EBUSY;
> +		goto unlock_exit;
> +	}
> +
> +	imgr->filename =3D kstrndup(buf, count - 1, GFP_KERNEL);
> +	if (!imgr->filename) {
> +		ret =3D -ENOMEM;
> +		goto unlock_exit;
> +	}
> +
> +	imgr->err_code =3D IFPGA_SEC_ERR_NONE;
> +	imgr->progress =3D IFPGA_SEC_PROG_READING;
> +	reinit_completion(&imgr->update_done);
> +	schedule_work(&imgr->work);
> +
> +unlock_exit:
> +	mutex_unlock(&imgr->lock);
> +	return ret;
> +}
> +static DEVICE_ATTR_WO(filename);
> +
> +static struct attribute *sec_mgr_update_attrs[] =3D {
> +	&dev_attr_filename.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_update_attr_group =3D {
> +	.name =3D "update",
> +	.attrs =3D sec_mgr_update_attrs,
> +};
> +
>  static ssize_t name_show(struct device *dev,
>  			 struct device_attribute *attr, char *buf)
>  {
> @@ -185,6 +322,7 @@ static struct attribute_group sec_mgr_attr_group =3D =
{
>  static const struct attribute_group *ifpga_sec_mgr_attr_groups[] =3D {
>  	&sec_mgr_attr_group,
>  	&sec_mgr_security_attr_group,
> +	&sec_mgr_update_attr_group,
>  	NULL,
>  };
>=20
> @@ -245,6 +383,12 @@ ifpga_sec_mgr_create(struct device *dev, const char
> *name,
>  	struct ifpga_sec_mgr *imgr;
>  	int id, ret;
>=20
> +	if (!iops || !iops->cancel || !iops->prepare ||
> +	    !iops->write_blk || !iops->poll_complete) {
> +		dev_err(dev, "Attempt to register without required ops\n");
> +		return NULL;
> +	}
> +
>  	if (!check_reh_handler(dev, iops, bmc) ||
>  	    !check_reh_handler(dev, iops, sr) ||
>  	    !check_reh_handler(dev, iops, pr) ||
> @@ -272,6 +416,8 @@ ifpga_sec_mgr_create(struct device *dev, const char
> *name,
>  	imgr->name =3D name;
>  	imgr->priv =3D priv;
>  	imgr->iops =3D iops;
> +	init_completion(&imgr->update_done);
> +	INIT_WORK(&imgr->work, ifpga_sec_mgr_update);
>=20
>  	device_initialize(&imgr->dev);
>  	imgr->dev.class =3D ifpga_sec_mgr_class;
> @@ -397,6 +543,17 @@ void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr
> *imgr)
>  {
>  	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>=20
> +	mutex_lock(&imgr->lock);
> +	imgr->driver_unload =3D true;

May need some comments here, do you mean get module doesn't work here
to prevent unexpected driver unloading? Or you mean parent device maybe
hot unplug in some cases?

Thanks
Hao

> +	if (imgr->progress =3D=3D IFPGA_SEC_PROG_IDLE) {
> +		mutex_unlock(&imgr->lock);
> +		goto unregister;
> +	}
> +
> +	mutex_unlock(&imgr->lock);
> +	wait_for_completion(&imgr->update_done);
> +
> +unregister:
>  	device_unregister(&imgr->dev);
>  }
>  EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpg=
a-
> sec-mgr.h
> index ded62090e9b9..27008abd8e75 100644
> --- a/include/linux/fpga/ifpga-sec-mgr.h
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -7,12 +7,26 @@
>  #ifndef _LINUX_IFPGA_SEC_MGR_H
>  #define _LINUX_IFPGA_SEC_MGR_H
>=20
> +#include <linux/completion.h>
>  #include <linux/device.h>
>  #include <linux/mutex.h>
>  #include <linux/types.h>
>=20
>  struct ifpga_sec_mgr;
>=20
> +enum ifpga_sec_err {
> +	IFPGA_SEC_ERR_NONE,
> +	IFPGA_SEC_ERR_HW_ERROR,
> +	IFPGA_SEC_ERR_TIMEOUT,
> +	IFPGA_SEC_ERR_CANCELED,
> +	IFPGA_SEC_ERR_BUSY,
> +	IFPGA_SEC_ERR_INVALID_SIZE,
> +	IFPGA_SEC_ERR_RW_ERROR,
> +	IFPGA_SEC_ERR_WEAROUT,
> +	IFPGA_SEC_ERR_FILE_READ,
> +	IFPGA_SEC_ERR_MAX
> +};
> +
>  /**
>   * struct ifpga_sec_mgr_ops - device specific operations
>   * @user_flash_count:	    Optional: Return sysfs string output for FPGA
> @@ -35,6 +49,17 @@ struct ifpga_sec_mgr;
>   * @bmc_reh_size:	    Optional: Return byte size for BMC root entry hash
>   * @sr_reh_size:	    Optional: Return byte size for SR root entry hash
>   * @pr_reh_size:	    Optional: Return byte size for PR root entry hash
> + * @prepare:		    Required: Prepare secure update
> + * @write_blk:		    Required: Write a block of data
> + * @poll_complete:	    Required: Check for the completion of the
> + *			    HW authentication/programming process. This
> + *			    function should check for imgr->driver_unload
> + *			    and abort with IFPGA_SEC_ERR_CANCELED when
> true.
> + * @cancel:		    Required: Signal HW to cancel update
> + * @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 ifpga_sec_mgr_ops {
>  	int (*user_flash_count)(struct ifpga_sec_mgr *imgr);
> @@ -56,6 +81,22 @@ struct ifpga_sec_mgr_ops {
>  	int (*bmc_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr);
>  	int (*sr_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr);
>  	int (*pr_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr);
> +	enum ifpga_sec_err (*prepare)(struct ifpga_sec_mgr *imgr);
> +	enum ifpga_sec_err (*write_blk)(struct ifpga_sec_mgr *imgr,
> +					u32 offset, u32 size);
> +	enum ifpga_sec_err (*poll_complete)(struct ifpga_sec_mgr *imgr);
> +	void (*cleanup)(struct ifpga_sec_mgr *imgr);
> +	enum ifpga_sec_err (*cancel)(struct ifpga_sec_mgr *imgr);
> +};
> +
> +/* Update progress codes */
> +enum ifpga_sec_prog {
> +	IFPGA_SEC_PROG_IDLE,
> +	IFPGA_SEC_PROG_READING,
> +	IFPGA_SEC_PROG_PREPARING,
> +	IFPGA_SEC_PROG_WRITING,
> +	IFPGA_SEC_PROG_PROGRAMMING,
> +	IFPGA_SEC_PROG_MAX
>  };
>=20
>  struct ifpga_sec_mgr {
> @@ -63,6 +104,14 @@ struct ifpga_sec_mgr {
>  	struct device dev;
>  	const struct ifpga_sec_mgr_ops *iops;
>  	struct mutex lock;		/* protect data structure contents */
> +	struct work_struct work;
> +	struct completion update_done;
> +	char *filename;
> +	const u8 *data;			/* pointer to update data */
> +	u32 remaining_size;		/* size remaining to transfer */
> +	enum ifpga_sec_prog progress;
> +	enum ifpga_sec_err err_code;	/* security manager error code */
> +	bool driver_unload;
>  	void *priv;
>  };
>=20
> --
> 2.17.1