[v1,01/12] fpga: fpga security manager class driver

Message ID	20200904235305.6254-2-russell.h.weight@intel.com (mailing list archive)
State	Superseded, archived
Headers	show Return-Path: <SRS0=Dg5O=CN=vger.kernel.org=linux-fpga-owner@kernel.org> IronPort-SDR: zwvtl0BSQfKgIA2Ket6v1uGdDXFyR9rmX1K1EgWDAvo6CwwU7HMVrpB9eFifpZl3tGFVY1qpES 0C6qL2gKI8yA== IronPort-SDR: aqsW3WlM7skxnnR+r/60bL8FBGA8BrdDxIQaiU1YKiPKjru629WYAI+4XTTUK9Iq0QOjUOv1b1 Y07LK9gJGOiA== From: Russ Weight <russell.h.weight@intel.com> To: mdf@kernel.org, lee.jones@linaro.org, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org Cc: trix@redhat.com, lgoncalv@redhat.com, yilun.xu@intel.com, hao.wu@intel.com, matthew.gerlach@intel.com, Russ Weight <russell.h.weight@intel.com> Subject: [PATCH v1 01/12] fpga: fpga security manager class driver Date: Fri, 4 Sep 2020 16:52:54 -0700 Message-Id: <20200904235305.6254-2-russell.h.weight@intel.com> In-Reply-To: <20200904235305.6254-1-russell.h.weight@intel.com> References: <20200904235305.6254-1-russell.h.weight@intel.com> Sender: linux-fpga-owner@vger.kernel.org Precedence: bulk
Series	Intel FPGA Security Manager Class Driver \| expand [v1,00/12] Intel FPGA Security Manager Class Driver [v1,01/12] fpga: fpga security manager class driver [v1,02/12] fpga: create intel max10 bmc security engine [v1,03/12] fpga: expose max10 flash update counts in sysfs [v1,04/12] fpga: expose max10 canceled keys in sysfs [v1,05/12] fpga: enable secure updates [v1,06/12] fpga: add max10 secure update functions [v1,07/12] fpga: expose sec-mgr update status [v1,08/12] fpga: expose sec-mgr update errors [v1,09/12] fpga: expose sec-mgr update size [v1,10/12] fpga: enable sec-mgr update cancel [v1,11/12] fpga: expose hardware error info in sysfs [v1,12/12] fpga: add max10 get_hw_errinfo callback func

Russ Weight Sept. 4, 2020, 11:52 p.m. UTC

Create the Intel Security Manager class driver. The security
manager provides interfaces to manage secure updates for the
FPGA and BMC images that are stored in FLASH. The driver can
also be used to update root entry hashes and to cancel code
signing keys.

This patch creates the class driver and provides sysfs
interfaces for displaying root entry hashes, canceled code
signing keys and flash counts.

Signed-off-by: Russ Weight <russell.h.weight@intel.com>
Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
 .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
 MAINTAINERS                                   |   8 +
 drivers/fpga/Kconfig                          |   9 +
 drivers/fpga/Makefile                         |   3 +
 drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
 include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
 6 files changed, 579 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
 create mode 100644 drivers/fpga/ifpga-sec-mgr.c
 create mode 100644 include/linux/fpga/ifpga-sec-mgr.h

Randy Dunlap Sept. 4, 2020, 11:57 p.m. UTC | #1

On 9/4/20 4:52 PM, Russ Weight wrote:
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 88f64fbf55e3..97c0a6cc2ba7 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>  	  to configure the programmable logic(PL) through PS
>  	  on ZynqMP SoC.
>  
> +config IFPGA_SEC_MGR
> +	tristate "Intel Security Manager for FPGA"
> +        help

Use one tab instead of spaces to indent "help".

> +	  The Intel Security Manager class driver presents a common
> +	  user API for managing secure updates for Intel FPGA
> +	  devices, including flash images for the FPGA static
> +	  region and for the BMC. Select this option to enable
> +	  updates for secure FPGA devices.
> +
>  endif # FPGA

thanks.

Moritz Fischer Sept. 5, 2020, 12:23 a.m. UTC | #2

Hi Russ,

On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
> Create the Intel Security Manager class driver. The security
> manager provides interfaces to manage secure updates for the
> FPGA and BMC images that are stored in FLASH. The driver can
> also be used to update root entry hashes and to cancel code
> signing keys.
> 
> This patch creates the class driver and provides sysfs
> interfaces for displaying root entry hashes, canceled code
> signing keys and flash counts.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>

As for Reviewed-by tags I had seen on other patches in the series, I'd
prefer for that to happen on public mailing lists. If Hao reviewed
patches on some internal Intel list I won't know about it, so please
have him properly Ack/Reviewed-by tag things on a public mailing list.

> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>  MAINTAINERS                                   |   8 +
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   3 +
>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>  6 files changed, 579 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> new file mode 100644
> index 000000000000..86f8992559bf
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -0,0 +1,75 @@
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Name of low level fpga security manager driver.
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the static
> +		region if one is programmed, else it returns the
> +		string: "hash not programmed".  This file is only
> +		visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the partial
> +		reconfiguration region if one is programmed, else it
> +		returns the string: "hash not programmed".  This file
> +		is only visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the BMC image
> +		if one is programmed, else it returns the string:
> +		"hash not programmed".  This file is only visible if the
> +		underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the static region. The standard bitmap
> +		list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the partial reconfiguration region. The
> +		standard bitmap list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the BMC.  The standard bitmap list format
> +		is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +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: "%d".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns number of times the BMC image has been
> +		flashed.
> +		Format: "%d".
> diff --git a/MAINTAINERS b/MAINTAINERS
> index deaafb617361..4a2ebe6b120d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>  F:	drivers/fpga/
>  F:	include/linux/fpga/
>  
> +INTEL FPGA SECURITY MANAGER DRIVERS
> +M:	Russ Weight <russell.h.weight@intel.com>
> +L:	linux-fpga@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +F:	drivers/fpga/ifpga-sec-mgr.c
> +F:	include/linux/fpga/ifpga-sec-mgr.h

Generally not against having more people help out, but do we need a
per driver maintainer?

> +
>  FPU EMULATOR
>  M:	Bill Metzenthen <billm@melbpc.org.au>
>  S:	Maintained
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 88f64fbf55e3..97c0a6cc2ba7 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>  	  to configure the programmable logic(PL) through PS
>  	  on ZynqMP SoC.
>  
> +config IFPGA_SEC_MGR
> +	tristate "Intel Security Manager for FPGA"
> +        help
> +	  The Intel Security Manager class driver presents a common
> +	  user API for managing secure updates for Intel FPGA
> +	  devices, including flash images for the FPGA static
> +	  region and for the BMC. Select this option to enable
> +	  updates for secure FPGA devices.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index c69bfc931519..ec9fbacdedd8 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  
> +# Intel FPGA Security Manager Framework
> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> new file mode 100644
> index 000000000000..97bf80277ed2
> --- /dev/null
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Security Manager for FPGA
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +
> +#include <linux/fpga/ifpga-sec-mgr.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +
> +static DEFINE_IDA(ifpga_sec_mgr_ida);
> +static struct class *ifpga_sec_mgr_class;
> +
> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
> +				 sysfs_csk_hndlr_t get_csk,
> +				 sysfs_csk_nbits_t get_csk_nbits,
> +				 char *buf)
> +{
> +	unsigned long *csk_map = NULL;
> +	unsigned int nbits;
> +	int cnt, ret;
> +
> +	ret = get_csk_nbits(imgr);
> +	if (ret < 0)
> +		return ret;
> +
> +	nbits = (unsigned int)ret;
> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
> +	if (!csk_map)
> +		return -ENOMEM;
> +
> +	ret = get_csk(imgr, csk_map, nbits);
> +	if (ret)
> +		goto vfree_exit;
> +
> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
> +
> +vfree_exit:
> +	vfree(csk_map);
> +	return ret ? : cnt;
> +}
> +
> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
> +				    sysfs_reh_hndlr_t get_reh,
> +				    sysfs_reh_size_t get_reh_size,
> +				    char *buf)
> +{
> +	unsigned int size, i;
> +	int ret, cnt = 0;
> +	u8 *hash;
> +
> +	ret = get_reh_size(imgr);
> +	if (ret < 0)
> +		return ret;
> +	else if (!ret)
> +		return sprintf(buf, "hash not programmed\n");
> +
> +	size = (unsigned int)ret;
> +	hash = vmalloc(size);
> +	if (!hash)
> +		return -ENOMEM;
> +
> +	ret = get_reh(imgr, hash, size);
> +	if (ret)
> +		goto vfree_exit;
> +
> +	cnt += sprintf(buf, "0x");
> +	for (i = 0; i < size; i++)
> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
> +	cnt += sprintf(buf + cnt, "\n");
> +
> +vfree_exit:
> +	vfree(hash);
> +	return ret ? : cnt;
> +}
> +
> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
> +
> +#define DEVICE_ATTR_SEC_CSK(_name) \
> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
> +					  struct device_attribute *attr, \
> +					  char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_canceled_csk(imgr, \
> +	       imgr->iops->_name##_canceled_csks, \
> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_canceled_csks)
> +
> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
> +				     struct device_attribute *attr, \
> +				     char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_root_entry_hash(imgr, \
> +	       imgr->iops->_name##_root_entry_hash, \
> +	       imgr->iops->_name##_reh_size, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
> +
> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
> +static ssize_t _name##_flash_count_show(struct device *dev, \
> +		    struct device_attribute *attr, char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
> +} \
> +static DEVICE_ATTR_RO(_name##_flash_count)
> +
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
> +DEVICE_ATTR_SEC_FLASH_CNT(user);
> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
> +DEVICE_ATTR_SEC_CSK(sr);
> +DEVICE_ATTR_SEC_CSK(pr);
> +DEVICE_ATTR_SEC_CSK(bmc);
> +
> +static struct attribute *sec_mgr_security_attrs[] = {
> +	&dev_attr_user_flash_count.attr,
> +	&dev_attr_bmc_flash_count.attr,
> +	&dev_attr_bmc_root_entry_hash.attr,
> +	&dev_attr_sr_root_entry_hash.attr,
> +	&dev_attr_pr_root_entry_hash.attr,
> +	&dev_attr_sr_canceled_csks.attr,
> +	&dev_attr_pr_canceled_csks.attr,
> +	&dev_attr_bmc_canceled_csks.attr,
> +	NULL,
> +};
> +
> +#define check_attr(attribute, _name) \
> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
> +
> +static umode_t sec_mgr_visible(struct kobject *kobj,
> +			       struct attribute *attr, int n)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
> +
> +	if (check_attr(attr, user_flash_count) ||
> +	    check_attr(attr, bmc_flash_count) ||
> +	    check_attr(attr, bmc_root_entry_hash) ||
> +	    check_attr(attr, sr_root_entry_hash) ||
> +	    check_attr(attr, pr_root_entry_hash) ||
> +	    check_attr(attr, sr_canceled_csks) ||
> +	    check_attr(attr, pr_canceled_csks) ||
> +	    check_attr(attr, bmc_canceled_csks))
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group sec_mgr_security_attr_group = {
> +	.name = "security",
> +	.attrs = sec_mgr_security_attrs,
> +	.is_visible = sec_mgr_visible,
> +};
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	return sprintf(buf, "%s\n", imgr->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *sec_mgr_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_attr_group = {
> +	.attrs = sec_mgr_attrs,
> +};
> +
> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
> +	&sec_mgr_attr_group,
> +	&sec_mgr_security_attr_group,
> +	NULL,
> +};
> +
> +static bool check_sysfs_handler(struct device *dev,
> +				void *sysfs_handler, void *size_handler,
> +				const char *sysfs_handler_name,
> +				const char *size_handler_name)
> +{
> +	if (sysfs_handler) {
> +		if (!size_handler) {
> +			dev_err(dev, "%s registered without %s\n",
> +				sysfs_handler_name, size_handler_name);
> +			return false;
> +		}
> +	} else if (size_handler) {
> +		dev_err(dev, "%s registered without %s\n",
> +			size_handler_name, sysfs_handler_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +#define check_reh_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
> +			    (_iops)->_name##_reh_size, \
> +			    __stringify(_name##_root_entry_hash), \
> +			    __stringify(_name##_reh_size))
> +
> +#define check_csk_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
> +			    (_iops)->_name##_canceled_csk_nbits, \
> +			    __stringify(_name##_canceled_csks), \
> +			    __stringify(_name##_canceled_csk_nbits))
> +
> +/**
> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
> + *
> + * @dev:  create ifpga security manager device from pdev
> + * @name: ifpga security manager name
> + * @iops: pointer to a structure of ifpga callback functions
> + * @priv: ifpga security manager private data
> + *
> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
> + */
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
> +{
> +	struct ifpga_sec_mgr *imgr;
> +	int id, ret;
> +
> +	if (!check_reh_handler(dev, iops, bmc) ||
> +	    !check_reh_handler(dev, iops, sr) ||
> +	    !check_reh_handler(dev, iops, pr) ||
> +	    !check_csk_handler(dev, iops, bmc) ||
> +	    !check_csk_handler(dev, iops, sr) ||
> +	    !check_csk_handler(dev, iops, pr)) {
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(dev, "Attempt to register with no name!\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
> +	if (!imgr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	imgr->name = name;
> +	imgr->priv = priv;
> +	imgr->iops = iops;
> +	mutex_init(&imgr->lock);
> +
> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto exit_free;
> +	}
> +
> +	imgr->dev.class = ifpga_sec_mgr_class;
> +	imgr->dev.parent = dev;
> +	imgr->dev.id = id;
> +
> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
> +	if (ret) {
> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
> +		goto exit_free;
> +	}
> +
> +	ret = device_register(&imgr->dev);
> +	if (ret) {
> +		put_device(&imgr->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return imgr;
> +
> +exit_free:
> +	kfree(dev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
> +
> +/**
> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
> + *
> + * @mgr: fpga manager struct
> + *
> + * This function is intended for use in a IFPGA security manager
> + * driver's remove() function.
> + */
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
> +{
> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
> +
> +	device_unregister(&imgr->dev);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> +
> +static void ifpga_sec_mgr_dev_release(struct device *dev)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	mutex_destroy(&imgr->lock);
> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
> +	kfree(imgr);
> +}
> +
> +static int __init ifpga_sec_mgr_class_init(void)
> +{
> +	pr_info("Intel FPGA Security Manager\n");
> +
> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
> +	if (IS_ERR(ifpga_sec_mgr_class))
> +		return PTR_ERR(ifpga_sec_mgr_class);
> +
> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
> +
> +	return 0;
> +}
> +
> +static void __exit ifpga_sec_mgr_class_exit(void)
> +{
> +	class_destroy(ifpga_sec_mgr_class);
> +	ida_destroy(&ifpga_sec_mgr_ida);
> +}
> +
> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +subsys_initcall(ifpga_sec_mgr_class_init);
> +module_exit(ifpga_sec_mgr_class_exit)
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
> new file mode 100644
> index 000000000000..e391b0c8f448
> --- /dev/null
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel FPGA Security Manager
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +#ifndef _LINUX_IFPGA_SEC_MGR_H
> +#define _LINUX_IFPGA_SEC_MGR_H
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct ifpga_sec_mgr;
> +
> +/**
> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the byte size of a
> + * root entry hash.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Byte count on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
> + *			       for root entry hashes
> + * @imgr:      pointer to security manager structure
> + * @hash:      pointer to an array of bytes in which to store the hash
> + * @size:      byte size of root entry hash
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return root entry hash data to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
> +				 unsigned int size);
> +
> +/**
> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
> + *			       for flash counts
> + * @imgr: pointer to security manager structure
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a flash count to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager
> + * Return: flash count or negative errno
> + */
> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
> + *				      a Code Signing Key cancellation vector
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the number of bits
> + * in a Code Signing Key cancellation vector.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Number of bits on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
> + *			       bit vector of canceled keys
> + *
> + * @imgr:    pointer to security manager structure
> + * @csk_map: pointer to a bitmap to contain cancellation key vector
> + * @nbits:   number of bits in CSK vector
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a bitmap of canceled keys to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> +				 unsigned long *csk_map, unsigned int nbits);
> +
> +/**
> + * struct ifpga_sec_mgr_ops - device specific operations
> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
> + *			    image flash count
> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
> + *			    image flash count
> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
> + *			    region root entry hash
> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration root entry hash
> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
> + *			    root entry hash
> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
> + *			    region canceled keys
> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration canceled keys
> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
> + *			    canceled keys
> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
> + * @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
> + */
> +struct ifpga_sec_mgr_ops {
> +	sysfs_cnt_hndlr_t user_flash_count;
> +	sysfs_cnt_hndlr_t bmc_flash_count;
> +	sysfs_cnt_hndlr_t smbus_flash_count;
> +	sysfs_reh_hndlr_t sr_root_entry_hash;
> +	sysfs_reh_hndlr_t pr_root_entry_hash;
> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
> +	sysfs_csk_hndlr_t sr_canceled_csks;
> +	sysfs_csk_hndlr_t pr_canceled_csks;
> +	sysfs_csk_hndlr_t bmc_canceled_csks;
> +	sysfs_reh_size_t bmc_reh_size;
> +	sysfs_reh_size_t sr_reh_size;
> +	sysfs_reh_size_t pr_reh_size;
> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
> +};
> +
> +struct ifpga_sec_mgr {
> +	const char *name;
> +	struct device dev;
> +	const struct ifpga_sec_mgr_ops *iops;
> +	struct mutex lock;		/* protect data structure contents */
> +	void *priv;
> +};
> +
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
> +
> +#endif
> -- 
> 2.17.1
> 

This will take me a while,

Thanks,
Moritz

Russ Weight Sept. 5, 2020, 12:44 a.m. UTC | #3

On 9/4/20 5:23 PM, Moritz Fischer wrote:
> Hi Russ,
>
> On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
>> Create the Intel Security Manager class driver. The security
>> manager provides interfaces to manage secure updates for the
>> FPGA and BMC images that are stored in FLASH. The driver can
>> also be used to update root entry hashes and to cancel code
>> signing keys.
>>
>> This patch creates the class driver and provides sysfs
>> interfaces for displaying root entry hashes, canceled code
>> signing keys and flash counts.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> As for Reviewed-by tags I had seen on other patches in the series, I'd
> prefer for that to happen on public mailing lists. If Hao reviewed
> patches on some internal Intel list I won't know about it, so please
> have him properly Ack/Reviewed-by tag things on a public mailing list.

Sure - I'll remove the Ack/Reviewed-by tags that were added internally 
before I submit the next version of the patchset (except where Hao 
re-adds them on the public list during this review cycle).

>
>> ---
>>   .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>>   MAINTAINERS                                   |   8 +
>>   drivers/fpga/Kconfig                          |   9 +
>>   drivers/fpga/Makefile                         |   3 +
>>   drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>>   include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>>   6 files changed, 579 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>   create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>>   create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> new file mode 100644
>> index 000000000000..86f8992559bf
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> @@ -0,0 +1,75 @@
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Name of low level fpga security manager driver.
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the static
>> +		region if one is programmed, else it returns the
>> +		string: "hash not programmed".  This file is only
>> +		visible if the underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the partial
>> +		reconfiguration region if one is programmed, else it
>> +		returns the string: "hash not programmed".  This file
>> +		is only visible if the underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the BMC image
>> +		if one is programmed, else it returns the string:
>> +		"hash not programmed".  This file is only visible if the
>> +		underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the static region. The standard bitmap
>> +		list format is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the partial reconfiguration region. The
>> +		standard bitmap list format is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the BMC.  The standard bitmap list format
>> +		is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +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: "%d".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns number of times the BMC image has been
>> +		flashed.
>> +		Format: "%d".
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index deaafb617361..4a2ebe6b120d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>>   F:	drivers/fpga/
>>   F:	include/linux/fpga/
>>   
>> +INTEL FPGA SECURITY MANAGER DRIVERS
>> +M:	Russ Weight <russell.h.weight@intel.com>
>> +L:	linux-fpga@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> +F:	drivers/fpga/ifpga-sec-mgr.c
>> +F:	include/linux/fpga/ifpga-sec-mgr.h
> Generally not against having more people help out, but do we need a
> per driver maintainer?
>
>> +
>>   FPU EMULATOR
>>   M:	Bill Metzenthen <billm@melbpc.org.au>
>>   S:	Maintained
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 88f64fbf55e3..97c0a6cc2ba7 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>>   	  to configure the programmable logic(PL) through PS
>>   	  on ZynqMP SoC.
>>   
>> +config IFPGA_SEC_MGR
>> +	tristate "Intel Security Manager for FPGA"
>> +        help
>> +	  The Intel Security Manager class driver presents a common
>> +	  user API for managing secure updates for Intel FPGA
>> +	  devices, including flash images for the FPGA static
>> +	  region and for the BMC. Select this option to enable
>> +	  updates for secure FPGA devices.
>> +
>>   endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index c69bfc931519..ec9fbacdedd8 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>>   obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>   obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>   
>> +# Intel FPGA Security Manager Framework
>> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>> +
>>   # FPGA Bridge Drivers
>>   obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>   obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>> new file mode 100644
>> index 000000000000..97bf80277ed2
>> --- /dev/null
>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Intel Security Manager for FPGA
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>> + */
>> +
>> +#include <linux/fpga/ifpga-sec-mgr.h>
>> +#include <linux/idr.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +static DEFINE_IDA(ifpga_sec_mgr_ida);
>> +static struct class *ifpga_sec_mgr_class;
>> +
>> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
>> +				 sysfs_csk_hndlr_t get_csk,
>> +				 sysfs_csk_nbits_t get_csk_nbits,
>> +				 char *buf)
>> +{
>> +	unsigned long *csk_map = NULL;
>> +	unsigned int nbits;
>> +	int cnt, ret;
>> +
>> +	ret = get_csk_nbits(imgr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	nbits = (unsigned int)ret;
>> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
>> +	if (!csk_map)
>> +		return -ENOMEM;
>> +
>> +	ret = get_csk(imgr, csk_map, nbits);
>> +	if (ret)
>> +		goto vfree_exit;
>> +
>> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
>> +
>> +vfree_exit:
>> +	vfree(csk_map);
>> +	return ret ? : cnt;
>> +}
>> +
>> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
>> +				    sysfs_reh_hndlr_t get_reh,
>> +				    sysfs_reh_size_t get_reh_size,
>> +				    char *buf)
>> +{
>> +	unsigned int size, i;
>> +	int ret, cnt = 0;
>> +	u8 *hash;
>> +
>> +	ret = get_reh_size(imgr);
>> +	if (ret < 0)
>> +		return ret;
>> +	else if (!ret)
>> +		return sprintf(buf, "hash not programmed\n");
>> +
>> +	size = (unsigned int)ret;
>> +	hash = vmalloc(size);
>> +	if (!hash)
>> +		return -ENOMEM;
>> +
>> +	ret = get_reh(imgr, hash, size);
>> +	if (ret)
>> +		goto vfree_exit;
>> +
>> +	cnt += sprintf(buf, "0x");
>> +	for (i = 0; i < size; i++)
>> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
>> +	cnt += sprintf(buf + cnt, "\n");
>> +
>> +vfree_exit:
>> +	vfree(hash);
>> +	return ret ? : cnt;
>> +}
>> +
>> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
>> +
>> +#define DEVICE_ATTR_SEC_CSK(_name) \
>> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
>> +					  struct device_attribute *attr, \
>> +					  char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	return show_canceled_csk(imgr, \
>> +	       imgr->iops->_name##_canceled_csks, \
>> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_canceled_csks)
>> +
>> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
>> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
>> +				     struct device_attribute *attr, \
>> +				     char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	return show_root_entry_hash(imgr, \
>> +	       imgr->iops->_name##_root_entry_hash, \
>> +	       imgr->iops->_name##_reh_size, buf); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
>> +
>> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
>> +static ssize_t _name##_flash_count_show(struct device *dev, \
>> +		    struct device_attribute *attr, char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
>> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_flash_count)
>> +
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
>> +DEVICE_ATTR_SEC_FLASH_CNT(user);
>> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
>> +DEVICE_ATTR_SEC_CSK(sr);
>> +DEVICE_ATTR_SEC_CSK(pr);
>> +DEVICE_ATTR_SEC_CSK(bmc);
>> +
>> +static struct attribute *sec_mgr_security_attrs[] = {
>> +	&dev_attr_user_flash_count.attr,
>> +	&dev_attr_bmc_flash_count.attr,
>> +	&dev_attr_bmc_root_entry_hash.attr,
>> +	&dev_attr_sr_root_entry_hash.attr,
>> +	&dev_attr_pr_root_entry_hash.attr,
>> +	&dev_attr_sr_canceled_csks.attr,
>> +	&dev_attr_pr_canceled_csks.attr,
>> +	&dev_attr_bmc_canceled_csks.attr,
>> +	NULL,
>> +};
>> +
>> +#define check_attr(attribute, _name) \
>> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>> +
>> +static umode_t sec_mgr_visible(struct kobject *kobj,
>> +			       struct attribute *attr, int n)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
>> +
>> +	if (check_attr(attr, user_flash_count) ||
>> +	    check_attr(attr, bmc_flash_count) ||
>> +	    check_attr(attr, bmc_root_entry_hash) ||
>> +	    check_attr(attr, sr_root_entry_hash) ||
>> +	    check_attr(attr, pr_root_entry_hash) ||
>> +	    check_attr(attr, sr_canceled_csks) ||
>> +	    check_attr(attr, pr_canceled_csks) ||
>> +	    check_attr(attr, bmc_canceled_csks))
>> +		return attr->mode;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct attribute_group sec_mgr_security_attr_group = {
>> +	.name = "security",
>> +	.attrs = sec_mgr_security_attrs,
>> +	.is_visible = sec_mgr_visible,
>> +};
>> +
>> +static ssize_t name_show(struct device *dev,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +
>> +	return sprintf(buf, "%s\n", imgr->name);
>> +}
>> +static DEVICE_ATTR_RO(name);
>> +
>> +static struct attribute *sec_mgr_attrs[] = {
>> +	&dev_attr_name.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group sec_mgr_attr_group = {
>> +	.attrs = sec_mgr_attrs,
>> +};
>> +
>> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
>> +	&sec_mgr_attr_group,
>> +	&sec_mgr_security_attr_group,
>> +	NULL,
>> +};
>> +
>> +static bool check_sysfs_handler(struct device *dev,
>> +				void *sysfs_handler, void *size_handler,
>> +				const char *sysfs_handler_name,
>> +				const char *size_handler_name)
>> +{
>> +	if (sysfs_handler) {
>> +		if (!size_handler) {
>> +			dev_err(dev, "%s registered without %s\n",
>> +				sysfs_handler_name, size_handler_name);
>> +			return false;
>> +		}
>> +	} else if (size_handler) {
>> +		dev_err(dev, "%s registered without %s\n",
>> +			size_handler_name, sysfs_handler_name);
>> +		return false;
>> +	}
>> +	return true;
>> +}
>> +
>> +#define check_reh_handler(_dev, _iops, _name) \
>> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
>> +			    (_iops)->_name##_reh_size, \
>> +			    __stringify(_name##_root_entry_hash), \
>> +			    __stringify(_name##_reh_size))
>> +
>> +#define check_csk_handler(_dev, _iops, _name) \
>> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
>> +			    (_iops)->_name##_canceled_csk_nbits, \
>> +			    __stringify(_name##_canceled_csks), \
>> +			    __stringify(_name##_canceled_csk_nbits))
>> +
>> +/**
>> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
>> + *
>> + * @dev:  create ifpga security manager device from pdev
>> + * @name: ifpga security manager name
>> + * @iops: pointer to a structure of ifpga callback functions
>> + * @priv: ifpga security manager private data
>> + *
>> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
>> + */
>> +struct ifpga_sec_mgr *
>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
>> +{
>> +	struct ifpga_sec_mgr *imgr;
>> +	int id, ret;
>> +
>> +	if (!check_reh_handler(dev, iops, bmc) ||
>> +	    !check_reh_handler(dev, iops, sr) ||
>> +	    !check_reh_handler(dev, iops, pr) ||
>> +	    !check_csk_handler(dev, iops, bmc) ||
>> +	    !check_csk_handler(dev, iops, sr) ||
>> +	    !check_csk_handler(dev, iops, pr)) {
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (!name || !strlen(name)) {
>> +		dev_err(dev, "Attempt to register with no name!\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
>> +	if (!imgr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	imgr->name = name;
>> +	imgr->priv = priv;
>> +	imgr->iops = iops;
>> +	mutex_init(&imgr->lock);
>> +
>> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
>> +	if (id < 0) {
>> +		ret = id;
>> +		goto exit_free;
>> +	}
>> +
>> +	imgr->dev.class = ifpga_sec_mgr_class;
>> +	imgr->dev.parent = dev;
>> +	imgr->dev.id = id;
>> +
>> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
>> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
>> +		goto exit_free;
>> +	}
>> +
>> +	ret = device_register(&imgr->dev);
>> +	if (ret) {
>> +		put_device(&imgr->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return imgr;
>> +
>> +exit_free:
>> +	kfree(dev);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
>> +
>> +/**
>> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
>> + *
>> + * @mgr: fpga manager struct
>> + *
>> + * This function is intended for use in a IFPGA security manager
>> + * driver's remove() function.
>> + */
>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
>> +{
>> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>> +
>> +	device_unregister(&imgr->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
>> +
>> +static void ifpga_sec_mgr_dev_release(struct device *dev)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +
>> +	mutex_destroy(&imgr->lock);
>> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
>> +	kfree(imgr);
>> +}
>> +
>> +static int __init ifpga_sec_mgr_class_init(void)
>> +{
>> +	pr_info("Intel FPGA Security Manager\n");
>> +
>> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
>> +	if (IS_ERR(ifpga_sec_mgr_class))
>> +		return PTR_ERR(ifpga_sec_mgr_class);
>> +
>> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
>> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit ifpga_sec_mgr_class_exit(void)
>> +{
>> +	class_destroy(ifpga_sec_mgr_class);
>> +	ida_destroy(&ifpga_sec_mgr_ida);
>> +}
>> +
>> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>> +subsys_initcall(ifpga_sec_mgr_class_init);
>> +module_exit(ifpga_sec_mgr_class_exit)
>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
>> new file mode 100644
>> index 000000000000..e391b0c8f448
>> --- /dev/null
>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>> @@ -0,0 +1,145 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Header file for Intel FPGA Security Manager
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>> + */
>> +#ifndef _LINUX_IFPGA_SEC_MGR_H
>> +#define _LINUX_IFPGA_SEC_MGR_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +struct ifpga_sec_mgr;
>> +
>> +/**
>> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
>> + *
>> + * @imgr:      pointer to security manager structure
>> + *
>> + * This datatype is used to define a function that returns the byte size of a
>> + * root entry hash.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  Byte count on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
>> + *			       for root entry hashes
>> + * @imgr:      pointer to security manager structure
>> + * @hash:      pointer to an array of bytes in which to store the hash
>> + * @size:      byte size of root entry hash
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return root entry hash data to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  0 on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
>> +				 unsigned int size);
>> +
>> +/**
>> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
>> + *			       for flash counts
>> + * @imgr: pointer to security manager structure
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return a flash count to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager
>> + * Return: flash count or negative errno
>> + */
>> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
>> + *				      a Code Signing Key cancellation vector
>> + *
>> + * @imgr:      pointer to security manager structure
>> + *
>> + * This datatype is used to define a function that returns the number of bits
>> + * in a Code Signing Key cancellation vector.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  Number of bits on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
>> + *			       bit vector of canceled keys
>> + *
>> + * @imgr:    pointer to security manager structure
>> + * @csk_map: pointer to a bitmap to contain cancellation key vector
>> + * @nbits:   number of bits in CSK vector
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return a bitmap of canceled keys to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  0 on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>> +				 unsigned long *csk_map, unsigned int nbits);
>> +
>> +/**
>> + * struct ifpga_sec_mgr_ops - device specific operations
>> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
>> + *			    image flash count
>> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
>> + *			    image flash count
>> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
>> + *			    region root entry hash
>> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
>> + *			    reconfiguration root entry hash
>> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
>> + *			    root entry hash
>> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
>> + *			    region canceled keys
>> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
>> + *			    reconfiguration canceled keys
>> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
>> + *			    canceled keys
>> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
>> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
>> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
>> + * @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
>> + */
>> +struct ifpga_sec_mgr_ops {
>> +	sysfs_cnt_hndlr_t user_flash_count;
>> +	sysfs_cnt_hndlr_t bmc_flash_count;
>> +	sysfs_cnt_hndlr_t smbus_flash_count;
>> +	sysfs_reh_hndlr_t sr_root_entry_hash;
>> +	sysfs_reh_hndlr_t pr_root_entry_hash;
>> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
>> +	sysfs_csk_hndlr_t sr_canceled_csks;
>> +	sysfs_csk_hndlr_t pr_canceled_csks;
>> +	sysfs_csk_hndlr_t bmc_canceled_csks;
>> +	sysfs_reh_size_t bmc_reh_size;
>> +	sysfs_reh_size_t sr_reh_size;
>> +	sysfs_reh_size_t pr_reh_size;
>> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
>> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
>> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
>> +};
>> +
>> +struct ifpga_sec_mgr {
>> +	const char *name;
>> +	struct device dev;
>> +	const struct ifpga_sec_mgr_ops *iops;
>> +	struct mutex lock;		/* protect data structure contents */
>> +	void *priv;
>> +};
>> +
>> +struct ifpga_sec_mgr *
>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
>> +
>> +#endif
>> -- 
>> 2.17.1
>>
> This will take me a while,
>
> Thanks,
> Moritz

Wu, Hao Sept. 5, 2020, 1:39 p.m. UTC | #4

> On 9/4/20 5:23 PM, Moritz Fischer wrote:
> > Hi Russ,
> >
> > On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
> >> Create the Intel Security Manager class driver. The security
> >> manager provides interfaces to manage secure updates for the
> >> FPGA and BMC images that are stored in FLASH. The driver can
> >> also be used to update root entry hashes and to cancel code
> >> signing keys.
> >>
> >> This patch creates the class driver and provides sysfs
> >> interfaces for displaying root entry hashes, canceled code
> >> signing keys and flash counts.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> >> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > As for Reviewed-by tags I had seen on other patches in the series, I'd
> > prefer for that to happen on public mailing lists. If Hao reviewed
> > patches on some internal Intel list I won't know about it, so please
> > have him properly Ack/Reviewed-by tag things on a public mailing list.
> 
> Sure - I'll remove the Ack/Reviewed-by tags that were added internally
> before I submit the next version of the patchset (except where Hao
> re-adds them on the public list during this review cycle).

Yes, please remove it to avoid confusing. I haven't looked at the latest code yet.
Anyway, let's follow up this in public mailing list.

Thanks
Hao

Tom Rix Sept. 5, 2020, 7:09 p.m. UTC | #5

On 9/4/20 4:52 PM, Russ Weight wrote:
> Create the Intel Security Manager class driver. The security
> manager provides interfaces to manage secure updates for the
> FPGA and BMC images that are stored in FLASH. The driver can
> also be used to update root entry hashes and to cancel code
> signing keys.
>
> This patch creates the class driver and provides sysfs
> interfaces for displaying root entry hashes, canceled code
> signing keys and flash counts.
>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>  MAINTAINERS                                   |   8 +
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   3 +
>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>  6 files changed, 579 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> new file mode 100644
> index 000000000000..86f8992559bf
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -0,0 +1,75 @@
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Name of low level fpga security manager driver.
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the static
> +		region if one is programmed, else it returns the
> +		string: "hash not programmed".  This file is only
> +		visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the partial
> +		reconfiguration region if one is programmed, else it
> +		returns the string: "hash not programmed".  This file
> +		is only visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the BMC image
> +		if one is programmed, else it returns the string:
> +		"hash not programmed".  This file is only visible if the
> +		underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the static region. The standard bitmap
> +		list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the partial reconfiguration region. The
> +		standard bitmap list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the BMC.  The standard bitmap list format
> +		is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +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: "%d".
could this be %u ?
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns number of times the BMC image has been
> +		flashed.
> +		Format: "%d".
> diff --git a/MAINTAINERS b/MAINTAINERS
> index deaafb617361..4a2ebe6b120d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>  F:	drivers/fpga/
>  F:	include/linux/fpga/
>  
> +INTEL FPGA SECURITY MANAGER DRIVERS
> +M:	Russ Weight <russell.h.weight@intel.com>
> +L:	linux-fpga@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +F:	drivers/fpga/ifpga-sec-mgr.c
> +F:	include/linux/fpga/ifpga-sec-mgr.h
> +
>  FPU EMULATOR
>  M:	Bill Metzenthen <billm@melbpc.org.au>
>  S:	Maintained
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 88f64fbf55e3..97c0a6cc2ba7 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>  	  to configure the programmable logic(PL) through PS
>  	  on ZynqMP SoC.
>  
> +config IFPGA_SEC_MGR
> +	tristate "Intel Security Manager for FPGA"
> +        help
> +	  The Intel Security Manager class driver presents a common
> +	  user API for managing secure updates for Intel FPGA
> +	  devices, including flash images for the FPGA static
> +	  region and for the BMC. Select this option to enable
> +	  updates for secure FPGA devices.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index c69bfc931519..ec9fbacdedd8 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  
> +# Intel FPGA Security Manager Framework
> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> new file mode 100644
> index 000000000000..97bf80277ed2
> --- /dev/null
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Security Manager for FPGA
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +
> +#include <linux/fpga/ifpga-sec-mgr.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +
> +static DEFINE_IDA(ifpga_sec_mgr_ida);
> +static struct class *ifpga_sec_mgr_class;
> +
> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
> +				 sysfs_csk_hndlr_t get_csk,
> +				 sysfs_csk_nbits_t get_csk_nbits,

Param 2&3 can be accessed by imgr->iops so the signature

of this and similar functions should be reduced.

> +				 char *buf)
> +{
> +	unsigned long *csk_map = NULL;
> +	unsigned int nbits;
> +	int cnt, ret;
> +
> +	ret = get_csk_nbits(imgr);

Any access to a function pointer must check if the

the pointer is valid.

> +	if (ret < 0)
> +		return ret;
> +
> +	nbits = (unsigned int)ret;
> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
> +	if (!csk_map)
> +		return -ENOMEM;
> +
> +	ret = get_csk(imgr, csk_map, nbits);

The type of returned by get_csk_nbits and its use should

be consistent. likely this is 'int'

> +	if (ret)
> +		goto vfree_exit;
> +
> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);

simplify to

ret = ..


> +
> +vfree_exit:
> +	vfree(csk_map);
> +	return ret ? : cnt;
> +}
> +
> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
> +				    sysfs_reh_hndlr_t get_reh,
> +				    sysfs_reh_size_t get_reh_size,
> +				    char *buf)
> +{
> +	unsigned int size, i;
> +	int ret, cnt = 0;
> +	u8 *hash;
> +
> +	ret = get_reh_size(imgr);
> +	if (ret < 0)
> +		return ret;
> +	else if (!ret)
> +		return sprintf(buf, "hash not programmed\n");
> +
> +	size = (unsigned int)ret;
does size and i need to unsigned?
> +	hash = vmalloc(size);
> +	if (!hash)
> +		return -ENOMEM;
> +
> +	ret = get_reh(imgr, hash, size);
> +	if (ret)
> +		goto vfree_exit;

ret is 0 here

so simplify replacing cnt with ret.

> +
> +	cnt += sprintf(buf, "0x");
or change += to =, this is the first time sprintf is done.
> +	for (i = 0; i < size; i++)
> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
> +	cnt += sprintf(buf + cnt, "\n");
> +
> +vfree_exit:
> +	vfree(hash);
> +	return ret ? : cnt;

with simplification this should be

return ret;

> +}
> +
> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
Since this is used widely move closer to top of file.
> +
> +#define DEVICE_ATTR_SEC_CSK(_name) \
> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
> +					  struct device_attribute *attr, \
> +					  char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_canceled_csk(imgr, \
> +	       imgr->iops->_name##_canceled_csks, \
> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_canceled_csks)
> +
> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
> +				     struct device_attribute *attr, \
> +				     char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_root_entry_hash(imgr, \
> +	       imgr->iops->_name##_root_entry_hash, \
> +	       imgr->iops->_name##_reh_size, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
> +
> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
> +static ssize_t _name##_flash_count_show(struct device *dev, \
> +		    struct device_attribute *attr, char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
> +} \
> +static DEVICE_ATTR_RO(_name##_flash_count)
> +
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
> +DEVICE_ATTR_SEC_FLASH_CNT(user);
> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
> +DEVICE_ATTR_SEC_CSK(sr);
> +DEVICE_ATTR_SEC_CSK(pr);
> +DEVICE_ATTR_SEC_CSK(bmc);
> +
> +static struct attribute *sec_mgr_security_attrs[] = {
> +	&dev_attr_user_flash_count.attr,
> +	&dev_attr_bmc_flash_count.attr,
> +	&dev_attr_bmc_root_entry_hash.attr,
> +	&dev_attr_sr_root_entry_hash.attr,
> +	&dev_attr_pr_root_entry_hash.attr,
> +	&dev_attr_sr_canceled_csks.attr,
> +	&dev_attr_pr_canceled_csks.attr,
> +	&dev_attr_bmc_canceled_csks.attr,
> +	NULL,
> +};
> +
> +#define check_attr(attribute, _name) \
> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
> +
> +static umode_t sec_mgr_visible(struct kobject *kobj,
> +			       struct attribute *attr, int n)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
> +
> +	if (check_attr(attr, user_flash_count) ||
> +	    check_attr(attr, bmc_flash_count) ||
> +	    check_attr(attr, bmc_root_entry_hash) ||
> +	    check_attr(attr, sr_root_entry_hash) ||
> +	    check_attr(attr, pr_root_entry_hash) ||
> +	    check_attr(attr, sr_canceled_csks) ||
> +	    check_attr(attr, pr_canceled_csks) ||
> +	    check_attr(attr, bmc_canceled_csks))
> +		return attr->mode;
> +

This is all or nothing, shouldn't the interface

allow for null iop ?

> +	return 0;
> +}
> +
> +static struct attribute_group sec_mgr_security_attr_group = {
> +	.name = "security",
> +	.attrs = sec_mgr_security_attrs,
> +	.is_visible = sec_mgr_visible,
> +};
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	return sprintf(buf, "%s\n", imgr->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *sec_mgr_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_attr_group = {
> +	.attrs = sec_mgr_attrs,
> +};
> +
> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
> +	&sec_mgr_attr_group,
> +	&sec_mgr_security_attr_group,
> +	NULL,
> +};
> +
> +static bool check_sysfs_handler(struct device *dev,
> +				void *sysfs_handler, void *size_handler,
> +				const char *sysfs_handler_name,
> +				const char *size_handler_name)
> +{
> +	if (sysfs_handler) {

These two checks can be simplified to

if (!sysfs_handler || !size_handler)

> +		if (!size_handler) {
> +			dev_err(dev, "%s registered without %s\n",
> +				sysfs_handler_name, size_handler_name);
> +			return false;
> +		}
> +	} else if (size_handler) {
> +		dev_err(dev, "%s registered without %s\n",
> +			size_handler_name, sysfs_handler_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +#define check_reh_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
> +			    (_iops)->_name##_reh_size, \
> +			    __stringify(_name##_root_entry_hash), \
> +			    __stringify(_name##_reh_size))
> +
> +#define check_csk_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
> +			    (_iops)->_name##_canceled_csk_nbits, \
> +			    __stringify(_name##_canceled_csks), \
> +			    __stringify(_name##_canceled_csk_nbits))
> +
> +/**
> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
> + *
> + * @dev:  create ifpga security manager device from pdev
> + * @name: ifpga security manager name
> + * @iops: pointer to a structure of ifpga callback functions
> + * @priv: ifpga security manager private data
> + *
> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
> + */
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
> +{
> +	struct ifpga_sec_mgr *imgr;
> +	int id, ret;
> +
> +	if (!check_reh_handler(dev, iops, bmc) ||
> +	    !check_reh_handler(dev, iops, sr) ||
> +	    !check_reh_handler(dev, iops, pr) ||
> +	    !check_csk_handler(dev, iops, bmc) ||
> +	    !check_csk_handler(dev, iops, sr) ||
> +	    !check_csk_handler(dev, iops, pr)) {
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(dev, "Attempt to register with no name!\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
> +	if (!imgr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	imgr->name = name;
should name be dup-ed?
> +	imgr->priv = priv;
> +	imgr->iops = iops;
> +	mutex_init(&imgr->lock);
> +
> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto exit_free;
> +	}
> +
> +	imgr->dev.class = ifpga_sec_mgr_class;
> +	imgr->dev.parent = dev;
> +	imgr->dev.id = id;
> +
> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
> +	if (ret) {
> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
> +		goto exit_free;
> +	}
> +
> +	ret = device_register(&imgr->dev);
> +	if (ret) {
> +		put_device(&imgr->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return imgr;
> +
> +exit_free:
> +	kfree(dev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
> +
> +/**
> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
> + *
> + * @mgr: fpga manager struct
> + *
> + * This function is intended for use in a IFPGA security manager
> + * driver's remove() function.
> + */
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
> +{
> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
> +
> +	device_unregister(&imgr->dev);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> +
> +static void ifpga_sec_mgr_dev_release(struct device *dev)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	mutex_destroy(&imgr->lock);
> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
> +	kfree(imgr);
> +}
> +
> +static int __init ifpga_sec_mgr_class_init(void)
> +{
> +	pr_info("Intel FPGA Security Manager\n");
> +
> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
> +	if (IS_ERR(ifpga_sec_mgr_class))
> +		return PTR_ERR(ifpga_sec_mgr_class);
> +
> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
> +
> +	return 0;
> +}
> +
> +static void __exit ifpga_sec_mgr_class_exit(void)
> +{
> +	class_destroy(ifpga_sec_mgr_class);
> +	ida_destroy(&ifpga_sec_mgr_ida);
> +}
> +
> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +subsys_initcall(ifpga_sec_mgr_class_init);
> +module_exit(ifpga_sec_mgr_class_exit)
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
> new file mode 100644
> index 000000000000..e391b0c8f448
> --- /dev/null
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel FPGA Security Manager
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +#ifndef _LINUX_IFPGA_SEC_MGR_H
> +#define _LINUX_IFPGA_SEC_MGR_H
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct ifpga_sec_mgr;
> +
> +/**
> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the byte size of a
> + * root entry hash.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Byte count on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
> + *			       for root entry hashes
> + * @imgr:      pointer to security manager structure
> + * @hash:      pointer to an array of bytes in which to store the hash
> + * @size:      byte size of root entry hash
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return root entry hash data to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
> +				 unsigned int size);
> +
> +/**
> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
> + *			       for flash counts
> + * @imgr: pointer to security manager structure
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a flash count to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager
> + * Return: flash count or negative errno
> + */
> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
> + *				      a Code Signing Key cancellation vector
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the number of bits
> + * in a Code Signing Key cancellation vector.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Number of bits on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
> + *			       bit vector of canceled keys
> + *
> + * @imgr:    pointer to security manager structure
> + * @csk_map: pointer to a bitmap to contain cancellation key vector
> + * @nbits:   number of bits in CSK vector
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a bitmap of canceled keys to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> +				 unsigned long *csk_map, unsigned int nbits);
> +
> +/**
> + * struct ifpga_sec_mgr_ops - device specific operations
> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
> + *			    image flash count
> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
> + *			    image flash count
> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
> + *			    region root entry hash
> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration root entry hash
> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
> + *			    root entry hash
> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
> + *			    region canceled keys
> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration canceled keys
> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
> + *			    canceled keys
> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
> + * @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
> + */
> +struct ifpga_sec_mgr_ops {
> +	sysfs_cnt_hndlr_t user_flash_count;

These typedef's hide the function signatures and are

not consistent with how the other headers in include/linux/fpga

specify ops.

> +	sysfs_cnt_hndlr_t bmc_flash_count;
> +	sysfs_cnt_hndlr_t smbus_flash_count;
> +	sysfs_reh_hndlr_t sr_root_entry_hash;
> +	sysfs_reh_hndlr_t pr_root_entry_hash;
> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
> +	sysfs_csk_hndlr_t sr_canceled_csks;
> +	sysfs_csk_hndlr_t pr_canceled_csks;
> +	sysfs_csk_hndlr_t bmc_canceled_csks;
> +	sysfs_reh_size_t bmc_reh_size;
> +	sysfs_reh_size_t sr_reh_size;
> +	sysfs_reh_size_t pr_reh_size;
> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
> +};
> +
> +struct ifpga_sec_mgr {
> +	const char *name;
> +	struct device dev;
> +	const struct ifpga_sec_mgr_ops *iops;
> +	struct mutex lock;		/* protect data structure contents */

comment is redundant for a lock.

Tom

> +	void *priv;
> +};
> +
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
> +
> +#endif

Tom Rix Sept. 10, 2020, 9:51 p.m. UTC | #6

On 9/10/20 1:22 PM, Russ Weight wrote:
>     
>
> On 9/5/20 12:09 PM, Tom Rix wrote:
>
>    
>
>>       
>> On 9/4/20 4:52 PM, Russ Weight wrote:
>>       
>>>         
>>> Create the Intel Security Manager class driver. The security
>>> manager provides interfaces to manage secure updates for the
>>> FPGA and BMC images that are stored in FLASH. The driver can
>>> also be used to update root entry hashes and to cancel code
>>> signing keys.
>>>
>>> This patch creates the class driver and provides sysfs
>>> interfaces for displaying root entry hashes, canceled code
>>> signing keys and flash counts.
>>>
>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> ---
>>>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>>>  MAINTAINERS                                   |   8 +
>>>  drivers/fpga/Kconfig                          |   9 +
>>>  drivers/fpga/Makefile                         |   3 +
>>>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>>>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>>>  6 files changed, 579 insertions(+)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>>>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>>
>>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> new file mode 100644
>>> index 000000000000..86f8992559bf
>>> --- /dev/null
>>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> @@ -0,0 +1,75 @@
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Name of low level fpga security manager driver.
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns the root entry hash for the static
>>> +		region if one is programmed, else it returns the
>>> +		string: "hash not programmed".  This file is only
>>> +		visible if the underlying device supports it.
>>> +		Format: "0x%x".
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns the root entry hash for the partial
>>> +		reconfiguration region if one is programmed, else it
>>> +		returns the string: "hash not programmed".  This file
>>> +		is only visible if the underlying device supports it.
>>> +		Format: "0x%x".
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns the root entry hash for the BMC image
>>> +		if one is programmed, else it returns the string:
>>> +		"hash not programmed".  This file is only visible if the
>>> +		underlying device supports it.
>>> +		Format: "0x%x".
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns a list of indices for canceled code
>>> +		signing keys for the static region. The standard bitmap
>>> +		list format is used (e.g. "1,2-6,9").
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns a list of indices for canceled code
>>> +		signing keys for the partial reconfiguration region. The
>>> +		standard bitmap list format is used (e.g. "1,2-6,9").
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns a list of indices for canceled code
>>> +		signing keys for the BMC.  The standard bitmap list format
>>> +		is used (e.g. "1,2-6,9").
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +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: "%d".
>>>       
>>       
>> could this be %u ?
>>     
> Yes - I'll make the change for both of the flash update counts.
>
>>       
>>>         
>>> +
>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
>>> +Date:		Sep 2020
>>> +KernelVersion:  5.10
>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>> +Description:	Read only. Returns number of times the BMC image has been
>>> +		flashed.
>>> +		Format: "%d".
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index deaafb617361..4a2ebe6b120d 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>>>  F:	drivers/fpga/
>>>  F:	include/linux/fpga/
>>>  
>>> +INTEL FPGA SECURITY MANAGER DRIVERS
>>> +M:	Russ Weight <russell.h.weight@intel.com>
>>> +L:	linux-fpga@vger.kernel.org
>>> +S:	Maintained
>>> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>> +F:	drivers/fpga/ifpga-sec-mgr.c
>>> +F:	include/linux/fpga/ifpga-sec-mgr.h
>>> +
>>>  FPU EMULATOR
>>>  M:	Bill Metzenthen <billm@melbpc.org.au>
>>>  S:	Maintained
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 88f64fbf55e3..97c0a6cc2ba7 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>>>  	  to configure the programmable logic(PL) through PS
>>>  	  on ZynqMP SoC.
>>>  
>>> +config IFPGA_SEC_MGR
>>> +	tristate "Intel Security Manager for FPGA"
>>> +        help
>>> +	  The Intel Security Manager class driver presents a common
>>> +	  user API for managing secure updates for Intel FPGA
>>> +	  devices, including flash images for the FPGA static
>>> +	  region and for the BMC. Select this option to enable
>>> +	  updates for secure FPGA devices.
>>> +
>>>  endif # FPGA
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>> index c69bfc931519..ec9fbacdedd8 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>>>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>>  
>>> +# Intel FPGA Security Manager Framework
>>> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>> +
>>>  # FPGA Bridge Drivers
>>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>>> new file mode 100644
>>> index 000000000000..97bf80277ed2
>>> --- /dev/null
>>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>>> @@ -0,0 +1,339 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Intel Security Manager for FPGA
>>> + *
>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>> + */
>>> +
>>> +#include <linux/fpga/ifpga-sec-mgr.h>
>>> +#include <linux/idr.h>
>>> +#include <linux/module.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/vmalloc.h>
>>> +
>>> +static DEFINE_IDA(ifpga_sec_mgr_ida);
>>> +static struct class *ifpga_sec_mgr_class;
>>> +
>>> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
>>> +				 sysfs_csk_hndlr_t get_csk,
>>> +				 sysfs_csk_nbits_t get_csk_nbits,
>>>       
>>       
>> Param 2&3 can be accessed by imgr->iops so the signature
>>
>> of this and similar functions should be reduced.
>>     
> There are three different types of CSK vectors (sr, pr, and bmc).
> For each type, there are two function pointers. When calling
> show_canceled_csk, it is necessary to identify the type of CSK.
> While it is true that all three pairs of pointers are in imgr->iops,
> I think the easiest way to identify the CSK type is by passing in
> the appropriate pair of pointers.
ok.
>>       
>>       
>>>         
>>> +				 char *buf)
>>> +{
>>> +	unsigned long *csk_map = NULL;
>>> +	unsigned int nbits;
>>> +	int cnt, ret;
>>> +
>>> +	ret = get_csk_nbits(imgr);
>>>       
>>       
>> Any access to a function pointer must check if the
>>
>> the pointer is valid.
>>     
> The pointers are all validated when a device driver registers with the class
> driver. The corresponding CSK sysfs file will only be visible if the pointers
> have been validated. Is that sufficient, or should I add a redundant check
> here? I don't mind doing it if you think it is needed.
ok, fine as-is.
>
>>       
>>       
>>>         
>>> +	if (ret < 0)
>>> +		return ret;
>>> +
>>> +	nbits = (unsigned int)ret;
>>> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
>>> +	if (!csk_map)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = get_csk(imgr, csk_map, nbits);
>>>       
>>       
>> The type of returned by get_csk_nbits and its use should
>>
>> be consistent. likely this is 'int'
> The CSK data is returned using memory provided by the csk_map pointer parameter. The
> return value is an integer value and provides error/success status. Do you think
> something should be changed here?

Later i comment on how the error code duplicate standard error codes.

It would be good the standard codes could be used, part of that

is returning int.

>
>>       
>>>         
>>> +	if (ret)
>>> +		goto vfree_exit;
>>> +
>>> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
>>>       
>>       
>> simplify to
>>
>> ret = ..
> Sure - I will do that.
>
>>
>>       
>>>         
>>> +
>>> +vfree_exit:
>>> +	vfree(csk_map);
>>> +	return ret ? : cnt;
>>> +}
>>> +
>>> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
>>> +				    sysfs_reh_hndlr_t get_reh,
>>> +				    sysfs_reh_size_t get_reh_size,
>>> +				    char *buf)
>>> +{
>>> +	unsigned int size, i;
>>> +	int ret, cnt = 0;
>>> +	u8 *hash;
>>> +
>>> +	ret = get_reh_size(imgr);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	else if (!ret)
>>> +		return sprintf(buf, "hash not programmed\n");
>>> +
>>> +	size = (unsigned int)ret;
>>>       
>>       
>> does size and i need to unsigned?
> In the context of their usage here, they should never be negative. Sizes are
> typically unsigned, right? Would there be a reason to prefer int over unsigned?

My issue is specifically the typecast. It would be good

if the typecast could be removed.

>
>>       
>>>         
>>> +	hash = vmalloc(size);
>>> +	if (!hash)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = get_reh(imgr, hash, size);
>>> +	if (ret)
>>> +		goto vfree_exit;
>>>       
>>       
>> ret is 0 here
>>
>> so simplify replacing cnt with ret.
> Is the goal to have less variables? It seems like a trade-off with readability...
>
> I can make this change.

The next comment/change will makes this comment superfluous.

ignore it. 

>
>>       
>>>         
>>> +
>>> +	cnt += sprintf(buf, "0x");
>>>       
>>       
>> or change += to =, this is the first time sprintf is done.
> Sure - I 'll just do the assignment instead if initializing to zero.
>
>>       
>>>         
>>> +	for (i = 0; i < size; i++)
>>> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
>>> +	cnt += sprintf(buf + cnt, "\n");
>>> +
>>> +vfree_exit:
>>> +	vfree(hash);
>>> +	return ret ? : cnt;
>>>       
>>       
>> with simplification this should be
>>
>> return ret;
>>
>>       
>>>         
>>> +}
>>> +
>>> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
>>>       
>>       
>> Since this is used widely move closer to top of file.
> OK
>
>>       
>>>         
>>> +
>>> +#define DEVICE_ATTR_SEC_CSK(_name) \
>>> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
>>> +					  struct device_attribute *attr, \
>>> +					  char *buf) \
>>> +{ \
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>> +	return show_canceled_csk(imgr, \
>>> +	       imgr->iops->_name##_canceled_csks, \
>>> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
>>> +} \
>>> +static DEVICE_ATTR_RO(_name##_canceled_csks)
>>> +
>>> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
>>> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
>>> +				     struct device_attribute *attr, \
>>> +				     char *buf) \
>>> +{ \
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>> +	return show_root_entry_hash(imgr, \
>>> +	       imgr->iops->_name##_root_entry_hash, \
>>> +	       imgr->iops->_name##_reh_size, buf); \
>>> +} \
>>> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
>>> +
>>> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
>>> +static ssize_t _name##_flash_count_show(struct device *dev, \
>>> +		    struct device_attribute *attr, char *buf) \
>>> +{ \
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
>>> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
>>> +} \
>>> +static DEVICE_ATTR_RO(_name##_flash_count)
>>> +
>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
>>> +DEVICE_ATTR_SEC_FLASH_CNT(user);
>>> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
>>> +DEVICE_ATTR_SEC_CSK(sr);
>>> +DEVICE_ATTR_SEC_CSK(pr);
>>> +DEVICE_ATTR_SEC_CSK(bmc);
>>> +
>>> +static struct attribute *sec_mgr_security_attrs[] = {
>>> +	&dev_attr_user_flash_count.attr,
>>> +	&dev_attr_bmc_flash_count.attr,
>>> +	&dev_attr_bmc_root_entry_hash.attr,
>>> +	&dev_attr_sr_root_entry_hash.attr,
>>> +	&dev_attr_pr_root_entry_hash.attr,
>>> +	&dev_attr_sr_canceled_csks.attr,
>>> +	&dev_attr_pr_canceled_csks.attr,
>>> +	&dev_attr_bmc_canceled_csks.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +#define check_attr(attribute, _name) \
>>> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>>> +
>>> +static umode_t sec_mgr_visible(struct kobject *kobj,
>>> +			       struct attribute *attr, int n)
>>> +{
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
>>> +
>>> +	if (check_attr(attr, user_flash_count) ||
>>> +	    check_attr(attr, bmc_flash_count) ||
>>> +	    check_attr(attr, bmc_root_entry_hash) ||
>>> +	    check_attr(attr, sr_root_entry_hash) ||
>>> +	    check_attr(attr, pr_root_entry_hash) ||
>>> +	    check_attr(attr, sr_canceled_csks) ||
>>> +	    check_attr(attr, pr_canceled_csks) ||
>>> +	    check_attr(attr, bmc_canceled_csks))
>>> +		return attr->mode;
>>> +
>>>       
>>       
>> This is all or nothing, shouldn't the interface
>>
>> allow for null iop ?
> All or nothing? Each of the above attributes is optional and is enabled by
> providing a handler function. This is the "is_visible" op for sysfs and it
> ensures that only the enabled sysfs entries are displayed (any combination
> of enabled sysfs files).
>
> While these operations are all optional, there are some required operations in iops
> for the update process (in a later patch), so for the overall patchset iops
> cannot be NULL.
Ok. then add a comment.
>
>>       
>>>         
>>> +	return 0;
>>> +}
>>> +
>>> +static struct attribute_group sec_mgr_security_attr_group = {
>>> +	.name = "security",
>>> +	.attrs = sec_mgr_security_attrs,
>>> +	.is_visible = sec_mgr_visible,
>>> +};
>>> +
>>> +static ssize_t name_show(struct device *dev,
>>> +			 struct device_attribute *attr, char *buf)
>>> +{
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>> +
>>> +	return sprintf(buf, "%s\n", imgr->name);
>>> +}
>>> +static DEVICE_ATTR_RO(name);
>>> +
>>> +static struct attribute *sec_mgr_attrs[] = {
>>> +	&dev_attr_name.attr,
>>> +	NULL,
>>> +};
>>> +
>>> +static struct attribute_group sec_mgr_attr_group = {
>>> +	.attrs = sec_mgr_attrs,
>>> +};
>>> +
>>> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
>>> +	&sec_mgr_attr_group,
>>> +	&sec_mgr_security_attr_group,
>>> +	NULL,
>>> +};
>>> +
>>> +static bool check_sysfs_handler(struct device *dev,
>>> +				void *sysfs_handler, void *size_handler,
>>> +				const char *sysfs_handler_name,
>>> +				const char *size_handler_name)
>>> +{
>>> +	if (sysfs_handler) {
>>>       
>>       
>> These two checks can be simplified to
>>
>> if (!sysfs_handler || !size_handler)
> That would require changes to the "else if" condition that follows, which
> assumes that syfs_handler is null. The purpose of this block of code is
> to ensure that if the size_handler or the sysfs_handler is defined, that
> they are both defined. It is OK if they are both undefined.
>
> If you wanted to reduce the code, I think we could do something like this, but
> but I'm afraid it might be confusing and less readable.
>
> if (!!size_handler != !!sysfs_handler) {
> 	dev_err(dev, "%s and %s must both be registered to enable the sysfs file\n",
> 		size_handler_name, sysfs_handler_name);
> 	return false;
> }
>
> What do you think?

The more creative uses of '!' the better. ;)

This is fine as-is, maybe add a comment.

>
>>       
>>>         
>>> +		if (!size_handler) {
>>> +			dev_err(dev, "%s registered without %s\n",
>>> +				sysfs_handler_name, size_handler_name);
>>> +			return false;
>>> +		}
>>> +	} else if (size_handler) {
>>> +		dev_err(dev, "%s registered without %s\n",
>>> +			size_handler_name, sysfs_handler_name);
>>> +		return false;
>>> +	}
>>> +	return true;
>>> +}
>>> +
>>> +#define check_reh_handler(_dev, _iops, _name) \
>>> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
>>> +			    (_iops)->_name##_reh_size, \
>>> +			    __stringify(_name##_root_entry_hash), \
>>> +			    __stringify(_name##_reh_size))
>>> +
>>> +#define check_csk_handler(_dev, _iops, _name) \
>>> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
>>> +			    (_iops)->_name##_canceled_csk_nbits, \
>>> +			    __stringify(_name##_canceled_csks), \
>>> +			    __stringify(_name##_canceled_csk_nbits))
>>> +
>>> +/**
>>> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
>>> + *
>>> + * @dev:  create ifpga security manager device from pdev
>>> + * @name: ifpga security manager name
>>> + * @iops: pointer to a structure of ifpga callback functions
>>> + * @priv: ifpga security manager private data
>>> + *
>>> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
>>> + */
>>> +struct ifpga_sec_mgr *
>>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
>>> +{
>>> +	struct ifpga_sec_mgr *imgr;
>>> +	int id, ret;
>>> +
>>> +	if (!check_reh_handler(dev, iops, bmc) ||
>>> +	    !check_reh_handler(dev, iops, sr) ||
>>> +	    !check_reh_handler(dev, iops, pr) ||
>>> +	    !check_csk_handler(dev, iops, bmc) ||
>>> +	    !check_csk_handler(dev, iops, sr) ||
>>> +	    !check_csk_handler(dev, iops, pr)) {
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>> +	if (!name || !strlen(name)) {
>>> +		dev_err(dev, "Attempt to register with no name!\n");
>>> +		return ERR_PTR(-EINVAL);
>>> +	}
>>> +
>>> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
>>> +	if (!imgr)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	imgr->name = name;
>>>       
>>       
>> should name be dup-ed?
> The name is string constant that is provided by the underlying device driver
> (from the kernel space) so I don't think it would be necessary to dup the string.

If the underlying driver unloads, the name ptr will be invalid.

Will the driver unregister remove its use here ?

>
>>       
>>>         
>>> +	imgr->priv = priv;
>>> +	imgr->iops = iops;
>>> +	mutex_init(&imgr->lock);
>>> +
>>> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
>>> +	if (id < 0) {
>>> +		ret = id;
>>> +		goto exit_free;
>>> +	}
>>> +
>>> +	imgr->dev.class = ifpga_sec_mgr_class;
>>> +	imgr->dev.parent = dev;
>>> +	imgr->dev.id = id;
>>> +
>>> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
>>> +	if (ret) {
>>> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
>>> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
>>> +		goto exit_free;
>>> +	}
>>> +
>>> +	ret = device_register(&imgr->dev);
>>> +	if (ret) {
>>> +		put_device(&imgr->dev);
>>> +		return ERR_PTR(ret);
>>> +	}
>>> +
>>> +	return imgr;
>>> +
>>> +exit_free:
>>> +	kfree(dev);
>>> +	return ERR_PTR(ret);
>>> +}
>>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
>>> +
>>> +/**
>>> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
>>> + *
>>> + * @mgr: fpga manager struct
>>> + *
>>> + * This function is intended for use in a IFPGA security manager
>>> + * driver's remove() function.
>>> + */
>>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
>>> +{
>>> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>>> +
>>> +	device_unregister(&imgr->dev);
>>> +}
>>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
>>> +
>>> +static void ifpga_sec_mgr_dev_release(struct device *dev)
>>> +{
>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>> +
>>> +	mutex_destroy(&imgr->lock);
>>> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
>>> +	kfree(imgr);
>>> +}
>>> +
>>> +static int __init ifpga_sec_mgr_class_init(void)
>>> +{
>>> +	pr_info("Intel FPGA Security Manager\n");
>>> +
>>> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
>>> +	if (IS_ERR(ifpga_sec_mgr_class))
>>> +		return PTR_ERR(ifpga_sec_mgr_class);
>>> +
>>> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
>>> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void __exit ifpga_sec_mgr_class_exit(void)
>>> +{
>>> +	class_destroy(ifpga_sec_mgr_class);
>>> +	ida_destroy(&ifpga_sec_mgr_ida);
>>> +}
>>> +
>>> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +
>>> +subsys_initcall(ifpga_sec_mgr_class_init);
>>> +module_exit(ifpga_sec_mgr_class_exit)
>>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
>>> new file mode 100644
>>> index 000000000000..e391b0c8f448
>>> --- /dev/null
>>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>>> @@ -0,0 +1,145 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Header file for Intel FPGA Security Manager
>>> + *
>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>> + */
>>> +#ifndef _LINUX_IFPGA_SEC_MGR_H
>>> +#define _LINUX_IFPGA_SEC_MGR_H
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/types.h>
>>> +
>>> +struct ifpga_sec_mgr;
>>> +
>>> +/**
>>> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
>>> + *
>>> + * @imgr:      pointer to security manager structure
>>> + *
>>> + * This datatype is used to define a function that returns the byte size of a
>>> + * root entry hash.
>>> + *
>>> + * Context: No locking requirements are imposed by the security manager.
>>> + * Return:  Byte count on success, negative errno on failure
>>> + */
>>> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
>>> +
>>> +/**
>>> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
>>> + *			       for root entry hashes
>>> + * @imgr:      pointer to security manager structure
>>> + * @hash:      pointer to an array of bytes in which to store the hash
>>> + * @size:      byte size of root entry hash
>>> + *
>>> + * This datatype is used to define a sysfs file handler function to
>>> + * return root entry hash data to be displayed via sysfs.
>>> + *
>>> + * Context: No locking requirements are imposed by the security manager.
>>> + * Return:  0 on success, negative errno on failure
>>> + */
>>> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
>>> +				 unsigned int size);
>>> +
>>> +/**
>>> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
>>> + *			       for flash counts
>>> + * @imgr: pointer to security manager structure
>>> + *
>>> + * This datatype is used to define a sysfs file handler function to
>>> + * return a flash count to be displayed via sysfs.
>>> + *
>>> + * Context: No locking requirements are imposed by the security manager
>>> + * Return: flash count or negative errno
>>> + */
>>> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
>>> +
>>> +/**
>>> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
>>> + *				      a Code Signing Key cancellation vector
>>> + *
>>> + * @imgr:      pointer to security manager structure
>>> + *
>>> + * This datatype is used to define a function that returns the number of bits
>>> + * in a Code Signing Key cancellation vector.
>>> + *
>>> + * Context: No locking requirements are imposed by the security manager.
>>> + * Return:  Number of bits on success, negative errno on failure
>>> + */
>>> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
>>> +
>>> +/**
>>> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
>>> + *			       bit vector of canceled keys
>>> + *
>>> + * @imgr:    pointer to security manager structure
>>> + * @csk_map: pointer to a bitmap to contain cancellation key vector
>>> + * @nbits:   number of bits in CSK vector
>>> + *
>>> + * This datatype is used to define a sysfs file handler function to
>>> + * return a bitmap of canceled keys to be displayed via sysfs.
>>> + *
>>> + * Context: No locking requirements are imposed by the security manager.
>>> + * Return:  0 on success, negative errno on failure
>>> + */
>>> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>>> +				 unsigned long *csk_map, unsigned int nbits);
>>> +
>>> +/**
>>> + * struct ifpga_sec_mgr_ops - device specific operations
>>> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
>>> + *			    image flash count
>>> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
>>> + *			    image flash count
>>> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
>>> + *			    region root entry hash
>>> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
>>> + *			    reconfiguration root entry hash
>>> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
>>> + *			    root entry hash
>>> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
>>> + *			    region canceled keys
>>> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
>>> + *			    reconfiguration canceled keys
>>> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
>>> + *			    canceled keys
>>> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
>>> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
>>> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
>>> + * @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
>>> + */
>>> +struct ifpga_sec_mgr_ops {
>>> +	sysfs_cnt_hndlr_t user_flash_count;
>>>       
>>       
>> These typedef's hide the function signatures and are
>>
>> not consistent with how the other headers in include/linux/fpga
>>
>> specify ops.
> OK - I can remove the typedefs.
>
>>       
>>>         
>>> +	sysfs_cnt_hndlr_t bmc_flash_count;
>>> +	sysfs_cnt_hndlr_t smbus_flash_count;
>>> +	sysfs_reh_hndlr_t sr_root_entry_hash;
>>> +	sysfs_reh_hndlr_t pr_root_entry_hash;
>>> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
>>> +	sysfs_csk_hndlr_t sr_canceled_csks;
>>> +	sysfs_csk_hndlr_t pr_canceled_csks;
>>> +	sysfs_csk_hndlr_t bmc_canceled_csks;
>>> +	sysfs_reh_size_t bmc_reh_size;
>>> +	sysfs_reh_size_t sr_reh_size;
>>> +	sysfs_reh_size_t pr_reh_size;
>>> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
>>> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
>>> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
>>> +};
>>> +
>>> +struct ifpga_sec_mgr {
>>> +	const char *name;
>>> +	struct device dev;
>>> +	const struct ifpga_sec_mgr_ops *iops;
>>> +	struct mutex lock;		/* protect data structure contents */
>>>       
>>       
>> comment is redundant for a lock.
> "scripts/checkpatch.pl --strict" will complain about mutex definitions that do not
> have a comment. Are you asking for a more descriptive comment? Or are you saying
> that the comment is not required?

Fine as-is.

If checkpatch wants it, just do it.

Tom

>
>> Tom
>>
>>       
>>>         
>>> +	void *priv;
>>> +};
>>> +
>>> +struct ifpga_sec_mgr *
>>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
>>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
>>> +
>>> +#endif
>>>       
>>

Russ Weight Sept. 10, 2020, 11:05 p.m. UTC | #7

On 9/10/20 2:51 PM, Tom Rix wrote:
> On 9/10/20 1:22 PM, Russ Weight wrote:
>>     
>>
>> On 9/5/20 12:09 PM, Tom Rix wrote:
>>
>>    
>>
>>>       
>>> On 9/4/20 4:52 PM, Russ Weight wrote:
>>>       
>>>>         
>>>> Create the Intel Security Manager class driver. The security
>>>> manager provides interfaces to manage secure updates for the
>>>> FPGA and BMC images that are stored in FLASH. The driver can
>>>> also be used to update root entry hashes and to cancel code
>>>> signing keys.
>>>>
>>>> This patch creates the class driver and provides sysfs
>>>> interfaces for displaying root entry hashes, canceled code
>>>> signing keys and flash counts.
>>>>
>>>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>>> ---
>>>>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>>>>  MAINTAINERS                                   |   8 +
>>>>  drivers/fpga/Kconfig                          |   9 +
>>>>  drivers/fpga/Makefile                         |   3 +
>>>>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>>>>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>>>>  6 files changed, 579 insertions(+)
>>>>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>>>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>>>>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>>> new file mode 100644
>>>> index 000000000000..86f8992559bf
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>>> @@ -0,0 +1,75 @@
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Name of low level fpga security manager driver.
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns the root entry hash for the static
>>>> +		region if one is programmed, else it returns the
>>>> +		string: "hash not programmed".  This file is only
>>>> +		visible if the underlying device supports it.
>>>> +		Format: "0x%x".
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns the root entry hash for the partial
>>>> +		reconfiguration region if one is programmed, else it
>>>> +		returns the string: "hash not programmed".  This file
>>>> +		is only visible if the underlying device supports it.
>>>> +		Format: "0x%x".
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns the root entry hash for the BMC image
>>>> +		if one is programmed, else it returns the string:
>>>> +		"hash not programmed".  This file is only visible if the
>>>> +		underlying device supports it.
>>>> +		Format: "0x%x".
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns a list of indices for canceled code
>>>> +		signing keys for the static region. The standard bitmap
>>>> +		list format is used (e.g. "1,2-6,9").
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns a list of indices for canceled code
>>>> +		signing keys for the partial reconfiguration region. The
>>>> +		standard bitmap list format is used (e.g. "1,2-6,9").
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns a list of indices for canceled code
>>>> +		signing keys for the BMC.  The standard bitmap list format
>>>> +		is used (e.g. "1,2-6,9").
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +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: "%d".
>>>>       
>>>       
>>> could this be %u ?
>>>     
>> Yes - I'll make the change for both of the flash update counts.
>>
>>>       
>>>>         
>>>> +
>>>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
>>>> +Date:		Sep 2020
>>>> +KernelVersion:  5.10
>>>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>>>> +Description:	Read only. Returns number of times the BMC image has been
>>>> +		flashed.
>>>> +		Format: "%d".
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index deaafb617361..4a2ebe6b120d 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>>>>  F:	drivers/fpga/
>>>>  F:	include/linux/fpga/
>>>>  
>>>> +INTEL FPGA SECURITY MANAGER DRIVERS
>>>> +M:	Russ Weight <russell.h.weight@intel.com>
>>>> +L:	linux-fpga@vger.kernel.org
>>>> +S:	Maintained
>>>> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>>> +F:	drivers/fpga/ifpga-sec-mgr.c
>>>> +F:	include/linux/fpga/ifpga-sec-mgr.h
>>>> +
>>>>  FPU EMULATOR
>>>>  M:	Bill Metzenthen <billm@melbpc.org.au>
>>>>  S:	Maintained
>>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>>> index 88f64fbf55e3..97c0a6cc2ba7 100644
>>>> --- a/drivers/fpga/Kconfig
>>>> +++ b/drivers/fpga/Kconfig
>>>> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>>>>  	  to configure the programmable logic(PL) through PS
>>>>  	  on ZynqMP SoC.
>>>>  
>>>> +config IFPGA_SEC_MGR
>>>> +	tristate "Intel Security Manager for FPGA"
>>>> +        help
>>>> +	  The Intel Security Manager class driver presents a common
>>>> +	  user API for managing secure updates for Intel FPGA
>>>> +	  devices, including flash images for the FPGA static
>>>> +	  region and for the BMC. Select this option to enable
>>>> +	  updates for secure FPGA devices.
>>>> +
>>>>  endif # FPGA
>>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>>> index c69bfc931519..ec9fbacdedd8 100644
>>>> --- a/drivers/fpga/Makefile
>>>> +++ b/drivers/fpga/Makefile
>>>> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>>>>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>>>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>>>  
>>>> +# Intel FPGA Security Manager Framework
>>>> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>>>> +
>>>>  # FPGA Bridge Drivers
>>>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>>>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>>>> new file mode 100644
>>>> index 000000000000..97bf80277ed2
>>>> --- /dev/null
>>>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>>>> @@ -0,0 +1,339 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Intel Security Manager for FPGA
>>>> + *
>>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>>> + */
>>>> +
>>>> +#include <linux/fpga/ifpga-sec-mgr.h>
>>>> +#include <linux/idr.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/vmalloc.h>
>>>> +
>>>> +static DEFINE_IDA(ifpga_sec_mgr_ida);
>>>> +static struct class *ifpga_sec_mgr_class;
>>>> +
>>>> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
>>>> +				 sysfs_csk_hndlr_t get_csk,
>>>> +				 sysfs_csk_nbits_t get_csk_nbits,
>>>>       
>>>       
>>> Param 2&3 can be accessed by imgr->iops so the signature
>>>
>>> of this and similar functions should be reduced.
>>>     
>> There are three different types of CSK vectors (sr, pr, and bmc).
>> For each type, there are two function pointers. When calling
>> show_canceled_csk, it is necessary to identify the type of CSK.
>> While it is true that all three pairs of pointers are in imgr->iops,
>> I think the easiest way to identify the CSK type is by passing in
>> the appropriate pair of pointers.
> ok.
>>>       
>>>       
>>>>         
>>>> +				 char *buf)
>>>> +{
>>>> +	unsigned long *csk_map = NULL;
>>>> +	unsigned int nbits;
>>>> +	int cnt, ret;
>>>> +
>>>> +	ret = get_csk_nbits(imgr);
>>>>       
>>>       
>>> Any access to a function pointer must check if the
>>>
>>> the pointer is valid.
>>>     
>> The pointers are all validated when a device driver registers with the class
>> driver. The corresponding CSK sysfs file will only be visible if the pointers
>> have been validated. Is that sufficient, or should I add a redundant check
>> here? I don't mind doing it if you think it is needed.
> ok, fine as-is.
>>>       
>>>       
>>>>         
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	nbits = (unsigned int)ret;
>>>> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
>>>> +	if (!csk_map)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ret = get_csk(imgr, csk_map, nbits);
>>>>       
>>>       
>>> The type of returned by get_csk_nbits and its use should
>>>
>>> be consistent. likely this is 'int'
>> The CSK data is returned using memory provided by the csk_map pointer parameter. The
>> return value is an integer value and provides error/success status. Do you think
>> something should be changed here?
> Later i comment on how the error code duplicate standard error codes.
>
> It would be good the standard codes could be used, part of that
>
> is returning int.

get_csk_nbits() _does_ return int.
 

>>>       
>>>>         
>>>> +	if (ret)
>>>> +		goto vfree_exit;
>>>> +
>>>> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
>>>>       
>>>       
>>> simplify to
>>>
>>> ret = ..
>> Sure - I will do that.
>>
>>>       
>>>>         
>>>> +
>>>> +vfree_exit:
>>>> +	vfree(csk_map);
>>>> +	return ret ? : cnt;
>>>> +}
>>>> +
>>>> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
>>>> +				    sysfs_reh_hndlr_t get_reh,
>>>> +				    sysfs_reh_size_t get_reh_size,
>>>> +				    char *buf)
>>>> +{
>>>> +	unsigned int size, i;
>>>> +	int ret, cnt = 0;
>>>> +	u8 *hash;
>>>> +
>>>> +	ret = get_reh_size(imgr);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +	else if (!ret)
>>>> +		return sprintf(buf, "hash not programmed\n");
>>>> +
>>>> +	size = (unsigned int)ret;
>>>>       
>>>       
>>> does size and i need to unsigned?
>> In the context of their usage here, they should never be negative. Sizes are
>> typically unsigned, right? Would there be a reason to prefer int over unsigned?
> My issue is specifically the typecast. It would be good
>
> if the typecast could be removed.

OK - I can change the types so that the typecast can be dropped.

>
>>>       
>>>>         
>>>> +	hash = vmalloc(size);
>>>> +	if (!hash)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	ret = get_reh(imgr, hash, size);
>>>> +	if (ret)
>>>> +		goto vfree_exit;
>>>>       
>>>       
>>> ret is 0 here
>>>
>>> so simplify replacing cnt with ret.
>> Is the goal to have less variables? It seems like a trade-off with readability...
>>
>> I can make this change.
> The next comment/change will makes this comment superfluous.
>
> ignore it. 
>
>>>       
>>>>         
>>>> +
>>>> +	cnt += sprintf(buf, "0x");
>>>>       
>>>       
>>> or change += to =, this is the first time sprintf is done.
>> Sure - I 'll just do the assignment instead if initializing to zero.
>>
>>>       
>>>>         
>>>> +	for (i = 0; i < size; i++)
>>>> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
>>>> +	cnt += sprintf(buf + cnt, "\n");
>>>> +
>>>> +vfree_exit:
>>>> +	vfree(hash);
>>>> +	return ret ? : cnt;
>>>>       
>>>       
>>> with simplification this should be
>>>
>>> return ret;
>>>
>>>       
>>>>         
>>>> +}
>>>> +
>>>> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
>>>>       
>>>       
>>> Since this is used widely move closer to top of file.
>> OK
>>
>>>       
>>>>         
>>>> +
>>>> +#define DEVICE_ATTR_SEC_CSK(_name) \
>>>> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
>>>> +					  struct device_attribute *attr, \
>>>> +					  char *buf) \
>>>> +{ \
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>>> +	return show_canceled_csk(imgr, \
>>>> +	       imgr->iops->_name##_canceled_csks, \
>>>> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
>>>> +} \
>>>> +static DEVICE_ATTR_RO(_name##_canceled_csks)
>>>> +
>>>> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
>>>> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
>>>> +				     struct device_attribute *attr, \
>>>> +				     char *buf) \
>>>> +{ \
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>>> +	return show_root_entry_hash(imgr, \
>>>> +	       imgr->iops->_name##_root_entry_hash, \
>>>> +	       imgr->iops->_name##_reh_size, buf); \
>>>> +} \
>>>> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
>>>> +
>>>> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
>>>> +static ssize_t _name##_flash_count_show(struct device *dev, \
>>>> +		    struct device_attribute *attr, char *buf) \
>>>> +{ \
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>>>> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
>>>> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
>>>> +} \
>>>> +static DEVICE_ATTR_RO(_name##_flash_count)
>>>> +
>>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
>>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
>>>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
>>>> +DEVICE_ATTR_SEC_FLASH_CNT(user);
>>>> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
>>>> +DEVICE_ATTR_SEC_CSK(sr);
>>>> +DEVICE_ATTR_SEC_CSK(pr);
>>>> +DEVICE_ATTR_SEC_CSK(bmc);
>>>> +
>>>> +static struct attribute *sec_mgr_security_attrs[] = {
>>>> +	&dev_attr_user_flash_count.attr,
>>>> +	&dev_attr_bmc_flash_count.attr,
>>>> +	&dev_attr_bmc_root_entry_hash.attr,
>>>> +	&dev_attr_sr_root_entry_hash.attr,
>>>> +	&dev_attr_pr_root_entry_hash.attr,
>>>> +	&dev_attr_sr_canceled_csks.attr,
>>>> +	&dev_attr_pr_canceled_csks.attr,
>>>> +	&dev_attr_bmc_canceled_csks.attr,
>>>> +	NULL,
>>>> +};
>>>> +
>>>> +#define check_attr(attribute, _name) \
>>>> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>>>> +
>>>> +static umode_t sec_mgr_visible(struct kobject *kobj,
>>>> +			       struct attribute *attr, int n)
>>>> +{
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
>>>> +
>>>> +	if (check_attr(attr, user_flash_count) ||
>>>> +	    check_attr(attr, bmc_flash_count) ||
>>>> +	    check_attr(attr, bmc_root_entry_hash) ||
>>>> +	    check_attr(attr, sr_root_entry_hash) ||
>>>> +	    check_attr(attr, pr_root_entry_hash) ||
>>>> +	    check_attr(attr, sr_canceled_csks) ||
>>>> +	    check_attr(attr, pr_canceled_csks) ||
>>>> +	    check_attr(attr, bmc_canceled_csks))
>>>> +		return attr->mode;
>>>> +
>>>>       
>>>       
>>> This is all or nothing, shouldn't the interface
>>>
>>> allow for null iop ?
>> All or nothing? Each of the above attributes is optional and is enabled by
>> providing a handler function. This is the "is_visible" op for sysfs and it
>> ensures that only the enabled sysfs entries are displayed (any combination
>> of enabled sysfs files).
>>
>> While these operations are all optional, there are some required operations in iops
>> for the update process (in a later patch), so for the overall patchset iops
>> cannot be NULL.
> Ok. then add a comment.

Will do.

>>>       
>>>>         
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct attribute_group sec_mgr_security_attr_group = {
>>>> +	.name = "security",
>>>> +	.attrs = sec_mgr_security_attrs,
>>>> +	.is_visible = sec_mgr_visible,
>>>> +};
>>>> +
>>>> +static ssize_t name_show(struct device *dev,
>>>> +			 struct device_attribute *attr, char *buf)
>>>> +{
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>>> +
>>>> +	return sprintf(buf, "%s\n", imgr->name);
>>>> +}
>>>> +static DEVICE_ATTR_RO(name);
>>>> +
>>>> +static struct attribute *sec_mgr_attrs[] = {
>>>> +	&dev_attr_name.attr,
>>>> +	NULL,
>>>> +};
>>>> +
>>>> +static struct attribute_group sec_mgr_attr_group = {
>>>> +	.attrs = sec_mgr_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
>>>> +	&sec_mgr_attr_group,
>>>> +	&sec_mgr_security_attr_group,
>>>> +	NULL,
>>>> +};
>>>> +
>>>> +static bool check_sysfs_handler(struct device *dev,
>>>> +				void *sysfs_handler, void *size_handler,
>>>> +				const char *sysfs_handler_name,
>>>> +				const char *size_handler_name)
>>>> +{
>>>> +	if (sysfs_handler) {
>>>>       
>>>       
>>> These two checks can be simplified to
>>>
>>> if (!sysfs_handler || !size_handler)
>> That would require changes to the "else if" condition that follows, which
>> assumes that syfs_handler is null. The purpose of this block of code is
>> to ensure that if the size_handler or the sysfs_handler is defined, that
>> they are both defined. It is OK if they are both undefined.
>>
>> If you wanted to reduce the code, I think we could do something like this, but
>> but I'm afraid it might be confusing and less readable.
>>
>> if (!!size_handler != !!sysfs_handler) {
>> 	dev_err(dev, "%s and %s must both be registered to enable the sysfs file\n",
>> 		size_handler_name, sysfs_handler_name);
>> 	return false;
>> }
>>
>> What do you think?
> The more creative uses of '!' the better. ;)
>
> This is fine as-is, maybe add a comment.
>
>>>       
>>>>         
>>>> +		if (!size_handler) {
>>>> +			dev_err(dev, "%s registered without %s\n",
>>>> +				sysfs_handler_name, size_handler_name);
>>>> +			return false;
>>>> +		}
>>>> +	} else if (size_handler) {
>>>> +		dev_err(dev, "%s registered without %s\n",
>>>> +			size_handler_name, sysfs_handler_name);
>>>> +		return false;
>>>> +	}
>>>> +	return true;
>>>> +}
>>>> +
>>>> +#define check_reh_handler(_dev, _iops, _name) \
>>>> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
>>>> +			    (_iops)->_name##_reh_size, \
>>>> +			    __stringify(_name##_root_entry_hash), \
>>>> +			    __stringify(_name##_reh_size))
>>>> +
>>>> +#define check_csk_handler(_dev, _iops, _name) \
>>>> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
>>>> +			    (_iops)->_name##_canceled_csk_nbits, \
>>>> +			    __stringify(_name##_canceled_csks), \
>>>> +			    __stringify(_name##_canceled_csk_nbits))
>>>> +
>>>> +/**
>>>> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
>>>> + *
>>>> + * @dev:  create ifpga security manager device from pdev
>>>> + * @name: ifpga security manager name
>>>> + * @iops: pointer to a structure of ifpga callback functions
>>>> + * @priv: ifpga security manager private data
>>>> + *
>>>> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
>>>> + */
>>>> +struct ifpga_sec_mgr *
>>>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>>>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
>>>> +{
>>>> +	struct ifpga_sec_mgr *imgr;
>>>> +	int id, ret;
>>>> +
>>>> +	if (!check_reh_handler(dev, iops, bmc) ||
>>>> +	    !check_reh_handler(dev, iops, sr) ||
>>>> +	    !check_reh_handler(dev, iops, pr) ||
>>>> +	    !check_csk_handler(dev, iops, bmc) ||
>>>> +	    !check_csk_handler(dev, iops, sr) ||
>>>> +	    !check_csk_handler(dev, iops, pr)) {
>>>> +		return ERR_PTR(-EINVAL);
>>>> +	}
>>>> +
>>>> +	if (!name || !strlen(name)) {
>>>> +		dev_err(dev, "Attempt to register with no name!\n");
>>>> +		return ERR_PTR(-EINVAL);
>>>> +	}
>>>> +
>>>> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
>>>> +	if (!imgr)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	imgr->name = name;
>>>>       
>>>       
>>> should name be dup-ed?
>> The name is string constant that is provided by the underlying device driver
>> (from the kernel space) so I don't think it would be necessary to dup the string.
> If the underlying driver unloads, the name ptr will be invalid.
>
> Will the driver unregister remove its use here ?
Yes. If the underlying driver is unloaded, it will unregister with the class driver.
>
>>>       
>>>>         
>>>> +	imgr->priv = priv;
>>>> +	imgr->iops = iops;
>>>> +	mutex_init(&imgr->lock);
>>>> +
>>>> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
>>>> +	if (id < 0) {
>>>> +		ret = id;
>>>> +		goto exit_free;
>>>> +	}
>>>> +
>>>> +	imgr->dev.class = ifpga_sec_mgr_class;
>>>> +	imgr->dev.parent = dev;
>>>> +	imgr->dev.id = id;
>>>> +
>>>> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
>>>> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
>>>> +		goto exit_free;
>>>> +	}
>>>> +
>>>> +	ret = device_register(&imgr->dev);
>>>> +	if (ret) {
>>>> +		put_device(&imgr->dev);
>>>> +		return ERR_PTR(ret);
>>>> +	}
>>>> +
>>>> +	return imgr;
>>>> +
>>>> +exit_free:
>>>> +	kfree(dev);
>>>> +	return ERR_PTR(ret);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
>>>> +
>>>> +/**
>>>> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
>>>> + *
>>>> + * @mgr: fpga manager struct
>>>> + *
>>>> + * This function is intended for use in a IFPGA security manager
>>>> + * driver's remove() function.
>>>> + */
>>>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
>>>> +{
>>>> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>>>> +
>>>> +	device_unregister(&imgr->dev);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
>>>> +
>>>> +static void ifpga_sec_mgr_dev_release(struct device *dev)
>>>> +{
>>>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>>>> +
>>>> +	mutex_destroy(&imgr->lock);
>>>> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
>>>> +	kfree(imgr);
>>>> +}
>>>> +
>>>> +static int __init ifpga_sec_mgr_class_init(void)
>>>> +{
>>>> +	pr_info("Intel FPGA Security Manager\n");
>>>> +
>>>> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
>>>> +	if (IS_ERR(ifpga_sec_mgr_class))
>>>> +		return PTR_ERR(ifpga_sec_mgr_class);
>>>> +
>>>> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
>>>> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void __exit ifpga_sec_mgr_class_exit(void)
>>>> +{
>>>> +	class_destroy(ifpga_sec_mgr_class);
>>>> +	ida_destroy(&ifpga_sec_mgr_ida);
>>>> +}
>>>> +
>>>> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> +
>>>> +subsys_initcall(ifpga_sec_mgr_class_init);
>>>> +module_exit(ifpga_sec_mgr_class_exit)
>>>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
>>>> new file mode 100644
>>>> index 000000000000..e391b0c8f448
>>>> --- /dev/null
>>>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>>>> @@ -0,0 +1,145 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * Header file for Intel FPGA Security Manager
>>>> + *
>>>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>>>> + */
>>>> +#ifndef _LINUX_IFPGA_SEC_MGR_H
>>>> +#define _LINUX_IFPGA_SEC_MGR_H
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct ifpga_sec_mgr;
>>>> +
>>>> +/**
>>>> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
>>>> + *
>>>> + * @imgr:      pointer to security manager structure
>>>> + *
>>>> + * This datatype is used to define a function that returns the byte size of a
>>>> + * root entry hash.
>>>> + *
>>>> + * Context: No locking requirements are imposed by the security manager.
>>>> + * Return:  Byte count on success, negative errno on failure
>>>> + */
>>>> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
>>>> +
>>>> +/**
>>>> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
>>>> + *			       for root entry hashes
>>>> + * @imgr:      pointer to security manager structure
>>>> + * @hash:      pointer to an array of bytes in which to store the hash
>>>> + * @size:      byte size of root entry hash
>>>> + *
>>>> + * This datatype is used to define a sysfs file handler function to
>>>> + * return root entry hash data to be displayed via sysfs.
>>>> + *
>>>> + * Context: No locking requirements are imposed by the security manager.
>>>> + * Return:  0 on success, negative errno on failure
>>>> + */
>>>> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
>>>> +				 unsigned int size);
>>>> +
>>>> +/**
>>>> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
>>>> + *			       for flash counts
>>>> + * @imgr: pointer to security manager structure
>>>> + *
>>>> + * This datatype is used to define a sysfs file handler function to
>>>> + * return a flash count to be displayed via sysfs.
>>>> + *
>>>> + * Context: No locking requirements are imposed by the security manager
>>>> + * Return: flash count or negative errno
>>>> + */
>>>> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
>>>> +
>>>> +/**
>>>> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
>>>> + *				      a Code Signing Key cancellation vector
>>>> + *
>>>> + * @imgr:      pointer to security manager structure
>>>> + *
>>>> + * This datatype is used to define a function that returns the number of bits
>>>> + * in a Code Signing Key cancellation vector.
>>>> + *
>>>> + * Context: No locking requirements are imposed by the security manager.
>>>> + * Return:  Number of bits on success, negative errno on failure
>>>> + */
>>>> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
>>>> +
>>>> +/**
>>>> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
>>>> + *			       bit vector of canceled keys
>>>> + *
>>>> + * @imgr:    pointer to security manager structure
>>>> + * @csk_map: pointer to a bitmap to contain cancellation key vector
>>>> + * @nbits:   number of bits in CSK vector
>>>> + *
>>>> + * This datatype is used to define a sysfs file handler function to
>>>> + * return a bitmap of canceled keys to be displayed via sysfs.
>>>> + *
>>>> + * Context: No locking requirements are imposed by the security manager.
>>>> + * Return:  0 on success, negative errno on failure
>>>> + */
>>>> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>>>> +				 unsigned long *csk_map, unsigned int nbits);
>>>> +
>>>> +/**
>>>> + * struct ifpga_sec_mgr_ops - device specific operations
>>>> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
>>>> + *			    image flash count
>>>> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
>>>> + *			    image flash count
>>>> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
>>>> + *			    region root entry hash
>>>> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
>>>> + *			    reconfiguration root entry hash
>>>> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
>>>> + *			    root entry hash
>>>> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
>>>> + *			    region canceled keys
>>>> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
>>>> + *			    reconfiguration canceled keys
>>>> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
>>>> + *			    canceled keys
>>>> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
>>>> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
>>>> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
>>>> + * @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
>>>> + */
>>>> +struct ifpga_sec_mgr_ops {
>>>> +	sysfs_cnt_hndlr_t user_flash_count;
>>>>       
>>>       
>>> These typedef's hide the function signatures and are
>>>
>>> not consistent with how the other headers in include/linux/fpga
>>>
>>> specify ops.
>> OK - I can remove the typedefs.
>>
>>>       
>>>>         
>>>> +	sysfs_cnt_hndlr_t bmc_flash_count;
>>>> +	sysfs_cnt_hndlr_t smbus_flash_count;
>>>> +	sysfs_reh_hndlr_t sr_root_entry_hash;
>>>> +	sysfs_reh_hndlr_t pr_root_entry_hash;
>>>> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
>>>> +	sysfs_csk_hndlr_t sr_canceled_csks;
>>>> +	sysfs_csk_hndlr_t pr_canceled_csks;
>>>> +	sysfs_csk_hndlr_t bmc_canceled_csks;
>>>> +	sysfs_reh_size_t bmc_reh_size;
>>>> +	sysfs_reh_size_t sr_reh_size;
>>>> +	sysfs_reh_size_t pr_reh_size;
>>>> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
>>>> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
>>>> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
>>>> +};
>>>> +
>>>> +struct ifpga_sec_mgr {
>>>> +	const char *name;
>>>> +	struct device dev;
>>>> +	const struct ifpga_sec_mgr_ops *iops;
>>>> +	struct mutex lock;		/* protect data structure contents */
>>>>       
>>>       
>>> comment is redundant for a lock.
>> "scripts/checkpatch.pl --strict" will complain about mutex definitions that do not
>> have a comment. Are you asking for a more descriptive comment? Or are you saying
>> that the comment is not required?
> Fine as-is.
>
> If checkpatch wants it, just do it.
>
> Tom
>
>>> Tom
>>>
>>>       
>>>>         
>>>> +	void *priv;
>>>> +};
>>>> +
>>>> +struct ifpga_sec_mgr *
>>>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>>>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
>>>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
>>>> +
>>>> +#endif
>>>>       
>>>

Moritz Fischer Sept. 16, 2020, 8:16 p.m. UTC | #8

Hi Russ,

On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
> Create the Intel Security Manager class driver. The security
> manager provides interfaces to manage secure updates for the
> FPGA and BMC images that are stored in FLASH. The driver can
> also be used to update root entry hashes and to cancel code
> signing keys.
> 
> This patch creates the class driver and provides sysfs
> interfaces for displaying root entry hashes, canceled code
> signing keys and flash counts.
> 
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>  MAINTAINERS                                   |   8 +
>  drivers/fpga/Kconfig                          |   9 +
>  drivers/fpga/Makefile                         |   3 +
>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>  6 files changed, 579 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> new file mode 100644
> index 000000000000..86f8992559bf
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> @@ -0,0 +1,75 @@
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Name of low level fpga security manager driver.
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the static
> +		region if one is programmed, else it returns the
> +		string: "hash not programmed".  This file is only
> +		visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the partial
> +		reconfiguration region if one is programmed, else it
> +		returns the string: "hash not programmed".  This file
> +		is only visible if the underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns the root entry hash for the BMC image
> +		if one is programmed, else it returns the string:
> +		"hash not programmed".  This file is only visible if the
> +		underlying device supports it.
> +		Format: "0x%x".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the static region. The standard bitmap
> +		list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the partial reconfiguration region. The
> +		standard bitmap list format is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns a list of indices for canceled code
> +		signing keys for the BMC.  The standard bitmap list format
> +		is used (e.g. "1,2-6,9").
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +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: "%d".
> +
> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
> +Date:		Sep 2020
> +KernelVersion:  5.10
> +Contact:	Russ Weight <russell.h.weight@intel.com>
> +Description:	Read only. Returns number of times the BMC image has been
> +		flashed.
> +		Format: "%d".
> diff --git a/MAINTAINERS b/MAINTAINERS
> index deaafb617361..4a2ebe6b120d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>  F:	drivers/fpga/
>  F:	include/linux/fpga/
>  
> +INTEL FPGA SECURITY MANAGER DRIVERS
> +M:	Russ Weight <russell.h.weight@intel.com>
> +L:	linux-fpga@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> +F:	drivers/fpga/ifpga-sec-mgr.c
> +F:	include/linux/fpga/ifpga-sec-mgr.h

Actually, ignore my previous comment, feel free to leave this in.
> +
>  FPU EMULATOR
>  M:	Bill Metzenthen <billm@melbpc.org.au>
>  S:	Maintained
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 88f64fbf55e3..97c0a6cc2ba7 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>  	  to configure the programmable logic(PL) through PS
>  	  on ZynqMP SoC.
>  
> +config IFPGA_SEC_MGR
> +	tristate "Intel Security Manager for FPGA"
> +        help
> +	  The Intel Security Manager class driver presents a common
> +	  user API for managing secure updates for Intel FPGA
> +	  devices, including flash images for the FPGA static
> +	  region and for the BMC. Select this option to enable
> +	  updates for secure FPGA devices.
> +
>  endif # FPGA
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index c69bfc931519..ec9fbacdedd8 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>  
> +# Intel FPGA Security Manager Framework
> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
> +
>  # FPGA Bridge Drivers
>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> new file mode 100644
> index 000000000000..97bf80277ed2
> --- /dev/null
> +++ b/drivers/fpga/ifpga-sec-mgr.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel Security Manager for FPGA
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +
> +#include <linux/fpga/ifpga-sec-mgr.h>
> +#include <linux/idr.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.h>
> +
> +static DEFINE_IDA(ifpga_sec_mgr_ida);
> +static struct class *ifpga_sec_mgr_class;
> +
> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
> +				 sysfs_csk_hndlr_t get_csk,
> +				 sysfs_csk_nbits_t get_csk_nbits,
> +				 char *buf)
> +{
> +	unsigned long *csk_map = NULL;
> +	unsigned int nbits;
> +	int cnt, ret;
> +
> +	ret = get_csk_nbits(imgr);
> +	if (ret < 0)
> +		return ret;
> +
> +	nbits = (unsigned int)ret;
> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
> +	if (!csk_map)
> +		return -ENOMEM;
> +
> +	ret = get_csk(imgr, csk_map, nbits);
> +	if (ret)
> +		goto vfree_exit;
> +
> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
> +
> +vfree_exit:
> +	vfree(csk_map);
> +	return ret ? : cnt;
> +}
> +
> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
> +				    sysfs_reh_hndlr_t get_reh,
> +				    sysfs_reh_size_t get_reh_size,
> +				    char *buf)
> +{
> +	unsigned int size, i;
> +	int ret, cnt = 0;
> +	u8 *hash;
> +
> +	ret = get_reh_size(imgr);
> +	if (ret < 0)
> +		return ret;
> +	else if (!ret)
> +		return sprintf(buf, "hash not programmed\n");
> +
> +	size = (unsigned int)ret;
> +	hash = vmalloc(size);
> +	if (!hash)
> +		return -ENOMEM;
> +
> +	ret = get_reh(imgr, hash, size);
> +	if (ret)
> +		goto vfree_exit;
> +
> +	cnt += sprintf(buf, "0x");
> +	for (i = 0; i < size; i++)
> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
> +	cnt += sprintf(buf + cnt, "\n");
> +
> +vfree_exit:
> +	vfree(hash);
> +	return ret ? : cnt;
> +}
> +
> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
> +
> +#define DEVICE_ATTR_SEC_CSK(_name) \
> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
> +					  struct device_attribute *attr, \
> +					  char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_canceled_csk(imgr, \
> +	       imgr->iops->_name##_canceled_csks, \
> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_canceled_csks)
> +
> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
> +				     struct device_attribute *attr, \
> +				     char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	return show_root_entry_hash(imgr, \
> +	       imgr->iops->_name##_root_entry_hash, \
> +	       imgr->iops->_name##_reh_size, buf); \
> +} \
> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
> +
> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
> +static ssize_t _name##_flash_count_show(struct device *dev, \
> +		    struct device_attribute *attr, char *buf) \
> +{ \
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
> +} \
> +static DEVICE_ATTR_RO(_name##_flash_count)
> +
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
> +DEVICE_ATTR_SEC_FLASH_CNT(user);
> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
> +DEVICE_ATTR_SEC_CSK(sr);
> +DEVICE_ATTR_SEC_CSK(pr);
> +DEVICE_ATTR_SEC_CSK(bmc);
> +
> +static struct attribute *sec_mgr_security_attrs[] = {
> +	&dev_attr_user_flash_count.attr,
> +	&dev_attr_bmc_flash_count.attr,
> +	&dev_attr_bmc_root_entry_hash.attr,
> +	&dev_attr_sr_root_entry_hash.attr,
> +	&dev_attr_pr_root_entry_hash.attr,
> +	&dev_attr_sr_canceled_csks.attr,
> +	&dev_attr_pr_canceled_csks.attr,
> +	&dev_attr_bmc_canceled_csks.attr,
> +	NULL,
> +};
> +
> +#define check_attr(attribute, _name) \
> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
> +
> +static umode_t sec_mgr_visible(struct kobject *kobj,
> +			       struct attribute *attr, int n)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
> +
> +	if (check_attr(attr, user_flash_count) ||
> +	    check_attr(attr, bmc_flash_count) ||
> +	    check_attr(attr, bmc_root_entry_hash) ||
> +	    check_attr(attr, sr_root_entry_hash) ||
> +	    check_attr(attr, pr_root_entry_hash) ||
> +	    check_attr(attr, sr_canceled_csks) ||
> +	    check_attr(attr, pr_canceled_csks) ||
> +	    check_attr(attr, bmc_canceled_csks))
> +		return attr->mode;
> +
> +	return 0;
> +}
> +
> +static struct attribute_group sec_mgr_security_attr_group = {
> +	.name = "security",
> +	.attrs = sec_mgr_security_attrs,
> +	.is_visible = sec_mgr_visible,
> +};
> +
> +static ssize_t name_show(struct device *dev,
> +			 struct device_attribute *attr, char *buf)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	return sprintf(buf, "%s\n", imgr->name);
> +}
> +static DEVICE_ATTR_RO(name);
> +
> +static struct attribute *sec_mgr_attrs[] = {
> +	&dev_attr_name.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sec_mgr_attr_group = {
> +	.attrs = sec_mgr_attrs,
> +};
> +
> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
> +	&sec_mgr_attr_group,
> +	&sec_mgr_security_attr_group,
> +	NULL,
> +};
> +
> +static bool check_sysfs_handler(struct device *dev,
> +				void *sysfs_handler, void *size_handler,
> +				const char *sysfs_handler_name,
> +				const char *size_handler_name)
> +{
> +	if (sysfs_handler) {
> +		if (!size_handler) {
> +			dev_err(dev, "%s registered without %s\n",
> +				sysfs_handler_name, size_handler_name);
> +			return false;
> +		}
> +	} else if (size_handler) {
> +		dev_err(dev, "%s registered without %s\n",
> +			size_handler_name, sysfs_handler_name);
> +		return false;
> +	}
> +	return true;
> +}
> +
> +#define check_reh_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
> +			    (_iops)->_name##_reh_size, \
> +			    __stringify(_name##_root_entry_hash), \
> +			    __stringify(_name##_reh_size))
> +
> +#define check_csk_handler(_dev, _iops, _name) \
> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
> +			    (_iops)->_name##_canceled_csk_nbits, \
> +			    __stringify(_name##_canceled_csks), \
> +			    __stringify(_name##_canceled_csk_nbits))
> +

> +/**
> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
> + *
> + * @dev:  create ifpga security manager device from pdev
Create or register? Consider splitting this into a create and a
register function.

Also it might be nice to have a devm_ifpga_create_sec_mgr /
ifpga_register_sec_mgr set.

> + * @name: ifpga security manager name
> + * @iops: pointer to a structure of ifpga callback functions
> + * @priv: ifpga security manager private data
> + *
> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
> + */
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
> +{
> +	struct ifpga_sec_mgr *imgr;
> +	int id, ret;
> +
> +	if (!check_reh_handler(dev, iops, bmc) ||
> +	    !check_reh_handler(dev, iops, sr) ||
> +	    !check_reh_handler(dev, iops, pr) ||
> +	    !check_csk_handler(dev, iops, bmc) ||
> +	    !check_csk_handler(dev, iops, sr) ||
> +	    !check_csk_handler(dev, iops, pr)) {
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (!name || !strlen(name)) {
> +		dev_err(dev, "Attempt to register with no name!\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
> +	if (!imgr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	imgr->name = name;
> +	imgr->priv = priv;
> +	imgr->iops = iops;
> +	mutex_init(&imgr->lock);
> +
> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> +	if (id < 0) {
> +		ret = id;
> +		goto exit_free;
> +	}
> +
> +	imgr->dev.class = ifpga_sec_mgr_class;
> +	imgr->dev.parent = dev;
> +	imgr->dev.id = id;
> +
> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
> +	if (ret) {
> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
> +		goto exit_free;
> +	}

Consider
   	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
   	if (ret) {
		goto exit_device;
   	}
and above exit_free:
exit_device:
   		ida_simple_remove(&ifpga_sec_mgr_ida, id);

> +
> +	ret = device_register(&imgr->dev);
> +	if (ret) {
> +		put_device(&imgr->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	return imgr;
> +
> +exit_free:
> +	kfree(dev);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
> +
> +/**
> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
Nit: a or an IFPGA?
> + *
> + * @mgr: fpga manager struct
> + *
> + * This function is intended for use in a IFPGA security manager
Nit: a or an?
> + * driver's remove() function.
> + */
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
> +{
> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
> +
> +	device_unregister(&imgr->dev);
> +}
> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> +
> +static void ifpga_sec_mgr_dev_release(struct device *dev)
> +{
> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> +
> +	mutex_destroy(&imgr->lock);
> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
> +	kfree(imgr);
> +}
> +
> +static int __init ifpga_sec_mgr_class_init(void)
> +{
> +	pr_info("Intel FPGA Security Manager\n");
> +
> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
> +	if (IS_ERR(ifpga_sec_mgr_class))
> +		return PTR_ERR(ifpga_sec_mgr_class);
> +
> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
> +
> +	return 0;
> +}
> +
> +static void __exit ifpga_sec_mgr_class_exit(void)
> +{
> +	class_destroy(ifpga_sec_mgr_class);
> +	ida_destroy(&ifpga_sec_mgr_ida);
> +}
> +
> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
> +MODULE_LICENSE("GPL v2");
> +
> +subsys_initcall(ifpga_sec_mgr_class_init);
> +module_exit(ifpga_sec_mgr_class_exit)
> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
> new file mode 100644
> index 000000000000..e391b0c8f448
> --- /dev/null
> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> @@ -0,0 +1,145 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Header file for Intel FPGA Security Manager
> + *
> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> + */
> +#ifndef _LINUX_IFPGA_SEC_MGR_H
> +#define _LINUX_IFPGA_SEC_MGR_H
> +
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +struct ifpga_sec_mgr;
> +
> +/**
> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the byte size of a
> + * root entry hash.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Byte count on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
> + *			       for root entry hashes
> + * @imgr:      pointer to security manager structure
> + * @hash:      pointer to an array of bytes in which to store the hash
> + * @size:      byte size of root entry hash
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return root entry hash data to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
> +				 unsigned int size);
> +
> +/**
> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
> + *			       for flash counts
> + * @imgr: pointer to security manager structure
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a flash count to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager
> + * Return: flash count or negative errno
> + */
> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
> + *				      a Code Signing Key cancellation vector
> + *
> + * @imgr:      pointer to security manager structure
> + *
> + * This datatype is used to define a function that returns the number of bits
> + * in a Code Signing Key cancellation vector.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  Number of bits on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
> +
> +/**
> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
> + *			       bit vector of canceled keys
> + *
> + * @imgr:    pointer to security manager structure
> + * @csk_map: pointer to a bitmap to contain cancellation key vector
> + * @nbits:   number of bits in CSK vector
> + *
> + * This datatype is used to define a sysfs file handler function to
> + * return a bitmap of canceled keys to be displayed via sysfs.
> + *
> + * Context: No locking requirements are imposed by the security manager.
> + * Return:  0 on success, negative errno on failure
> + */
> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> +				 unsigned long *csk_map, unsigned int nbits);
> +
> +/**
> + * struct ifpga_sec_mgr_ops - device specific operations
> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
> + *			    image flash count
> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
> + *			    image flash count
> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
> + *			    region root entry hash
> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration root entry hash
> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
> + *			    root entry hash
> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
> + *			    region canceled keys
> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
> + *			    reconfiguration canceled keys
> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
> + *			    canceled keys
> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
> + * @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
> + */
> +struct ifpga_sec_mgr_ops {
> +	sysfs_cnt_hndlr_t user_flash_count;
> +	sysfs_cnt_hndlr_t bmc_flash_count;
> +	sysfs_cnt_hndlr_t smbus_flash_count;
> +	sysfs_reh_hndlr_t sr_root_entry_hash;
> +	sysfs_reh_hndlr_t pr_root_entry_hash;
> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
> +	sysfs_csk_hndlr_t sr_canceled_csks;
> +	sysfs_csk_hndlr_t pr_canceled_csks;
> +	sysfs_csk_hndlr_t bmc_canceled_csks;
> +	sysfs_reh_size_t bmc_reh_size;
> +	sysfs_reh_size_t sr_reh_size;
> +	sysfs_reh_size_t pr_reh_size;
> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
> +};

I agree with Tom's feedback, please don't use typedefs here.
> +
> +struct ifpga_sec_mgr {
> +	const char *name;
> +	struct device dev;
> +	const struct ifpga_sec_mgr_ops *iops;
> +	struct mutex lock;		/* protect data structure contents */
> +	void *priv;
> +};
> +
> +struct ifpga_sec_mgr *
> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
> +
> +#endif
> -- 
> 2.17.1
> 

Thanks,
Moritz

Russ Weight Sept. 30, 2020, 8:54 p.m. UTC | #9

On 9/16/20 1:16 PM, Moritz Fischer wrote:
> Hi Russ,
>
> On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
>> Create the Intel Security Manager class driver. The security
>> manager provides interfaces to manage secure updates for the
>> FPGA and BMC images that are stored in FLASH. The driver can
>> also be used to update root entry hashes and to cancel code
>> signing keys.
>>
>> This patch creates the class driver and provides sysfs
>> interfaces for displaying root entry hashes, canceled code
>> signing keys and flash counts.
>>
>> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>> ---
>>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
>>  MAINTAINERS                                   |   8 +
>>  drivers/fpga/Kconfig                          |   9 +
>>  drivers/fpga/Makefile                         |   3 +
>>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
>>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
>>  6 files changed, 579 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
>>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> new file mode 100644
>> index 000000000000..86f8992559bf
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> @@ -0,0 +1,75 @@
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Name of low level fpga security manager driver.
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the static
>> +		region if one is programmed, else it returns the
>> +		string: "hash not programmed".  This file is only
>> +		visible if the underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the partial
>> +		reconfiguration region if one is programmed, else it
>> +		returns the string: "hash not programmed".  This file
>> +		is only visible if the underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns the root entry hash for the BMC image
>> +		if one is programmed, else it returns the string:
>> +		"hash not programmed".  This file is only visible if the
>> +		underlying device supports it.
>> +		Format: "0x%x".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the static region. The standard bitmap
>> +		list format is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the partial reconfiguration region. The
>> +		standard bitmap list format is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns a list of indices for canceled code
>> +		signing keys for the BMC.  The standard bitmap list format
>> +		is used (e.g. "1,2-6,9").
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +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: "%d".
>> +
>> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
>> +Date:		Sep 2020
>> +KernelVersion:  5.10
>> +Contact:	Russ Weight <russell.h.weight@intel.com>
>> +Description:	Read only. Returns number of times the BMC image has been
>> +		flashed.
>> +		Format: "%d".
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index deaafb617361..4a2ebe6b120d 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
>>  F:	drivers/fpga/
>>  F:	include/linux/fpga/
>>  
>> +INTEL FPGA SECURITY MANAGER DRIVERS
>> +M:	Russ Weight <russell.h.weight@intel.com>
>> +L:	linux-fpga@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
>> +F:	drivers/fpga/ifpga-sec-mgr.c
>> +F:	include/linux/fpga/ifpga-sec-mgr.h
> Actually, ignore my previous comment, feel free to leave this in.
OK. I'll also add the intel-m10-bmc-secure.c file to the maintained files (in
the appropriate patch), as it is the first use of the security manager class
driver.
>> +
>>  FPU EMULATOR
>>  M:	Bill Metzenthen <billm@melbpc.org.au>
>>  S:	Maintained
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index 88f64fbf55e3..97c0a6cc2ba7 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
>>  	  to configure the programmable logic(PL) through PS
>>  	  on ZynqMP SoC.
>>  
>> +config IFPGA_SEC_MGR
>> +	tristate "Intel Security Manager for FPGA"
>> +        help
>> +	  The Intel Security Manager class driver presents a common
>> +	  user API for managing secure updates for Intel FPGA
>> +	  devices, including flash images for the FPGA static
>> +	  region and for the BMC. Select this option to enable
>> +	  updates for secure FPGA devices.
>> +
>>  endif # FPGA
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index c69bfc931519..ec9fbacdedd8 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
>>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
>>  
>> +# Intel FPGA Security Manager Framework
>> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
>> +
>>  # FPGA Bridge Drivers
>>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
>>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
>> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
>> new file mode 100644
>> index 000000000000..97bf80277ed2
>> --- /dev/null
>> +++ b/drivers/fpga/ifpga-sec-mgr.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Intel Security Manager for FPGA
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>> + */
>> +
>> +#include <linux/fpga/ifpga-sec-mgr.h>
>> +#include <linux/idr.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/vmalloc.h>
>> +
>> +static DEFINE_IDA(ifpga_sec_mgr_ida);
>> +static struct class *ifpga_sec_mgr_class;
>> +
>> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
>> +				 sysfs_csk_hndlr_t get_csk,
>> +				 sysfs_csk_nbits_t get_csk_nbits,
>> +				 char *buf)
>> +{
>> +	unsigned long *csk_map = NULL;
>> +	unsigned int nbits;
>> +	int cnt, ret;
>> +
>> +	ret = get_csk_nbits(imgr);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	nbits = (unsigned int)ret;
>> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
>> +	if (!csk_map)
>> +		return -ENOMEM;
>> +
>> +	ret = get_csk(imgr, csk_map, nbits);
>> +	if (ret)
>> +		goto vfree_exit;
>> +
>> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
>> +
>> +vfree_exit:
>> +	vfree(csk_map);
>> +	return ret ? : cnt;
>> +}
>> +
>> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
>> +				    sysfs_reh_hndlr_t get_reh,
>> +				    sysfs_reh_size_t get_reh_size,
>> +				    char *buf)
>> +{
>> +	unsigned int size, i;
>> +	int ret, cnt = 0;
>> +	u8 *hash;
>> +
>> +	ret = get_reh_size(imgr);
>> +	if (ret < 0)
>> +		return ret;
>> +	else if (!ret)
>> +		return sprintf(buf, "hash not programmed\n");
>> +
>> +	size = (unsigned int)ret;
>> +	hash = vmalloc(size);
>> +	if (!hash)
>> +		return -ENOMEM;
>> +
>> +	ret = get_reh(imgr, hash, size);
>> +	if (ret)
>> +		goto vfree_exit;
>> +
>> +	cnt += sprintf(buf, "0x");
>> +	for (i = 0; i < size; i++)
>> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
>> +	cnt += sprintf(buf + cnt, "\n");
>> +
>> +vfree_exit:
>> +	vfree(hash);
>> +	return ret ? : cnt;
>> +}
>> +
>> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
>> +
>> +#define DEVICE_ATTR_SEC_CSK(_name) \
>> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
>> +					  struct device_attribute *attr, \
>> +					  char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	return show_canceled_csk(imgr, \
>> +	       imgr->iops->_name##_canceled_csks, \
>> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_canceled_csks)
>> +
>> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
>> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
>> +				     struct device_attribute *attr, \
>> +				     char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	return show_root_entry_hash(imgr, \
>> +	       imgr->iops->_name##_root_entry_hash, \
>> +	       imgr->iops->_name##_reh_size, buf); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
>> +
>> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
>> +static ssize_t _name##_flash_count_show(struct device *dev, \
>> +		    struct device_attribute *attr, char *buf) \
>> +{ \
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
>> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
>> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
>> +} \
>> +static DEVICE_ATTR_RO(_name##_flash_count)
>> +
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
>> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
>> +DEVICE_ATTR_SEC_FLASH_CNT(user);
>> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
>> +DEVICE_ATTR_SEC_CSK(sr);
>> +DEVICE_ATTR_SEC_CSK(pr);
>> +DEVICE_ATTR_SEC_CSK(bmc);
>> +
>> +static struct attribute *sec_mgr_security_attrs[] = {
>> +	&dev_attr_user_flash_count.attr,
>> +	&dev_attr_bmc_flash_count.attr,
>> +	&dev_attr_bmc_root_entry_hash.attr,
>> +	&dev_attr_sr_root_entry_hash.attr,
>> +	&dev_attr_pr_root_entry_hash.attr,
>> +	&dev_attr_sr_canceled_csks.attr,
>> +	&dev_attr_pr_canceled_csks.attr,
>> +	&dev_attr_bmc_canceled_csks.attr,
>> +	NULL,
>> +};
>> +
>> +#define check_attr(attribute, _name) \
>> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
>> +
>> +static umode_t sec_mgr_visible(struct kobject *kobj,
>> +			       struct attribute *attr, int n)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
>> +
>> +	if (check_attr(attr, user_flash_count) ||
>> +	    check_attr(attr, bmc_flash_count) ||
>> +	    check_attr(attr, bmc_root_entry_hash) ||
>> +	    check_attr(attr, sr_root_entry_hash) ||
>> +	    check_attr(attr, pr_root_entry_hash) ||
>> +	    check_attr(attr, sr_canceled_csks) ||
>> +	    check_attr(attr, pr_canceled_csks) ||
>> +	    check_attr(attr, bmc_canceled_csks))
>> +		return attr->mode;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct attribute_group sec_mgr_security_attr_group = {
>> +	.name = "security",
>> +	.attrs = sec_mgr_security_attrs,
>> +	.is_visible = sec_mgr_visible,
>> +};
>> +
>> +static ssize_t name_show(struct device *dev,
>> +			 struct device_attribute *attr, char *buf)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +
>> +	return sprintf(buf, "%s\n", imgr->name);
>> +}
>> +static DEVICE_ATTR_RO(name);
>> +
>> +static struct attribute *sec_mgr_attrs[] = {
>> +	&dev_attr_name.attr,
>> +	NULL,
>> +};
>> +
>> +static struct attribute_group sec_mgr_attr_group = {
>> +	.attrs = sec_mgr_attrs,
>> +};
>> +
>> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
>> +	&sec_mgr_attr_group,
>> +	&sec_mgr_security_attr_group,
>> +	NULL,
>> +};
>> +
>> +static bool check_sysfs_handler(struct device *dev,
>> +				void *sysfs_handler, void *size_handler,
>> +				const char *sysfs_handler_name,
>> +				const char *size_handler_name)
>> +{
>> +	if (sysfs_handler) {
>> +		if (!size_handler) {
>> +			dev_err(dev, "%s registered without %s\n",
>> +				sysfs_handler_name, size_handler_name);
>> +			return false;
>> +		}
>> +	} else if (size_handler) {
>> +		dev_err(dev, "%s registered without %s\n",
>> +			size_handler_name, sysfs_handler_name);
>> +		return false;
>> +	}
>> +	return true;
>> +}
>> +
>> +#define check_reh_handler(_dev, _iops, _name) \
>> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
>> +			    (_iops)->_name##_reh_size, \
>> +			    __stringify(_name##_root_entry_hash), \
>> +			    __stringify(_name##_reh_size))
>> +
>> +#define check_csk_handler(_dev, _iops, _name) \
>> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
>> +			    (_iops)->_name##_canceled_csk_nbits, \
>> +			    __stringify(_name##_canceled_csks), \
>> +			    __stringify(_name##_canceled_csk_nbits))
>> +
>> +/**
>> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
>> + *
>> + * @dev:  create ifpga security manager device from pdev
> Create or register? Consider splitting this into a create and a
> register function.
I had originally coded it with both a create() and a register() function, but I
became convinced that there wasn't a use case for doing anything between the create()
and register() in the success case, or for doing anything other than failing out when
create() or register() fails - and that error handling would be simpler for the caller
if these functions were combined.

If I do split these up, then I believe the caller would be responsible for doing
"put_device(&imgr->dev)" if register() fails.

Do you think it is better, more standard to split these up? I can do that for the
next patch submission if you would like to see it that way.

>
> Also it might be nice to have a devm_ifpga_create_sec_mgr /
> ifpga_register_sec_mgr set.

I also considered a devm_ function, as is done in the fpga manager. As I understand it,
the purpose of the devm_ functions is to automatically clean up the memory resources
so that it does not have to be done explicitly in error handling. Currently, there
is a class dev_release() function ifpga_sec_mgr_dev_release() that frees the resources.
It seemed to me that it was essentially performing the same function as the managed
resource approach.

Looking at the core device function, device_release(), I can see that it calls first
the managed device functions, and later the class dev_release functions. So if I
provide both methods, then both methods would be called in the devm_ case, right?
So I would need to alter ifpga_sec_mgr_dev_release() to check/avoid a double free?

Am I understanding this correctly? I'm not sure I understand why the devm_ version of
a function might be preferred in this case, but I'm happy to code this up if you think
it is needed.
>> + * @name: ifpga security manager name
>> + * @iops: pointer to a structure of ifpga callback functions
>> + * @priv: ifpga security manager private data
>> + *
>> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
>> + */
>> +struct ifpga_sec_mgr *
>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
>> +{
>> +	struct ifpga_sec_mgr *imgr;
>> +	int id, ret;
>> +
>> +	if (!check_reh_handler(dev, iops, bmc) ||
>> +	    !check_reh_handler(dev, iops, sr) ||
>> +	    !check_reh_handler(dev, iops, pr) ||
>> +	    !check_csk_handler(dev, iops, bmc) ||
>> +	    !check_csk_handler(dev, iops, sr) ||
>> +	    !check_csk_handler(dev, iops, pr)) {
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	if (!name || !strlen(name)) {
>> +		dev_err(dev, "Attempt to register with no name!\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
>> +
>> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
>> +	if (!imgr)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	imgr->name = name;
>> +	imgr->priv = priv;
>> +	imgr->iops = iops;
>> +	mutex_init(&imgr->lock);
>> +
>> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
>> +	if (id < 0) {
>> +		ret = id;
>> +		goto exit_free;
>> +	}
>> +
>> +	imgr->dev.class = ifpga_sec_mgr_class;
>> +	imgr->dev.parent = dev;
>> +	imgr->dev.id = id;
>> +
>> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
>> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
>> +		goto exit_free;
>> +	}
> Consider
>    	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
>    	if (ret) {
> 		goto exit_device;
>    	}
> and above exit_free:
> exit_device:
>    		ida_simple_remove(&ifpga_sec_mgr_ida, id);
Yes - I'll change it this way.
>
>> +
>> +	ret = device_register(&imgr->dev);
>> +	if (ret) {
>> +		put_device(&imgr->dev);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>> +	return imgr;
>> +
>> +exit_free:
>> +	kfree(dev);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
>> +
>> +/**
>> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
> Nit: a or an IFPGA?
Yes - I'll change it. Actually, I think I'll change IFPGA to Intel FPGA as well.
>> + *
>> + * @mgr: fpga manager struct
>> + *
>> + * This function is intended for use in a IFPGA security manager
> Nit: a or an?
Same
>> + * driver's remove() function.
>> + */
>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
>> +{
>> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
>> +
>> +	device_unregister(&imgr->dev);
>> +}
>> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
>> +
>> +static void ifpga_sec_mgr_dev_release(struct device *dev)
>> +{
>> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
>> +
>> +	mutex_destroy(&imgr->lock);
>> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
>> +	kfree(imgr);
>> +}
>> +
>> +static int __init ifpga_sec_mgr_class_init(void)
>> +{
>> +	pr_info("Intel FPGA Security Manager\n");
>> +
>> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
>> +	if (IS_ERR(ifpga_sec_mgr_class))
>> +		return PTR_ERR(ifpga_sec_mgr_class);
>> +
>> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
>> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
>> +
>> +	return 0;
>> +}
>> +
>> +static void __exit ifpga_sec_mgr_class_exit(void)
>> +{
>> +	class_destroy(ifpga_sec_mgr_class);
>> +	ida_destroy(&ifpga_sec_mgr_ida);
>> +}
>> +
>> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
>> +MODULE_LICENSE("GPL v2");
>> +
>> +subsys_initcall(ifpga_sec_mgr_class_init);
>> +module_exit(ifpga_sec_mgr_class_exit)
>> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
>> new file mode 100644
>> index 000000000000..e391b0c8f448
>> --- /dev/null
>> +++ b/include/linux/fpga/ifpga-sec-mgr.h
>> @@ -0,0 +1,145 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Header file for Intel FPGA Security Manager
>> + *
>> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
>> + */
>> +#ifndef _LINUX_IFPGA_SEC_MGR_H
>> +#define _LINUX_IFPGA_SEC_MGR_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/mutex.h>
>> +#include <linux/types.h>
>> +
>> +struct ifpga_sec_mgr;
>> +
>> +/**
>> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
>> + *
>> + * @imgr:      pointer to security manager structure
>> + *
>> + * This datatype is used to define a function that returns the byte size of a
>> + * root entry hash.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  Byte count on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
>> + *			       for root entry hashes
>> + * @imgr:      pointer to security manager structure
>> + * @hash:      pointer to an array of bytes in which to store the hash
>> + * @size:      byte size of root entry hash
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return root entry hash data to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  0 on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
>> +				 unsigned int size);
>> +
>> +/**
>> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
>> + *			       for flash counts
>> + * @imgr: pointer to security manager structure
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return a flash count to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager
>> + * Return: flash count or negative errno
>> + */
>> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
>> + *				      a Code Signing Key cancellation vector
>> + *
>> + * @imgr:      pointer to security manager structure
>> + *
>> + * This datatype is used to define a function that returns the number of bits
>> + * in a Code Signing Key cancellation vector.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  Number of bits on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
>> +
>> +/**
>> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
>> + *			       bit vector of canceled keys
>> + *
>> + * @imgr:    pointer to security manager structure
>> + * @csk_map: pointer to a bitmap to contain cancellation key vector
>> + * @nbits:   number of bits in CSK vector
>> + *
>> + * This datatype is used to define a sysfs file handler function to
>> + * return a bitmap of canceled keys to be displayed via sysfs.
>> + *
>> + * Context: No locking requirements are imposed by the security manager.
>> + * Return:  0 on success, negative errno on failure
>> + */
>> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
>> +				 unsigned long *csk_map, unsigned int nbits);
>> +
>> +/**
>> + * struct ifpga_sec_mgr_ops - device specific operations
>> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
>> + *			    image flash count
>> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
>> + *			    image flash count
>> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
>> + *			    region root entry hash
>> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
>> + *			    reconfiguration root entry hash
>> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
>> + *			    root entry hash
>> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
>> + *			    region canceled keys
>> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
>> + *			    reconfiguration canceled keys
>> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
>> + *			    canceled keys
>> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
>> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
>> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
>> + * @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
>> + */
>> +struct ifpga_sec_mgr_ops {
>> +	sysfs_cnt_hndlr_t user_flash_count;
>> +	sysfs_cnt_hndlr_t bmc_flash_count;
>> +	sysfs_cnt_hndlr_t smbus_flash_count;
>> +	sysfs_reh_hndlr_t sr_root_entry_hash;
>> +	sysfs_reh_hndlr_t pr_root_entry_hash;
>> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
>> +	sysfs_csk_hndlr_t sr_canceled_csks;
>> +	sysfs_csk_hndlr_t pr_canceled_csks;
>> +	sysfs_csk_hndlr_t bmc_canceled_csks;
>> +	sysfs_reh_size_t bmc_reh_size;
>> +	sysfs_reh_size_t sr_reh_size;
>> +	sysfs_reh_size_t pr_reh_size;
>> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
>> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
>> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
>> +};
> I agree with Tom's feedback, please don't use typedefs here.
I'll remove them.

Thanks for the comments! I've been reworking the patches. I'll split up the class
driver and the Max10 BMC security engine into two patch sets for the next submission.

- Russ
>> +
>> +struct ifpga_sec_mgr {
>> +	const char *name;
>> +	struct device dev;
>> +	const struct ifpga_sec_mgr_ops *iops;
>> +	struct mutex lock;		/* protect data structure contents */
>> +	void *priv;
>> +};
>> +
>> +struct ifpga_sec_mgr *
>> +ifpga_sec_mgr_register(struct device *dev, const char *name,
>> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
>> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
>> +
>> +#endif
>> -- 
>> 2.17.1
>>
> Thanks,
> Moritz

Moritz Fischer Oct. 1, 2020, 12:31 a.m. UTC | #10

Hi Russ,

On Wed, Sep 30, 2020 at 01:54:50PM -0700, Russ Weight wrote:
> 
> 
> On 9/16/20 1:16 PM, Moritz Fischer wrote:
> > Hi Russ,
> >
> > On Fri, Sep 04, 2020 at 04:52:54PM -0700, Russ Weight wrote:
> >> Create the Intel Security Manager class driver. The security
> >> manager provides interfaces to manage secure updates for the
> >> FPGA and BMC images that are stored in FLASH. The driver can
> >> also be used to update root entry hashes and to cancel code
> >> signing keys.
> >>
> >> This patch creates the class driver and provides sysfs
> >> interfaces for displaying root entry hashes, canceled code
> >> signing keys and flash counts.
> >>
> >> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> >> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> >> ---
> >>  .../ABI/testing/sysfs-class-ifpga-sec-mgr     |  75 ++++
> >>  MAINTAINERS                                   |   8 +
> >>  drivers/fpga/Kconfig                          |   9 +
> >>  drivers/fpga/Makefile                         |   3 +
> >>  drivers/fpga/ifpga-sec-mgr.c                  | 339 ++++++++++++++++++
> >>  include/linux/fpga/ifpga-sec-mgr.h            | 145 ++++++++
> >>  6 files changed, 579 insertions(+)
> >>  create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> >>  create mode 100644 drivers/fpga/ifpga-sec-mgr.c
> >>  create mode 100644 include/linux/fpga/ifpga-sec-mgr.h
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> >> new file mode 100644
> >> index 000000000000..86f8992559bf
> >> --- /dev/null
> >> +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> >> @@ -0,0 +1,75 @@
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/name
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
Looking at the timing, those'll probably become 5.11

> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Name of low level fpga security manager driver.
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns the root entry hash for the static
> >> +		region if one is programmed, else it returns the
> >> +		string: "hash not programmed".  This file is only
> >> +		visible if the underlying device supports it.
> >> +		Format: "0x%x".
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_root_entry_hash
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns the root entry hash for the partial
> >> +		reconfiguration region if one is programmed, else it
> >> +		returns the string: "hash not programmed".  This file
> >> +		is only visible if the underlying device supports it.
> >> +		Format: "0x%x".
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_root_entry_hash
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns the root entry hash for the BMC image
> >> +		if one is programmed, else it returns the string:
> >> +		"hash not programmed".  This file is only visible if the
> >> +		underlying device supports it.
> >> +		Format: "0x%x".
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_canceled_csks
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns a list of indices for canceled code
> >> +		signing keys for the static region. The standard bitmap
> >> +		list format is used (e.g. "1,2-6,9").
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/pr_canceled_csks
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns a list of indices for canceled code
> >> +		signing keys for the partial reconfiguration region. The
> >> +		standard bitmap list format is used (e.g. "1,2-6,9").
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_canceled_csks
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns a list of indices for canceled code
> >> +		signing keys for the BMC.  The standard bitmap list format
> >> +		is used (e.g. "1,2-6,9").
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/user_flash_count
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +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: "%d".
> >> +
> >> +What: 		/sys/class/ifpga_sec_mgr/ifpga_secX/security/bmc_flash_count
> >> +Date:		Sep 2020
> >> +KernelVersion:  5.10
> >> +Contact:	Russ Weight <russell.h.weight@intel.com>
> >> +Description:	Read only. Returns number of times the BMC image has been
> >> +		flashed.
> >> +		Format: "%d".
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index deaafb617361..4a2ebe6b120d 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -6830,6 +6830,14 @@ F:	Documentation/fpga/
> >>  F:	drivers/fpga/
> >>  F:	include/linux/fpga/
> >>  
> >> +INTEL FPGA SECURITY MANAGER DRIVERS
> >> +M:	Russ Weight <russell.h.weight@intel.com>
> >> +L:	linux-fpga@vger.kernel.org
> >> +S:	Maintained
> >> +F:	Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr
> >> +F:	drivers/fpga/ifpga-sec-mgr.c
> >> +F:	include/linux/fpga/ifpga-sec-mgr.h
> > Actually, ignore my previous comment, feel free to leave this in.
> OK. I'll also add the intel-m10-bmc-secure.c file to the maintained files (in
> the appropriate patch), as it is the first use of the security manager class
> driver.

Works for me.
> >> +
> >>  FPU EMULATOR
> >>  M:	Bill Metzenthen <billm@melbpc.org.au>
> >>  S:	Maintained
> >> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> >> index 88f64fbf55e3..97c0a6cc2ba7 100644
> >> --- a/drivers/fpga/Kconfig
> >> +++ b/drivers/fpga/Kconfig
> >> @@ -235,4 +235,13 @@ config FPGA_MGR_ZYNQMP_FPGA
> >>  	  to configure the programmable logic(PL) through PS
> >>  	  on ZynqMP SoC.
> >>  
> >> +config IFPGA_SEC_MGR
> >> +	tristate "Intel Security Manager for FPGA"
> >> +        help
> >> +	  The Intel Security Manager class driver presents a common
> >> +	  user API for managing secure updates for Intel FPGA
> >> +	  devices, including flash images for the FPGA static
> >> +	  region and for the BMC. Select this option to enable
> >> +	  updates for secure FPGA devices.
> >> +
> >>  endif # FPGA
> >> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> >> index c69bfc931519..ec9fbacdedd8 100644
> >> --- a/drivers/fpga/Makefile
> >> +++ b/drivers/fpga/Makefile
> >> @@ -21,6 +21,9 @@ obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
> >>  obj-$(CONFIG_ALTERA_PR_IP_CORE)         += altera-pr-ip-core.o
> >>  obj-$(CONFIG_ALTERA_PR_IP_CORE_PLAT)    += altera-pr-ip-core-plat.o
> >>  
> >> +# Intel FPGA Security Manager Framework
> >> +obj-$(CONFIG_IFPGA_SEC_MGR)		+= ifpga-sec-mgr.o
> >> +
> >>  # FPGA Bridge Drivers
> >>  obj-$(CONFIG_FPGA_BRIDGE)		+= fpga-bridge.o
> >>  obj-$(CONFIG_SOCFPGA_FPGA_BRIDGE)	+= altera-hps2fpga.o altera-fpga2sdram.o
> >> diff --git a/drivers/fpga/ifpga-sec-mgr.c b/drivers/fpga/ifpga-sec-mgr.c
> >> new file mode 100644
> >> index 000000000000..97bf80277ed2
> >> --- /dev/null
> >> +++ b/drivers/fpga/ifpga-sec-mgr.c
> >> @@ -0,0 +1,339 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Intel Security Manager for FPGA
> >> + *
> >> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> >> + */
> >> +
> >> +#include <linux/fpga/ifpga-sec-mgr.h>
> >> +#include <linux/idr.h>
> >> +#include <linux/module.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/vmalloc.h>
> >> +
> >> +static DEFINE_IDA(ifpga_sec_mgr_ida);
> >> +static struct class *ifpga_sec_mgr_class;
> >> +
> >> +static ssize_t show_canceled_csk(struct ifpga_sec_mgr *imgr,
> >> +				 sysfs_csk_hndlr_t get_csk,
> >> +				 sysfs_csk_nbits_t get_csk_nbits,
> >> +				 char *buf)
> >> +{
> >> +	unsigned long *csk_map = NULL;
> >> +	unsigned int nbits;
> >> +	int cnt, ret;
> >> +
> >> +	ret = get_csk_nbits(imgr);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	nbits = (unsigned int)ret;
> >> +	csk_map = vmalloc(sizeof(unsigned long) * BITS_TO_LONGS(nbits));
> >> +	if (!csk_map)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = get_csk(imgr, csk_map, nbits);
> >> +	if (ret)
> >> +		goto vfree_exit;
> >> +
> >> +	cnt = bitmap_print_to_pagebuf(1, buf, csk_map, nbits);
> >> +
> >> +vfree_exit:
> >> +	vfree(csk_map);
> >> +	return ret ? : cnt;
> >> +}
> >> +
> >> +static ssize_t show_root_entry_hash(struct ifpga_sec_mgr *imgr,
> >> +				    sysfs_reh_hndlr_t get_reh,
> >> +				    sysfs_reh_size_t get_reh_size,
> >> +				    char *buf)
> >> +{
> >> +	unsigned int size, i;
> >> +	int ret, cnt = 0;
> >> +	u8 *hash;
> >> +
> >> +	ret = get_reh_size(imgr);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +	else if (!ret)
> >> +		return sprintf(buf, "hash not programmed\n");
> >> +
> >> +	size = (unsigned int)ret;
> >> +	hash = vmalloc(size);
> >> +	if (!hash)
> >> +		return -ENOMEM;
> >> +
> >> +	ret = get_reh(imgr, hash, size);
> >> +	if (ret)
> >> +		goto vfree_exit;
> >> +
> >> +	cnt += sprintf(buf, "0x");
> >> +	for (i = 0; i < size; i++)
> >> +		cnt += sprintf(buf + cnt, "%02x", hash[i]);
> >> +	cnt += sprintf(buf + cnt, "\n");
> >> +
> >> +vfree_exit:
> >> +	vfree(hash);
> >> +	return ret ? : cnt;
> >> +}
> >> +
> >> +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev)
> >> +
> >> +#define DEVICE_ATTR_SEC_CSK(_name) \
> >> +static ssize_t _name##_canceled_csks_show(struct device *dev, \
> >> +					  struct device_attribute *attr, \
> >> +					  char *buf) \
> >> +{ \
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> >> +	return show_canceled_csk(imgr, \
> >> +	       imgr->iops->_name##_canceled_csks, \
> >> +	       imgr->iops->_name##_canceled_csk_nbits, buf); \
> >> +} \
> >> +static DEVICE_ATTR_RO(_name##_canceled_csks)
> >> +
> >> +#define DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(_name) \
> >> +static ssize_t _name##_root_entry_hash_show(struct device *dev, \
> >> +				     struct device_attribute *attr, \
> >> +				     char *buf) \
> >> +{ \
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> >> +	return show_root_entry_hash(imgr, \
> >> +	       imgr->iops->_name##_root_entry_hash, \
> >> +	       imgr->iops->_name##_reh_size, buf); \
> >> +} \
> >> +static DEVICE_ATTR_RO(_name##_root_entry_hash)
> >> +
> >> +#define DEVICE_ATTR_SEC_FLASH_CNT(_name) \
> >> +static ssize_t _name##_flash_count_show(struct device *dev, \
> >> +		    struct device_attribute *attr, char *buf) \
> >> +{ \
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev); \
> >> +	int cnt = imgr->iops->_name##_flash_count(imgr); \
> >> +	return cnt < 0 ? cnt : sprintf(buf, "%d\n", cnt); \
> >> +} \
> >> +static DEVICE_ATTR_RO(_name##_flash_count)
> >> +
> >> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(sr);
> >> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(pr);
> >> +DEVICE_ATTR_SEC_ROOT_ENTRY_HASH(bmc);
> >> +DEVICE_ATTR_SEC_FLASH_CNT(user);
> >> +DEVICE_ATTR_SEC_FLASH_CNT(bmc);
> >> +DEVICE_ATTR_SEC_CSK(sr);
> >> +DEVICE_ATTR_SEC_CSK(pr);
> >> +DEVICE_ATTR_SEC_CSK(bmc);
> >> +
> >> +static struct attribute *sec_mgr_security_attrs[] = {
> >> +	&dev_attr_user_flash_count.attr,
> >> +	&dev_attr_bmc_flash_count.attr,
> >> +	&dev_attr_bmc_root_entry_hash.attr,
> >> +	&dev_attr_sr_root_entry_hash.attr,
> >> +	&dev_attr_pr_root_entry_hash.attr,
> >> +	&dev_attr_sr_canceled_csks.attr,
> >> +	&dev_attr_pr_canceled_csks.attr,
> >> +	&dev_attr_bmc_canceled_csks.attr,
> >> +	NULL,
> >> +};
> >> +
> >> +#define check_attr(attribute, _name) \
> >> +	((attribute) == &dev_attr_##_name.attr && imgr->iops->_name)
> >> +
> >> +static umode_t sec_mgr_visible(struct kobject *kobj,
> >> +			       struct attribute *attr, int n)
> >> +{
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(kobj_to_dev(kobj));
> >> +
> >> +	if (check_attr(attr, user_flash_count) ||
> >> +	    check_attr(attr, bmc_flash_count) ||
> >> +	    check_attr(attr, bmc_root_entry_hash) ||
> >> +	    check_attr(attr, sr_root_entry_hash) ||
> >> +	    check_attr(attr, pr_root_entry_hash) ||
> >> +	    check_attr(attr, sr_canceled_csks) ||
> >> +	    check_attr(attr, pr_canceled_csks) ||
> >> +	    check_attr(attr, bmc_canceled_csks))
> >> +		return attr->mode;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static struct attribute_group sec_mgr_security_attr_group = {
> >> +	.name = "security",
> >> +	.attrs = sec_mgr_security_attrs,
> >> +	.is_visible = sec_mgr_visible,
> >> +};
> >> +
> >> +static ssize_t name_show(struct device *dev,
> >> +			 struct device_attribute *attr, char *buf)
> >> +{
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> >> +
> >> +	return sprintf(buf, "%s\n", imgr->name);
> >> +}
> >> +static DEVICE_ATTR_RO(name);
> >> +
> >> +static struct attribute *sec_mgr_attrs[] = {
> >> +	&dev_attr_name.attr,
> >> +	NULL,
> >> +};
> >> +
> >> +static struct attribute_group sec_mgr_attr_group = {
> >> +	.attrs = sec_mgr_attrs,
> >> +};
> >> +
> >> +static const struct attribute_group *ifpga_sec_mgr_attr_groups[] = {
> >> +	&sec_mgr_attr_group,
> >> +	&sec_mgr_security_attr_group,
> >> +	NULL,
> >> +};
> >> +
> >> +static bool check_sysfs_handler(struct device *dev,
> >> +				void *sysfs_handler, void *size_handler,
> >> +				const char *sysfs_handler_name,
> >> +				const char *size_handler_name)
> >> +{
> >> +	if (sysfs_handler) {
> >> +		if (!size_handler) {
> >> +			dev_err(dev, "%s registered without %s\n",
> >> +				sysfs_handler_name, size_handler_name);
> >> +			return false;
> >> +		}
> >> +	} else if (size_handler) {
> >> +		dev_err(dev, "%s registered without %s\n",
> >> +			size_handler_name, sysfs_handler_name);
> >> +		return false;
> >> +	}
> >> +	return true;
> >> +}
> >> +
> >> +#define check_reh_handler(_dev, _iops, _name) \
> >> +	check_sysfs_handler(_dev, (_iops)->_name##_root_entry_hash, \
> >> +			    (_iops)->_name##_reh_size, \
> >> +			    __stringify(_name##_root_entry_hash), \
> >> +			    __stringify(_name##_reh_size))
> >> +
> >> +#define check_csk_handler(_dev, _iops, _name) \
> >> +	check_sysfs_handler(_dev, (_iops)->_name##_canceled_csks, \
> >> +			    (_iops)->_name##_canceled_csk_nbits, \
> >> +			    __stringify(_name##_canceled_csks), \
> >> +			    __stringify(_name##_canceled_csk_nbits))
> >> +
> >> +/**
> >> + * ifpga_sec_mgr_register - register an IFPGA security manager struct
> >> + *
> >> + * @dev:  create ifpga security manager device from pdev
> > Create or register? Consider splitting this into a create and a
> > register function.
> I had originally coded it with both a create() and a register() function, but I
> became convinced that there wasn't a use case for doing anything between the create()
> and register() in the success case, or for doing anything other than failing out when
> create() or register() fails - and that error handling would be simpler for the caller
> if these functions were combined.
> 
> If I do split these up, then I believe the caller would be responsible for doing
> "put_device(&imgr->dev)" if register() fails.

You can look at the fpga_mgr/bridge/region for a working pattern. I
think devres and the release functions as a result take care of it.

> Do you think it is better, more standard to split these up? I can do that for the
> next patch submission if you would like to see it that way.

I think providing the devm_ managed APIs is nicer, and makes it easier
for the consumer of the API to do the right thing.
> 
> >
> > Also it might be nice to have a devm_ifpga_create_sec_mgr /
> > ifpga_register_sec_mgr set.
> 
> I also considered a devm_ function, as is done in the fpga manager. As I understand it,
> the purpose of the devm_ functions is to automatically clean up the memory resources
> so that it does not have to be done explicitly in error handling. Currently, there
> is a class dev_release() function ifpga_sec_mgr_dev_release() that frees the resources.
> It seemed to me that it was essentially performing the same function as the managed
> resource approach.

I'd prefer if we could keep the pattern the same within drivers/fpga,
that makes refactoring easier if we decide to do so.

I think the networking subsystem uses a similar pattern with alloc() and
register() split up.

The SPI subsystem uses a variation of it.

> Looking at the core device function, device_release(), I can see that it calls first
> the managed device functions, and later the class dev_release functions. So if I
> provide both methods, then both methods would be called in the devm_ case, right?
> So I would need to alter ifpga_sec_mgr_dev_release() to check/avoid a double free?

If you look at FPGA manager/bridge code the class release ones are empty.
> 
> Am I understanding this correctly? I'm not sure I understand why the devm_ version of
> a function might be preferred in this case, but I'm happy to code this up if you think
> it is needed.

You might want to have finer control over when you unregister vs free.
The current pattern provides that.

I think we had originally discussed making it a devm_register() API that
would do all the cleanup and unregister in one go, but I don't recall
why we didn't go that far. I'll try to remember :)

> >> + * @name: ifpga security manager name
> >> + * @iops: pointer to a structure of ifpga callback functions
> >> + * @priv: ifpga security manager private data
> >> + *
> >> + * Returns &struct ifpga_sec_mgr pointer on success, or ERR_PTR() on error.
> >> + */
> >> +struct ifpga_sec_mgr *
> >> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> >> +		       const struct ifpga_sec_mgr_ops *iops, void *priv)
> >> +{
> >> +	struct ifpga_sec_mgr *imgr;
> >> +	int id, ret;
> >> +
> >> +	if (!check_reh_handler(dev, iops, bmc) ||
> >> +	    !check_reh_handler(dev, iops, sr) ||
> >> +	    !check_reh_handler(dev, iops, pr) ||
> >> +	    !check_csk_handler(dev, iops, bmc) ||
> >> +	    !check_csk_handler(dev, iops, sr) ||
> >> +	    !check_csk_handler(dev, iops, pr)) {
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> >> +
> >> +	if (!name || !strlen(name)) {
> >> +		dev_err(dev, "Attempt to register with no name!\n");
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> >> +
> >> +	imgr = kzalloc(sizeof(*imgr), GFP_KERNEL);
> >> +	if (!imgr)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	imgr->name = name;
> >> +	imgr->priv = priv;
> >> +	imgr->iops = iops;
> >> +	mutex_init(&imgr->lock);
> >> +
> >> +	id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL);
> >> +	if (id < 0) {
> >> +		ret = id;
> >> +		goto exit_free;
> >> +	}
> >> +
> >> +	imgr->dev.class = ifpga_sec_mgr_class;
> >> +	imgr->dev.parent = dev;
> >> +	imgr->dev.id = id;
> >> +
> >> +	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
> >> +	if (ret) {
> >> +		dev_err(dev, "Failed to set device name: ifpga_sec%d\n", id);
> >> +		ida_simple_remove(&ifpga_sec_mgr_ida, id);
> >> +		goto exit_free;
> >> +	}
> > Consider
> >    	ret = dev_set_name(&imgr->dev, "ifpga_sec%d", id);
> >    	if (ret) {
> > 		goto exit_device;
> >    	}
> > and above exit_free:
> > exit_device:
> >    		ida_simple_remove(&ifpga_sec_mgr_ida, id);
> Yes - I'll change it this way.
> >
> >> +
> >> +	ret = device_register(&imgr->dev);
> >> +	if (ret) {
> >> +		put_device(&imgr->dev);
> >> +		return ERR_PTR(ret);
> >> +	}
> >> +
> >> +	return imgr;
> >> +
> >> +exit_free:
> >> +	kfree(dev);
> >> +	return ERR_PTR(ret);
> >> +}
> >> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register);
> >> +
> >> +/**
> >> + * ifpga_sec_mgr_unregister - unregister a IFPGA security manager
> > Nit: a or an IFPGA?
> Yes - I'll change it. Actually, I think I'll change IFPGA to Intel FPGA as well.
> >> + *
> >> + * @mgr: fpga manager struct
> >> + *
> >> + * This function is intended for use in a IFPGA security manager
> > Nit: a or an?
> Same
> >> + * driver's remove() function.
> >> + */
> >> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr)
> >> +{
> >> +	dev_info(&imgr->dev, "%s %s\n", __func__, imgr->name);
> >> +
> >> +	device_unregister(&imgr->dev);
> >> +}
> >> +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_unregister);
> >> +
> >> +static void ifpga_sec_mgr_dev_release(struct device *dev)
> >> +{
> >> +	struct ifpga_sec_mgr *imgr = to_sec_mgr(dev);
> >> +
> >> +	mutex_destroy(&imgr->lock);
> >> +	ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id);
> >> +	kfree(imgr);
> >> +}
> >> +
> >> +static int __init ifpga_sec_mgr_class_init(void)
> >> +{
> >> +	pr_info("Intel FPGA Security Manager\n");
> >> +
> >> +	ifpga_sec_mgr_class = class_create(THIS_MODULE, "ifpga_sec_mgr");
> >> +	if (IS_ERR(ifpga_sec_mgr_class))
> >> +		return PTR_ERR(ifpga_sec_mgr_class);
> >> +
> >> +	ifpga_sec_mgr_class->dev_groups = ifpga_sec_mgr_attr_groups;
> >> +	ifpga_sec_mgr_class->dev_release = ifpga_sec_mgr_dev_release;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static void __exit ifpga_sec_mgr_class_exit(void)
> >> +{
> >> +	class_destroy(ifpga_sec_mgr_class);
> >> +	ida_destroy(&ifpga_sec_mgr_ida);
> >> +}
> >> +
> >> +MODULE_DESCRIPTION("Intel FPGA Security Manager Driver");
> >> +MODULE_LICENSE("GPL v2");
> >> +
> >> +subsys_initcall(ifpga_sec_mgr_class_init);
> >> +module_exit(ifpga_sec_mgr_class_exit)
> >> diff --git a/include/linux/fpga/ifpga-sec-mgr.h b/include/linux/fpga/ifpga-sec-mgr.h
> >> new file mode 100644
> >> index 000000000000..e391b0c8f448
> >> --- /dev/null
> >> +++ b/include/linux/fpga/ifpga-sec-mgr.h
> >> @@ -0,0 +1,145 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/*
> >> + * Header file for Intel FPGA Security Manager
> >> + *
> >> + * Copyright (C) 2019-2020 Intel Corporation, Inc.
> >> + */
> >> +#ifndef _LINUX_IFPGA_SEC_MGR_H
> >> +#define _LINUX_IFPGA_SEC_MGR_H
> >> +
> >> +#include <linux/device.h>
> >> +#include <linux/mutex.h>
> >> +#include <linux/types.h>
> >> +
> >> +struct ifpga_sec_mgr;
> >> +
> >> +/**
> >> + * typedef sysfs_reh_size_t - Function to return byte size of root entry hash
> >> + *
> >> + * @imgr:      pointer to security manager structure
> >> + *
> >> + * This datatype is used to define a function that returns the byte size of a
> >> + * root entry hash.
> >> + *
> >> + * Context: No locking requirements are imposed by the security manager.
> >> + * Return:  Byte count on success, negative errno on failure
> >> + */
> >> +typedef int (*sysfs_reh_size_t)(struct ifpga_sec_mgr *imgr);

As I had mentioned can we keep those as function pointers?
> >> +
> >> +/**
> >> + * typedef sysfs_reh_hndlr_t - Function pointer to sysfs file handler
> >> + *			       for root entry hashes
> >> + * @imgr:      pointer to security manager structure
> >> + * @hash:      pointer to an array of bytes in which to store the hash
> >> + * @size:      byte size of root entry hash
> >> + *
> >> + * This datatype is used to define a sysfs file handler function to
> >> + * return root entry hash data to be displayed via sysfs.
> >> + *
> >> + * Context: No locking requirements are imposed by the security manager.
> >> + * Return:  0 on success, negative errno on failure
> >> + */
> >> +typedef int (*sysfs_reh_hndlr_t)(struct ifpga_sec_mgr *imgr, u8 *hash,
> >> +				 unsigned int size);
> >> +
> >> +/**
> >> + * typedef sysfs_cnt_hndlr_t - Function pointer to sysfs file handler
> >> + *			       for flash counts
> >> + * @imgr: pointer to security manager structure
> >> + *
> >> + * This datatype is used to define a sysfs file handler function to
> >> + * return a flash count to be displayed via sysfs.
> >> + *
> >> + * Context: No locking requirements are imposed by the security manager
> >> + * Return: flash count or negative errno
> >> + */
> >> +typedef int (*sysfs_cnt_hndlr_t)(struct ifpga_sec_mgr *imgr);
> >> +
> >> +/**
> >> + * typedef sysfs_csk_nbits_t - Function to return the number of bits in
> >> + *				      a Code Signing Key cancellation vector
> >> + *
> >> + * @imgr:      pointer to security manager structure
> >> + *
> >> + * This datatype is used to define a function that returns the number of bits
> >> + * in a Code Signing Key cancellation vector.
> >> + *
> >> + * Context: No locking requirements are imposed by the security manager.
> >> + * Return:  Number of bits on success, negative errno on failure
> >> + */
> >> +typedef int (*sysfs_csk_nbits_t)(struct ifpga_sec_mgr *imgr);
> >> +
> >> +/**
> >> + * typedef sysfs_csk_hndlr_t - Function pointer to sysfs file handler
> >> + *			       bit vector of canceled keys
> >> + *
> >> + * @imgr:    pointer to security manager structure
> >> + * @csk_map: pointer to a bitmap to contain cancellation key vector
> >> + * @nbits:   number of bits in CSK vector
> >> + *
> >> + * This datatype is used to define a sysfs file handler function to
> >> + * return a bitmap of canceled keys to be displayed via sysfs.
> >> + *
> >> + * Context: No locking requirements are imposed by the security manager.
> >> + * Return:  0 on success, negative errno on failure
> >> + */
> >> +typedef int (*sysfs_csk_hndlr_t)(struct ifpga_sec_mgr *imgr,
> >> +				 unsigned long *csk_map, unsigned int nbits);
> >> +
> >> +/**
> >> + * struct ifpga_sec_mgr_ops - device specific operations
> >> + * @user_flash_count:	    Optional: Return sysfs string output for FPGA
> >> + *			    image flash count
> >> + * @bmc_flash_count:	    Optional: Return sysfs string output for BMC
> >> + *			    image flash count
> >> + * @sr_root_entry_hash:	    Optional: Return sysfs string output for static
> >> + *			    region root entry hash
> >> + * @pr_root_entry_hash:	    Optional: Return sysfs string output for partial
> >> + *			    reconfiguration root entry hash
> >> + * @bmc_root_entry_hash:    Optional: Return sysfs string output for BMC
> >> + *			    root entry hash
> >> + * @sr_canceled_csks:	    Optional: Return sysfs string output for static
> >> + *			    region canceled keys
> >> + * @pr_canceled_csks:	    Optional: Return sysfs string output for partial
> >> + *			    reconfiguration canceled keys
> >> + * @bmc_canceled_csks:	    Optional: Return sysfs string output for bmc
> >> + *			    canceled keys
> >> + * @bmc_canceled_csk_nbits: Optional: Return BMC canceled csk vector bit count
> >> + * @sr_canceled_csk_nbits:  Optional: Return SR canceled csk vector bit count
> >> + * @pr_canceled_csk_nbits:  Optional: Return PR canceled csk vector bit count
> >> + * @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
> >> + */
> >> +struct ifpga_sec_mgr_ops {
> >> +	sysfs_cnt_hndlr_t user_flash_count;
> >> +	sysfs_cnt_hndlr_t bmc_flash_count;
> >> +	sysfs_cnt_hndlr_t smbus_flash_count;
> >> +	sysfs_reh_hndlr_t sr_root_entry_hash;
> >> +	sysfs_reh_hndlr_t pr_root_entry_hash;
> >> +	sysfs_reh_hndlr_t bmc_root_entry_hash;
> >> +	sysfs_csk_hndlr_t sr_canceled_csks;
> >> +	sysfs_csk_hndlr_t pr_canceled_csks;
> >> +	sysfs_csk_hndlr_t bmc_canceled_csks;
> >> +	sysfs_reh_size_t bmc_reh_size;
> >> +	sysfs_reh_size_t sr_reh_size;
> >> +	sysfs_reh_size_t pr_reh_size;
> >> +	sysfs_csk_nbits_t bmc_canceled_csk_nbits;
> >> +	sysfs_csk_nbits_t sr_canceled_csk_nbits;
> >> +	sysfs_csk_nbits_t pr_canceled_csk_nbits;
> >> +};
> > I agree with Tom's feedback, please don't use typedefs here.
> I'll remove them.
> 
> Thanks for the comments! I've been reworking the patches. I'll split up the class
> driver and the Max10 BMC security engine into two patch sets for the next submission.

Great. Send a new version, and then we can see where it's at :-)
> 
> - Russ
> >> +
> >> +struct ifpga_sec_mgr {
> >> +	const char *name;
> >> +	struct device dev;
> >> +	const struct ifpga_sec_mgr_ops *iops;
> >> +	struct mutex lock;		/* protect data structure contents */
> >> +	void *priv;
> >> +};
> >> +
> >> +struct ifpga_sec_mgr *
> >> +ifpga_sec_mgr_register(struct device *dev, const char *name,
> >> +		       const struct ifpga_sec_mgr_ops *iops, void *priv);
> >> +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr);
> >> +
> >> +#endif
> >> -- 
> >> 2.17.1
> >>
> > Thanks,
> > Moritz
> 

Thanks,
Moritz

Russ Weight Oct. 1, 2020, 1:07 a.m. UTC | #11

Hi Moritz,

On 9/30/20 5:31 PM, Moritz Fischer wrote:
> I think providing the devm_ managed APIs is nicer, and makes it easier
> for the consumer of the API to do the right thing.

I see that the fpga_mgr code has support for two versions of the create()
and register() functions, one uses the devm_ approach, and the other does
not. Would also want to have two versions for the security manager?

- Russ

Moritz Fischer Oct. 1, 2020, 7:07 p.m. UTC | #12

Hi Russ,

On Wed, Sep 30, 2020 at 06:07:00PM -0700, Russ Weight wrote:
> 
> Hi Moritz,
> 
> On 9/30/20 5:31 PM, Moritz Fischer wrote:
> > I think providing the devm_ managed APIs is nicer, and makes it easier
> > for the consumer of the API to do the right thing.
> 
> I see that the fpga_mgr code has support for two versions of the create()
> and register() functions, one uses the devm_ approach, and the other does
> not. Would also want to have two versions for the security manager?

The devm_fpga_mgr_create() just wraps the other one. I think that's a
good way to handle it for now.

I'd keep the breakdown the same.

Cheers,
Moritz

[v1,01/12] fpga: fpga security manager class driver

Commit Message

Comments

Patch