From: Daniel Gutson <daniel@eclypsium.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Derek Kiernan <derek.kiernan@xilinx.com>,
Tudor Ambarus <tudor.ambarus@microchip.com>,
Miquel Raynal <miquel.raynal@bootlin.com>,
Richard Weinberger <richard@nod.at>,
Vignesh Raghavendra <vigneshr@ti.com>,
Mika Westerberg <mika.westerberg@linux.intel.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Richard Hughes <hughsient@gmail.com>,
Alex Bazhaniuk <alex@eclypsium.com>
Subject: Re: [PATCH] Platform lockdown information in sysfs (v2)
Date: Thu, 27 Aug 2020 12:04:26 -0300 [thread overview]
Message-ID: <CAFmMkTHWBpJNMHAP_hKFdRZDOYs-ESU453Ldy78s0u-E1SJPag@mail.gmail.com> (raw)
In-Reply-To: <CAK8P3a3Z_mw4k+FZbGiz8Qg-YOHo7f=bWgpZ2gGZYqZuKo-8Hw@mail.gmail.com>
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
prev parent reply other threads:[~2020-08-27 15:05 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-20 14:51 [PATCH] Platform lockdown information in sysfs (v2) Daniel Gutson
2020-08-26 21:19 ` Daniel Gutson
2020-08-27 10:15 ` Arnd Bergmann
2020-08-27 15:04 ` Daniel Gutson [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFmMkTHWBpJNMHAP_hKFdRZDOYs-ESU453Ldy78s0u-E1SJPag@mail.gmail.com \
--to=daniel@eclypsium.com \
--cc=alex@eclypsium.com \
--cc=arnd@arndb.de \
--cc=derek.kiernan@xilinx.com \
--cc=gregkh@linuxfoundation.org \
--cc=hughsient@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mchehab+huawei@kernel.org \
--cc=mika.westerberg@linux.intel.com \
--cc=miquel.raynal@bootlin.com \
--cc=richard@nod.at \
--cc=tudor.ambarus@microchip.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).