Messages in this thread | | | From | Daniel Gutson <> | Date | Wed, 21 Oct 2020 16:55:06 -0300 | Subject | Re: [PATCH 2/2] Platform integrity information in sysfs (version 9) |
| |
On Fri, Oct 2, 2020 at 10:43 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Sep 30, 2020 at 01:37:14PM -0300, Daniel Gutson wrote: > > This patch exports the BIOS Write Enable (bioswe), BIOS > > Lock Enable (biosle), and the SMM BIOS Write Protect (SMM_BIOSWP) fields of > > the BIOS Control register using the platform-integrity misc kernel module. > > The idea is to keep adding more flags, not only from the BC but also from > > other registers in following versions. > > > > The goal is that the attributes are avilable to fwupd when SecureBoot > > is turned on. > > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> > > The subject line doesn't match what this patch does, please fix that up. > > But there are more core issues in this patch: > > > > > --- > > drivers/mtd/spi-nor/controllers/Kconfig | 1 + > > .../mtd/spi-nor/controllers/intel-spi-pci.c | 75 ++++++++++++++- > > .../spi-nor/controllers/intel-spi-platform.c | 2 +- > > drivers/mtd/spi-nor/controllers/intel-spi.c | 91 ++++++++++++++++++- > > drivers/mtd/spi-nor/controllers/intel-spi.h | 9 +- > > 5 files changed, 171 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig b/drivers/mtd/spi-nor/controllers/Kconfig > > index 5c0e0ec2e6d1..e7eaef506fc2 100644 > > --- a/drivers/mtd/spi-nor/controllers/Kconfig > > +++ b/drivers/mtd/spi-nor/controllers/Kconfig > > @@ -29,6 +29,7 @@ config SPI_NXP_SPIFI > > > > config SPI_INTEL_SPI > > tristate > > + depends on PLATFORM_INTEGRITY_DATA > > > > config SPI_INTEL_SPI_PCI > > tristate "Intel PCH/PCU SPI flash PCI driver (DANGEROUS)" > > diff --git a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > > index c72aa1ab71ad..644b1a6091dc 100644 > > --- a/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > > +++ b/drivers/mtd/spi-nor/controllers/intel-spi-pci.c > > @@ -10,11 +10,19 @@ > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/pci.h> > > +#include <linux/platform-integrity.h> > > > > #include "intel-spi.h" > > > > #define BCR 0xdc > > #define BCR_WPD BIT(0) > > +#define BCR_BLE BIT(1) > > +#define BCR_SMM_BWP BIT(5) > > + > > +struct cnl_spi_attr { > > + struct device_attribute dev_attr; > > + u32 mask; > > +}; > > > > static const struct intel_spi_boardinfo bxt_info = { > > .type = INTEL_SPI_BXT, > > @@ -24,6 +32,70 @@ static const struct intel_spi_boardinfo cnl_info = { > > .type = INTEL_SPI_CNL, > > }; > > > > +#ifdef CONFIG_PLATFORM_INTEGRITY_DATA > > +static ssize_t intel_spi_cnl_spi_attr_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf, u32 mask) > > +{ > > + u32 bcr; > > + > > + if (dev->parent == NULL) > > + return -EIO; > > + > > + if (pci_read_config_dword(container_of(dev->parent, struct pci_dev, dev), > > + BCR, &bcr) != PCIBIOS_SUCCESSFUL) > > + return -EIO; > > + > > + return sprintf(buf, "%d\n", (int)!!(bcr & mask)); > > +} > > + > > +static ssize_t bioswe_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_WPD); > > +} > > +static DEVICE_ATTR_RO(bioswe); > > + > > +static ssize_t biosle_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_BLE); > > +} > > +static DEVICE_ATTR_RO(biosle); > > + > > +static ssize_t smm_bioswp_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + return intel_spi_cnl_spi_attr_show(dev, attr, buf, BCR_SMM_BWP); > > +} > > +static DEVICE_ATTR_RO(smm_bioswp); > > + > > +static struct attribute *cnl_attrs[] = { > > + &dev_attr_bioswe.attr, > > + &dev_attr_biosle.attr, > > + &dev_attr_smm_bioswp.attr, > > + NULL > > +}; > > +ATTRIBUTE_GROUPS(cnl); > > If you are forcing the driver to create the groups, then you are forcing > us to audit each driver and verify that the files are the same name and > such. Put the files in the "common" code please, and if you really need > a way to get the data out, make that a callback or something.
If I understand you correctly, you are asking the opposite that Arnd asked me in a previous patch version: he told me no new callbacks, just use the device attribute API. However I'm not sure I understand your proposal, do you mean to let the device attr stay in the driver file, and do the group inside the common part? Therefore, to pass a dev attributes array to the common part? If not, could you please explain your proposal again?
> > Also, this name "platform integrity" is really really generic, while in > reality you are describing something very specific. Are you sure you > want that? > > thanks, > > greg k-h
--
Daniel Gutson Engineering Director Eclypsium, Inc.
Below The Surface: Get the latest threat research and insights on firmware and supply chain threats from the research team at Eclypsium. https://eclypsium.com/research/#threatreport
| |