Re: [PATCH] Platform lockdown information in SYSFS

From: Daniel Gutson
Date: Mon Aug 03 2020 - 18:05:10 EST

Next message: Jakub Kicinski: "Re: [PATCH net-next v3 1/2] hinic: add generating mailbox random index support"
Previous message: David Miller: "Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation"
Next in thread: Greg Kroah-Hartman: "Re: [PATCH] Platform lockdown information in SYSFS"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Jul 31, 2020 at 11:15 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Jul 31, 2020 at 10:30:43AM -0300, Daniel Gutson wrote:
> > On Fri, Jul 31, 2020 at 4:01 AM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Jul 30, 2020 at 06:41:36PM -0300, Daniel Gutson wrote:
> > > > This patch exports information about the platform lockdown
> > > > firmware configuration in the sysfs filesystem.
> > > > In this initial patch, I include some configuration attributes
> > > > for the system SPI chip.
> > > >
> > > > 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.
> > > >
> > > > 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
> > > > class_attributes corresponding to the fields mentioned above.
> > >
> > > Better, but you are abusing sysfs (note, no CAPS) a lot here...
> > >
> > >
> > > >
> > > > Signed-off-by: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > ---
> > > > .../ABI/stable/sysfs-class-platform-lockdown | 23 +++++++
> > > > MAINTAINERS | 7 +++
> > > > drivers/misc/Kconfig | 9 +++
> > > > drivers/misc/Makefile | 1 +
> > > > drivers/misc/platform-lockdown-attrs.c | 57 +++++++++++++++++
> > > > drivers/mtd/spi-nor/controllers/Kconfig | 1 +
> > > > .../mtd/spi-nor/controllers/intel-spi-pci.c | 49 +++++++++++++++
> > > > drivers/mtd/spi-nor/controllers/intel-spi.c | 62 +++++++++++++++++++
> > > > .../platform_data/platform-lockdown-attrs.h | 19 ++++++
> > > > 9 files changed, 228 insertions(+)
> > > > create mode 100644 Documentation/ABI/stable/sysfs-class-platform-lockdown
> > > > create mode 100644 drivers/misc/platform-lockdown-attrs.c
> > > > create mode 100644 include/linux/platform_data/platform-lockdown-attrs.h
> > > >
> > > > diff --git a/Documentation/ABI/stable/sysfs-class-platform-lockdown b/Documentation/ABI/stable/sysfs-class-platform-lockdown
> > > > new file mode 100644
> > > > index 000000000000..6034d6cbefac
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/stable/sysfs-class-platform-lockdown
> > > > @@ -0,0 +1,23 @@
> > > > +What: /sys/class/platform-lockdown/bioswe
> > > > +Date: July 2020
> > > > +KernelVersion: 5.8.0
> > > > +Contact: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > +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-lockdown/ble
> > > > +Date: July 2020
> > > > +KernelVersion: 5.8.0
> > > > +Contact: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > +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-lockdown/smm_bwp
> > > > +Date: July 2020
> > > > +KernelVersion: 5.8.0
> > > > +Contact: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > +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/MAINTAINERS b/MAINTAINERS
> > > > index f0569cf304ca..771ed1693d28 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -13608,6 +13608,13 @@ S: Maintained
> > > > F: Documentation/devicetree/bindings/iio/chemical/plantower,pms7003.yaml
> > > > F: drivers/iio/chemical/pms7003.c
> > > >
> > > > +PLATFORM LOCKDOWN ATTRIBUTES MODULE
> > > > +M: Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > +S: Supported
> > > > +F: Documentation/ABI/sysfs-class-platform-lockdown
> > > > +F: drivers/misc/platform-lockdown-attrs.c
> > > > +F: include/linux/platform_data/platform-lockdown-attrs.h
> > > > +
> > > > PLX DMA DRIVER
> > > > M: Logan Gunthorpe <logang@xxxxxxxxxxxx>
> > > > S: Maintained
> > > > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> > > > index e1b1ba5e2b92..058d4ba3cefd 100644
> > > > --- a/drivers/misc/Kconfig
> > > > +++ b/drivers/misc/Kconfig
> > > > @@ -456,6 +456,15 @@ config PVPANIC
> > > > a paravirtualized device provided by QEMU; it lets a virtual machine
> > > > (guest) communicate panic events to the host.
> > > >
> > > > +config PLATFORM_LOCKDOWN_ATTRS
> > > > + tristate "Platform lockdown information in the SYSFS"
> > >
> > > "Platform lockdown information for some hardware information displayed
> > > in sysfs" ?
> > >
> > > > + depends on SYSFS
> > > > + help
> > > > + This kernel module is a helper driver to provide information about
> > > > + platform lockdown settings and configuration.
> > >
> > > Is that what this really is?
> > >
> > > > + This module is used by other device drivers -such as the intel-spi-
> > > > + to publish the information in /sys/class/platform-lockdown.
> > > > +
> > > > source "drivers/misc/c2port/Kconfig"
> > > > source "drivers/misc/eeprom/Kconfig"
> > > > source "drivers/misc/cb710/Kconfig"
> > > > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
> > > > index c7bd01ac6291..e29b45c564f9 100644
> > > > --- a/drivers/misc/Makefile
> > > > +++ b/drivers/misc/Makefile
> > > > @@ -57,3 +57,4 @@ obj-$(CONFIG_PVPANIC) += pvpanic.o
> > > > obj-$(CONFIG_HABANA_AI) += habanalabs/
> > > > obj-$(CONFIG_UACCE) += uacce/
> > > > obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o
> > > > +obj-$(CONFIG_PLATFORM_LOCKDOWN_ATTRS) += platform-lockdown-attrs.o
> > > > diff --git a/drivers/misc/platform-lockdown-attrs.c b/drivers/misc/platform-lockdown-attrs.c
> > > > new file mode 100644
> > > > index 000000000000..d08b3d895064
> > > > --- /dev/null
> > > > +++ b/drivers/misc/platform-lockdown-attrs.c
> > > > @@ -0,0 +1,57 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * Platform lockdown attributes kernel module
> > > > + *
> > > > + * Copyright (C) 2020 Daniel Gutson <daniel.gutson@xxxxxxxxxxxxx>
> > > > + * Copyright (C) 2020 Eclypsium Inc.
> > > > + */
> > > > +#include <linux/kobject.h>
> > > > +#include <linux/sysfs.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/init.h>
> > > > +#include <linux/list.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/platform_data/platform-lockdown-attrs.h>
> > > > +
> > > > +static struct class platform_lockdown_class = {
> > > > + .name = "platform-lockdown",
> > > > + .owner = THIS_MODULE,
> > > > +};
> > > > +
> > > > +int register_platform_lockdown_attribute(
> > > > + struct class_attribute *lockdown_attribute)
> > > > +{
> > > > + /* attempt to create the file: */
> > > > + return class_create_file(&platform_lockdown_class,
> > > > + lockdown_attribute);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(register_platform_lockdown_attribute);
> > >
> > > nit, global symbols should put the noun first:
> > > platform_lockdown_attribute_register()
> > >
> > > But, this all is not ok.
> > >
> > > You are trying to create/remove random sysfs files that end up in a
> > > single class. What you want to do is create a device that is associated
> > > with this class, and make that device a child of the device you are
> > > registering these attributes for.
> > >
> > > Think of this as an input device. You don't put the random input
> > > attributes all in one place, you create a new device that represents the
> > > input interface and register that.

I'm having trouble with this. What's the dev_t for the child devices?
I'm doing
child_device = device_create(&my_class, &pdev->dev, MKDEV(0, 0),
NULL, "child");
pdev is the pci_device (intel-spi-pci)
dmesg shows

sysfs: cannot create duplicate filename '/class/my-class'
(call trace)
kobject_add_internal failed for my-class with -EEXIST, don't try
to register things with the same name in the same directory.

Even worse when I tried to remove the class:
I can't use device_destroy because there is no dev_t, so since it is
put_device(child_device);
device_unregister(child_device);
that caused a core dump too.
I tried also doing nothing with the child_device, but then I get a core dump.

Is all these caused by the MKDEV(0, 0) ?
.

Then userspace can iterate over all
> > > devices that are of the input class and see the individual attributes of
> > > them.
> >
> > So I create the child device.
> > And these attributes, will be class attributes or device attributes of
> > the child device?
>
> Device attributes of course :)
>
> greg k-h
--
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

Next message: Jakub Kicinski: "Re: [PATCH net-next v3 1/2] hinic: add generating mailbox random index support"
Previous message: David Miller: "Re: [PATCH v4 2/2] net: dsa: ocelot: Add support for QinQ Operation"
Next in thread: Greg Kroah-Hartman: "Re: [PATCH] Platform lockdown information in SYSFS"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]