[PATCH 0/2] firmware: Add boot information to sysfs

[PATCH 0/2] firmware: Add boot information to sysfs

[Joel Stanley]

This is the second iteration of this idea. The first used socinfo
custom attribute groups, but Arnd suggested we make this something
standardised under /sys/firmware instead:

 http://lore.kernel.org/all/CAK8P3a3MRf0aGt1drkgsuZyBbeoy+S7Ha18SBM01q+3f33oL+Q@mail.gmail.com

Some ARM systems have a firmware that provides a hardware root of
trust. It's useful for the system to know how this root of trust has
been configured, so provide a standardised interface that expose this
information to userspace.

This is implemented as a sysfs attribute group registration to be called
at boot, with the properties described in the ABI document.

Alternatively we could put the properties in the driver core, and have
platforms register callbacks for each supported property. This would
make it harder to insert non-standard properties, with the trade off of
more code to selectively show supported properties.

An user of the properties is provided in the second patch.

Joel Stanley (2):
  firmware: Add boot information sysfs
  ARM: aspeed: Add secure boot controller support

 .../ABI/testing/sysfs-firmware-bootinfo       | 43 ++++++++++
 drivers/base/firmware.c                       | 14 ++++
 drivers/soc/aspeed/aspeed-socinfo.c           | 84 ++++++++++++++++++-
 include/linux/firmware_bootinfo.h             |  8 ++
 4 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-bootinfo
 create mode 100644 include/linux/firmware_bootinfo.h

-- 
2.34.1

[PATCH 1/2] firmware: Add boot information sysfs

[Joel Stanley]

Machines often have firmware that perform some action before Linux is
loaded. It's useful to know how this firmware is configured, so create a
sysfs directory and some example properties that a system can choose to
expose to describe how the system was started.

Currently the intended use describes five files, relating to hardware
root of trust configuration.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 .../ABI/testing/sysfs-firmware-bootinfo       | 43 +++++++++++++++++++
 drivers/base/firmware.c                       | 14 ++++++
 include/linux/firmware_bootinfo.h             |  8 ++++
 3 files changed, 65 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-bootinfo
 create mode 100644 include/linux/firmware_bootinfo.h

diff --git a/Documentation/ABI/testing/sysfs-firmware-bootinfo b/Documentation/ABI/testing/sysfs-firmware-bootinfo
new file mode 100644
index 000000000000..cd6c42316345
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-bootinfo
@@ -0,0 +1,43 @@
+What:		/sys/firmware/bootinfo/*
+Date:		Jan 2022
+Description:
+		A system can expose information about how it was started in
+		this directory.
+
+		This information is agnostic as to the firmware implementation.
+
+		A system may expose a subset of these properties as applicable.
+
+
+What:		/sys/firmware/bootinfo/secure_boot
+Date:		Jan 2022
+Description:
+		Indicates the system was started with secure boot enabled in
+		the firmware.
+
+
+What:		/sys/firmware/bootinfo/abr_image
+Date:		Jan 2022
+Description:
+		Indicates the system was started from the alternate image
+		loaded from an Alternate Boot Region. Often this is a result of
+		the primary firmware image failing to start the system.
+
+
+What:		/sys/firmware/bootinfo/low_security_key
+Date:		Jan 2022
+Description:
+		Indicates the system's secure boot was verified with a low
+		security or development key.
+
+What:		/sys/firmware/bootinfo/otp_protected
+Date:		Jan 2022
+Description:
+		Indicates the system's boot configuration region is write
+		protected and cannot be modified.
+
+What:		/sys/firmware/bootinfo/uart_boot
+Date:		Jan 2022
+Description:
+		Indicates the system firmware was loaded from a UART instead of
+		an internal boot device.
diff --git a/drivers/base/firmware.c b/drivers/base/firmware.c
index 8dff940e0db9..2a6478aa178d 100644
--- a/drivers/base/firmware.c
+++ b/drivers/base/firmware.c
@@ -11,6 +11,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/device.h>
+#include <linux/firmware_bootinfo.h>
 
 #include "base.h"
 
@@ -24,3 +25,16 @@ int __init firmware_init(void)
 		return -ENOMEM;
 	return 0;
 }
+
+/*
+ * Exposes attributes documented in Documentation/ABI/testing/sysfs-firmware-bootinfo
+ */
+int __init firmware_bootinfo_init(const struct attribute_group *attr_group)
+{
+	struct kobject *kobj= kobject_create_and_add("bootinfo", firmware_kobj);
+	if (!kobj)
+		return -ENOMEM;
+
+	return sysfs_create_group(kobj, attr_group);
+}
+EXPORT_SYMBOL_GPL(firmware_bootinfo_init);
diff --git a/include/linux/firmware_bootinfo.h b/include/linux/firmware_bootinfo.h
new file mode 100644
index 000000000000..581b68508ec8
--- /dev/null
+++ b/include/linux/firmware_bootinfo.h
@@ -0,0 +1,8 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright 2022 IBM Corp
+
+#include <linux/sysfs.h>
+#include <linux/init.h>
+
+int __init firmware_bootinfo_init(const struct attribute_group *attr_group);
+
-- 
2.34.1

[PATCH 2/2] ARM: aspeed: Add secure boot controller support

[Joel Stanley]

This reads out the status of the secure boot controller and exposes it
in sysfs using the bootinfo sysfs api.

An example on a AST2600A3 QEMU model:

 # grep -r . /sys/firmware/bootinfo/*
 /sys/firmware/bootinfo/abr_image:0
 /sys/firmware/bootinfo/low_security_key:0
 /sys/firmware/bootinfo/otp_protected:0
 /sys/firmware/bootinfo/secure_boot:1
 /sys/firmware/bootinfo/uart_boot:0

On boot the state of the system according to the secure boot controller
will be printed:

 [    0.037634] AST2600 secure boot enabled

or

 [    0.037935] AST2600 secure boot disabled

The initialisation is changed from early_initcall to subsys_initcall
because we need the firmware_kobj to be initialised, and because there's
no requirement to print this information early.

Signed-off-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Ryan Chen <ryan_chen@aspeedtech.com>
---
 drivers/soc/aspeed/aspeed-socinfo.c | 84 ++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/aspeed/aspeed-socinfo.c b/drivers/soc/aspeed/aspeed-socinfo.c
index 1ca140356a08..fe77b31e4d1d 100644
--- a/drivers/soc/aspeed/aspeed-socinfo.c
+++ b/drivers/soc/aspeed/aspeed-socinfo.c
@@ -8,6 +8,9 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
+#include <linux/firmware_bootinfo.h>
+
+static u32 security_status;
 
 static struct {
 	const char *name;
@@ -74,6 +77,83 @@ static const char *siliconid_to_rev(u32 siliconid)
 	return "??";
 }
 
+#define SEC_STATUS		0x14
+#define ABR_IMAGE_SOURCE	BIT(13)
+#define OTP_PROTECTED		BIT(8)
+#define LOW_SEC_KEY		BIT(7)
+#define SECURE_BOOT		BIT(6)
+#define UART_BOOT		BIT(5)
+
+static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(security_status & ABR_IMAGE_SOURCE));
+}
+static DEVICE_ATTR_RO(abr_image);
+
+static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(security_status & LOW_SEC_KEY));
+}
+static DEVICE_ATTR_RO(low_security_key);
+
+static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(security_status & OTP_PROTECTED));
+}
+static DEVICE_ATTR_RO(otp_protected);
+
+static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sprintf(buf, "%d\n", !!(security_status & SECURE_BOOT));
+}
+static DEVICE_ATTR_RO(secure_boot);
+
+static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	/* Invert the bit, as 1 is boot from SPI/eMMC */
+	return sprintf(buf, "%d\n", !(security_status & UART_BOOT));
+}
+static DEVICE_ATTR_RO(uart_boot);
+
+static struct attribute *aspeed_attrs[] = {
+	&dev_attr_abr_image.attr,
+	&dev_attr_low_security_key.attr,
+	&dev_attr_otp_protected.attr,
+	&dev_attr_secure_boot.attr,
+	&dev_attr_uart_boot.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(aspeed);
+
+static int __init aspeed_bootinfo_init(void)
+{
+	struct device_node *np;
+	void __iomem *base;
+
+	/* AST2600 only */
+	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
+	if (!of_device_is_available(np))
+		return -ENODEV;
+
+	base = of_iomap(np, 0);
+	if (!base) {
+		of_node_put(np);
+		return -ENODEV;
+	}
+
+	security_status = readl(base + SEC_STATUS);
+
+	iounmap(base);
+	of_node_put(np);
+
+	firmware_bootinfo_init(aspeed_groups[0]);
+
+	pr_info("AST2600 secure boot %s\n",
+		(security_status & SECURE_BOOT) ? "enabled" : "disabled");
+
+	return 0;
+}
+
 static int __init aspeed_socinfo_init(void)
 {
 	struct soc_device_attribute *attrs;
@@ -148,6 +228,8 @@ static int __init aspeed_socinfo_init(void)
 			attrs->revision,
 			attrs->soc_id);
 
+	aspeed_bootinfo_init();
+
 	return 0;
 }
-early_initcall(aspeed_socinfo_init);
+subsys_initcall(aspeed_socinfo_init);
-- 
2.34.1

Re: [PATCH 2/2] ARM: aspeed: Add secure boot controller support

[Greg Kroah-Hartman]

On Tue, Feb 01, 2022 at 03:35:01PM +1030, Joel Stanley wrote:
> This reads out the status of the secure boot controller and exposes it
> in sysfs using the bootinfo sysfs api.
> 
> An example on a AST2600A3 QEMU model:
> 
>  # grep -r . /sys/firmware/bootinfo/*
>  /sys/firmware/bootinfo/abr_image:0
>  /sys/firmware/bootinfo/low_security_key:0
>  /sys/firmware/bootinfo/otp_protected:0
>  /sys/firmware/bootinfo/secure_boot:1
>  /sys/firmware/bootinfo/uart_boot:0
> 
> On boot the state of the system according to the secure boot controller
> will be printed:
> 
>  [    0.037634] AST2600 secure boot enabled
> 
> or
> 
>  [    0.037935] AST2600 secure boot disabled
> 
> The initialisation is changed from early_initcall to subsys_initcall
> because we need the firmware_kobj to be initialised, and because there's
> no requirement to print this information early.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Ryan Chen <ryan_chen@aspeedtech.com>
> ---
>  drivers/soc/aspeed/aspeed-socinfo.c | 84 ++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-socinfo.c b/drivers/soc/aspeed/aspeed-socinfo.c
> index 1ca140356a08..fe77b31e4d1d 100644
> --- a/drivers/soc/aspeed/aspeed-socinfo.c
> +++ b/drivers/soc/aspeed/aspeed-socinfo.c
> @@ -8,6 +8,9 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/sys_soc.h>
> +#include <linux/firmware_bootinfo.h>
> +
> +static u32 security_status;
>  
>  static struct {
>  	const char *name;
> @@ -74,6 +77,83 @@ static const char *siliconid_to_rev(u32 siliconid)
>  	return "??";
>  }
>  
> +#define SEC_STATUS		0x14
> +#define ABR_IMAGE_SOURCE	BIT(13)
> +#define OTP_PROTECTED		BIT(8)
> +#define LOW_SEC_KEY		BIT(7)
> +#define SECURE_BOOT		BIT(6)
> +#define UART_BOOT		BIT(5)

Where do these bits come from?

> +
> +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", !!(security_status & ABR_IMAGE_SOURCE));

sysfs_emit() here and everywhere else you use sprintf() please.

> +}
> +static DEVICE_ATTR_RO(abr_image);
> +
> +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", !!(security_status & LOW_SEC_KEY));
> +}
> +static DEVICE_ATTR_RO(low_security_key);
> +
> +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", !!(security_status & OTP_PROTECTED));
> +}
> +static DEVICE_ATTR_RO(otp_protected);
> +
> +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", !!(security_status & SECURE_BOOT));
> +}
> +static DEVICE_ATTR_RO(secure_boot);
> +
> +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	/* Invert the bit, as 1 is boot from SPI/eMMC */
> +	return sprintf(buf, "%d\n", !(security_status & UART_BOOT));
> +}
> +static DEVICE_ATTR_RO(uart_boot);
> +
> +static struct attribute *aspeed_attrs[] = {
> +	&dev_attr_abr_image.attr,
> +	&dev_attr_low_security_key.attr,
> +	&dev_attr_otp_protected.attr,
> +	&dev_attr_secure_boot.attr,
> +	&dev_attr_uart_boot.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(aspeed);
> +
> +static int __init aspeed_bootinfo_init(void)
> +{
> +	struct device_node *np;
> +	void __iomem *base;
> +
> +	/* AST2600 only */
> +	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
> +	if (!of_device_is_available(np))
> +		return -ENODEV;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		of_node_put(np);
> +		return -ENODEV;
> +	}
> +
> +	security_status = readl(base + SEC_STATUS);
> +
> +	iounmap(base);
> +	of_node_put(np);
> +
> +	firmware_bootinfo_init(aspeed_groups[0]);
> +
> +	pr_info("AST2600 secure boot %s\n",
> +		(security_status & SECURE_BOOT) ? "enabled" : "disabled");

When all is good, no need to print anything out.

> +
> +	return 0;
> +}
> +
>  static int __init aspeed_socinfo_init(void)
>  {
>  	struct soc_device_attribute *attrs;
> @@ -148,6 +228,8 @@ static int __init aspeed_socinfo_init(void)
>  			attrs->revision,
>  			attrs->soc_id);
>  
> +	aspeed_bootinfo_init();

Why does this function return a value and yet you ignore it?


thanks,

greg k-h

Re: [PATCH 2/2] ARM: aspeed: Add secure boot controller support

[Joel Stanley]

On Tue, 1 Feb 2022 at 08:41, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 01, 2022 at 03:35:01PM +1030, Joel Stanley wrote:

> > --- a/drivers/soc/aspeed/aspeed-socinfo.c
> > +++ b/drivers/soc/aspeed/aspeed-socinfo.c
> > @@ -8,6 +8,9 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/sys_soc.h>
> > +#include <linux/firmware_bootinfo.h>
> > +
> > +static u32 security_status;
> >
> >  static struct {
> >       const char *name;
> > @@ -74,6 +77,83 @@ static const char *siliconid_to_rev(u32 siliconid)
> >       return "??";
> >  }
> >
> > +#define SEC_STATUS           0x14
> > +#define ABR_IMAGE_SOURCE     BIT(13)
> > +#define OTP_PROTECTED                BIT(8)
> > +#define LOW_SEC_KEY          BIT(7)
> > +#define SECURE_BOOT          BIT(6)
> > +#define UART_BOOT            BIT(5)
>
> Where do these bits come from?

They are taken from the datasheet.

> > +     pr_info("AST2600 secure boot %s\n",
> > +             (security_status & SECURE_BOOT) ? "enabled" : "disabled");
>
> When all is good, no need to print anything out.

We had some back and forth on this in an earlier iteration of this change:

 https://lore.kernel.org/all/57584776-06e7-0faf-aeb2-eab0c7c5ae1f@molgen.mpg.de/

It boils down to what is "good"? The system is fine if it is not
provisioned with secure boot keys, if that's the intent of the system
builder.

A similar thing is done for efi secure boot, where it prints out
whether it's enabled, disabled or unable to determine.

I'll send out a v2 that takes on the suggestions you made in the cover letter.

Cheers,

Joel

Re: [PATCH 0/2] firmware: Add boot information to sysfs

[Greg Kroah-Hartman]

On Tue, Feb 01, 2022 at 03:34:59PM +1030, Joel Stanley wrote:
> This is the second iteration of this idea. The first used socinfo
> custom attribute groups, but Arnd suggested we make this something
> standardised under /sys/firmware instead:
> 
>  http://lore.kernel.org/all/CAK8P3a3MRf0aGt1drkgsuZyBbeoy+S7Ha18SBM01q+3f33oL+Q@mail.gmail.com
> 
> Some ARM systems have a firmware that provides a hardware root of
> trust. It's useful for the system to know how this root of trust has
> been configured, so provide a standardised interface that expose this
> information to userspace.
> 
> This is implemented as a sysfs attribute group registration to be called
> at boot, with the properties described in the ABI document.
> 
> Alternatively we could put the properties in the driver core, and have
> platforms register callbacks for each supported property. This would
> make it harder to insert non-standard properties, with the trade off of
> more code to selectively show supported properties.

It is trivial to selectively show properties in sysfs, the is_visible()
callback is there for this very reason.

So yes, this should be in the driver core so that we do not have random
platform values and fields that have no relation to each other at all.

For example, you could provide these fields for UEFI today, right?  That
would be a good proof if this really can work for multiple systems or
not.

thanks,

greg k-h