[PATCH] mfd: Export LPC attributes for the system SPI chip

[PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

Export standard SPI-specific config values from various LPC
controllers.
This allows userspace components such as fwupd to verify the most basic
SPI
protections are set correctly. For instance, checking BIOSWE is
disabled
and BLE is enabled.

Exporting these values from the kernel allows us to report the security
level of the platform without rebooting and running an EFI binary like
chipsec.

Signed-off-by: Richard Hughes <richard@hughsie.com>
---
 Documentation/ABI/testing/sysfs-security-spi |  17 ++
 drivers/mfd/lpc_ich.c                        | 225 ++++++++++++++++++-
 include/linux/platform_data/intel-spi.h      |   7 +-
 3 files changed, 242 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-security-spi

diff --git a/Documentation/ABI/testing/sysfs-security-spi
b/Documentation/ABI/testing/sysfs-security-spi
new file mode 100644
index 000000000000..ee867b1366f9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-security-spi
@@ -0,0 +1,17 @@
+What:           /sys/kernel/security/spi/bioswe
+Date:           May 2020
+Contact:        platform-driver-x86@vger.kernel.org
+Description:    If the system firmware set BIOS Write Enable.
+		0: writes disabled, 1: writes enabled.
+
+What:           /sys/kernel/security/spi/ble
+Date:           May 2020
+Contact:        platform-driver-x86@vger.kernel.org
+Description:    If the system firmware set Bios Lock Enable.
+		0: SMM lock disabled, 1: SMM lock enabled.
+
+What:           /sys/kernel/security/spi/smm_bwp
+Date:           May 2020
+Contact:        platform-driver-x86@vger.kernel.org
+Description:    If the system firmware set SMM Bios Write Protect.
+		0: writes disabled unless in SMM, 1: writes enabled.
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 3bbb29a7e7a5..e9a97c461d9a 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -33,6 +33,8 @@
  *	document number 322169-001, 322170-003: 5 Series, 3400 Series
(PCH)
  *	document number 320066-003, 320257-008: EP80597 (IICH)
  *	document number 324645-001, 324646-001: Cougar Point (CPT)
+ *	document number 332690-006, 332691-003: C230 (CPT)
+ *	document number 337867-003, 337868-002: Cannon Point (PCH)
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -46,6 +48,10 @@
 #include <linux/mfd/lpc_ich.h>
 #include <linux/platform_data/itco_wdt.h>
 
+#if IS_ENABLED(CONFIG_SECURITY)
+#include <linux/security.h>
+#endif
+
 #define ACPIBASE		0x40
 #define ACPIBASE_GPE_OFF	0x28
 #define ACPIBASE_GPE_END	0x2f
@@ -68,6 +74,8 @@
 #define SPIBASE_LPT_SZ		512
 #define BCR			0xdc
 #define BCR_WPD			BIT(0)
+#define BCR_BLE			BIT(1)
+#define BCR_SMM_BWP		BIT(5)
 
 #define SPIBASE_APL_SZ		4096
 
@@ -93,6 +101,13 @@ struct lpc_ich_priv {
 	int abase_save;		/* Cached ACPI base value */
 	int actrl_pbase_save;		/* Cached ACPI control or PMC
base value */
 	int gctrl_save;		/* Cached GPIO control value */
+
+#if IS_ENABLED(CONFIG_SECURITY)
+	struct dentry *spi_dir;		/* SecurityFS entries */
+	struct dentry *spi_bioswe;
+	struct dentry *spi_ble;
+	struct dentry *spi_smm_bwp;
+#endif
 };
 
 static struct resource wdt_ich_res[] = {
@@ -221,6 +236,16 @@ enum lpc_chipsets {
 	LPC_APL,	/* Apollo Lake SoC */
 	LPC_GLK,	/* Gemini Lake SoC */
 	LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
+	LPC_SPT,	/* Sunrise Point */
+	LPC_KLK,	/* Kaby Lake */
+	LPC_CNPT,	/* Cannon Point */
+	LPC_CLK,	/* Comet Lake */
+	LPC_ILK,	/* Ice Lake */
+	LPC_ELK,	/* Elkhart Lake */
+	LPC_JLK,	/* Jasper Lake */
+	LPC_TLK,	/* Tiger Lake */
+	LPC_CNLK,	/* Cannon Lake */
+	LPC_CRDG,	/* Cactus Ridge */
 };
 
 static struct lpc_ich_info lpc_chipset_info[] = {
@@ -557,6 +582,46 @@ static struct lpc_ich_info lpc_chipset_info[] = {
 		.name = "Cougar Mountain SoC",
 		.iTCO_version = 3,
 	},
+	[LPC_SPT] = {
+		.name = "Sunrise Point",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_KLK] = {
+		.name = "Kaby Lake-H",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_CNPT] = {
+		.name = "Cannon Point",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_CLK] = {
+		.name = "Comet Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_ILK] = {
+		.name = "Ice Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_ELK] = {
+		.name = "Elkhart Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_JLK] = {
+		.name = "Jasper Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_TLK] = {
+		.name = "Tiger Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_CNLK] = {
+		.name = "Cannon Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_CRDG] = {
+		.name = "Cactus Ridge",
+		.spi_type = INTEL_SPI_LPC,
+	},
 };
 
 /*
@@ -566,6 +631,8 @@ static struct lpc_ich_info lpc_chipset_info[] = {
  * functions that probably will be registered by other drivers.
  */
 static const struct pci_device_id lpc_ich_ids[] = {
+	{ PCI_VDEVICE(INTEL, 0x02a4), LPC_CLK},
+	{ PCI_VDEVICE(INTEL, 0x06a4), LPC_CLK},
 	{ PCI_VDEVICE(INTEL, 0x0f1c), LPC_BAYTRAIL},
 	{ PCI_VDEVICE(INTEL, 0x1c41), LPC_CPT},
 	{ PCI_VDEVICE(INTEL, 0x1c42), LPC_CPTD},
@@ -687,6 +754,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x2919), LPC_ICH9M},
 	{ PCI_VDEVICE(INTEL, 0x3197), LPC_GLK},
 	{ PCI_VDEVICE(INTEL, 0x2b9c), LPC_COUGARMOUNTAIN},
+	{ PCI_VDEVICE(INTEL, 0x34a4), LPC_ILK},
 	{ PCI_VDEVICE(INTEL, 0x3a14), LPC_ICH10DO},
 	{ PCI_VDEVICE(INTEL, 0x3a16), LPC_ICH10R},
 	{ PCI_VDEVICE(INTEL, 0x3a18), LPC_ICH10},
@@ -706,6 +774,8 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x3b12), LPC_3400},
 	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
 	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
+	{ PCI_VDEVICE(INTEL, 0x4b24), LPC_ELK},
+	{ PCI_VDEVICE(INTEL, 0x4da4), LPC_JLK},
 	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
 	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
 	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
@@ -785,6 +855,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x9c45), LPC_LPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9c46), LPC_LPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9c47), LPC_LPT_LP},
+	{ PCI_VDEVICE(INTEL, 0x9c66), LPC_CRDG},
 	{ PCI_VDEVICE(INTEL, 0x9cc1), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc2), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc3), LPC_WPT_LP},
@@ -792,6 +863,33 @@ static const struct pci_device_id lpc_ich_ids[] =
{
 	{ PCI_VDEVICE(INTEL, 0x9cc6), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc7), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc9), LPC_WPT_LP},
+	{ PCI_VDEVICE(INTEL, 0x9ce6), LPC_WPT_LP},
+	{ PCI_VDEVICE(INTEL, 0x9d2a), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0x9d4e), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0x9da4), LPC_CNPT},
+	{ PCI_VDEVICE(INTEL, 0xa0a4), LPC_TLK},
+	{ PCI_VDEVICE(INTEL, 0xa140), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa141), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa142), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa143), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa144), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa145), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa146), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa147), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa148), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa149), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14a), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14b), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14c), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14d), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14e), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14f), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa150), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa151), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa152), LPC_KLK},
+	{ PCI_VDEVICE(INTEL, 0xa153), LPC_KLK},
+	{ PCI_VDEVICE(INTEL, 0xa154), LPC_KLK},
+	{ PCI_VDEVICE(INTEL, 0xa155), LPC_SPT},
 	{ PCI_VDEVICE(INTEL, 0xa1c1), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa1c2), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa1c3), LPC_LEWISBURG},
@@ -801,6 +899,12 @@ static const struct pci_device_id lpc_ich_ids[] =
{
 	{ PCI_VDEVICE(INTEL, 0xa1c7), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa242), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa243), LPC_LEWISBURG},
+	{ PCI_VDEVICE(INTEL, 0xa304), LPC_CNLK},
+	{ PCI_VDEVICE(INTEL, 0xa305), LPC_CNLK},
+	{ PCI_VDEVICE(INTEL, 0xa306), LPC_CNLK},
+	{ PCI_VDEVICE(INTEL, 0xa30c), LPC_CNLK},
+	{ PCI_VDEVICE(INTEL, 0xa324), LPC_CNLK},
+	{ PCI_VDEVICE(INTEL, 0xa3a4), LPC_CLK},
 	{ 0, },			/* End of list */
 };
 MODULE_DEVICE_TABLE(pci, lpc_ich_ids);
@@ -1083,6 +1187,104 @@ static int lpc_ich_init_wdt(struct pci_dev
*dev)
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_SECURITY)
+static ssize_t bioswe_read(struct file *filp, char __user *buf,
+			   size_t count, loff_t *ppos)
+{
+	struct intel_spi_boardinfo *info =
lpc_ich_spi_cell.platform_data;
+	char tmp[2];
+
+	sprintf(tmp, "%d\n", info->writeable ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_bioswe_ops = {
+	.read  = bioswe_read,
+};
+
+static ssize_t ble_read(struct file *filp, char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct intel_spi_boardinfo *info =
lpc_ich_spi_cell.platform_data;
+	char tmp[2];
+
+	sprintf(tmp, "%d\n", info->ble ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_ble_ops = {
+	.read  = ble_read,
+};
+
+static ssize_t smm_bwp_read(struct file *filp, char __user *buf,
+			    size_t count, loff_t *ppos)
+{
+	struct intel_spi_boardinfo *info =
lpc_ich_spi_cell.platform_data;
+	char tmp[2];
+
+	sprintf(tmp, "%d\n", info->smm_bwp ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_smm_bwp_ops = {
+	.read  = smm_bwp_read,
+};
+
+static int lpc_ich_init_securityfs(struct pci_dev *dev)
+{
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	priv->spi_dir = securityfs_create_dir("spi", NULL);
+	if (IS_ERR(priv->spi_dir))
+		return -1;
+
+	priv->spi_bioswe =
+	    securityfs_create_file("bioswe",
+				   0600, priv->spi_dir, dev,
+				   &spi_bioswe_ops);
+	if (IS_ERR(priv->spi_bioswe))
+		goto out;
+	priv->spi_ble =
+	    securityfs_create_file("ble",
+				   0600, priv->spi_dir, dev,
+				   &spi_ble_ops);
+	if (IS_ERR(priv->spi_ble))
+		goto out;
+	priv->spi_smm_bwp =
+	    securityfs_create_file("smm_bwp",
+				   0600, priv->spi_dir, dev,
+				   &spi_smm_bwp_ops);
+	if (IS_ERR(priv->spi_smm_bwp))
+		goto out;
+	return 0;
+out:
+	securityfs_remove(priv->spi_ble);
+	securityfs_remove(priv->spi_bioswe);
+	securityfs_remove(priv->spi_dir);
+	return -1;
+}
+
+static void lpc_ich_uninit_securityfs(struct pci_dev *dev)
+{
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	securityfs_remove(priv->spi_smm_bwp);
+	securityfs_remove(priv->spi_ble);
+	securityfs_remove(priv->spi_bioswe);
+	securityfs_remove(priv->spi_dir);
+}
+#else
+static inline int lpc_ich_init_securityfs(struct pci_dev *dev) {
return 0; }
+static inline void lpc_ich_uninit_securityfs(struct pci_dev *dev) {
return 0; }
+#endif
+
+static void lpc_ich_init_spi_bcr(struct intel_spi_boardinfo *info, u32
bcr)
+{
+	info->writeable = !!(bcr & BCR_WPD);
+	info->ble = !!(bcr & BCR_BLE);
+	info->smm_bwp = !!(bcr & BCR_SMM_BWP);
+}
+
 static int lpc_ich_init_spi(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
@@ -1112,9 +1314,14 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->start = spi_base + SPIBASE_LPT;
 			res->end = res->start + SPIBASE_LPT_SZ - 1;
 
-			pci_read_config_dword(dev, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
 		}
+		pci_read_config_dword(dev, BCR, &bcr);
+		lpc_ich_init_spi_bcr(info, bcr);
+		break;
+
+	case INTEL_SPI_LPC:
+		pci_read_config_dword(dev, BCR, &bcr);
+		lpc_ich_init_spi_bcr(info, bcr);
 		break;
 
 	case INTEL_SPI_BXT: {
@@ -1135,7 +1342,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 			res->end = res->start + SPIBASE_APL_SZ - 1;
 
 			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
+			lpc_ich_init_spi_bcr(info, bcr);
 		}
 
 		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
@@ -1146,8 +1353,10 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		return -EINVAL;
 	}
 
-	if (!res->start)
-		return -ENODEV;
+	if (info->type != INTEL_SPI_LPC) {
+		if (!res->start)
+			return -ENODEV;
+	}
 
 	lpc_ich_spi_cell.platform_data = info;
 	lpc_ich_spi_cell.pdata_size = sizeof(*info);
@@ -1201,8 +1410,11 @@ static int lpc_ich_probe(struct pci_dev *dev,
 
 	if (lpc_chipset_info[priv->chipset].spi_type) {
 		ret = lpc_ich_init_spi(dev);
-		if (!ret)
+		if (!ret) {
+			if (lpc_ich_init_securityfs(dev))
+				return -EINVAL;
 			cell_added = true;
+		}
 	}
 
 	/*
@@ -1221,6 +1433,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
 static void lpc_ich_remove(struct pci_dev *dev)
 {
 	mfd_remove_devices(&dev->dev);
+	lpc_ich_uninit_securityfs(dev);
 	lpc_ich_restore_config_space(dev);
 }
 
diff --git a/include/linux/platform_data/intel-spi.h
b/include/linux/platform_data/intel-spi.h
index 7f53a5c6f35e..049cc726f868 100644
--- a/include/linux/platform_data/intel-spi.h
+++ b/include/linux/platform_data/intel-spi.h
@@ -14,16 +14,21 @@ enum intel_spi_type {
 	INTEL_SPI_LPT,
 	INTEL_SPI_BXT,
 	INTEL_SPI_CNL,
+	INTEL_SPI_LPC,
 };
 
 /**
  * struct intel_spi_boardinfo - Board specific data for Intel SPI
driver
  * @type: Type which this controller is compatible with
- * @writeable: The chip is writeable
+ * @writeable: The chip is writeable, a.k.a. BIOSWE
+ * @ble: a SMM is raised when setting BIOSWE
+ * @smm_bwp: the BIOS region is non-writable unless all processors are
in SMM
  */
 struct intel_spi_boardinfo {
 	enum intel_spi_type type;
 	bool writeable;
+	bool ble;
+	bool smm_bwp;
 };
 
 #endif /* INTEL_SPI_PDATA_H */

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

Hi Richard,

On Tue, May 12, 2020 at 09:42:43PM +0100, Richard Hughes wrote:
> Export standard SPI-specific config values from various LPC
> controllers.
> This allows userspace components such as fwupd to verify the most basic
> SPI
> protections are set correctly. For instance, checking BIOSWE is
> disabled
> and BLE is enabled.
> 
> Exporting these values from the kernel allows us to report the security
> level of the platform without rebooting and running an EFI binary like
> chipsec.
> 
> Signed-off-by: Richard Hughes <richard@hughsie.com>
> ---
>  Documentation/ABI/testing/sysfs-security-spi |  17 ++
>  drivers/mfd/lpc_ich.c                        | 225 ++++++++++++++++++-
>  include/linux/platform_data/intel-spi.h      |   7 +-
>  3 files changed, 242 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-security-spi
> 
> diff --git a/Documentation/ABI/testing/sysfs-security-spi
> b/Documentation/ABI/testing/sysfs-security-spi
> new file mode 100644
> index 000000000000..ee867b1366f9
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-security-spi
> @@ -0,0 +1,17 @@
> +What:           /sys/kernel/security/spi/bioswe
> +Date:           May 2020

I think this one should contain KernelVersion as well, see
Documentation/ABI/README.

> +Contact:        platform-driver-x86@vger.kernel.org
> +Description:    If the system firmware set BIOS Write Enable.
> +		0: writes disabled, 1: writes enabled.
> +
> +What:           /sys/kernel/security/spi/ble
> +Date:           May 2020
> +Contact:        platform-driver-x86@vger.kernel.org
> +Description:    If the system firmware set Bios Lock Enable.
> +		0: SMM lock disabled, 1: SMM lock enabled.
> +
> +What:           /sys/kernel/security/spi/smm_bwp
> +Date:           May 2020
> +Contact:        platform-driver-x86@vger.kernel.org
> +Description:    If the system firmware set SMM Bios Write Protect.
> +		0: writes disabled unless in SMM, 1: writes enabled.
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 3bbb29a7e7a5..e9a97c461d9a 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -33,6 +33,8 @@
>   *	document number 322169-001, 322170-003: 5 Series, 3400 Series
> (PCH)
>   *	document number 320066-003, 320257-008: EP80597 (IICH)
>   *	document number 324645-001, 324646-001: Cougar Point (CPT)
> + *	document number 332690-006, 332691-003: C230 (CPT)
> + *	document number 337867-003, 337868-002: Cannon Point (PCH)
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -46,6 +48,10 @@
>  #include <linux/mfd/lpc_ich.h>
>  #include <linux/platform_data/itco_wdt.h>
>  
> +#if IS_ENABLED(CONFIG_SECURITY)
> +#include <linux/security.h>
> +#endif

I think you can always include this header without #ifs

> +
>  #define ACPIBASE		0x40
>  #define ACPIBASE_GPE_OFF	0x28
>  #define ACPIBASE_GPE_END	0x2f
> @@ -68,6 +74,8 @@
>  #define SPIBASE_LPT_SZ		512
>  #define BCR			0xdc
>  #define BCR_WPD			BIT(0)
> +#define BCR_BLE			BIT(1)
> +#define BCR_SMM_BWP		BIT(5)
>  
>  #define SPIBASE_APL_SZ		4096
>  
> @@ -93,6 +101,13 @@ struct lpc_ich_priv {
>  	int abase_save;		/* Cached ACPI base value */
>  	int actrl_pbase_save;		/* Cached ACPI control or PMC
> base value */
>  	int gctrl_save;		/* Cached GPIO control value */
> +
> +#if IS_ENABLED(CONFIG_SECURITY)
> +	struct dentry *spi_dir;		/* SecurityFS entries */
> +	struct dentry *spi_bioswe;
> +	struct dentry *spi_ble;
> +	struct dentry *spi_smm_bwp;
> +#endif

Maybe these ones can also be added always.

>  };
>  
>  static struct resource wdt_ich_res[] = {
> @@ -221,6 +236,16 @@ enum lpc_chipsets {
>  	LPC_APL,	/* Apollo Lake SoC */
>  	LPC_GLK,	/* Gemini Lake SoC */
>  	LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
> +	LPC_SPT,	/* Sunrise Point */
> +	LPC_KLK,	/* Kaby Lake */

KBL for Kaby Lake

> +	LPC_CNPT,	/* Cannon Point */

CNL for Cannon Lake

> +	LPC_CLK,	/* Comet Lake */

CML for Comet Lake

> +	LPC_ILK,	/* Ice Lake */

ICL for Ice Lake

> +	LPC_ELK,	/* Elkhart Lake */

EHL for Elkhart Lake

> +	LPC_JLK,	/* Jasper Lake */

JSL for Jasper Lake

> +	LPC_TLK,	/* Tiger Lake */

TGL for Tiger Lake

> +	LPC_CNLK,	/* Cannon Lake */

This is redundant with CNL

> +	LPC_CRDG,	/* Cactus Ridge */

This is not PCH, Cactus Ridge is Thunderbolt host controller AFAIK.

>  };
>  
>  static struct lpc_ich_info lpc_chipset_info[] = {
> @@ -557,6 +582,46 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  		.name = "Cougar Mountain SoC",
>  		.iTCO_version = 3,
>  	},
> +	[LPC_SPT] = {
> +		.name = "Sunrise Point",
> +		.spi_type = INTEL_SPI_LPC,

Problem here and with the rest of the Lakes is that on those systems the
SPI-NOR controller is actually not part of the LPC/eSPI device. Instead
it is a separate PCI device (00:1f.5), not compatible with LPC.

So I don't think we can add these entries here without careful
verification that these are accessible through LPC.

I suggested lpc_ich.c because I think I saw in your previous patch that
you were adding LPC PCI IDs as well. Those are fine to add here. Sorry
if that confused you.

> +	},
> +	[LPC_KLK] = {
> +		.name = "Kaby Lake-H",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_CNPT] = {
> +		.name = "Cannon Point",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_CLK] = {
> +		.name = "Comet Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_ILK] = {
> +		.name = "Ice Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_ELK] = {
> +		.name = "Elkhart Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_JLK] = {
> +		.name = "Jasper Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_TLK] = {
> +		.name = "Tiger Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_CNLK] = {
> +		.name = "Cannon Lake",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_CRDG] = {
> +		.name = "Cactus Ridge",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
>  };
>  
>  /*
> @@ -566,6 +631,8 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>   * functions that probably will be registered by other drivers.
>   */
>  static const struct pci_device_id lpc_ich_ids[] = {
> +	{ PCI_VDEVICE(INTEL, 0x02a4), LPC_CLK},
> +	{ PCI_VDEVICE(INTEL, 0x06a4), LPC_CLK},

For example these PCI IDs are for the SPI-NOR controller (not LPC
controller) so this causes this driver to try to bind to a completely
different device which it cannot handle.

Therefore I suggest adding this feature only for the hardware you can
actually test. At least for starters.

>  	{ PCI_VDEVICE(INTEL, 0x0f1c), LPC_BAYTRAIL},
>  	{ PCI_VDEVICE(INTEL, 0x1c41), LPC_CPT},
>  	{ PCI_VDEVICE(INTEL, 0x1c42), LPC_CPTD},
> @@ -687,6 +754,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x2919), LPC_ICH9M},
>  	{ PCI_VDEVICE(INTEL, 0x3197), LPC_GLK},
>  	{ PCI_VDEVICE(INTEL, 0x2b9c), LPC_COUGARMOUNTAIN},
> +	{ PCI_VDEVICE(INTEL, 0x34a4), LPC_ILK},

Ditto this one.

>  	{ PCI_VDEVICE(INTEL, 0x3a14), LPC_ICH10DO},
>  	{ PCI_VDEVICE(INTEL, 0x3a16), LPC_ICH10R},
>  	{ PCI_VDEVICE(INTEL, 0x3a18), LPC_ICH10},
> @@ -706,6 +774,8 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x3b12), LPC_3400},
>  	{ PCI_VDEVICE(INTEL, 0x3b14), LPC_3420},
>  	{ PCI_VDEVICE(INTEL, 0x3b16), LPC_3450},
> +	{ PCI_VDEVICE(INTEL, 0x4b24), LPC_ELK},
> +	{ PCI_VDEVICE(INTEL, 0x4da4), LPC_JLK},
>  	{ PCI_VDEVICE(INTEL, 0x5031), LPC_EP80579},
>  	{ PCI_VDEVICE(INTEL, 0x5ae8), LPC_APL},
>  	{ PCI_VDEVICE(INTEL, 0x8c40), LPC_LPT},
> @@ -785,6 +855,7 @@ static const struct pci_device_id lpc_ich_ids[] = {
>  	{ PCI_VDEVICE(INTEL, 0x9c45), LPC_LPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9c46), LPC_LPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9c47), LPC_LPT_LP},
> +	{ PCI_VDEVICE(INTEL, 0x9c66), LPC_CRDG},
>  	{ PCI_VDEVICE(INTEL, 0x9cc1), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc2), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc3), LPC_WPT_LP},
> @@ -792,6 +863,33 @@ static const struct pci_device_id lpc_ich_ids[] =
> {
>  	{ PCI_VDEVICE(INTEL, 0x9cc6), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc7), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc9), LPC_WPT_LP},
> +	{ PCI_VDEVICE(INTEL, 0x9ce6), LPC_WPT_LP},
> +	{ PCI_VDEVICE(INTEL, 0x9d2a), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0x9d4e), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0x9da4), LPC_CNPT},
> +	{ PCI_VDEVICE(INTEL, 0xa0a4), LPC_TLK},
> +	{ PCI_VDEVICE(INTEL, 0xa140), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa141), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa142), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa143), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa144), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa145), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa146), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa147), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa148), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa149), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14a), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14b), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14c), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14d), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14e), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14f), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa150), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa151), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa152), LPC_KLK},
> +	{ PCI_VDEVICE(INTEL, 0xa153), LPC_KLK},
> +	{ PCI_VDEVICE(INTEL, 0xa154), LPC_KLK},
> +	{ PCI_VDEVICE(INTEL, 0xa155), LPC_SPT},
>  	{ PCI_VDEVICE(INTEL, 0xa1c1), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa1c2), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa1c3), LPC_LEWISBURG},
> @@ -801,6 +899,12 @@ static const struct pci_device_id lpc_ich_ids[] =
> {
>  	{ PCI_VDEVICE(INTEL, 0xa1c7), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa242), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa243), LPC_LEWISBURG},
> +	{ PCI_VDEVICE(INTEL, 0xa304), LPC_CNLK},
> +	{ PCI_VDEVICE(INTEL, 0xa305), LPC_CNLK},
> +	{ PCI_VDEVICE(INTEL, 0xa306), LPC_CNLK},
> +	{ PCI_VDEVICE(INTEL, 0xa30c), LPC_CNLK},
> +	{ PCI_VDEVICE(INTEL, 0xa324), LPC_CNLK},
> +	{ PCI_VDEVICE(INTEL, 0xa3a4), LPC_CLK},
>  	{ 0, },			/* End of list */
>  };
>  MODULE_DEVICE_TABLE(pci, lpc_ich_ids);
> @@ -1083,6 +1187,104 @@ static int lpc_ich_init_wdt(struct pci_dev
> *dev)
>  	return ret;
>  }
>  
> +#if IS_ENABLED(CONFIG_SECURITY)
> +static ssize_t bioswe_read(struct file *filp, char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	struct intel_spi_boardinfo *info =
> lpc_ich_spi_cell.platform_data;

I think the patch has some wrapping issues.

> +	char tmp[2];

Wouldn't this need to account the '\0' as well?

> +
> +	sprintf(tmp, "%d\n", info->writeable ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));
> +}
> +
> +static const struct file_operations spi_bioswe_ops = {
> +	.read  = bioswe_read,
> +};
> +
> +static ssize_t ble_read(struct file *filp, char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct intel_spi_boardinfo *info =
> lpc_ich_spi_cell.platform_data;
> +	char tmp[2];
> +
> +	sprintf(tmp, "%d\n", info->ble ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));
> +}
> +
> +static const struct file_operations spi_ble_ops = {
> +	.read  = ble_read,
> +};
> +
> +static ssize_t smm_bwp_read(struct file *filp, char __user *buf,
> +			    size_t count, loff_t *ppos)
> +{
> +	struct intel_spi_boardinfo *info =
> lpc_ich_spi_cell.platform_data;
> +	char tmp[2];
> +
> +	sprintf(tmp, "%d\n", info->smm_bwp ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));
> +}
> +
> +static const struct file_operations spi_smm_bwp_ops = {
> +	.read  = smm_bwp_read,
> +};
> +
> +static int lpc_ich_init_securityfs(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +
> +	priv->spi_dir = securityfs_create_dir("spi", NULL);

I think "spi" is bit too general name here. I would expect "spi" to
actually refer to something connected to spi bus and possibly coming
from drivers/spi/*.

Perhaps "bios_protections" or something like that.

> +	if (IS_ERR(priv->spi_dir))
> +		return -1;
> +
> +	priv->spi_bioswe =
> +	    securityfs_create_file("bioswe",
> +				   0600, priv->spi_dir, dev,
> +				   &spi_bioswe_ops);
> +	if (IS_ERR(priv->spi_bioswe))
> +		goto out;
> +	priv->spi_ble =
> +	    securityfs_create_file("ble",
> +				   0600, priv->spi_dir, dev,
> +				   &spi_ble_ops);
> +	if (IS_ERR(priv->spi_ble))
> +		goto out;
> +	priv->spi_smm_bwp =
> +	    securityfs_create_file("smm_bwp",
> +				   0600, priv->spi_dir, dev,
> +				   &spi_smm_bwp_ops);
> +	if (IS_ERR(priv->spi_smm_bwp))
> +		goto out;
> +	return 0;
> +out:
> +	securityfs_remove(priv->spi_ble);
> +	securityfs_remove(priv->spi_bioswe);
> +	securityfs_remove(priv->spi_dir);
> +	return -1;

I don't know securityfs well enought but I think -1 is not correct here
and if you want that then maybe -EPERM instead.

> +}
> +
> +static void lpc_ich_uninit_securityfs(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +
> +	securityfs_remove(priv->spi_smm_bwp);
> +	securityfs_remove(priv->spi_ble);
> +	securityfs_remove(priv->spi_bioswe);
> +	securityfs_remove(priv->spi_dir);

I wonder if you can simply call 

	securityfs_remove(priv->spi_dir);

and that removes the children automatically? I'm do not know securityfs
so it may not be the case.

> +}
> +#else
> +static inline int lpc_ich_init_securityfs(struct pci_dev *dev) {
> return 0; }
> +static inline void lpc_ich_uninit_securityfs(struct pci_dev *dev) {
> return 0; }
> +#endif
> +
> +static void lpc_ich_init_spi_bcr(struct intel_spi_boardinfo *info, u32
> bcr)
> +{
> +	info->writeable = !!(bcr & BCR_WPD);
> +	info->ble = !!(bcr & BCR_BLE);
> +	info->smm_bwp = !!(bcr & BCR_SMM_BWP);
> +}
> +
>  static int lpc_ich_init_spi(struct pci_dev *dev)
>  {
>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> @@ -1112,9 +1314,14 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  			res->start = spi_base + SPIBASE_LPT;
>  			res->end = res->start + SPIBASE_LPT_SZ - 1;
>  
> -			pci_read_config_dword(dev, BCR, &bcr);
> -			info->writeable = !!(bcr & BCR_WPD);
>  		}
> +		pci_read_config_dword(dev, BCR, &bcr);
> +		lpc_ich_init_spi_bcr(info, bcr);
> +		break;
> +
> +	case INTEL_SPI_LPC:
> +		pci_read_config_dword(dev, BCR, &bcr);
> +		lpc_ich_init_spi_bcr(info, bcr);
>  		break;
>  
>  	case INTEL_SPI_BXT: {
> @@ -1135,7 +1342,7 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  			res->end = res->start + SPIBASE_APL_SZ - 1;
>  
>  			pci_bus_read_config_dword(bus, spi, BCR, &bcr);
> -			info->writeable = !!(bcr & BCR_WPD);
> +			lpc_ich_init_spi_bcr(info, bcr);
>  		}
>  
>  		pci_bus_write_config_byte(bus, p2sb, 0xe1, 0x1);
> @@ -1146,8 +1353,10 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  		return -EINVAL;
>  	}
>  
> -	if (!res->start)
> -		return -ENODEV;
> +	if (info->type != INTEL_SPI_LPC) {
> +		if (!res->start)
> +			return -ENODEV;
> +	}
>  
>  	lpc_ich_spi_cell.platform_data = info;
>  	lpc_ich_spi_cell.pdata_size = sizeof(*info);
> @@ -1201,8 +1410,11 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  
>  	if (lpc_chipset_info[priv->chipset].spi_type) {
>  		ret = lpc_ich_init_spi(dev);
> -		if (!ret)
> +		if (!ret) {
> +			if (lpc_ich_init_securityfs(dev))
> +				return -EINVAL;
>  			cell_added = true;
> +		}
>  	}
>  
>  	/*
> @@ -1221,6 +1433,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  static void lpc_ich_remove(struct pci_dev *dev)
>  {
>  	mfd_remove_devices(&dev->dev);
> +	lpc_ich_uninit_securityfs(dev);
>  	lpc_ich_restore_config_space(dev);
>  }
>  
> diff --git a/include/linux/platform_data/intel-spi.h
> b/include/linux/platform_data/intel-spi.h
> index 7f53a5c6f35e..049cc726f868 100644
> --- a/include/linux/platform_data/intel-spi.h
> +++ b/include/linux/platform_data/intel-spi.h
> @@ -14,16 +14,21 @@ enum intel_spi_type {
>  	INTEL_SPI_LPT,
>  	INTEL_SPI_BXT,
>  	INTEL_SPI_CNL,
> +	INTEL_SPI_LPC,
>  };
>  
>  /**
>   * struct intel_spi_boardinfo - Board specific data for Intel SPI
> driver
>   * @type: Type which this controller is compatible with
> - * @writeable: The chip is writeable
> + * @writeable: The chip is writeable, a.k.a. BIOSWE
> + * @ble: a SMM is raised when setting BIOSWE
> + * @smm_bwp: the BIOS region is non-writable unless all processors are
> in SMM
>   */
>  struct intel_spi_boardinfo {
>  	enum intel_spi_type type;
>  	bool writeable;
> +	bool ble;
> +	bool smm_bwp;

I don't think these belong here. They should be part of the lpc private
structure instead (lpc_ich_priv).

>  };
>  
>  #endif /* INTEL_SPI_PDATA_H */
> 

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

On Wed, 13 May 2020 at 08:08, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I think this one should contain KernelVersion as well, see
> Documentation/ABI/README.

Thanks, I'll fix that up.

> I think you can always include this header without #ifs

Thanks.

> >  static struct resource wdt_ich_res[] = {
> > @@ -221,6 +236,16 @@ enum lpc_chipsets {
> >       LPC_APL,        /* Apollo Lake SoC */
> >       LPC_GLK,        /* Gemini Lake SoC */
> >       LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
> > +     LPC_SPT,        /* Sunrise Point */
> > +     LPC_KLK,        /* Kaby Lake */
> KBL for Kaby Lake

I can fix up all those, but out of interest how did you "know" the
right three digit identifier to use?

> This is not PCH, Cactus Ridge is Thunderbolt host controller AFAIK.

This was suggested from someone testing the original spi_lpc.c code on
a macbook, I can remove it for now and work out if it's incorrect
later.

> For example these PCI IDs are for the SPI-NOR controller (not LPC
> controller) so this causes this driver to try to bind to a completely
> different device which it cannot handle.

I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
this kind of "just expose one byte of PCI config space" functionality.
Certainly drivers/platform/x86/intel_spi_lpc.c is much simpler, and
would also allow me to do some of the chipsec tests in the future --
for instance if BIOSWE is unset but BLE is set, try setting BIOSWE and
check that SMM clears it back.

> > +     char tmp[2];
>
> Wouldn't this need to account the '\0' as well?

It's one char ('1' or '0') plus '`\0` -- no?

> I think "spi" is bit too general name here. I would expect "spi" to
> actually refer to something connected to spi bus and possibly coming
> from drivers/spi/*.
> Perhaps "bios_protections" or something like that.

Sure, that's a good idea. I know BIOS is a badword now, so how about
just "firmware"? so /sys/kernel/security/firmware/bioswe

> > +     securityfs_remove(priv->spi_dir);
> > +     return -1;
> I don't know securityfs well enought but I think -1 is not correct here
> and if you want that then maybe -EPERM instead.

I will look, I don't think the actual value is terribly important. The
only time I can trigger this is forgetting to remove the securityfs
file in module unload, and then trying to re-insert the module --
which failed with -EEXIST from memory.

> I wonder if you can simply call
>         securityfs_remove(priv->spi_dir);
> and that removes the children automatically? I'm do not know securityfs
> so it may not be the case.

No, that doesn't work.

> >  struct intel_spi_boardinfo {
> >       enum intel_spi_type type;
> >       bool writeable;
> > +     bool ble;
> > +     bool smm_bwp;
>
> I don't think these belong here. They should be part of the lpc private
> structure instead (lpc_ich_priv).

Can fix, thanks.

Richard.

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Wed, May 13, 2020 at 09:48:55AM +0100, Richard Hughes wrote:
> On Wed, 13 May 2020 at 08:08, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > I think this one should contain KernelVersion as well, see
> > Documentation/ABI/README.
> 
> Thanks, I'll fix that up.
> 
> > I think you can always include this header without #ifs
> 
> Thanks.
> 
> > >  static struct resource wdt_ich_res[] = {
> > > @@ -221,6 +236,16 @@ enum lpc_chipsets {
> > >       LPC_APL,        /* Apollo Lake SoC */
> > >       LPC_GLK,        /* Gemini Lake SoC */
> > >       LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
> > > +     LPC_SPT,        /* Sunrise Point */
> > > +     LPC_KLK,        /* Kaby Lake */
> > KBL for Kaby Lake
> 
> I can fix up all those, but out of interest how did you "know" the
> right three digit identifier to use?

I work for Intel ;-)

> > This is not PCH, Cactus Ridge is Thunderbolt host controller AFAIK.
> 
> This was suggested from someone testing the original spi_lpc.c code on
> a macbook, I can remove it for now and work out if it's incorrect
> later.

It is definitely incorrect. They are completely different things.

> > For example these PCI IDs are for the SPI-NOR controller (not LPC
> > controller) so this causes this driver to try to bind to a completely
> > different device which it cannot handle.
> 
> I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
> this kind of "just expose one byte of PCI config space" functionality.
> Certainly drivers/platform/x86/intel_spi_lpc.c is much simpler, and
> would also allow me to do some of the chipsec tests in the future --
> for instance if BIOSWE is unset but BLE is set, try setting BIOSWE and
> check that SMM clears it back.

Ideally there is one driver per device. Otherwise we end up issues when
the device appears and there are several to choose from, which one to
pick.

If this is touching the 00:1f.5 PCI device (SPI-NOR controller) then the
right place is the intel-spi-pci.c as that's the driver for this
controller. We can put this there so that it does not enable the SPI-NOR
functionality itself and the mark the SPI-NOR functionality only as
being dangerous or something like that.

> 
> > > +     char tmp[2];
> >
> > Wouldn't this need to account the '\0' as well?
> 
> It's one char ('1' or '0') plus '`\0` -- no?

You sprint() there "%d\n", so that includes a number, '\n' and '\0' unless
I'm missing something.

> > I think "spi" is bit too general name here. I would expect "spi" to
> > actually refer to something connected to spi bus and possibly coming
> > from drivers/spi/*.
> > Perhaps "bios_protections" or something like that.
> 
> Sure, that's a good idea. I know BIOS is a badword now, so how about
> just "firmware"? so /sys/kernel/security/firmware/bioswe

Yup, sounds good :)

> > > +     securityfs_remove(priv->spi_dir);
> > > +     return -1;
> > I don't know securityfs well enought but I think -1 is not correct here
> > and if you want that then maybe -EPERM instead.
> 
> I will look, I don't think the actual value is terribly important. The
> only time I can trigger this is forgetting to remove the securityfs
> file in module unload, and then trying to re-insert the module --
> which failed with -EEXIST from memory.
> 
> > I wonder if you can simply call
> >         securityfs_remove(priv->spi_dir);
> > and that removes the children automatically? I'm do not know securityfs
> > so it may not be the case.
> 
> No, that doesn't work.

OK

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

On Wed, 13 May 2020 at 10:11, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> > I can fix up all those, but out of interest how did you "know" the
> > right three digit identifier to use?
> I work for Intel ;-)

Hah, okay, thanks :)

> > I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
> > this kind of "just expose one byte of PCI config space" functionality.
> Ideally there is one driver per device.

My idea in https://github.com/hughsie/spi_lpc was to not actually
register a pci_driver.

> If this is touching the 00:1f.5 PCI device (SPI-NOR controller) then the
> right place is the intel-spi-pci.c as that's the driver for this
> controller.

So Cannon Lake, Cannon Point and Ice Lake would go into
drivers/mtd/spi-nor/controllers/intel-spi-pci.c and the systems like
Sunrise Point using an ISA bridge would use drivers/mfd/lpc_ich.c?

> We can put this there so that it does not enable the SPI-NOR
> functionality itself and the mark the SPI-NOR functionality only as
> being dangerous or something like that.

I think getting the distros to enable SPI_INTEL_SPI_PCI might be a
tough sell. Could we perhaps remove the DANGEROUS label as it's not
writeable without a module option?

> > > > +     char tmp[2];
> > > Wouldn't this need to account the '\0' as well?
> You sprint() there "%d\n", so that includes a number, '\n' and '\0' unless
> I'm missing something.

Doh, of course you're right. Will fix, thanks.

Richard

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Wed, May 13, 2020 at 03:13:28PM +0100, Richard Hughes wrote:
> On Wed, 13 May 2020 at 10:11, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > > I can fix up all those, but out of interest how did you "know" the
> > > right three digit identifier to use?
> > I work for Intel ;-)
> 
> Hah, okay, thanks :)
> 
> > > I'm really wondering if drivers/mfd/lpc_ich.c is the right place for
> > > this kind of "just expose one byte of PCI config space" functionality.
> > Ideally there is one driver per device.
> 
> My idea in https://github.com/hughsie/spi_lpc was to not actually
> register a pci_driver.

OK, I see the original code iterated over PCI devices finding anything
that matches the IDs in the table.

This may be problematic if there is driver bound to the device and
accessing the hardware simultaneusly. Although this is just read side
and I don't think these registers have any side effects when you read
them, so should not be an issue.

> 
> > If this is touching the 00:1f.5 PCI device (SPI-NOR controller) then the
> > right place is the intel-spi-pci.c as that's the driver for this
> > controller.
> 
> So Cannon Lake, Cannon Point and Ice Lake would go into
> drivers/mtd/spi-nor/controllers/intel-spi-pci.c and the systems like
> Sunrise Point using an ISA bridge would use drivers/mfd/lpc_ich.c?

Yes, something like that.

> > We can put this there so that it does not enable the SPI-NOR
> > functionality itself and the mark the SPI-NOR functionality only as
> > being dangerous or something like that.
> 
> I think getting the distros to enable SPI_INTEL_SPI_PCI might be a
> tough sell. Could we perhaps remove the DANGEROUS label as it's not
> writeable without a module option?

I would like to keep it (the label) there but I think we can label
SPI_INTEL_SPI with that instead and then make the -pci.c to work
standalone if CONFIG_SPI_INTEL_SPI is not enabled.

config SPI_INTEL_SPI
        tristate "Intel PCH/PCU SPI flash core driver (DANGEROUS)"
	depends on SPI_INTEL_SPI_PCI || SPI_INTEL_SPI_PLATFORM
	...

config SPI_INTEL_SPI_PCI
        tristate "Intel PCH/PCU SPI flash PCI driver"
	depends on PCI
	...

Then distros could enable only CONFIG_SPI_INTEL_SPI_PCI which would only
expose the security bits. Of course I'm open to any other ideas :)

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

On Wed, 2020-05-13 at 19:25 +0300, Mika Westerberg wrote:
> This may be problematic if there is driver bound to the device and
> accessing the hardware simultaneusly. Although this is just read side
> and I don't think these registers have any side effects when you read
> them, so should not be an issue.

Right, agreed.

> > So Cannon Lake, Cannon Point and Ice Lake would go into
> > drivers/mtd/spi-nor/controllers/intel-spi-pci.c and the systems
> > like
> > Sunrise Point using an ISA bridge would use drivers/mfd/lpc_ich.c?
> 
> Yes, something like that.

Okay, lets get lpc_ich.c sorted, then I'll look at implementing the
same securityfs interface for intel-spi-pci.c

> I would like to keep it (the label) there but I think we can label
> SPI_INTEL_SPI with that instead and then make the -pci.c to work
> standalone if CONFIG_SPI_INTEL_SPI is not enabled.

Okay, good idea. I'll have a look at this next week after we get the
ISB bridges agreed on.

New patch here, if you want me to split it up a bit please just ask how
you would like it split. I fear Evolution is mangling the patch so I
might have to indeed start using git-send-email. :(

Thanks,

Richard.

From 0b395efde8da7d5099c87945def473ff164d1c4c Mon Sep 17 00:00:00 2001
From: Richard Hughes <richard@hughsie.com>
Date: Mon, 11 May 2020 14:19:27 +0100
Subject: [PATCH] mfd: Export LPC attributes for the system SPI chip

Export standard SPI-specific config values from various LPC
controllers.
This allows userspace components such as fwupd to verify the most basic
SPI
protections are set correctly. For instance, checking BIOSWE is
disabled
and BLE is enabled.

Exporting these values from the kernel allows us to report the security
level of the platform without rebooting and running an EFI binary like
chipsec.

Signed-off-by: Richard Hughes <richard@hughsie.com>
---
 Documentation/ABI/testing/sysfs-security-spi |  23 +++
 drivers/mfd/lpc_ich.c                        | 169 ++++++++++++++++++-
 include/linux/platform_data/intel-spi.h      |   1 +
 3 files changed, 187 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-security-spi

diff --git a/Documentation/ABI/testing/sysfs-security-spi
b/Documentation/ABI/testing/sysfs-security-spi
new file mode 100644
index 000000000000..6805b74d7036
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-security-spi
@@ -0,0 +1,23 @@
+What:		/sys/kernel/security/firmware/bioswe
+Date:		May 2020
+KernelVersion:	5.7.0
+Contact:	richard@hughsie.com
+Description:	If the system firmware set BIOS Write Enable.
+		0: writes disabled, 1: writes enabled.
+Users:		https://github.com/fwupd/fwupd
+
+What:		/sys/kernel/security/firmware/ble
+Date:		May 2020
+KernelVersion:	5.7.0
+Contact:	richard@hughsie.com
+Description:	If the system firmware set Bios Lock Enable.
+		0: SMM lock disabled, 1: SMM lock enabled.
+Users:		https://github.com/fwupd/fwupd
+
+What:		/sys/kernel/security/firmware/smm_bwp
+Date:		May 2020
+KernelVersion:	5.7.0
+Contact:	richard@hughsie.com
+Description:	If the system firmware set SMM Bios Write Protect.
+		0: writes disabled unless in SMM, 1: writes enabled.
+Users:		https://github.com/fwupd/fwupd
diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
index 3bbb29a7e7a5..fab017efdb9d 100644
--- a/drivers/mfd/lpc_ich.c
+++ b/drivers/mfd/lpc_ich.c
@@ -33,6 +33,7 @@
  *	document number 322169-001, 322170-003: 5 Series, 3400 Series
(PCH)
  *	document number 320066-003, 320257-008: EP80597 (IICH)
  *	document number 324645-001, 324646-001: Cougar Point (CPT)
+ *	document number 332690-006, 332691-003: C230 (CPT)
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -40,6 +41,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/errno.h>
+#include <linux/security.h>
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/mfd/core.h>
@@ -68,6 +70,8 @@
 #define SPIBASE_LPT_SZ		512
 #define BCR			0xdc
 #define BCR_WPD			BIT(0)
+#define BCR_BLE			BIT(1)
+#define BCR_SMM_BWP		BIT(5)
 
 #define SPIBASE_APL_SZ		4096
 
@@ -93,6 +97,11 @@ struct lpc_ich_priv {
 	int abase_save;		/* Cached ACPI base value */
 	int actrl_pbase_save;		/* Cached ACPI control or PMC
base value */
 	int gctrl_save;		/* Cached GPIO control value */
+
+	struct dentry *firmware_dir;	/* SecurityFS entries */
+	struct dentry *firmware_bioswe;
+	struct dentry *firmware_ble;
+	struct dentry *firmware_smm_bwp;
 };
 
 static struct resource wdt_ich_res[] = {
@@ -221,6 +230,9 @@ enum lpc_chipsets {
 	LPC_APL,	/* Apollo Lake SoC */
 	LPC_GLK,	/* Gemini Lake SoC */
 	LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
+	LPC_SPT,	/* Sunrise Point */
+	LPC_KBL,	/* Kaby Lake */
+	LPC_TGL,	/* Tiger Lake */
 };
 
 static struct lpc_ich_info lpc_chipset_info[] = {
@@ -557,6 +569,18 @@ static struct lpc_ich_info lpc_chipset_info[] = {
 		.name = "Cougar Mountain SoC",
 		.iTCO_version = 3,
 	},
+	[LPC_SPT] = {
+		.name = "Sunrise Point",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_KBL] = {
+		.name = "Kaby Lake-H",
+		.spi_type = INTEL_SPI_LPC,
+	},
+	[LPC_TGL] = {
+		.name = "Tiger Lake",
+		.spi_type = INTEL_SPI_LPC,
+	},
 };
 
 /*
@@ -792,6 +816,32 @@ static const struct pci_device_id lpc_ich_ids[] =
{
 	{ PCI_VDEVICE(INTEL, 0x9cc6), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc7), LPC_WPT_LP},
 	{ PCI_VDEVICE(INTEL, 0x9cc9), LPC_WPT_LP},
+	{ PCI_VDEVICE(INTEL, 0x9ce6), LPC_WPT_LP},
+	{ PCI_VDEVICE(INTEL, 0x9d2a), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0x9d4e), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa0a4), LPC_TGL},
+	{ PCI_VDEVICE(INTEL, 0xa140), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa141), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa142), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa143), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa144), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa145), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa146), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa147), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa148), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa149), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14a), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14b), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14c), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14d), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14e), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa14f), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa150), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa151), LPC_SPT},
+	{ PCI_VDEVICE(INTEL, 0xa152), LPC_KBL},
+	{ PCI_VDEVICE(INTEL, 0xa153), LPC_KBL},
+	{ PCI_VDEVICE(INTEL, 0xa154), LPC_KBL},
+	{ PCI_VDEVICE(INTEL, 0xa155), LPC_SPT},
 	{ PCI_VDEVICE(INTEL, 0xa1c1), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa1c2), LPC_LEWISBURG},
 	{ PCI_VDEVICE(INTEL, 0xa1c3), LPC_LEWISBURG},
@@ -1083,6 +1133,103 @@ static int lpc_ich_init_wdt(struct pci_dev
*dev)
 	return ret;
 }
 
+#if IS_ENABLED(CONFIG_SECURITY)
+static ssize_t bioswe_read(struct file *filp, char __user *buf,
+			   size_t count, loff_t *ppos)
+{
+	struct pci_dev *dev = filp->f_inode->i_private;
+	char tmp[3];
+	u32 bcr;
+
+	pci_read_config_dword(dev, BCR, &bcr);
+	sprintf(tmp, "%d\n", bcr & BCR_WPD ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_bioswe_ops = {
+	.read  = bioswe_read,
+};
+
+static ssize_t ble_read(struct file *filp, char __user *buf,
+			size_t count, loff_t *ppos)
+{
+	struct pci_dev *dev = filp->f_inode->i_private;
+	char tmp[3];
+	u32 bcr;
+
+	pci_read_config_dword(dev, BCR, &bcr);
+	sprintf(tmp, "%d\n", bcr & BCR_BLE ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_ble_ops = {
+	.read  = ble_read,
+};
+
+static ssize_t smm_bwp_read(struct file *filp, char __user *buf,
+			    size_t count, loff_t *ppos)
+{
+	struct pci_dev *dev = filp->f_inode->i_private;
+	char tmp[3];
+	u32 bcr;
+
+	pci_read_config_dword(dev, BCR, &bcr);
+	sprintf(tmp, "%d\n", bcr & BCR_SMM_BWP ? 1 : 0);
+	return simple_read_from_buffer(buf, count, ppos, tmp,
sizeof(tmp));
+}
+
+static const struct file_operations spi_smm_bwp_ops = {
+	.read  = smm_bwp_read,
+};
+
+static int lpc_ich_init_securityfs(struct pci_dev *dev)
+{
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	priv->firmware_dir = securityfs_create_dir("firmware", NULL);
+	if (IS_ERR(priv->firmware_dir))
+		return -1;
+
+	priv->firmware_bioswe =
+	    securityfs_create_file("bioswe",
+				   0600, priv->firmware_dir, dev,
+				   &spi_bioswe_ops);
+	if (IS_ERR(priv->firmware_bioswe))
+		goto out;
+	priv->firmware_ble =
+	    securityfs_create_file("ble",
+				   0600, priv->firmware_dir, dev,
+				   &spi_ble_ops);
+	if (IS_ERR(priv->firmware_ble))
+		goto out;
+	priv->firmware_smm_bwp =
+	    securityfs_create_file("smm_bwp",
+				   0600, priv->firmware_dir, dev,
+				   &spi_smm_bwp_ops);
+	if (IS_ERR(priv->firmware_smm_bwp))
+		goto out;
+	return 0;
+out:
+	securityfs_remove(priv->firmware_ble);
+	securityfs_remove(priv->firmware_bioswe);
+	securityfs_remove(priv->firmware_dir);
+	return -1;
+}
+
+static void lpc_ich_uninit_securityfs(struct pci_dev *dev)
+{
+	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
+
+	securityfs_remove(priv->firmware_smm_bwp);
+	securityfs_remove(priv->firmware_ble);
+	securityfs_remove(priv->firmware_bioswe);
+	securityfs_remove(priv->firmware_dir);
+}
+#else
+static inline int lpc_ich_init_securityfs(struct pci_dev *dev) {
return 0; }
+static inline void lpc_ich_uninit_securityfs(struct pci_dev *dev) {
return 0; }
+#endif
+
 static int lpc_ich_init_spi(struct pci_dev *dev)
 {
 	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
@@ -1111,10 +1258,14 @@ static int lpc_ich_init_spi(struct pci_dev
*dev)
 			spi_base = round_down(rcba, SPIBASE_LPT_SZ);
 			res->start = spi_base + SPIBASE_LPT;
 			res->end = res->start + SPIBASE_LPT_SZ - 1;
-
-			pci_read_config_dword(dev, BCR, &bcr);
-			info->writeable = !!(bcr & BCR_WPD);
 		}
+		pci_read_config_dword(dev, BCR, &bcr);
+		info->writeable = !!(bcr & BCR_WPD);
+		break;
+
+	case INTEL_SPI_LPC:
+		pci_read_config_dword(dev, BCR, &bcr);
+		info->writeable = !!(bcr & BCR_WPD);
 		break;
 
 	case INTEL_SPI_BXT: {
@@ -1146,8 +1297,10 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
 		return -EINVAL;
 	}
 
-	if (!res->start)
-		return -ENODEV;
+	if (info->type != INTEL_SPI_LPC) {
+		if (!res->start)
+			return -ENODEV;
+	}
 
 	lpc_ich_spi_cell.platform_data = info;
 	lpc_ich_spi_cell.pdata_size = sizeof(*info);
@@ -1201,8 +1354,11 @@ static int lpc_ich_probe(struct pci_dev *dev,
 
 	if (lpc_chipset_info[priv->chipset].spi_type) {
 		ret = lpc_ich_init_spi(dev);
-		if (!ret)
+		if (!ret) {
+			if (lpc_ich_init_securityfs(dev))
+				return -EINVAL;
 			cell_added = true;
+		}
 	}
 
 	/*
@@ -1221,6 +1377,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
 static void lpc_ich_remove(struct pci_dev *dev)
 {
 	mfd_remove_devices(&dev->dev);
+	lpc_ich_uninit_securityfs(dev);
 	lpc_ich_restore_config_space(dev);
 }
 
diff --git a/include/linux/platform_data/intel-spi.h
b/include/linux/platform_data/intel-spi.h
index 7f53a5c6f35e..ed5b527cf1c6 100644
--- a/include/linux/platform_data/intel-spi.h
+++ b/include/linux/platform_data/intel-spi.h
@@ -14,6 +14,7 @@ enum intel_spi_type {
 	INTEL_SPI_LPT,
 	INTEL_SPI_BXT,
 	INTEL_SPI_CNL,
+	INTEL_SPI_LPC,
 };
 
 /**
-- 
2.26.2

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Wed, May 13, 2020 at 07:27:50PM +0100, Richard Hughes wrote:
> On Wed, 2020-05-13 at 19:25 +0300, Mika Westerberg wrote:
> > This may be problematic if there is driver bound to the device and
> > accessing the hardware simultaneusly. Although this is just read side
> > and I don't think these registers have any side effects when you read
> > them, so should not be an issue.
> 
> Right, agreed.
> 
> > > So Cannon Lake, Cannon Point and Ice Lake would go into
> > > drivers/mtd/spi-nor/controllers/intel-spi-pci.c and the systems
> > > like
> > > Sunrise Point using an ISA bridge would use drivers/mfd/lpc_ich.c?
> > 
> > Yes, something like that.
> 
> Okay, lets get lpc_ich.c sorted, then I'll look at implementing the
> same securityfs interface for intel-spi-pci.c

Yup, sounds reasonable.

> > I would like to keep it (the label) there but I think we can label
> > SPI_INTEL_SPI with that instead and then make the -pci.c to work
> > standalone if CONFIG_SPI_INTEL_SPI is not enabled.
> 
> Okay, good idea. I'll have a look at this next week after we get the
> ISB bridges agreed on.
> 
> New patch here, if you want me to split it up a bit please just ask how
> you would like it split. I fear Evolution is mangling the patch so I
> might have to indeed start using git-send-email. :(

Right, I'm using git send-email and it works reasonably well.

I think Lee is the one who takes this so what he prefers. I suggest
using git send-email.

> Thanks,
> 
> Richard.
> 
> >From 0b395efde8da7d5099c87945def473ff164d1c4c Mon Sep 17 00:00:00 2001
> From: Richard Hughes <richard@hughsie.com>
> Date: Mon, 11 May 2020 14:19:27 +0100
> Subject: [PATCH] mfd: Export LPC attributes for the system SPI chip
> 
> Export standard SPI-specific config values from various LPC
> controllers.
> This allows userspace components such as fwupd to verify the most basic
> SPI
> protections are set correctly. For instance, checking BIOSWE is
> disabled
> and BLE is enabled.
> 
> Exporting these values from the kernel allows us to report the security
> level of the platform without rebooting and running an EFI binary like
> chipsec.
> 
> Signed-off-by: Richard Hughes <richard@hughsie.com>
> ---
>  Documentation/ABI/testing/sysfs-security-spi |  23 +++
>  drivers/mfd/lpc_ich.c                        | 169 ++++++++++++++++++-
>  include/linux/platform_data/intel-spi.h      |   1 +
>  3 files changed, 187 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-security-spi
> 
> diff --git a/Documentation/ABI/testing/sysfs-security-spi
> b/Documentation/ABI/testing/sysfs-security-spi
> new file mode 100644
> index 000000000000..6805b74d7036
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-security-spi
> @@ -0,0 +1,23 @@
> +What:		/sys/kernel/security/firmware/bioswe

Should this still be "firmware_protections" or similar. Plain "firmware"
sounds again too generic. Maybe its just me..

> +Date:		May 2020
> +KernelVersion:	5.7.0
> +Contact:	richard@hughsie.com
> +Description:	If the system firmware set BIOS Write Enable.
> +		0: writes disabled, 1: writes enabled.
> +Users:		https://github.com/fwupd/fwupd
> +
> +What:		/sys/kernel/security/firmware/ble
> +Date:		May 2020
> +KernelVersion:	5.7.0
> +Contact:	richard@hughsie.com
> +Description:	If the system firmware set Bios Lock Enable.
> +		0: SMM lock disabled, 1: SMM lock enabled.
> +Users:		https://github.com/fwupd/fwupd
> +
> +What:		/sys/kernel/security/firmware/smm_bwp
> +Date:		May 2020
> +KernelVersion:	5.7.0
> +Contact:	richard@hughsie.com
> +Description:	If the system firmware set SMM Bios Write Protect.
> +		0: writes disabled unless in SMM, 1: writes enabled.
> +Users:		https://github.com/fwupd/fwupd
> diff --git a/drivers/mfd/lpc_ich.c b/drivers/mfd/lpc_ich.c
> index 3bbb29a7e7a5..fab017efdb9d 100644
> --- a/drivers/mfd/lpc_ich.c
> +++ b/drivers/mfd/lpc_ich.c
> @@ -33,6 +33,7 @@
>   *	document number 322169-001, 322170-003: 5 Series, 3400 Series
> (PCH)
>   *	document number 320066-003, 320257-008: EP80597 (IICH)
>   *	document number 324645-001, 324646-001: Cougar Point (CPT)
> + *	document number 332690-006, 332691-003: C230 (CPT)
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -40,6 +41,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/errno.h>
> +#include <linux/security.h>
>  #include <linux/acpi.h>
>  #include <linux/pci.h>
>  #include <linux/mfd/core.h>
> @@ -68,6 +70,8 @@
>  #define SPIBASE_LPT_SZ		512
>  #define BCR			0xdc
>  #define BCR_WPD			BIT(0)
> +#define BCR_BLE			BIT(1)
> +#define BCR_SMM_BWP		BIT(5)
>  
>  #define SPIBASE_APL_SZ		4096
>  
> @@ -93,6 +97,11 @@ struct lpc_ich_priv {
>  	int abase_save;		/* Cached ACPI base value */
>  	int actrl_pbase_save;		/* Cached ACPI control or PMC
> base value */
>  	int gctrl_save;		/* Cached GPIO control value */
> +
> +	struct dentry *firmware_dir;	/* SecurityFS entries */
> +	struct dentry *firmware_bioswe;
> +	struct dentry *firmware_ble;
> +	struct dentry *firmware_smm_bwp;
>  };
>  
>  static struct resource wdt_ich_res[] = {
> @@ -221,6 +230,9 @@ enum lpc_chipsets {
>  	LPC_APL,	/* Apollo Lake SoC */
>  	LPC_GLK,	/* Gemini Lake SoC */
>  	LPC_COUGARMOUNTAIN,/* Cougar Mountain SoC*/
> +	LPC_SPT,	/* Sunrise Point */
> +	LPC_KBL,	/* Kaby Lake */
> +	LPC_TGL,	/* Tiger Lake */

These all have the SPI-NOR controller as separate PCI device (as ICL and
others).

I suggest you to add this feature to the existing entries instead of
adding these PCI IDs if you can't test them.

There are bunch of existing devices listed there that set .spi_type.

>  };
>  
>  static struct lpc_ich_info lpc_chipset_info[] = {
> @@ -557,6 +569,18 @@ static struct lpc_ich_info lpc_chipset_info[] = {
>  		.name = "Cougar Mountain SoC",
>  		.iTCO_version = 3,
>  	},
> +	[LPC_SPT] = {
> +		.name = "Sunrise Point",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_KBL] = {
> +		.name = "Kaby Lake-H",
> +		.spi_type = INTEL_SPI_LPC,
> +	},
> +	[LPC_TGL] = {
> +		.name = "Tiger Lake",
> +		.spi_type = INTEL_SPI_LPC,

So all of these have LCP/eSPI controller but the SPI-NOR controller is
not accessible through it - it is a separate PCI device.

> +	},
>  };
>  
>  /*
> @@ -792,6 +816,32 @@ static const struct pci_device_id lpc_ich_ids[] =
> {
>  	{ PCI_VDEVICE(INTEL, 0x9cc6), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc7), LPC_WPT_LP},
>  	{ PCI_VDEVICE(INTEL, 0x9cc9), LPC_WPT_LP},
> +	{ PCI_VDEVICE(INTEL, 0x9ce6), LPC_WPT_LP},
> +	{ PCI_VDEVICE(INTEL, 0x9d2a), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0x9d4e), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa0a4), LPC_TGL},
> +	{ PCI_VDEVICE(INTEL, 0xa140), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa141), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa142), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa143), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa144), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa145), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa146), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa147), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa148), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa149), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14a), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14b), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14c), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14d), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14e), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa14f), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa150), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa151), LPC_SPT},
> +	{ PCI_VDEVICE(INTEL, 0xa152), LPC_KBL},
> +	{ PCI_VDEVICE(INTEL, 0xa153), LPC_KBL},
> +	{ PCI_VDEVICE(INTEL, 0xa154), LPC_KBL},
> +	{ PCI_VDEVICE(INTEL, 0xa155), LPC_SPT},
>  	{ PCI_VDEVICE(INTEL, 0xa1c1), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa1c2), LPC_LEWISBURG},
>  	{ PCI_VDEVICE(INTEL, 0xa1c3), LPC_LEWISBURG},
> @@ -1083,6 +1133,103 @@ static int lpc_ich_init_wdt(struct pci_dev
> *dev)
>  	return ret;
>  }
>  
> +#if IS_ENABLED(CONFIG_SECURITY)
> +static ssize_t bioswe_read(struct file *filp, char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	struct pci_dev *dev = filp->f_inode->i_private;
> +	char tmp[3];
> +	u32 bcr;
> +
> +	pci_read_config_dword(dev, BCR, &bcr);
> +	sprintf(tmp, "%d\n", bcr & BCR_WPD ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));

Like you said, Evolution seems to mangle these.

> +}
> +
> +static const struct file_operations spi_bioswe_ops = {
> +	.read  = bioswe_read,
> +};
> +
> +static ssize_t ble_read(struct file *filp, char __user *buf,
> +			size_t count, loff_t *ppos)
> +{
> +	struct pci_dev *dev = filp->f_inode->i_private;
> +	char tmp[3];
> +	u32 bcr;
> +
> +	pci_read_config_dword(dev, BCR, &bcr);
> +	sprintf(tmp, "%d\n", bcr & BCR_BLE ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));
> +}
> +
> +static const struct file_operations spi_ble_ops = {
> +	.read  = ble_read,
> +};
> +
> +static ssize_t smm_bwp_read(struct file *filp, char __user *buf,
> +			    size_t count, loff_t *ppos)
> +{
> +	struct pci_dev *dev = filp->f_inode->i_private;
> +	char tmp[3];
> +	u32 bcr;
> +
> +	pci_read_config_dword(dev, BCR, &bcr);
> +	sprintf(tmp, "%d\n", bcr & BCR_SMM_BWP ? 1 : 0);
> +	return simple_read_from_buffer(buf, count, ppos, tmp,
> sizeof(tmp));
> +}
> +
> +static const struct file_operations spi_smm_bwp_ops = {
> +	.read  = smm_bwp_read,
> +};
> +
> +static int lpc_ich_init_securityfs(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +
> +	priv->firmware_dir = securityfs_create_dir("firmware", NULL);
> +	if (IS_ERR(priv->firmware_dir))
> +		return -1;
> +
> +	priv->firmware_bioswe =
> +	    securityfs_create_file("bioswe",
> +				   0600, priv->firmware_dir, dev,
> +				   &spi_bioswe_ops);
> +	if (IS_ERR(priv->firmware_bioswe))
> +		goto out;
> +	priv->firmware_ble =
> +	    securityfs_create_file("ble",
> +				   0600, priv->firmware_dir, dev,
> +				   &spi_ble_ops);
> +	if (IS_ERR(priv->firmware_ble))
> +		goto out;
> +	priv->firmware_smm_bwp =
> +	    securityfs_create_file("smm_bwp",
> +				   0600, priv->firmware_dir, dev,
> +				   &spi_smm_bwp_ops);
> +	if (IS_ERR(priv->firmware_smm_bwp))
> +		goto out;
> +	return 0;
> +out:
> +	securityfs_remove(priv->firmware_ble);
> +	securityfs_remove(priv->firmware_bioswe);
> +	securityfs_remove(priv->firmware_dir);
> +	return -1;
> +}
> +
> +static void lpc_ich_uninit_securityfs(struct pci_dev *dev)
> +{
> +	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> +
> +	securityfs_remove(priv->firmware_smm_bwp);
> +	securityfs_remove(priv->firmware_ble);
> +	securityfs_remove(priv->firmware_bioswe);
> +	securityfs_remove(priv->firmware_dir);
> +}
> +#else
> +static inline int lpc_ich_init_securityfs(struct pci_dev *dev) {
> return 0; }
> +static inline void lpc_ich_uninit_securityfs(struct pci_dev *dev) {
> return 0; }
> +#endif
> +
>  static int lpc_ich_init_spi(struct pci_dev *dev)
>  {
>  	struct lpc_ich_priv *priv = pci_get_drvdata(dev);
> @@ -1111,10 +1258,14 @@ static int lpc_ich_init_spi(struct pci_dev
> *dev)
>  			spi_base = round_down(rcba, SPIBASE_LPT_SZ);
>  			res->start = spi_base + SPIBASE_LPT;
>  			res->end = res->start + SPIBASE_LPT_SZ - 1;
> -
> -			pci_read_config_dword(dev, BCR, &bcr);
> -			info->writeable = !!(bcr & BCR_WPD);
>  		}
> +		pci_read_config_dword(dev, BCR, &bcr);
> +		info->writeable = !!(bcr & BCR_WPD);
> +		break;
> +
> +	case INTEL_SPI_LPC:

So instead of this, you can add the security attributes to the existing
entries where we are sure there is SPI-NOR controller behind LPC. Here
it is not the case and further..

> +		pci_read_config_dword(dev, BCR, &bcr);
> +		info->writeable = !!(bcr & BCR_WPD);
>  		break;
>  
>  	case INTEL_SPI_BXT: {
> @@ -1146,8 +1297,10 @@ static int lpc_ich_init_spi(struct pci_dev *dev)
>  		return -EINVAL;
>  	}
>  
> -	if (!res->start)
> -		return -ENODEV;
> +	if (info->type != INTEL_SPI_LPC) {

.. we can avoid checks like this ;-)

> +		if (!res->start)
> +			return -ENODEV;
> +	}
>  
>  	lpc_ich_spi_cell.platform_data = info;
>  	lpc_ich_spi_cell.pdata_size = sizeof(*info);
> @@ -1201,8 +1354,11 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  
>  	if (lpc_chipset_info[priv->chipset].spi_type) {
>  		ret = lpc_ich_init_spi(dev);
> -		if (!ret)
> +		if (!ret) {
> +			if (lpc_ich_init_securityfs(dev))
> +				return -EINVAL;
>  			cell_added = true;
> +		}
>  	}
>  
>  	/*
> @@ -1221,6 +1377,7 @@ static int lpc_ich_probe(struct pci_dev *dev,
>  static void lpc_ich_remove(struct pci_dev *dev)
>  {
>  	mfd_remove_devices(&dev->dev);
> +	lpc_ich_uninit_securityfs(dev);
>  	lpc_ich_restore_config_space(dev);
>  }
>  
> diff --git a/include/linux/platform_data/intel-spi.h
> b/include/linux/platform_data/intel-spi.h
> index 7f53a5c6f35e..ed5b527cf1c6 100644
> --- a/include/linux/platform_data/intel-spi.h
> +++ b/include/linux/platform_data/intel-spi.h
> @@ -14,6 +14,7 @@ enum intel_spi_type {
>  	INTEL_SPI_LPT,
>  	INTEL_SPI_BXT,
>  	INTEL_SPI_CNL,
> +	INTEL_SPI_LPC,

So I would not add this type here at all. Just add the security nodes to
the existing entries for now. Rest need to be part of the
intel-spi-pci.c, I think.

Otherwise this looks good, nice work :)

>  };
>  
>  /**
> -- 
> 2.26.2
> 
> 
> 

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Richard Hughes]

On Thu, 14 May 2020 at 13:15, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> > +What:                /sys/kernel/security/firmware/bioswe
> Should this still be "firmware_protections" or similar. Plain "firmware"
> sounds again too generic. Maybe its just me..

It's not always going to be protections provided by the firmware; it
might also be restrictions put on the firmware. My first choice was
/sys/kernel/security/firmware_security/ but having the double
'security' just looked redundant.

> > +     LPC_SPT,        /* Sunrise Point */
> > +     LPC_KBL,        /* Kaby Lake */
> > +     LPC_TGL,        /* Tiger Lake */
>
> These all have the SPI-NOR controller as separate PCI device (as ICL and
> others).

For Sunrise Point I see:

00:1f.0 ISA bridge [0601]: Intel Corporation CM236 Chipset LPC/eSPI
Controller [8086:a150] (rev 31)
00:1f.0 ISA bridge [0601]: Intel Corporation Sunrise Point LPC
Controller/eSPI Controller [8086:9d4e] (rev 21)

For Kaby Lake I see:

00:1f.0 ISA bridge [0601]: Intel Corporation HM175 Chipset LPC/eSPI
Controller [8086:a152] (rev 31)",

You're indeed correct about Tiger Lake, my apologies.

> > +     [LPC_SPT] = {
> > +             .name = "Sunrise Point",
> > +             .spi_type = INTEL_SPI_LPC,
> > +     },
>
> So all of these have LCP/eSPI controller but the SPI-NOR controller is
> not accessible through it - it is a separate PCI device.

I have a Sunrise Point system here -- the lspci is here:
https://people.freedesktop.org/~hughsient/temp/lspci.txt

Is the SPI-NOR controller perhaps hidden? If I read the BCR @ 0xdc
from the 00:1f.0 ISB bridge I get the expected BIOS_WE, BLE and
SMM_BWP results.

> Like you said, Evolution seems to mangle these.

I'll use git for future patches, thanks.

> > +             pci_read_config_dword(dev, BCR, &bcr);
> > +             info->writeable = !!(bcr & BCR_WPD);
> > +             break;
> > +
> > +     case INTEL_SPI_LPC:
>
> So instead of this, you can add the security attributes to the existing
> entries where we are sure there is SPI-NOR controller behind LPC. Here
> it is not the case and further..

Sooo I'd use INTEL_SPI_LPT? On my system RCBA isn't set, and so "if
(!res->start)" bails out with  return -ENODEV;"

> Otherwise this looks good, nice work :)

I thank you for your patience so far, what I've got the hang of this I
promise to start being more useful.

Richard.

Re: [PATCH] mfd: Export LPC attributes for the system SPI chip

[Mika Westerberg]

On Thu, May 14, 2020 at 01:53:23PM +0100, Richard Hughes wrote:
> On Thu, 14 May 2020 at 13:15, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > > +What:                /sys/kernel/security/firmware/bioswe
> > Should this still be "firmware_protections" or similar. Plain "firmware"
> > sounds again too generic. Maybe its just me..
> 
> It's not always going to be protections provided by the firmware; it
> might also be restrictions put on the firmware. My first choice was
> /sys/kernel/security/firmware_security/ but having the double
> 'security' just looked redundant.

OK.

> > > +     LPC_SPT,        /* Sunrise Point */
> > > +     LPC_KBL,        /* Kaby Lake */
> > > +     LPC_TGL,        /* Tiger Lake */
> >
> > These all have the SPI-NOR controller as separate PCI device (as ICL and
> > others).
> 
> For Sunrise Point I see:
> 
> 00:1f.0 ISA bridge [0601]: Intel Corporation CM236 Chipset LPC/eSPI
> Controller [8086:a150] (rev 31)
> 00:1f.0 ISA bridge [0601]: Intel Corporation Sunrise Point LPC
> Controller/eSPI Controller [8086:9d4e] (rev 21)
> 
> For Kaby Lake I see:
> 
> 00:1f.0 ISA bridge [0601]: Intel Corporation HM175 Chipset LPC/eSPI
> Controller [8086:a152] (rev 31)",

Yes, both of these have LPC device (1f.0) but the SPI-NOR controller is
separate PCI device and most likely hidden.

> You're indeed correct about Tiger Lake, my apologies.
> 
> > > +     [LPC_SPT] = {
> > > +             .name = "Sunrise Point",
> > > +             .spi_type = INTEL_SPI_LPC,
> > > +     },
> >
> > So all of these have LCP/eSPI controller but the SPI-NOR controller is
> > not accessible through it - it is a separate PCI device.
> 
> I have a Sunrise Point system here -- the lspci is here:
> https://people.freedesktop.org/~hughsient/temp/lspci.txt
> 
> Is the SPI-NOR controller perhaps hidden? If I read the BCR @ 0xdc
> from the 00:1f.0 ISB bridge I get the expected BIOS_WE, BLE and
> SMM_BWP results.

OK, I checked datasheet of KBL and indeed the LPC still has the BIOS
Control (BC) register at 0xdc so that should work. Incidently the same
register is part of the SPI-NOR controller register set.

> > Like you said, Evolution seems to mangle these.
> 
> I'll use git for future patches, thanks.
> 
> > > +             pci_read_config_dword(dev, BCR, &bcr);
> > > +             info->writeable = !!(bcr & BCR_WPD);
> > > +             break;
> > > +
> > > +     case INTEL_SPI_LPC:
> >
> > So instead of this, you can add the security attributes to the existing
> > entries where we are sure there is SPI-NOR controller behind LPC. Here
> > it is not the case and further..
> 
> Sooo I'd use INTEL_SPI_LPT? On my system RCBA isn't set, and so "if
> (!res->start)" bails out with  return -ENODEV;"

I think the INTEL_SPI_LPC is slightly misleading because the SPI is not
accessible through LPC. Instead what if we read the BIOS control
register first in lpc_ich_init_spi() and then bail out since .spi_type
is not set?

Probably we can rename the function lpc_ich_init_spi() to
lpc_ich_init_security_and_spi() or something like that.