platform/x86: Export LPC attributes for the system SPI chip

Message ID	18e48255d68a1408b3e3152780f0e789df540059.camel@gmail.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=wiIN=6U=vger.kernel.org=linux-security-module-owner@kernel.org> Message-ID: <18e48255d68a1408b3e3152780f0e789df540059.camel@gmail.com> Subject: [PATCH] platform/x86: Export LPC attributes for the system SPI chip From: Richard Hughes <hughsient@gmail.com> To: platform-driver-x86@vger.kernel.org Cc: linux-security-module@vger.kernel.org Date: Wed, 06 May 2020 16:52:46 +0100 Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.36.2 (3.36.2-1.fc32) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: owner-linux-security-module@vger.kernel.org Precedence: bulk
Series	platform/x86: Export LPC attributes for the system SPI chip \| expand platform/x86: Export LPC attributes for the system SPI chip

Richard Hughes May 6, 2020, 3:52 p.m. UTC

Export standard LPC configuration values from various LPC/eSPI
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.

More cutting-edge checks (e.g. PRx and BootGuard) can be added
once the basics are in place. Exporting these values from the
kernel allows us to report the security level of the platform
without rebooting and running an unsigned EFI binary like
chipsec.

Signed-off-by: Richard Hughes <richard@hughsie.com>
---
 MAINTAINERS                          |   6 +
 drivers/platform/x86/Kconfig         |  10 ++
 drivers/platform/x86/Makefile        |   1 +
 drivers/platform/x86/intel_spi_lpc.c | 183 +++++++++++++++++++++++++++
 4 files changed, 200 insertions(+)
 create mode 100644 drivers/platform/x86/intel_spi_lpc.c

+	return -1;
+}
+
+static void __exit mod_exit(void)
+{
+	securityfs_remove(spi_bioswe);
+	securityfs_remove(spi_ble);
+	securityfs_remove(spi_smm_bwp);
+	securityfs_remove(spi_dir);
+}
+
+module_init(mod_init);
+module_exit(mod_exit);
+
+MODULE_DESCRIPTION("SPI LPC flash platform security driver");
+MODULE_AUTHOR("Richard Hughes <richard@hughsie.com>");
+MODULE_LICENSE("GPL");

Andy Shevchenko May 6, 2020, 4:29 p.m. UTC | #1

On Wed, May 6, 2020 at 6:55 PM Richard Hughes <hughsient@gmail.com> wrote:
>
> Export standard LPC configuration values from various LPC/eSPI
> 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.
>
> More cutting-edge checks (e.g. PRx and BootGuard) can be added
> once the basics are in place. Exporting these values from the
> kernel allows us to report the security level of the platform
> without rebooting and running an unsigned EFI binary like
> chipsec.
>

Isn't it covered by Intel SPI NOR driver?

Richard Hughes May 6, 2020, 4:39 p.m. UTC | #2

On Wed, 6 May 2020 at 17:29, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> Isn't it covered by Intel SPI NOR driver?

No, I don't think so. The Intel eSPI controllers don't operate at the
NOR level. This patch really just exports some PCI configuration (LPC)
registers to userspace.

Richard.

Javier Martinez Canillas May 7, 2020, 5:05 p.m. UTC | #3

Hello Richard,

On Wed, May 6, 2020 at 5:52 PM Richard Hughes <hughsient@gmail.com> wrote:
>
> Export standard LPC configuration values from various LPC/eSPI
> 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.
>
> More cutting-edge checks (e.g. PRx and BootGuard) can be added
> once the basics are in place. Exporting these values from the
> kernel allows us to report the security level of the platform
> without rebooting and running an unsigned EFI binary like
> chipsec.
>
> Signed-off-by: Richard Hughes <richard@hughsie.com>
> ---

The patch looks good to me, I just have some small comments.

> +config INTEL_SPI_LPC
> +       tristate "Intel SPI LPC configuration"
> +       depends on SECURITY

Maybe "depends on SECURITYFS" instead? I would also add ||
COMPILE_TEST to have more build coverage.

Another option is to not even add a dependency here since the
securityfs_*() functions are still defined if SECURITYFS isn't
enabled. They just return -ENODEV.

[snip]

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

I don't think this is needed since if smm_bwp was successfully created
then it will never reach the error handling.

> +       securityfs_remove(spi_dir);

The convention is to remove these in the reverse order that were created, i.e:

securityfs_remove(spi_ble);
securityfs_remove(spi_bioswe);
securityfs_remove(spi_dir);

> +static void __exit mod_exit(void)
> +{
> +       securityfs_remove(spi_bioswe);
> +       securityfs_remove(spi_ble);
> +       securityfs_remove(spi_smm_bwp);
> +       securityfs_remove(spi_dir);
> +}
> +

Same here. It makes it easier if in the future other entries are added.

I wonder if these new entries should be documented in
Documentation/ABI/. Or maybe that's not a requirement for securityfs?

Best regards,
Javier

Andy Shevchenko May 7, 2020, 5:22 p.m. UTC | #4

On Thu, May 7, 2020 at 8:06 PM Javier Martinez Canillas
<javier@dowhile0.org> wrote:
> On Wed, May 6, 2020 at 5:52 PM Richard Hughes <hughsient@gmail.com> wrote:

> I wonder if these new entries should be documented in
> Documentation/ABI/. Or maybe that's not a requirement for securityfs?

ABI must be documented. There is no exceptions.

Javier Martinez Canillas May 7, 2020, 5:28 p.m. UTC | #5

On Thu, May 7, 2020 at 7:22 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, May 7, 2020 at 8:06 PM Javier Martinez Canillas
> <javier@dowhile0.org> wrote:
> > On Wed, May 6, 2020 at 5:52 PM Richard Hughes <hughsient@gmail.com> wrote:
>
> > I wonder if these new entries should be documented in
> > Documentation/ABI/. Or maybe that's not a requirement for securityfs?
>
> ABI must be documented. There is no exceptions.
>

Thanks for the clarification. That's what I thought but wasn't sure.

Best regards,
Javier

Limonciello, Mario May 7, 2020, 5:45 p.m. UTC | #6

> Export standard LPC configuration values from various LPC/eSPI
> 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.
> 
> More cutting-edge checks (e.g. PRx and BootGuard) can be added
> once the basics are in place. Exporting these values from the
> kernel allows us to report the security level of the platform
> without rebooting and running an unsigned EFI binary like
> chipsec.
> 
> Signed-off-by: Richard Hughes <richard@hughsie.com>
> ---
>  MAINTAINERS                          |   6 +
>  drivers/platform/x86/Kconfig         |  10 ++
>  drivers/platform/x86/Makefile        |   1 +
>  drivers/platform/x86/intel_spi_lpc.c | 183 +++++++++++++++++++++++++++
>  4 files changed, 200 insertions(+)
>  create mode 100644 drivers/platform/x86/intel_spi_lpc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2926327e4976..2779a8d48f1c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -401,6 +401,12 @@ L:	platform-driver-x86@vger.kernel.org
>  S:	Maintained
>  F:	drivers/platform/x86/i2c-multi-instantiate.c
> 
> +SPI LPC configuration
> +M:	Richard Hughes <richard@hughsie.com>
> +L:	platform-driver-x86@vger.kernel.org
> +S:	Maintained
> +F:	drivers/platform/x86/intel_spi_lpc.c
> +
>  ACPI PMIC DRIVERS
>  M:	"Rafael J. Wysocki" <rjw@rjwysocki.net>
>  M:	Len Brown <lenb@kernel.org>
> diff --git a/drivers/platform/x86/Kconfig
> b/drivers/platform/x86/Kconfig
> index 0ad7ad8cf8e1..5f7441cde5e7 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -837,6 +837,16 @@ config INTEL_VBTN
>  	  To compile this driver as a module, choose M here: the module
> will
>  	  be called intel_vbtn.
> 
> +config INTEL_SPI_LPC
> +	tristate "Intel SPI LPC configuration"
> +	depends on SECURITY
> +	help
> +	  Export LPC configuration attributes for the system SPI chip.
> +
> +	  To compile this driver as a module, choose M here: the module
> will
> +	  be called intel_spi_lpc.
> +	  If unsure, say N.
> +
>  config SURFACE3_WMI
>  	tristate "Surface 3 WMI Driver"
>  	depends on ACPI_WMI
> diff --git a/drivers/platform/x86/Makefile
> b/drivers/platform/x86/Makefile
> index 53408d965874..e8f6901bb165 100644
> --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -78,6 +78,7 @@ obj-$(CONFIG_INTEL_INT0002_VGPIO)	+=
> intel_int0002_vgpio.o
>  obj-$(CONFIG_INTEL_MENLOW)		+= intel_menlow.o
>  obj-$(CONFIG_INTEL_OAKTRAIL)		+= intel_oaktrail.o
>  obj-$(CONFIG_INTEL_VBTN)		+= intel-vbtn.o
> +obj-$(CONFIG_INTEL_SPI_LPC)		+= intel_spi_lpc.o
> 
>  # Microsoft
>  obj-$(CONFIG_SURFACE3_WMI)		+= surface3-wmi.o
> diff --git a/drivers/platform/x86/intel_spi_lpc.c
> b/drivers/platform/x86/intel_spi_lpc.c
> new file mode 100644
> index 000000000000..dd573593a0f5
> --- /dev/null
> +++ b/drivers/platform/x86/intel_spi_lpc.c
> @@ -0,0 +1,183 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SPI LPC flash platform security driver
> + *
> + * Copyright 2020 (c) Richard Hughes (richard@hughsie.com)
> + *
> + * This file is licensed under  the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/security.h>
> +#include <linux/pci.h>
> +
> +/* LPC bridge PCI config space registers */
> +#define BIOS_CNTL_REG				0xDC
> +#define BIOS_CNTL_WRITE_ENABLE_MASK		0x01
> +#define BIOS_CNTL_LOCK_ENABLE_MASK		0x02
> +#define BIOS_CNTL_WP_DISABLE_MASK		0x20
> +
> +/*
> + * This data only exists for exporting the supported PCI ids via
> + * MODULE_DEVICE_TABLE.  We do not actually register a pci_driver.
> + */
> +static const struct pci_device_id pci_tbl[] = {
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x02a4)}, /* Comet Lake SPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x34a4)}, /* Ice Lake-LP SPI
> */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9c66)}, /* 8 Series SPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9ce6)}, /* Wildcat Point-LP
> GSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9d2a)}, /* Sunrise Point-
> LP/SPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9d4e)}, /* Sunrise Point
> LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x9da4)}, /* Cannon Point-LP
> SPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa140)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa141)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa142)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa143)}, /* H110 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa144)}, /* H170 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa145)}, /* Z170 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa146)}, /* Q170 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa147)}, /* Q150 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa148)}, /* B150 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa149)}, /* C236 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14a)}, /* C232 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14b)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14c)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14d)}, /* QM170 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14e)}, /* HM170 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa14f)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa150)}, /* CM236 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa151)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa152)}, /* HM175 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa153)}, /* QM175 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa154)}, /* CM238 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa155)}, /* Sunrise Point-H
> LPC */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c1)}, /* C621 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c2)}, /* C622 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c3)}, /* C624 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c4)}, /* C625 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c5)}, /* C626 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c6)}, /* C627 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa1c7)}, /* C628 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa304)}, /* H370 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa305)}, /* Z390 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa306)}, /* Q370 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa30c)}, /* QM370 LPC/eSPI */
> +	{PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa324)}, /* Cannon Lake PCH

This captures a lot of old ones, but I think you're missing a few of
the other more recent ones the kernel knows about.

Tiger Lake: f13e18048bdfcea2c3e25ec691cb6b4d8ab3cf21
Canon Lake: 4b97ba73dcdc24fd968cbeb970ae57212e2c1c73
Jasper Lake: 307dd80885af7183696ab6d81d73afc7a5148df6
Comet Lake H: 5a0feb6287e37018af4cbd7754786522ae712980
Comet Lake V: 701a1676f313dbae578f31da4e06c5487c8aa7bb
Elkhart Lake: ba0d4e04a5b57ef048dbf3afd9107ae6ca353258

To echo Andy's question, I would wonder if it makes sense to just export
these attributes in securityfs directly from the intel-spi-pci driver rather
than to have another driver in platform-x86 to get the information.

Then for the eSPI cases, can it export using intel-spi-platform instead of
a large whitelist in this driver?


> SPI */
> +	{0, }
> +};
> +MODULE_DEVICE_TABLE(pci, pci_tbl);
> +
> +struct dentry *spi_dir;
> +struct dentry *spi_bioswe;
> +struct dentry *spi_ble;
> +struct dentry *spi_smm_bwp;
> +struct pci_dev *dev;
> +const u8 bios_cntl_off = BIOS_CNTL_REG;
> +
> +static ssize_t bioswe_read(struct file *filp, char __user *buf,
> +			   size_t count, loff_t *ppos)
> +{
> +	char tmp[2];
> +	u8 bios_cntl_val;
> +
> +	pci_read_config_byte(dev, bios_cntl_off, &bios_cntl_val);
> +	sprintf(tmp, "%d\n",
> +		bios_cntl_val & BIOS_CNTL_WRITE_ENABLE_MASK ? 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)
> +{
> +	char tmp[2];
> +	u8 bios_cntl_val;
> +
> +	pci_read_config_byte(dev, bios_cntl_off, &bios_cntl_val);
> +	sprintf(tmp, "%d\n",
> +		bios_cntl_val & BIOS_CNTL_LOCK_ENABLE_MASK ? 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)
> +{
> +	char tmp[2];
> +	u8 bios_cntl_val;
> +
> +	pci_read_config_byte(dev, bios_cntl_off, &bios_cntl_val);
> +	sprintf(tmp, "%d\n",
> +		bios_cntl_val & BIOS_CNTL_WP_DISABLE_MASK ? 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 __init mod_init(void)
> +{
> +	int i;
> +
> +	/* Find SPI Controller */
> +	for (i = 0; !dev && pci_tbl[i].vendor; ++i)
> +		dev = pci_get_device(pci_tbl[i].vendor,
> +				     pci_tbl[i].device, NULL);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	spi_dir = securityfs_create_dir("spi", NULL);
> +	if (IS_ERR(spi_dir))
> +		return -1;
> +
> +	spi_bioswe =
> +	    securityfs_create_file("bioswe",
> +				   0600, spi_dir, NULL,
> +				   &spi_bioswe_ops);
> +	if (IS_ERR(spi_bioswe))
> +		goto out;
> +	spi_ble =
> +	    securityfs_create_file("ble",
> +				   0600, spi_dir, NULL,
> +				   &spi_ble_ops);
> +	if (IS_ERR(spi_ble))
> +		goto out;
> +	spi_smm_bwp =
> +	    securityfs_create_file("smm_bwp",
> +				   0600, spi_dir, NULL,
> +				   &spi_smm_bwp_ops);
> +	if (IS_ERR(spi_smm_bwp))
> +		goto out;
> +
> +	return 0;
> +out:
> +	securityfs_remove(spi_bioswe);
> +	securityfs_remove(spi_ble);
> +	securityfs_remove(spi_smm_bwp);
> +	securityfs_remove(spi_dir);
> +	return -1;
> +}
> +
> +static void __exit mod_exit(void)
> +{
> +	securityfs_remove(spi_bioswe);
> +	securityfs_remove(spi_ble);
> +	securityfs_remove(spi_smm_bwp);
> +	securityfs_remove(spi_dir);
> +}
> +
> +module_init(mod_init);
> +module_exit(mod_exit);
> +
> +MODULE_DESCRIPTION("SPI LPC flash platform security driver");
> +MODULE_AUTHOR("Richard Hughes <richard@hughsie.com>");
> +MODULE_LICENSE("GPL");
>

Richard Hughes May 7, 2020, 6:47 p.m. UTC | #7

On Thu, 7 May 2020 at 18:45, <Mario.Limonciello@dell.com> wrote:
> To echo Andy's question, I would wonder if it makes sense to just export
> these attributes in securityfs directly from the intel-spi-pci driver rather
> than to have another driver in platform-x86 to get the information.

The "DANGEROUS" in the SPI_INTEL_SPI_PCI and SPI_INTEL_SPI_PLATFORM
worried me somewhat. I'm guessing this is why most distros don't
compile it as a module by default. If the module isn't actually still
considered dangerous, and we can remove the warning I can of course
respin my patch on top of that instead.

Richard.

Limonciello, Mario May 7, 2020, 7:22 p.m. UTC | #8

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Richard Hughes
> Sent: Thursday, May 7, 2020 1:47 PM
> To: Limonciello, Mario
> Cc: Platform Driver; linux-security-module
> Subject: Re: [PATCH] platform/x86: Export LPC attributes for the system SPI
> chip
> 
> 
> [EXTERNAL EMAIL]
> 
> On Thu, 7 May 2020 at 18:45, <Mario.Limonciello@dell.com> wrote:
> > To echo Andy's question, I would wonder if it makes sense to just export
> > these attributes in securityfs directly from the intel-spi-pci driver rather
> > than to have another driver in platform-x86 to get the information.
> 
> The "DANGEROUS" in the SPI_INTEL_SPI_PCI and SPI_INTEL_SPI_PLATFORM
> worried me somewhat. I'm guessing this is why most distros don't
> compile it as a module by default. If the module isn't actually still
> considered dangerous, and we can remove the warning I can of course
> respin my patch on top of that instead.
> 
> Richard.

Well reading the documentation for it can explain why it's potentially dangerous.
If a manufacturer hasn't properly configured lock down bits like those you're trying
to expose, that would allow doing dangerous things.

https://www.kernel.org/doc/html/latest/driver-api/mtd/intel-spi.html
"
 The intel-spi driver makes it possible to read and write the SPI serial flash, if 
certain protection bits are not set and locked. If it finds any of them set, the 
whole MTD device is made read-only to prevent partial overwrites.
By default the driver exposes SPI serial flash contents as read-only but it can
be changed from kernel command line, passing “intel-spi.writeable=1”.
"

Richard Hughes May 7, 2020, 7:49 p.m. UTC | #9

On Thu, 7 May 2020 at 20:22, <Mario.Limonciello@dell.com> wrote:
> By default the driver exposes SPI serial flash contents as read-only but it can
> be changed from kernel command line, passing “intel-spi.writeable=1”.

Ahh, that was the bit I didn't know; having the SPI as readonly by
default is certainly a good idea, and probably sane enough to enable
for Fedora/RHEL as you still need to "do" something manual to enable
SPI writing. I guess I can add my securityfs additions to
intel-spi-pci.c with Mikas approval.

Richard

Limonciello, Mario May 7, 2020, 8:03 p.m. UTC | #10

> -----Original Message-----
> From: Richard Hughes <hughsient@gmail.com>
> Sent: Thursday, May 7, 2020 2:49 PM
> To: Limonciello, Mario
> Cc: Platform Driver; linux-security-module; mika.westerberg@linux.intel.com
> Subject: Re: [PATCH] platform/x86: Export LPC attributes for the system SPI
> chip
> 
> 
> [EXTERNAL EMAIL]
> 
> On Thu, 7 May 2020 at 20:22, <Mario.Limonciello@dell.com> wrote:
> > By default the driver exposes SPI serial flash contents as read-only but it
> can
> > be changed from kernel command line, passing “intel-spi.writeable=1”.
> 
> Ahh, that was the bit I didn't know; having the SPI as readonly by
> default is certainly a good idea, and probably sane enough to enable
> for Fedora/RHEL as you still need to "do" something manual to enable
> SPI writing. I guess I can add my securityfs additions to
> intel-spi-pci.c with Mikas approval.
> 
> Richard

Mika,

Since you're being joined into the thread late, here is the context:
https://www.spinics.net/lists/platform-driver-x86/msg21646.html

Thanks,

Mika Westerberg May 8, 2020, 8:20 a.m. UTC | #11

On Thu, May 07, 2020 at 08:03:21PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Richard Hughes <hughsient@gmail.com>
> > Sent: Thursday, May 7, 2020 2:49 PM
> > To: Limonciello, Mario
> > Cc: Platform Driver; linux-security-module; mika.westerberg@linux.intel.com
> > Subject: Re: [PATCH] platform/x86: Export LPC attributes for the system SPI
> > chip
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Thu, 7 May 2020 at 20:22, <Mario.Limonciello@dell.com> wrote:
> > > By default the driver exposes SPI serial flash contents as read-only but it
> > can
> > > be changed from kernel command line, passing “intel-spi.writeable=1”.
> > 
> > Ahh, that was the bit I didn't know; having the SPI as readonly by
> > default is certainly a good idea, and probably sane enough to enable
> > for Fedora/RHEL as you still need to "do" something manual to enable
> > SPI writing. I guess I can add my securityfs additions to
> > intel-spi-pci.c with Mikas approval.
> > 
> > Richard
> 
> Mika,
> 
> Since you're being joined into the thread late, here is the context:
> https://www.spinics.net/lists/platform-driver-x86/msg21646.html

Thanks for the information. I actually prefer that this would be in a
separate driver because I do not want distros to enable intel-spi just
for this. It is really only meant for special setups where firmware
upgrade/access flow has been thoroughly tested.

Richard Hughes May 8, 2020, 4:15 p.m. UTC | #12

On Fri, 8 May 2020 at 09:20, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Thanks for the information. I actually prefer that this would be in a
> separate driver because I do not want distros to enable intel-spi just
> for this. It is really only meant for special setups where firmware
> upgrade/access flow has been thoroughly tested.

Do you think the driver should be part of mtd (e.g. something like
drivers/mtd/spi-nor/controllers/intel-spi-pci-lpc.c) or be something
like I proposed in drivers/platform/x86? Ideas very welcome, thanks.

Richard

Limonciello, Mario May 8, 2020, 5:27 p.m. UTC | #13

> -----Original Message-----
> From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> owner@vger.kernel.org> On Behalf Of Mika Westerberg
> Sent: Friday, May 8, 2020 3:20 AM
> To: Limonciello, Mario
> Cc: hughsient@gmail.com; platform-driver-x86@vger.kernel.org; linux-
> security-module@vger.kernel.org
> Subject: Re: [PATCH] platform/x86: Export LPC attributes for the system
> SPI chip
> 
> 
> [EXTERNAL EMAIL]
> 
> On Thu, May 07, 2020 at 08:03:21PM +0000, Mario.Limonciello@dell.com
> wrote:
> > > -----Original Message-----
> > > From: Richard Hughes <hughsient@gmail.com>
> > > Sent: Thursday, May 7, 2020 2:49 PM
> > > To: Limonciello, Mario
> > > Cc: Platform Driver; linux-security-module;
> mika.westerberg@linux.intel.com
> > > Subject: Re: [PATCH] platform/x86: Export LPC attributes for the
> system SPI
> > > chip
> > >
> > >
> > > [EXTERNAL EMAIL]
> > >
> > > On Thu, 7 May 2020 at 20:22, <Mario.Limonciello@dell.com> wrote:
> > > > By default the driver exposes SPI serial flash contents as read-
> only but it
> > > can
> > > > be changed from kernel command line, passing “intel-
> spi.writeable=1”.
> > >
> > > Ahh, that was the bit I didn't know; having the SPI as readonly by
> > > default is certainly a good idea, and probably sane enough to enable
> > > for Fedora/RHEL as you still need to "do" something manual to enable
> > > SPI writing. I guess I can add my securityfs additions to
> > > intel-spi-pci.c with Mikas approval.
> > >
> > > Richard
> >
> > Mika,
> >
> > Since you're being joined into the thread late, here is the context:
> > https://www.spinics.net/lists/platform-driver-x86/msg21646.html
> 
> Thanks for the information. I actually prefer that this would be in a
> separate driver because I do not want distros to enable intel-spi just
> for this. It is really only meant for special setups where firmware
> upgrade/access flow has been thoroughly tested.

Mika,

Thanks for those comments and context on that driver.  Considering this,
what do you think about as part of this new driver, moving the list of
supported IDs in there to something that can be sourced by both drivers?
I think it should help avoid having to keep the two lists fully in sync
as new silicon comes out.

Thanks,

Mika Westerberg May 11, 2020, 10:41 a.m. UTC | #14

On Fri, May 08, 2020 at 05:27:12PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: platform-driver-x86-owner@vger.kernel.org <platform-driver-x86-
> > owner@vger.kernel.org> On Behalf Of Mika Westerberg
> > Sent: Friday, May 8, 2020 3:20 AM
> > To: Limonciello, Mario
> > Cc: hughsient@gmail.com; platform-driver-x86@vger.kernel.org; linux-
> > security-module@vger.kernel.org
> > Subject: Re: [PATCH] platform/x86: Export LPC attributes for the system
> > SPI chip
> > 
> > 
> > [EXTERNAL EMAIL]
> > 
> > On Thu, May 07, 2020 at 08:03:21PM +0000, Mario.Limonciello@dell.com
> > wrote:
> > > > -----Original Message-----
> > > > From: Richard Hughes <hughsient@gmail.com>
> > > > Sent: Thursday, May 7, 2020 2:49 PM
> > > > To: Limonciello, Mario
> > > > Cc: Platform Driver; linux-security-module;
> > mika.westerberg@linux.intel.com
> > > > Subject: Re: [PATCH] platform/x86: Export LPC attributes for the
> > system SPI
> > > > chip
> > > >
> > > >
> > > > [EXTERNAL EMAIL]
> > > >
> > > > On Thu, 7 May 2020 at 20:22, <Mario.Limonciello@dell.com> wrote:
> > > > > By default the driver exposes SPI serial flash contents as read-
> > only but it
> > > > can
> > > > > be changed from kernel command line, passing “intel-
> > spi.writeable=1”.
> > > >
> > > > Ahh, that was the bit I didn't know; having the SPI as readonly by
> > > > default is certainly a good idea, and probably sane enough to enable
> > > > for Fedora/RHEL as you still need to "do" something manual to enable
> > > > SPI writing. I guess I can add my securityfs additions to
> > > > intel-spi-pci.c with Mikas approval.
> > > >
> > > > Richard
> > >
> > > Mika,
> > >
> > > Since you're being joined into the thread late, here is the context:
> > > https://www.spinics.net/lists/platform-driver-x86/msg21646.html
> > 
> > Thanks for the information. I actually prefer that this would be in a
> > separate driver because I do not want distros to enable intel-spi just
> > for this. It is really only meant for special setups where firmware
> > upgrade/access flow has been thoroughly tested.
> 
> Mika,
> 
> Thanks for those comments and context on that driver.  Considering this,
> what do you think about as part of this new driver, moving the list of
> supported IDs in there to something that can be sourced by both drivers?
> I think it should help avoid having to keep the two lists fully in sync
> as new silicon comes out.

Hi,

Since this is binding to the LPC/eSPI device I think you may be adding
this functionality to drivers/mfd/lpc_ich.c. It is also filling the data
for intel-spi so it is natural place to re-use the code. I actually
think it already has some of these PCI IDs.

Mika Westerberg May 11, 2020, 10:45 a.m. UTC | #15

On Fri, May 08, 2020 at 05:15:07PM +0100, Richard Hughes wrote:
> On Fri, 8 May 2020 at 09:20, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > Thanks for the information. I actually prefer that this would be in a
> > separate driver because I do not want distros to enable intel-spi just
> > for this. It is really only meant for special setups where firmware
> > upgrade/access flow has been thoroughly tested.
> 
> Do you think the driver should be part of mtd (e.g. something like
> drivers/mtd/spi-nor/controllers/intel-spi-pci-lpc.c) or be something
> like I proposed in drivers/platform/x86? Ideas very welcome, thanks.

I think you may want to look at drivers/mfd/lpc_ich.c and see if some of
this can be placed there. It is the "LPC" driver that binds to the
LPC/eSPI PCI device so it already has at least some of these PCI IDs.

Richard Hughes May 11, 2020, 3:40 p.m. UTC | #16

On Mon, 11 May 2020 at 11:45, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I think you may want to look at drivers/mfd/lpc_ich.c and see if some of
> this can be placed there. It is the "LPC" driver that binds to the
> LPC/eSPI PCI device so it already has at least some of these PCI IDs.

Ahh, that's useful, thanks. I've attached something based on lpc_ich
that works, although there are a few things needing discussion:

* Some of the reported eSPI IDs are missing, and I can't easily find
references to them in the official Intel specifications. Do you need
me to hunt them down, or can I add DIDs without referencing a document
number? I can certainly split out the new DIDs and the securityfs
stuff when this patchset looks half-acceptable.

* Do you want the CONFIG_SECURITY functionality put in a different
file, perhaps with a different Kconfig entry? e.g. LPC_SCH_SECURITYFS
-- if so, how do you want the hooks implemented? Declare a dummy
lpc_ich_init_securityfs() if there is no support for securityfs like
iommu does? Put the securityfs dentries static in this new file rather
than in the lpc_ich_priv struct? Any advice welcome.

* My hardware seems to not set RCBA and so res->start never gets
defined.I don't think it's a problem from my naive point of view, but
the -ENODEV is presumably there for a reason.

If you want the patch inline, that's fine too, please just ask and I
can resend. Thanks for your help so far, and sorry for all the silly
mistakes -- I'm new to all this kernel stuff.

Richard.

Mika Westerberg May 11, 2020, 4:28 p.m. UTC | #17

On Mon, May 11, 2020 at 04:40:36PM +0100, Richard Hughes wrote:
> On Mon, 11 May 2020 at 11:45, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > I think you may want to look at drivers/mfd/lpc_ich.c and see if some of
> > this can be placed there. It is the "LPC" driver that binds to the
> > LPC/eSPI PCI device so it already has at least some of these PCI IDs.
> 
> Ahh, that's useful, thanks. I've attached something based on lpc_ich
> that works, although there are a few things needing discussion:
> 
> * Some of the reported eSPI IDs are missing, and I can't easily find
> references to them in the official Intel specifications. Do you need
> me to hunt them down, or can I add DIDs without referencing a document
> number? I can certainly split out the new DIDs and the securityfs
> stuff when this patchset looks half-acceptable.

I don't think you need to hunt them down. It should be fine to add PCI
IDs to the driver without reference document. Of course I'm not the
maintainer of this driver but I suspect nobody cares.

> * Do you want the CONFIG_SECURITY functionality put in a different
> file, perhaps with a different Kconfig entry? e.g. LPC_SCH_SECURITYFS
> -- if so, how do you want the hooks implemented? Declare a dummy
> lpc_ich_init_securityfs() if there is no support for securityfs like
> iommu does? Put the securityfs dentries static in this new file rather
> than in the lpc_ich_priv struct? Any advice welcome.

If it depends on that functionality then you may simply add something
like this in lpc_ich.c:

#if IS_ENABLED(CONFIG_SECURITY)
static int lpc_ich_init_securityfs(struct pci_dev *dev)
{
	...
}
#else
static inline int lpc_ich_init_securityfs(struct pci_dev *dev) { return 0; }
#endif

I mean empty stubs when the feature is not enabled so the callers can
always call them. This avoids most ugly #ifdefs.

> * My hardware seems to not set RCBA and so res->start never gets
> defined.I don't think it's a problem from my naive point of view, but
> the -ENODEV is presumably there for a reason.

Yes, so for these recent CPUs the SPI-NOR is actually a PCI device
itself so it is not exposed through LPC/eSPI. In many cases the device
is disabled by the BIOS so you can't see it if you run lspci. For
example my SPT based laptop the device is not visible.

For the security stuff you are adding, do you need to look at the PCI
device registers as well? Then some of these bits (at least WPD) is part
of the config space of that device. In that case lpc_ich may not be the
right place for this after all.

> If you want the patch inline, that's fine too, please just ask and I
> can resend. Thanks for your help so far, and sorry for all the silly
> mistakes -- I'm new to all this kernel stuff.

No problem :)

Typically inline is good. I suggest you to run

  $ scripts/get_maintainers.pl path/to/the/patch

and see who actually maintains this thing. Then, based on the above, if
you still add it this driver you can send the patch again (I suggest to
use git send-email) and Cc the maintainer(s). Actually there is this
entry in MAINTAINERS:

ICH LPC AND GPIO DRIVER
M:      Peter Tyser <ptyser@xes-inc.com>
S:      Maintained
F:      drivers/gpio/gpio-ich.c
F:      drivers/mfd/lpc_ich.c

So you should Cc Peter as well. And also the MFD maintainer (Lee Jones)
but get_maintainers.pl should list him.

Richard Hughes May 11, 2020, 8:08 p.m. UTC | #18

On Mon, 11 May 2020 at 17:28, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> #if IS_ENABLED(CONFIG_SECURITY)
> static int lpc_ich_init_securityfs(struct pci_dev *dev)

I've done that, thanks.

> For the security stuff you are adding, do you need to look at the PCI
> device registers as well?

No, just the config space. I guess at some point I might want to put
some of the chipsec vulnerability checks into the module, although I'm
not happy adding anything remotely dangerous.

>   $ scripts/get_maintainers.pl path/to/the/patch

Will do, thanks. I'll polish up the list of DIDs and submit it again.

Richard.

Mika Westerberg May 12, 2020, 6:44 a.m. UTC | #19

On Mon, May 11, 2020 at 09:08:31PM +0100, Richard Hughes wrote:
> > For the security stuff you are adding, do you need to look at the PCI
> > device registers as well?
> 
> No, just the config space. I guess at some point I might want to put
> some of the chipsec vulnerability checks into the module, although I'm
> not happy adding anything remotely dangerous.

I mean for the SPI-NOR controller PCI device registers (not the LPC PCI
device, sorry about not being clear), like config space. Typically it is
PCI device 1f.5 and in that case you can't use the LPC driver because it
should not touch other devices than what it is bound to (OK, there are
exceptions but still).

If that's the case then I guess this should go to intel-spi-pci/platform
drivers after all. I think one option is that we add Kconfig option that
makes the driver load but only provide the security bits without
actually calling intel_spi_probe().

Richard Hughes May 12, 2020, 8:37 p.m. UTC | #20

On Tue, 12 May 2020 at 07:44, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> I mean for the SPI-NOR controller PCI device registers (not the LPC PCI
> device, sorry about not being clear), like config space.

I don't think I need to care about those, but I'll admit I'm a bit of
a newbie with all the terminology. I'll respin the patch now and cc
you on the new version too.

> If that's the case then I guess this should go to intel-spi-pci/platform
> drivers after all. I think one option is that we add Kconfig option that
> makes the driver load but only provide the security bits without
> actually calling intel_spi_probe().

I think getting distros to enable any of the SPI_INTEL_SPI* options
might be an uphill battle.

Richard.

platform/x86: Export LPC attributes for the system SPI chip

Commit Message

Comments

Patch