Re: [PATCH 2/2] Platform integrity information in sysfs (version 9)

[Greg Kroah-Hartman]

On Wed, Sep 30, 2020 at 01:37:14PM -0300, Daniel Gutson wrote:
> This patch exports the BIOS Write Enable (bioswe), BIOS
> Lock Enable (biosle), and the SMM BIOS Write Protect (SMM_BIOSWP) fields of
> the BIOS Control register using the platform-integrity misc kernel module.
> The idea is to keep adding more flags, not only from the BC but also from
> other registers in following versions.
> 
> The goal is that the attributes are avilable to fwupd when SecureBoot
> is turned on.
> 
> Signed-off-by: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>

The subject line doesn't match what this patch does, please fix that up.

But there are more core issues in this patch:

> 
> ---
>  drivers/mtd/spi-nor/controllers/Kconfig       |  1 +
>  .../mtd/spi-nor/controllers/intel-spi-pci.c   | 75 ++++++++++++++-
>  .../spi-nor/controllers/intel-spi-platform.c  |  2 +-
>  drivers/mtd/spi-nor/controllers/intel-spi.c   | 91 ++++++++++++++++++-
>  drivers/mtd/spi-nor/controllers/intel-spi.h   |  9 +-
>  5 files changed, 171 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig
> index 5c0e0ec2e6d1..e7eaef506fc2 100644
> --- a/drivers/mtd/spi-nor/controllers/Kconfig
> +++ b/drivers/mtd/spi-nor/controllers/Kconfig
> @@ -29,6 +29,7 @@ config SPI_NXP_SPIFI
>  
>  config SPI_INTEL_SPI
>  	tristate
> +	depends on PLATFORM_INTEGRITY_DATA
>  
>  config SPI_INTEL_SPI_PCI
>  	tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)"
> diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> index c72aa1ab71ad..644b1a6091dc 100644
> --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c
> @@ -10,11 +10,19 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/platform-integrity.h>
>  
>  #include "intel-spi.h"
>  
>  #define BCR		0xdc
>  #define BCR_WPD		BIT(0)
> +#define BCR_BLE		BIT(1)
> +#define BCR_SMM_BWP	BIT(5)
> +
> +struct cnl_spi_attr {
> +	struct device_attribute dev_attr;
> +	u32 mask;
> +};
>  
>  static const struct intel_spi_boardinfo bxt_info = {
>  	.type = INTEL_SPI_BXT,
> @@ -24,6 +32,70 @@ static const struct intel_spi_boardinfo cnl_info = {
>  	.type = INTEL_SPI_CNL,
>  };
>  
> +#ifdef CONFIG_PLATFORM_INTEGRITY_DATA
> +static ssize_t intel_spi_cnl_spi_attr_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf, u32 mask)
> +{
> +	u32 bcr;
> +
> +	if (dev->parent == NULL)
> +		return -EIO;
> +
> +	if (pci_read_config_dword(container_of(dev->parent, struct pci_dev, dev),
> +				  BCR, &bcr) != PCIBIOS_SUCCESSFUL)
> +		return -EIO;
> +
> +	return sprintf(buf, "%d\n", (int)!!(bcr & mask));
> +}
> +
> +static ssize_t bioswe_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_WPD);
> +}
> +static DEVICE_ATTR_RO(bioswe);
> +
> +static ssize_t biosle_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_BLE);
> +}
> +static DEVICE_ATTR_RO(biosle);
> +
> +static ssize_t smm_bioswp_show(struct device *dev, struct device_attribute *attr,
> +			       char *buf)
> +{
> +	return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_SMM_BWP);
> +}
> +static DEVICE_ATTR_RO(smm_bioswp);
> +
> +static struct attribute *cnl_attrs[] = {
> +	&dev_attr_bioswe.attr,
> +	&dev_attr_biosle.attr,
> +	&dev_attr_smm_bioswp.attr,
> +	NULL
> +};
> +ATTRIBUTE_GROUPS(cnl);

If you are forcing the driver to create the groups, then you are forcing
us to audit each driver and verify that the files are the same name and
such.  Put the files in the "common" code please, and if you really need
a way to get the data out, make that a callback or something.

Also, this name "platform integrity" is really really generic, while in
reality you are describing something very specific.  Are you sure you
want that?

thanks,

greg k-h