linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] firmware: Add boot information to sysfs
@ 2022-02-03 11:53 Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-03 11:53 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman, Rafael J . Wysocki
  Cc: linux-kernel, linux-arm-kernel, linux-aspeed

v2 reworks the series to put the sysfs properties in the core, and
optionally show them with the is_visible() callback.

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 in the
driver core, with platforms populating values obtained from firmware at
kernel boot time.

Patch 2 provides a user of the properties on an ARM system.

Patch 3 is new in v2 and is an example of populating bootinfo with the
EFI secure boot status.

Joel Stanley (3):
  firmware: Add boot information to sysfs
  ARM: aspeed: Add secure boot controller support
  x86/setup: Populate bootinfo with secure boot status

 .../ABI/testing/sysfs-firmware-bootinfo       | 43 +++++++++
 arch/x86/kernel/setup.c                       |  6 ++
 drivers/base/firmware.c                       | 90 +++++++++++++++++++
 drivers/soc/aspeed/aspeed-socinfo.c           | 46 +++++++++-
 include/linux/firmware_bootinfo.h             | 22 +++++
 5 files changed, 206 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-bootinfo
 create mode 100644 include/linux/firmware_bootinfo.h

-- 
2.34.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 11:53 [PATCH v2 0/3] firmware: Add boot information to sysfs Joel Stanley
@ 2022-02-03 11:53 ` Joel Stanley
  2022-02-03 12:44   ` Greg Kroah-Hartman
                     ` (2 more replies)
  2022-02-03 11:53 ` [PATCH v2 2/3] ARM: aspeed: Add secure boot controller support Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 3/3] x86/setup: Populate bootinfo with secure boot status Joel Stanley
  2 siblings, 3 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-03 11:53 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman, Rafael J . Wysocki
  Cc: linux-kernel, linux-arm-kernel, linux-aspeed

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 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.

These properties are populated by platform code at startup. Using fixed
values is suitable as the state that the system booted in will not
change after firmware has handed over.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
 - Rewrite so properties are present in common code and are exposed based
   on the is_visible callback.
 - Use sysfs_emit
---
 .../ABI/testing/sysfs-firmware-bootinfo       | 43 +++++++++
 drivers/base/firmware.c                       | 90 +++++++++++++++++++
 include/linux/firmware_bootinfo.h             | 22 +++++
 3 files changed, 155 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..24b931232eb2 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,92 @@ int __init firmware_init(void)
 		return -ENOMEM;
 	return 0;
 }
+
+/*
+ * Exposes attributes documented in Documentation/ABI/testing/sysfs-firmware-bootinfo
+ */
+static struct bootinfo bootinfo;
+
+static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%d\n", bootinfo.abr_image.val);
+}
+static DEVICE_ATTR_RO(abr_image);
+
+static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%d\n", bootinfo.low_security_key.val);
+}
+static DEVICE_ATTR_RO(low_security_key);
+
+static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%d\n", bootinfo.otp_protected.val);
+}
+static DEVICE_ATTR_RO(otp_protected);
+
+static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%d\n", bootinfo.secure_boot.val);
+}
+static DEVICE_ATTR_RO(secure_boot);
+
+static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%d\n", bootinfo.uart_boot.val);
+}
+static DEVICE_ATTR_RO(uart_boot);
+
+#define ATTR_ENABLED(a) ((attr == &dev_attr_##a.attr) && bootinfo.a.en)
+
+static umode_t bootinfo_attr_mode(struct kobject *kobj, struct attribute *attr, int index)
+{
+	if (ATTR_ENABLED(abr_image))
+		return 0444;
+
+	if (ATTR_ENABLED(otp_protected))
+		return 0444;
+
+	if (ATTR_ENABLED(low_security_key))
+		return 0444;
+
+	if (ATTR_ENABLED(otp_protected))
+		return 0444;
+
+	if (ATTR_ENABLED(low_security_key))
+		return 0444;
+
+	if (ATTR_ENABLED(secure_boot))
+		return 0444;
+
+	if (ATTR_ENABLED(uart_boot))
+		return 0444;
+
+	return 0;
+}
+
+static struct attribute *bootinfo_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,
+};
+
+static const struct attribute_group bootinfo_attr_group = {
+	.attrs = bootinfo_attrs,
+	.is_visible = bootinfo_attr_mode,
+};
+
+int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init)
+{
+	struct kobject *kobj = kobject_create_and_add("bootinfo", firmware_kobj);
+	if (!kobj)
+		return -ENOMEM;
+
+	memcpy(&bootinfo, bootinfo_init, sizeof(bootinfo));
+
+	return sysfs_create_group(kobj, &bootinfo_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..3fe630b061b9
--- /dev/null
+++ b/include/linux/firmware_bootinfo.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/* Copyright 2022 IBM Corp. */
+
+#include <linux/sysfs.h>
+#include <linux/init.h>
+
+#define BOOTINFO_SET(b, n, v) b.n.en = true; b.n.val = v
+
+struct bootinfo_entry {
+	bool en;
+	bool val;
+};
+
+struct bootinfo {
+	struct bootinfo_entry abr_image;
+	struct bootinfo_entry low_security_key;
+	struct bootinfo_entry otp_protected;
+	struct bootinfo_entry secure_boot;
+	struct bootinfo_entry uart_boot;
+};
+
+int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] ARM: aspeed: Add secure boot controller support
  2022-02-03 11:53 [PATCH v2 0/3] firmware: Add boot information to sysfs Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
@ 2022-02-03 11:53 ` Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 3/3] x86/setup: Populate bootinfo with secure boot status Joel Stanley
  2 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-03 11:53 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman, Rafael J . Wysocki
  Cc: linux-kernel, linux-arm-kernel, linux-aspeed

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>
---
v2:
   - Rewrite to new bootinfo api
   - Get rid of unused return values
---
 drivers/soc/aspeed/aspeed-socinfo.c | 46 ++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/aspeed/aspeed-socinfo.c b/drivers/soc/aspeed/aspeed-socinfo.c
index 1ca140356a08..dc4dfd3df55f 100644
--- a/drivers/soc/aspeed/aspeed-socinfo.c
+++ b/drivers/soc/aspeed/aspeed-socinfo.c
@@ -8,6 +8,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/sys_soc.h>
+#include <linux/firmware_bootinfo.h>
 
 static struct {
 	const char *name;
@@ -74,6 +75,47 @@ static const char *siliconid_to_rev(u32 siliconid)
 	return "??";
 }
 
+/* Secure Boot Controller register */
+#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 void __init aspeed_bootinfo_init(void)
+{
+	struct device_node *np;
+	void __iomem *base;
+	struct bootinfo bootinfo = {};
+	u32 reg;
+
+	/* AST2600 only */
+	np = of_find_compatible_node(NULL, NULL, "aspeed,ast2600-sbc");
+	if (!of_device_is_available(np))
+		return;
+
+	base = of_iomap(np, 0);
+	if (!base)
+		of_node_put(np);
+
+	reg = readl(base + SEC_STATUS);
+
+	iounmap(base);
+	of_node_put(np);
+
+	BOOTINFO_SET(bootinfo, abr_image,        reg & ABR_IMAGE_SOURCE);
+	BOOTINFO_SET(bootinfo, low_security_key, reg & LOW_SEC_KEY);
+	BOOTINFO_SET(bootinfo, otp_protected,    reg & OTP_PROTECTED);
+	BOOTINFO_SET(bootinfo, secure_boot,      reg & SECURE_BOOT);
+	/* Invert the bit; as 1 is boot from SPI/eMMC */
+	BOOTINFO_SET(bootinfo, uart_boot,        !(reg & UART_BOOT));
+
+	firmware_bootinfo_init(&bootinfo);
+
+	pr_info("AST2600 secure boot %s\n", (reg & SECURE_BOOT) ? "enabled" : "disabled");
+}
+
 static int __init aspeed_socinfo_init(void)
 {
 	struct soc_device_attribute *attrs;
@@ -148,6 +190,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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] x86/setup: Populate bootinfo with secure boot status
  2022-02-03 11:53 [PATCH v2 0/3] firmware: Add boot information to sysfs Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
  2022-02-03 11:53 ` [PATCH v2 2/3] ARM: aspeed: Add secure boot controller support Joel Stanley
@ 2022-02-03 11:53 ` Joel Stanley
  2 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-03 11:53 UTC (permalink / raw)
  To: Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman, Rafael J . Wysocki
  Cc: linux-kernel, linux-arm-kernel, linux-aspeed

bootinfo indicates to userspace that firmware is configured to boot with
secure boot.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: new
---
 arch/x86/kernel/setup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index f7a132eb794d..b805b758478f 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -23,6 +23,7 @@
 #include <linux/usb/xhci-dbgp.h>
 #include <linux/static_call.h>
 #include <linux/swiotlb.h>
+#include <linux/firmware_bootinfo.h>
 
 #include <uapi/linux/mount.h>
 
@@ -1100,17 +1101,22 @@ void __init setup_arch(char **cmdline_p)
 	setup_log_buf(1);
 
 	if (efi_enabled(EFI_BOOT)) {
+		struct bootinfo bootinfo = {};
+
 		switch (boot_params.secure_boot) {
 		case efi_secureboot_mode_disabled:
 			pr_info("Secure boot disabled\n");
+			BOOTINFO_SET(bootinfo, secure_boot, false);
 			break;
 		case efi_secureboot_mode_enabled:
 			pr_info("Secure boot enabled\n");
+			BOOTINFO_SET(bootinfo, secure_boot, true);
 			break;
 		default:
 			pr_info("Secure boot could not be determined\n");
 			break;
 		}
+		firmware_bootinfo_init(&bootinfo);
 	}
 
 	reserve_initrd();
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
@ 2022-02-03 12:44   ` Greg Kroah-Hartman
  2022-02-04  6:55     ` Joel Stanley
  2022-02-03 14:22   ` Robin Murphy
  2022-02-07  6:39   ` Daniel Axtens
  2 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-03 12:44 UTC (permalink / raw)
  To: Joel Stanley
  Cc: Arnd Bergmann, Andrew Jeffery, Rafael J . Wysocki, linux-kernel,
	linux-arm-kernel, linux-aspeed

On Thu, Feb 03, 2022 at 10:23:42PM +1030, Joel Stanley wrote:
> 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 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.
> 
> These properties are populated by platform code at startup. Using fixed
> values is suitable as the state that the system booted in will not
> change after firmware has handed over.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>  - Rewrite so properties are present in common code and are exposed based
>    on the is_visible callback.
>  - Use sysfs_emit
> ---
>  .../ABI/testing/sysfs-firmware-bootinfo       | 43 +++++++++
>  drivers/base/firmware.c                       | 90 +++++++++++++++++++
>  include/linux/firmware_bootinfo.h             | 22 +++++
>  3 files changed, 155 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..24b931232eb2 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,92 @@ int __init firmware_init(void)
>  		return -ENOMEM;
>  	return 0;
>  }
> +
> +/*
> + * Exposes attributes documented in Documentation/ABI/testing/sysfs-firmware-bootinfo
> + */
> +static struct bootinfo bootinfo;
> +
> +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.abr_image.val);
> +}
> +static DEVICE_ATTR_RO(abr_image);
> +
> +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.low_security_key.val);
> +}
> +static DEVICE_ATTR_RO(low_security_key);
> +
> +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.otp_protected.val);
> +}
> +static DEVICE_ATTR_RO(otp_protected);
> +
> +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.secure_boot.val);
> +}
> +static DEVICE_ATTR_RO(secure_boot);
> +
> +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.uart_boot.val);
> +}
> +static DEVICE_ATTR_RO(uart_boot);
> +
> +#define ATTR_ENABLED(a) ((attr == &dev_attr_##a.attr) && bootinfo.a.en)
> +
> +static umode_t bootinfo_attr_mode(struct kobject *kobj, struct attribute *attr, int index)
> +{
> +	if (ATTR_ENABLED(abr_image))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(otp_protected))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(low_security_key))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(otp_protected))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(low_security_key))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(secure_boot))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(uart_boot))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute *bootinfo_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,
> +};
> +
> +static const struct attribute_group bootinfo_attr_group = {
> +	.attrs = bootinfo_attrs,
> +	.is_visible = bootinfo_attr_mode,
> +};
> +
> +int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init)
> +{
> +	struct kobject *kobj = kobject_create_and_add("bootinfo", firmware_kobj);
> +	if (!kobj)
> +		return -ENOMEM;
> +
> +	memcpy(&bootinfo, bootinfo_init, sizeof(bootinfo));
> +
> +	return sysfs_create_group(kobj, &bootinfo_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..3fe630b061b9
> --- /dev/null
> +++ b/include/linux/firmware_bootinfo.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

I have to ask, do you really mean "or later"?


> +/* Copyright 2022 IBM Corp. */
> +
> +#include <linux/sysfs.h>
> +#include <linux/init.h>
> +
> +#define BOOTINFO_SET(b, n, v) b.n.en = true; b.n.val = v

Please use a while {} loop around these two statements.

Didn't checkpatch warn you about that?

> +struct bootinfo_entry {
> +	bool en;

What does "en" mean?  "enabled"?  If so, please spell it out.

> +	bool val;

How can a "value" have a boolean?  Is this "valid"?  Again, please spell
it out, we have no lack of letters to use here to keep people from being
confused.

Can you please use kernel-doc comments to describe this structure?


> +};
> +
> +struct bootinfo {
> +	struct bootinfo_entry abr_image;
> +	struct bootinfo_entry low_security_key;
> +	struct bootinfo_entry otp_protected;
> +	struct bootinfo_entry secure_boot;
> +	struct bootinfo_entry uart_boot;
> +};

Same here, please use kernel-doc

> +
> +int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init);

__init is not needed on a function definition like this.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
  2022-02-03 12:44   ` Greg Kroah-Hartman
@ 2022-02-03 14:22   ` Robin Murphy
  2022-02-04  5:53     ` Joel Stanley
  2022-02-07  6:39   ` Daniel Axtens
  2 siblings, 1 reply; 9+ messages in thread
From: Robin Murphy @ 2022-02-03 14:22 UTC (permalink / raw)
  To: Joel Stanley, Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman,
	Rafael J . Wysocki
  Cc: linux-kernel, linux-arm-kernel, linux-aspeed

On 2022-02-03 11:53, Joel Stanley wrote:
> 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 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.
> 
> These properties are populated by platform code at startup. Using fixed
> values is suitable as the state that the system booted in will not
> change after firmware has handed over.
> 
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>   - Rewrite so properties are present in common code and are exposed based
>     on the is_visible callback.
>   - Use sysfs_emit
> ---
>   .../ABI/testing/sysfs-firmware-bootinfo       | 43 +++++++++
>   drivers/base/firmware.c                       | 90 +++++++++++++++++++
>   include/linux/firmware_bootinfo.h             | 22 +++++
>   3 files changed, 155 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.

I'd be concerned about how well details like "uart_boot" and "abr_image" 
scale beyond one SoC family from one vendor. The combinatorial explosion 
of possible boot configurations across Linux-capable SoCs and firmware 
in general is larger than I'd care to imagine, even before considering 
that the nuances don't necessarily stop there - e.g. whether a given 
storage interface is hard-wired or user-accessible is not a fixed 
property on many SoCs, and even a socketed SD card might be "trusted" if 
a board is deployed in a product with a sealed enclosure.

Cheers,
Robin.

> diff --git a/drivers/base/firmware.c b/drivers/base/firmware.c
> index 8dff940e0db9..24b931232eb2 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,92 @@ int __init firmware_init(void)
>   		return -ENOMEM;
>   	return 0;
>   }
> +
> +/*
> + * Exposes attributes documented in Documentation/ABI/testing/sysfs-firmware-bootinfo
> + */
> +static struct bootinfo bootinfo;
> +
> +static ssize_t abr_image_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.abr_image.val);
> +}
> +static DEVICE_ATTR_RO(abr_image);
> +
> +static ssize_t low_security_key_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.low_security_key.val);
> +}
> +static DEVICE_ATTR_RO(low_security_key);
> +
> +static ssize_t otp_protected_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.otp_protected.val);
> +}
> +static DEVICE_ATTR_RO(otp_protected);
> +
> +static ssize_t secure_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.secure_boot.val);
> +}
> +static DEVICE_ATTR_RO(secure_boot);
> +
> +static ssize_t uart_boot_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%d\n", bootinfo.uart_boot.val);
> +}
> +static DEVICE_ATTR_RO(uart_boot);
> +
> +#define ATTR_ENABLED(a) ((attr == &dev_attr_##a.attr) && bootinfo.a.en)
> +
> +static umode_t bootinfo_attr_mode(struct kobject *kobj, struct attribute *attr, int index)
> +{
> +	if (ATTR_ENABLED(abr_image))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(otp_protected))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(low_security_key))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(otp_protected))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(low_security_key))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(secure_boot))
> +		return 0444;
> +
> +	if (ATTR_ENABLED(uart_boot))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static struct attribute *bootinfo_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,
> +};
> +
> +static const struct attribute_group bootinfo_attr_group = {
> +	.attrs = bootinfo_attrs,
> +	.is_visible = bootinfo_attr_mode,
> +};
> +
> +int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init)
> +{
> +	struct kobject *kobj = kobject_create_and_add("bootinfo", firmware_kobj);
> +	if (!kobj)
> +		return -ENOMEM;
> +
> +	memcpy(&bootinfo, bootinfo_init, sizeof(bootinfo));
> +
> +	return sysfs_create_group(kobj, &bootinfo_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..3fe630b061b9
> --- /dev/null
> +++ b/include/linux/firmware_bootinfo.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/* Copyright 2022 IBM Corp. */
> +
> +#include <linux/sysfs.h>
> +#include <linux/init.h>
> +
> +#define BOOTINFO_SET(b, n, v) b.n.en = true; b.n.val = v
> +
> +struct bootinfo_entry {
> +	bool en;
> +	bool val;
> +};
> +
> +struct bootinfo {
> +	struct bootinfo_entry abr_image;
> +	struct bootinfo_entry low_security_key;
> +	struct bootinfo_entry otp_protected;
> +	struct bootinfo_entry secure_boot;
> +	struct bootinfo_entry uart_boot;
> +};
> +
> +int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 14:22   ` Robin Murphy
@ 2022-02-04  5:53     ` Joel Stanley
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-04  5:53 UTC (permalink / raw)
  To: Robin Murphy
  Cc: Arnd Bergmann, Andrew Jeffery, Greg Kroah-Hartman,
	Rafael J . Wysocki, Linux Kernel Mailing List, Linux ARM,
	linux-aspeed

On Thu, 3 Feb 2022 at 14:23, Robin Murphy <robin.murphy@arm.com> wrote:
> > 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.
>
> I'd be concerned about how well details like "uart_boot" and "abr_image"
> scale beyond one SoC family from one vendor. The combinatorial explosion
> of possible boot configurations across Linux-capable SoCs and firmware
> in general is larger than I'd care to imagine, even before considering
> that the nuances don't necessarily stop there - e.g. whether a given
> storage interface is hard-wired or user-accessible is not a fixed
> property on many SoCs, and even a socketed SD card might be "trusted" if
> a board is deployed in a product with a sealed enclosure.

This is a fair point. The first iteration of this idea was specific to
the aspeed soc.

For the system I'm building, secure_boot and otp_locked tell our
manufacturing test process that the machine has been correctly
provisioned. I'd like to get something agreed upon so we can start
using those sysfs files in the testing without having to go back and
change things.

abr_image is an indication that something went wrong at boot time. I
thought this was a standard eMMC concept, but taking a closer look
it's specific to the aspeed soc.

Would it help if we gave them generic names?

 - abr_image -> alternate_boot

I welcome other suggestions.

I'll drop the uart_boot property for now.

Cheers,

Joel

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 12:44   ` Greg Kroah-Hartman
@ 2022-02-04  6:55     ` Joel Stanley
  0 siblings, 0 replies; 9+ messages in thread
From: Joel Stanley @ 2022-02-04  6:55 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Arnd Bergmann, Andrew Jeffery, Rafael J . Wysocki,
	Linux Kernel Mailing List, Linux ARM, linux-aspeed

On Thu, 3 Feb 2022 at 12:44, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Feb 03, 2022 at 10:23:42PM +1030, Joel Stanley wrote:

> > diff --git a/include/linux/firmware_bootinfo.h b/include/linux/firmware_bootinfo.h
> > new file mode 100644
> > index 000000000000..3fe630b061b9
> > --- /dev/null
> > +++ b/include/linux/firmware_bootinfo.h
> > @@ -0,0 +1,22 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
>
> I have to ask, do you really mean "or later"?

Yeah. That's what we're told we should use.

> > +/* Copyright 2022 IBM Corp. */
> > +
> > +#include <linux/sysfs.h>
> > +#include <linux/init.h>
> > +
> > +#define BOOTINFO_SET(b, n, v) b.n.en = true; b.n.val = v
>
> Please use a while {} loop around these two statements.
>
> Didn't checkpatch warn you about that?

No, it didn't. I'll add it.

>
> > +struct bootinfo_entry {
> > +     bool en;
>
> What does "en" mean?  "enabled"?  If so, please spell it out.
>
> > +     bool val;
>
> How can a "value" have a boolean?  Is this "valid"?  Again, please spell
> it out, we have no lack of letters to use here to keep people from being
> confused.

I meant value. I think it's reasonable for a value to be true or
false. I'll make the names clearer with docs as you suggest.

>
> Can you please use kernel-doc comments to describe this structure?
>
>
> > +};
> > +
> > +struct bootinfo {
> > +     struct bootinfo_entry abr_image;
> > +     struct bootinfo_entry low_security_key;
> > +     struct bootinfo_entry otp_protected;
> > +     struct bootinfo_entry secure_boot;
> > +     struct bootinfo_entry uart_boot;
> > +};
>
> Same here, please use kernel-doc
>
> > +
> > +int __init firmware_bootinfo_init(struct bootinfo *bootinfo_init);
>
> __init is not needed on a function definition like this.

ack.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] firmware: Add boot information to sysfs
  2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
  2022-02-03 12:44   ` Greg Kroah-Hartman
  2022-02-03 14:22   ` Robin Murphy
@ 2022-02-07  6:39   ` Daniel Axtens
  2 siblings, 0 replies; 9+ messages in thread
From: Daniel Axtens @ 2022-02-07  6:39 UTC (permalink / raw)
  To: joel
  Cc: andrew, arnd, gregkh, linux-arm-kernel, linux-aspeed,
	linux-kernel, rafael, nayna, Daniel Axtens

Hi,

I really like this design.

Currently on powerpc, a user wanting to check if firmware was
configured to boot with secure boot has to understand both OpenPower
and PowerVM secure boot. On OpenPower they need to check for the
presence of a device-tree property. If they're on PowerVM they need to
decode a different device-tree property and check it's 2 or
greater. Of course, it's not stored as ASCII, it's \x02. And it's
stored big-endian too.

So if powerpc implemented this infrastructure, it would provide users
with one single place to look, and it would represent the value as
ASCII. All very lovely, and it would simplify some scripts enormously.

Reviewed-by: Daniel Axtens <dja@axtens.net>

Kind regards,
Daniel

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-02-07  7:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-03 11:53 [PATCH v2 0/3] firmware: Add boot information to sysfs Joel Stanley
2022-02-03 11:53 ` [PATCH v2 1/3] " Joel Stanley
2022-02-03 12:44   ` Greg Kroah-Hartman
2022-02-04  6:55     ` Joel Stanley
2022-02-03 14:22   ` Robin Murphy
2022-02-04  5:53     ` Joel Stanley
2022-02-07  6:39   ` Daniel Axtens
2022-02-03 11:53 ` [PATCH v2 2/3] ARM: aspeed: Add secure boot controller support Joel Stanley
2022-02-03 11:53 ` [PATCH v2 3/3] x86/setup: Populate bootinfo with secure boot status Joel Stanley

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).