From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8C04BC35257 for ; Sat, 3 Oct 2020 01:02:21 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 45B32206F8 for ; Sat, 3 Oct 2020 01:02:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1601686941; bh=PeCDsyl7OHxaiRYXzNNsKLURTlgGlwQzEKYShiRzRL8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=j1wPLWy8GRG4x31sidwaPKCzIAVvdyLvrtSMomnVsypC6j0KrCHHi/Y+44H7nttr2 tw/jxsRB1+dgZTyb9EzbCB+04beNUn0nSf3oeHX9lNFrvKaoAZMEIDbiutJYESrXU7 y2tJnj/awHvLFMk2GhXitp+RWNBzUH3PcaK+OlD0= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1725772AbgJCBCT (ORCPT ); Fri, 2 Oct 2020 21:02:19 -0400 Received: from mail-pj1-f65.google.com ([209.85.216.65]:34123 "EHLO mail-pj1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725283AbgJCBCT (ORCPT ); Fri, 2 Oct 2020 21:02:19 -0400 Received: by mail-pj1-f65.google.com with SMTP id nl2so245474pjb.1; Fri, 02 Oct 2020 18:02:18 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=9dzYPgnqedy5EhPgi/EhcujZ1+4TX7/my4zwn+pARqU=; b=ua+8A4xfWeOBzSBT9h06VXDkhy5Y4g6Xec0R69Okmr4wcY/7DLu8xCFlpzzztz/FBN E40xdg6ccn47//7Y9z3kPtoP1b0y+3a2ObSzg268DaEhiThQTB4v9VktHOXPQGGhBWW/ lrt8c5wqZNX4e+lXzVw56TU1gtETyebC7kVHPV9POPzc9mNcZKtoXPGV1ZDOXiAD4noN GqLqQmRPRVfCek5VEu0ELZU3NAjA8iW+vVf6WzkXQP1qXqtiuT9FUatpxoo7NvUG+P6a DqvqXT/t4d2Iy6fhWTicQ4VTOcCzG9GCuajJFIZfcfVT2/BWdw+vYgARStzjU925WiuD a4Yg== X-Gm-Message-State: AOAM530oLw5SRs/xuY4XcKjKpA1h/E0WR0kjlyg5iOXTsEnORILIctKj tLONHze2rAi0E8+SYW5Cy8A= X-Google-Smtp-Source: ABdhPJwIS/4TAxGx2mAipPaT+4AbXEb9QTHCCqTL6EUZRq5LSOLnSD8qxAnzXp9EmVpLT7vFuTipcA== X-Received: by 2002:a17:90a:fe85:: with SMTP id co5mr5117146pjb.236.1601686937646; Fri, 02 Oct 2020 18:02:17 -0700 (PDT) Received: from localhost ([2601:647:5b00:1162:1ac0:17a6:4cc6:d1ef]) by smtp.gmail.com with ESMTPSA id v205sm3570556pfc.110.2020.10.02.18.02.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Oct 2020 18:02:17 -0700 (PDT) Date: Fri, 2 Oct 2020 18:02:16 -0700 From: Moritz Fischer To: Russ Weight Cc: mdf@kernel.org, linux-fpga@vger.kernel.org, linux-kernel@vger.kernel.org, trix@redhat.com, lgoncalv@redhat.com, yilun.xu@intel.com, hao.wu@intel.com, matthew.gerlach@intel.com Subject: Re: [PATCH v2 1/7] fpga: sec-mgr: intel fpga security manager class driver Message-ID: <20201003010216.GA134824@archbook> References: <20201002223701.1317-1-russell.h.weight@intel.com> <20201002223701.1317-2-russell.h.weight@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Russ, On Fri, Oct 02, 2020 at 04:03:59PM -0700, Russ Weight wrote: > Hi Moritz, > > This patch aligns with FPGA Manager implementation as you requested by > splitting up the create() and register() functions and adding a devm_ > version of the create() function. I have a question (below) regarding the > device_add() function call. > > > On 10/2/20 3:36 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 > > Signed-off-by: Xu Yilun > > --- > > v2: > > - Bumped documentation dates and versions > > - Added Documentation/fpga/ifpga-sec-mgr.rst > > - Removed references to bmc_flash_count & smbus_flash_count (not supported) > > - Split ifpga_sec_mgr_register() into create() and register() functions > > - Added devm_ifpga_sec_mgr_create() > > - Removed typedefs for imgr ops > > --- > > .../ABI/testing/sysfs-class-ifpga-sec-mgr | 67 +++ > > Documentation/fpga/ifpga-sec-mgr.rst | 50 ++ > > Documentation/fpga/index.rst | 1 + > > MAINTAINERS | 9 + > > drivers/fpga/Kconfig | 9 + > > drivers/fpga/Makefile | 3 + > > drivers/fpga/ifpga-sec-mgr.c | 432 ++++++++++++++++++ > > include/linux/fpga/ifpga-sec-mgr.h | 81 ++++ > > 8 files changed, 652 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr > > create mode 100644 Documentation/fpga/ifpga-sec-mgr.rst > > 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..707958971bcb > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr > > @@ -0,0 +1,67 @@ > > +What: /sys/class/ifpga_sec_mgr/ifpga_secX/name > > +Date: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +Description: Name of low level fpga security manager driver. > > + > > +What: /sys/class/ifpga_sec_mgr/ifpga_secX/security/sr_root_entry_hash > > +Date: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +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: Oct 2020 > > +KernelVersion: 5.11 > > +Contact: Russ Weight > > +Description: Read only. Returns number of times the user image for the > > + static region has been flashed. > > + Format: "%u". > > diff --git a/Documentation/fpga/ifpga-sec-mgr.rst b/Documentation/fpga/ifpga-sec-mgr.rst > > new file mode 100644 > > index 000000000000..02f3f65b182b > > --- /dev/null > > +++ b/Documentation/fpga/ifpga-sec-mgr.rst > > @@ -0,0 +1,50 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +======================================== > > +Intel FPGA Security Manager Class Driver > > +======================================== > > + > > +The Intel FPGA Security Manager class driver provides a common > > +API for user-space tools to manage updates for secure Intel FPGA > > +devices. Device drivers that instantiate the Intel Security > > +Manager class driver will interact with a HW secure update > > +engine in order to transfer new FPGA and BMC images to FLASH so > > +that they will be automatically loaded when the FPGA card reboots. > > + > > +A significant difference between the FPGA Manager and the Intel FPGA > > +Security Manager is that the FPGA Manager does a live update (Partial > > +Reconfiguration) to a device, whereas the Intel FPGA Security Manager > > +updates the FLASH images for the Static Region and the BMC so that > > +they will be loaded the next time the FPGA card boots. Security is > > +enforced by hardware and firmware. The security manager interacts > > +with the firmware to initiate an update, pass in the necessary data, > > +and collect status on the update. > > + > > +In addition to managing secure updates of the FPGA and BMC images, > > +the Intel FPGA Security Manager update process may also used to > > +program root entry hashes and cancellation keys for the FPGA static > > +region, the FPGA partial reconfiguration region, and the BMC. > > + > > +Secure updates make use of the request_firmware framework, which > > +requires that image files are accessible under /lib/firmware. A request > > +for a secure update returns immediately, while the update itself > > +proceeds in the context of a kernel worker thread. Sysfs files provide > > +a means for monitoring the progress of a secure update and for > > +retrieving error information in the event of a failure. > > + > > +Sysfs Attributes > > +================ > > + > > +The API consists of two groups of sysfs attributes as described below. > > + > > +1. Files in the *security* sub-directory can be used to read security > > + information including: Root Entry Hashes (REH), Cancelled Code > > + Signing Keys (CSK), and the flash update count for FPGA images. > > + > > +2. Files in the *update* sub-directory can be used to instantiate and > > + monitor a secure update. > > + > > + > > +See `<../ABI/testing/sysfs-class-ifpga-sec-mgr>`__ for a full > > +description of the sysfs attributes for the Intel FPGA Security > > +Manager. > > diff --git a/Documentation/fpga/index.rst b/Documentation/fpga/index.rst > > index f80f95667ca2..ba9c6b1917bd 100644 > > --- a/Documentation/fpga/index.rst > > +++ b/Documentation/fpga/index.rst > > @@ -8,6 +8,7 @@ fpga > > :maxdepth: 1 > > > > dfl > > + ifpga-sec-mgr > > > > .. only:: subproject and html > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 6d1175bf8529..5ffc0e02d741 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -6858,6 +6858,15 @@ F: Documentation/fpga/ > > F: drivers/fpga/ > > F: include/linux/fpga/ > > > > +INTEL FPGA SECURITY MANAGER DRIVERS > > +M: Russ Weight > > +L: linux-fpga@vger.kernel.org > > +S: Maintained > > +F: Documentation/ABI/testing/sysfs-class-ifpga-sec-mgr > > +F: Documentation/fpga/ifpga-sec-mgr.rst > > +F: drivers/fpga/ifpga-sec-mgr.c > > +F: include/linux/fpga/ifpga-sec-mgr.h > > + > > FPU EMULATOR > > M: Bill Metzenthen > > S: Maintained > > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig > > index 7cd5a29fc437..bf566a625be7 100644 > > --- a/drivers/fpga/Kconfig > > +++ b/drivers/fpga/Kconfig > > @@ -215,4 +215,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 d8e21dfc6778..2e1d29c3d915 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..f1caa4602ab3 > > --- /dev/null > > +++ b/drivers/fpga/ifpga-sec-mgr.c > > @@ -0,0 +1,432 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Intel Security Manager for FPGA > > + * > > + * Copyright (C) 2019-2020 Intel Corporation, Inc. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +static DEFINE_IDA(ifpga_sec_mgr_ida); > > +static struct class *ifpga_sec_mgr_class; > > + > > +#define to_sec_mgr(d) container_of(d, struct ifpga_sec_mgr, dev) > > + > > +static ssize_t > > +show_canceled_csk(struct ifpga_sec_mgr *imgr, > > + int (*get_csk)(struct ifpga_sec_mgr *imgr, > > + unsigned long *csk_map, unsigned int nbits), > > + int (*get_csk_nbits)(struct ifpga_sec_mgr *imgr), > > + char *buf) > > +{ > > + unsigned long *csk_map = NULL; > > + unsigned int nbits; > > + int 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; > > + > > + ret = bitmap_print_to_pagebuf(1, buf, csk_map, nbits); > > + > > +vfree_exit: > > + vfree(csk_map); > > + return ret; > > +} > > + > > +static ssize_t > > +show_root_entry_hash(struct ifpga_sec_mgr *imgr, > > + int (*get_reh)(struct ifpga_sec_mgr *imgr, u8 *hash, > > + unsigned int size), > > + int (*get_reh_size)(struct ifpga_sec_mgr *imgr), > > + char *buf) > > +{ > > + int size, i, cnt, ret; > > + u8 *hash; > > + > > + ret = get_reh_size(imgr); > > + if (ret < 0) > > + return ret; > > + else if (!ret) > > + return sprintf(buf, "hash not programmed\n"); > > + > > + size = 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 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) > > + > > +static ssize_t user_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->user_flash_count(imgr); > > + return cnt < 0 ? cnt : sprintf(buf, "%u\n", cnt); > > +} > > +static DEVICE_ATTR_RO(user_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_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_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)); > > + > > + /* > > + * Only display optional sysfs attributes if a > > + * corresponding handler is provided > > + */ > > + if (check_attr(attr, user_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) > > +{ > > + /* > > + * sysfs_handler and size_handler must either both be > > + * defined or both be NULL. > > + */ > > + 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_create - create and initialize an Intel FPGA > > + * security manager struct > > + * > > + * @dev: Intel fpga 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 > > + * > > + * The caller of this function is responsible for freeing the struct > > + * with ifpg_sec_mgr_free(). Using devm_ifpga_sec_mgr_create() instead > > + * is recommended. > > + * > > + * Return: pointer to struct ifpga_sec_mgr or NULL > > + */ > > +struct ifpga_sec_mgr * > > +ifpga_sec_mgr_create(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 NULL; > > + } > > + > > + if (!name || !strlen(name)) { > > + dev_err(dev, "Attempt to register with no name!\n"); > > + return NULL; > > + } > > + > > + imgr = kzalloc(sizeof(*imgr), GFP_KERNEL); > > + if (!imgr) > > + return NULL; > > + > > + id = ida_simple_get(&ifpga_sec_mgr_ida, 0, 0, GFP_KERNEL); > > + if (id < 0) > > + goto error_kfree; > > + > > + mutex_init(&imgr->lock); > > + > > + imgr->name = name; > > + imgr->priv = priv; > > + imgr->iops = iops; > > + > > + device_initialize(&imgr->dev); > > + 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); > > + goto error_device; > > + } > > + > > + return imgr; > > + > > +error_device: > > + ida_simple_remove(&ifpga_sec_mgr_ida, id); > > + > > +error_kfree: > > + kfree(imgr); > > + > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_create); > > + > > +/** > > + * ifpga_sec_mgr_free - free an Intel FPGA security manager created > > + * with ifpga_sec_mgr_create() > > + * > > + * @imgr: Intel FPGA security manager structure > > + */ > > +void ifpga_sec_mgr_free(struct ifpga_sec_mgr *imgr) > > +{ > > + ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id); > > + kfree(imgr); > > +} > > +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_free); > > + > > +static void devm_ifpga_sec_mgr_release(struct device *dev, void *res) > > +{ > > + struct ifpga_sec_mgr *imgr = *(struct ifpga_sec_mgr **)res; > > + > > + ifpga_sec_mgr_free(imgr); > > +} > > + > > +/** > > + * devm_ifpga_sec_mgr_create - create and initialize an Intel FPGA > > + * security manager struct > > + * > > + * @dev: Intel fpga 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 > > + * > > + * This function is intended for use in a Intel FPGA Security manager > > + * driver's probe function. After the security manager driver creates > > + * the ifpga_sec_mgr struct with devm_fpga_mgr_create(), it should > > + * register it with ifpga_sec_mgr_register(). The security manager > > + * driver's remove function should call ifpga_sec_mgr_unregister(). > > + * The ifpga_sec_mgr struct allocated with this function will be freed > > + * automatically on driver detach. This includes the case of a probe > > + * function returning error before calling fpga_mgr_register(), the > > + * struct will still get cleaned up. > > + * > > + * Return: pointer to struct ifpga_sec_mgr or NULL > > + */ > > +struct ifpga_sec_mgr * > > +devm_ifpga_sec_mgr_create(struct device *dev, const char *name, > > + const struct ifpga_sec_mgr_ops *iops, void *priv) > > +{ > > + struct ifpga_sec_mgr **ptr, *imgr; > > + > > + ptr = devres_alloc(devm_ifpga_sec_mgr_release, sizeof(*ptr), GFP_KERNEL); > > + if (!ptr) > > + return NULL; > > + > > + imgr = ifpga_sec_mgr_create(dev, name, iops, priv); > > + if (!imgr) { > > + devres_free(ptr); > > + } else { > > + *ptr = imgr; > > + devres_add(dev, ptr); > > + } > > + > > + return imgr; > > +} > > +EXPORT_SYMBOL_GPL(devm_ifpga_sec_mgr_create); > > + > > +/** > > + * ifpga_sec_mgr_register - register an Intel FPGA security manager > > + * > > + * @imgr: Intel fpga security manager struct > > + * > > + * Return: 0 on success, negative error code otherwise. > > + */ > > +int ifpga_sec_mgr_register(struct ifpga_sec_mgr *imgr) > > +{ > > + int ret; > > + > > + ret = device_add(&imgr->dev); > > + if (ret) > > + goto error_device; > The comment header for device_add() includes the following text: > >  * NOTE: _Never_ directly free @dev after calling this function, even >  * if it returned an error! Always use put_device() to give up your >  * reference instead. >  * >  * Rule of thumb is: if device_add() succeeds, you should call >  * device_del() when you want to get rid of it. If device_add() has >  * *not* succeeded, use *only* put_device() to drop the reference >  * count. > > This implementation of the security manager requires that the parent > driver (which implements the security manager) explicitly free the > ifpga_sec_mgr structure or implicitly free it as a managed resource > (of the parent device) using the devm_ version of the create() function. > The device structure that is passed to device_add() above is embedded > in the ifpga_sec_mgr structure. It is confusing I'll admit :) It's been a while since I looked at the gory details, but you might be onto something here. I'll definitely look into this some more this weekend. Appreciate the question! > > Does this implementation conflict with the core driver functions? > Is it possible that we are deleting a device structure that is still > being tracked by the core driver code? > > - Russ > > > + > > + dev_info(&imgr->dev, "%s registered\n", imgr->name); > > + > > + return 0; > > + > > +error_device: > > + ida_simple_remove(&ifpga_sec_mgr_ida, imgr->dev.id); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(ifpga_sec_mgr_register); > > + > > +/** > > + * ifpga_sec_mgr_unregister - unregister an Intel FPGA security manager > > + * > > + * @mgr: fpga manager struct > > + * > > + * This function is intended for use in an Intel FPGA 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) > > +{ > > +} > > + > > +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..ded62090e9b9 > > --- /dev/null > > +++ b/include/linux/fpga/ifpga-sec-mgr.h > > @@ -0,0 +1,81 @@ > > +/* 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 > > +#include > > +#include > > + > > +struct ifpga_sec_mgr; > > + > > +/** > > + * struct ifpga_sec_mgr_ops - device specific operations > > + * @user_flash_count: Optional: Return sysfs string output for FPGA > > + * 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 { > > + int (*user_flash_count)(struct ifpga_sec_mgr *imgr); > > + int (*bmc_root_entry_hash)(struct ifpga_sec_mgr *imgr, u8 *hash, > > + unsigned int size); > > + int (*sr_root_entry_hash)(struct ifpga_sec_mgr *imgr, u8 *hash, > > + unsigned int size); > > + int (*pr_root_entry_hash)(struct ifpga_sec_mgr *imgr, u8 *hash, > > + unsigned int size); > > + int (*bmc_canceled_csks)(struct ifpga_sec_mgr *imgr, > > + unsigned long *csk_map, unsigned int nbits); > > + int (*sr_canceled_csks)(struct ifpga_sec_mgr *imgr, > > + unsigned long *csk_map, unsigned int nbits); > > + int (*pr_canceled_csks)(struct ifpga_sec_mgr *imgr, > > + unsigned long *csk_map, unsigned int nbits); > > + int (*bmc_reh_size)(struct ifpga_sec_mgr *imgr); > > + int (*sr_reh_size)(struct ifpga_sec_mgr *imgr); > > + int (*pr_reh_size)(struct ifpga_sec_mgr *imgr); > > + int (*bmc_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr); > > + int (*sr_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr); > > + int (*pr_canceled_csk_nbits)(struct ifpga_sec_mgr *imgr); > > +}; > > + > > +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_create(struct device *dev, const char *name, > > + const struct ifpga_sec_mgr_ops *iops, void *priv); > > + > > +struct ifpga_sec_mgr * > > +devm_ifpga_sec_mgr_create(struct device *dev, const char *name, > > + const struct ifpga_sec_mgr_ops *iops, void *priv); > > + > > +int ifpga_sec_mgr_register(struct ifpga_sec_mgr *imgr); > > +void ifpga_sec_mgr_unregister(struct ifpga_sec_mgr *imgr); > > +void ifpga_sec_mgr_free(struct ifpga_sec_mgr *imgr); > > + > > +#endif > Cheers, Moritz