On Tue, Sep 8, 2020 at 5:48 AM Vignesh Raghavendra <[email protected]> wrote: > > Hi,
Hi, thanks for your review. > > On 9/3/20 9:48 PM, Daniel Gutson wrote: > > This patch exports information about the platform integrity > > firmware configuration in the sysfs filesystem. > > In this initial patch, I include some configuration attributes > > for the system SPI chip. > > > > Please avoid first-person singular pronouns instead use imperative mood. > > > This initial version exports the BIOS Write Enable (bioswe), > > BIOS Lock Enable (ble), and the SMM BIOS Write Protect (SMM_BWP) > > fields of the BIOS Control register. 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. > > > > s/avilable/available > > > The patch provides a new misc driver, as proposed in the previous patch, > > that provides a registration function for HW Driver devices to register > > class_attributes. > > In this case, the intel SPI flash chip (intel-spi) registers three > > s/intel SPI/Intel SPI > > > class_attributes corresponding to the fields mentioned above. > > > > This version of the patch provides a new API supporting regular > > device attributes rather than custom attributes, and also avoids > > a race condition when exporting the driver sysfs dir and the > > attributes files inside it. > > Also, this patch renames 'platform lockdown' by 'platform integrity'. > > Changes wrt to previous versions should not be part of commit message > but instead should be below tearline (b/w "---" and diffstat below) > > > > > Signed-off-by: Daniel Gutson <[email protected]> > > --- > > Changes wrt previous versions go here... > > > .../ABI/stable/sysfs-class-platform-integrity | 23 +++++ > > MAINTAINERS | 7 ++ > > drivers/misc/Kconfig | 11 +++ > > drivers/misc/Makefile | 1 + > > drivers/misc/platform-integrity.c | 61 +++++++++++++ > > drivers/mtd/spi-nor/controllers/Kconfig | 1 + > > .../mtd/spi-nor/controllers/intel-spi-pci.c | 64 ++++++++++++- > > .../spi-nor/controllers/intel-spi-platform.c | 2 +- > > drivers/mtd/spi-nor/controllers/intel-spi.c | 89 ++++++++++++++++++- > > drivers/mtd/spi-nor/controllers/intel-spi.h | 5 +- > > include/linux/platform-integrity.h | 20 +++++ > > 11 files changed, 278 insertions(+), 6 deletions(-) > > create mode 100644 Documentation/ABI/stable/sysfs-class-platform-integrity > > create mode 100644 drivers/misc/platform-integrity.c > > create mode 100644 include/linux/platform-integrity.h > > > > > You forgot to cc linux-mtd lists and hence patch did not show up in my > queue.. MTD Patches are managed via patchoworks and if linux-mtd is not > cc'd then they will most likely be ignored. Please use > scripts/get_maintainer.pl to get list of maintainters and mailing lists > to cc. Sorry, I don't know when I lost the list in the emails, I included it in previous patches. > > Also, this patch needs to be split into at least two patches: one > introducing platform-integrity module and another adding suuport for > intel-spi driver to use the same. > > Also, I see there are multiple iterations of the patch posted but are > not versioned. Please use appropriate version number in $subject. > > Please read Documentation/process/submitting-patches.rst for more details. > > > > diff --git a/Documentation/ABI/stable/sysfs-class-platform-integrity > > b/Documentation/ABI/stable/sysfs-class-platform-integrity > > new file mode 100644 > > index 000000000000..b31ec051ca48 > > --- /dev/null > > +++ b/Documentation/ABI/stable/sysfs-class-platform-integrity > > @@ -0,0 +1,23 @@ > > +What: /sys/class/platform-integrity/intel-spi/bioswe > > +Date: September 2020 > > +KernelVersion: 5.9.1 > > 5.9 merge window is closed. This won't make it to 5.9... Maybe 5.10 > > > +Contact: Daniel Gutson <[email protected]> > > +Description: If the system firmware set BIOS Write Enable. > > + 0: writes disabled, 1: writes enabled. > > +Users: https://github.com/fwupd/fwupd > > + > > +What: /sys/class/platform-integrity/intel-spi/ble > > Naming seems inconsistent... Previous entry was bioswe (BIOS write > enable) but this one is called ble (BIOS latch enable). Maybe rename > previous one as bwe to be consistent with other entries? > > > +Date: September 2020 > > +KernelVersion: 5.9.1 > > +Contact: Daniel Gutson <[email protected]> > > +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/class/platform-integrity/intel-spi/smm_bwp > > +Date: September 2020 > > +KernelVersion: 5.9.1 > > +Contact: Daniel Gutson <[email protected]> > > +Description: If the system firmware set SMM BIOS Write Protect. > > + 0: writes disabled unless in SMM, 1: writes enabled. > > Is SMM a standard abbreviation in Intel world? If not you may want to > expand what it means in Description. > > > +Users: https://github.com/fwupd/fwupd > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e4647c84c987..771eaf715427 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -13744,6 +13744,13 @@ S: Maintained > > F: Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml > > F: drivers/iio/chemical/pms7003.c > > > > +PLATFORM INTEGRITY DATA MODULE > > +M: Daniel Gutson <[email protected]> > > +S: Supported > > +F: Documentation/ABI/sysfs-class-platform-integrity > > +F: drivers/misc/platform-integrity.c > > +F: include/linux/platform-integrity.h > > + > > PLDMFW LIBRARY > > M: Jacob Keller <[email protected]> > > S: Maintained > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > > index ce136d685d14..d5d0de5b5706 100644 > > --- a/drivers/misc/Kconfig > > +++ b/drivers/misc/Kconfig > > @@ -456,6 +456,17 @@ config PVPANIC > > a paravirtualized device provided by QEMU; it lets a virtual machine > > (guest) communicate panic events to the host. > > > > +config PLATFORM_INTEGRITY_ATTRS > > + tristate "Platform integrity information in the sysfs" > > + depends on SYSFS > > + help > > + This kernel module is a helper driver to provide information about > > + platform integrity settings and configuration. > > + This module is used by other device drivers -such as the intel-spi- > > + to publish the information in /sys/class/platform-integrity which is > > + consumed by software such as fwupd which can verify the platform > > + has been configured in a secure way. > > + > > [...] > > > diff --git a/drivers/mtd/spi-nor/controllers/Kconfig > > b/drivers/mtd/spi-nor/controllers/Kconfig > > index 5c0e0ec2e6d1..113e349a826b 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 > > + select PLATFORM_INTEGRITY_ATTRS > > > > Not a good idea to use select clause on symbols that have dependencies > as select will force a symbol to a value without visiting the > dependencies. Instead use depends on I just tried this, but when selecting the Intel SPI drivers, my new platform integrity driver was not selected, as it was with the select clause. > > > > 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..e1bca8aedf7c 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,59 @@ static const struct intel_spi_boardinfo cnl_info = { > > .type = INTEL_SPI_CNL, > > }; > > > > +static ssize_t cnl_spi_attr_show(struct device *dev, > > + struct device_attribute *attr, char *buf, u32 mask) > > +{ > > Please be consistent with existing code and use intel_spi_ prefix for > function names. > > Also Alignment should match open parenthesis.. > Please run scripts/checkpatch.pl --strict on patch and fix all issue > reported. I just added the --strict flag and indeed a lot of alignment issues showed up. I will fix this in the next version. Thanks. > > [...] > > Regards > Vignesh -- Daniel Gutson Argentina Site Director Enginieering Director Eclypsium 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
