Messages in this thread | ![/](/images/icornerl.gif) | | From | Daniel Gutson <> | Date | Thu, 27 Aug 2020 12:04:26 -0300 | Subject | Re: [PATCH] Platform lockdown information in sysfs (v2) |
| |
On Thu, Aug 27, 2020 at 7:15 AM Arnd Bergmann <arnd@arndb.de> wrote: > > On Thu, Aug 20, 2020 at 4:51 PM Daniel Gutson > <daniel.gutson@eclypsium.com> 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. > > > > This version of the patch replaces class attributes by device > > attributes. > > > > Signed-off-by: Daniel Gutson <daniel.gutson@eclypsium.com> > > This looks much better than before, thanks for addressing the feedback. Thanks for providing it.
> > diff --git a/Documentation/ABI/stable/sysfs-class-platform-lockdown b/Documentation/ABI/stable/sysfs-class-platform-lockdown > > new file mode 100644 > > index 000000000000..3fe75d775a42 > > --- /dev/null > > +++ b/Documentation/ABI/stable/sysfs-class-platform-lockdown > > @@ -0,0 +1,23 @@ > > +What: /sys/class/platform-lockdown/bioswe > > platform-lockdown is a much better name than the previous suggestions. > I'm still hoping for an even better suggestion. Like everything the term > "lockdown" is also overloaded a bit, with the other common meaning > referring to the effort to give root users less privilege than the > kernel itself, > see https://lwn.net/Articles/750730/
I'd want to set the name for good before I proceed with the rest. A list of suggestions: platform-firmware platform-defence firmware-surety firmware-control platform-inspection platform-lookout platform-diligence platform-integrity
I like the last want, what do you think? Any other suggestion?
Thanks again!
Daniel. > > Shouldn't there be a device name between the class name > ("platform-lockdown") and the attribute name? > > > +PLATFORM LOCKDOWN ATTRIBUTES MODULE > > +M: Daniel Gutson <daniel.gutson@eclypsium.com> > > +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 > > include/linux/platform_data/ is not the right place for the header, > this is defined to be the place for defining properties of devices > that are created from old-style board files. > > Just put the header into include/linux/ directly. > the host. > > > > +config PLATFORM_LOCKDOWN_ATTRS > > + tristate "Platform lockdown information in the sysfs" > > + depends on SYSFS > > + help > > + This kernel module is a helper driver to provide information about > > + platform lockdown settings and configuration. > > + This module is used by other device drivers -such as the intel-spi- > > + to publish the information in /sys/class/platform-lockdown. > > Maybe mention fwupd in the description in some form. > > > + > > +static struct class platform_lockdown_class = { > > + .name = "platform-lockdown", > > + .owner = THIS_MODULE, > > +}; > > + > > +struct device *register_platform_lockdown_data_device(struct device *parent, > > + const char *name) > > +{ > > + return device_create(&platform_lockdown_class, parent, MKDEV(0, 0), > > + NULL, name); > > +} > > +EXPORT_SYMBOL_GPL(register_platform_lockdown_data_device); > > + > > +void unregister_platform_lockdown_data_device(struct device *dev) > > +{ > > + device_unregister(dev); > > +} > > +EXPORT_SYMBOL_GPL(unregister_platform_lockdown_data_device); > > + > > +int register_platform_lockdown_attribute(struct device *dev, > > + struct device_attribute *dev_attr) > > +{ > > + return device_create_file(dev, dev_attr); > > +} > > +EXPORT_SYMBOL_GPL(register_platform_lockdown_attribute); > > + > > +void register_platform_lockdown_attributes(struct device *dev, > > + struct device_attribute dev_attrs[]) > > +{ > > + u32 idx = 0; > > + > > + while (dev_attrs[idx].attr.name != NULL) { > > + register_platform_lockdown_attribute(dev, &dev_attrs[idx]); > > + idx++; > > + } > > There is a bit of a race with creating the device first and then > the attributes. Generally it seems better to me to use > device_create_with_groups() instead so the device shows up > with all attributes in place already. > > > +void register_platform_lockdown_custom_attributes(struct device *dev, > > + void *custom_attrs, > > + size_t dev_attr_offset, > > + size_t custom_attr_size) > > This interface seems to be overly complex, I would hope it can be avoided. > > > +static ssize_t cnl_spi_attr_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + u32 bcr; > > + struct cnl_spi_attr *cnl_spi_attr = container_of(attr, > > + struct cnl_spi_attr, dev_attr); > > + > > + if (class_child_device != dev) > > + return -EIO; > > + > > + 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 & cnl_spi_attr->mask)); > > +} > > If I understand it right, that complexity comes from attempting to > have a single show callback for three different flags. To me that > actually feels more complicated than having an attribute group > with three similar but simpler show callbacks. > > > static void intel_spi_pci_remove(struct pci_dev *pdev) > > { > > + if (class_child_device != NULL) { > > Please avoid the global variable here and just add a member in the > per-device data. > > > + unregister_platform_lockdown_custom_attributes( > > + class_child_device, > > + cnl_spi_attrs, > > + offsetof(struct cnl_spi_attr, dev_attr), > > + sizeof(struct cnl_spi_attr)); > > + > > + unregister_platform_lockdown_data_device(class_child_device); > > It should be possible to just destroy the attributes as part of > unregister_platform_lockdown_data_device. > > Arnd
-- 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
| ![\](/images/icornerr.gif) |