[PATCH v2 0/2] ARM: aspeed: Add secure boot controller debugfs

[PATCH v2 0/2] ARM: aspeed: Add secure boot controller debugfs

[Joel Stanley]

We're creating a common API for a subset of this information in sysfs:

 https://lore.kernel.org/all/20220204072234.304543-1-joel@jms.id.au/

However, machines with an ASPEED soc need the detailed information from
the SBE that is not relevant for other systems, so expose it all in
debugfs.

v2 Moves the files to aspeed/sbc, and uses arch_debugfs_dir instead of a
driver specific directory.

v1: https://lore.kernel.org/linux-arm-kernel/20220304030336.1017197-1-joel@jms.id.au/

Joel Stanley (2):
  ARM: aspeed: Add debugfs directory
  ARM: soc: aspeed: Add secure boot controller support

 arch/arm/mach-aspeed/debugfs.c  | 16 ++++++++
 drivers/soc/aspeed/aspeed-sbc.c | 73 +++++++++++++++++++++++++++++++++
 arch/arm/mach-aspeed/Makefile   |  1 +
 drivers/soc/aspeed/Kconfig      |  7 ++++
 drivers/soc/aspeed/Makefile     |  1 +
 5 files changed, 98 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/debugfs.c
 create mode 100644 drivers/soc/aspeed/aspeed-sbc.c

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/2] ARM: aspeed: Add debugfs directory

[Joel Stanley]

This will be used by other drivers to hold machine specific debugfs
information.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/arm/mach-aspeed/debugfs.c | 16 ++++++++++++++++
 arch/arm/mach-aspeed/Makefile  |  1 +
 2 files changed, 17 insertions(+)
 create mode 100644 arch/arm/mach-aspeed/debugfs.c

diff --git a/arch/arm/mach-aspeed/debugfs.c b/arch/arm/mach-aspeed/debugfs.c
new file mode 100644
index 000000000000..b7d1b8f28435
--- /dev/null
+++ b/arch/arm/mach-aspeed/debugfs.c
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright 2022 IBM Corp.
+
+#include <linux/debugfs.h>
+#include <linux/export.h>
+#include <linux/init.h>
+
+struct dentry *arch_debugfs_dir;
+EXPORT_SYMBOL(arch_debugfs_dir);
+
+static int __init aspeed_debugfs_init(void)
+{
+	arch_debugfs_dir = debugfs_create_dir("aspeed", NULL);
+	return 0;
+}
+arch_initcall(aspeed_debugfs_init);
diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile
index 1951b3317a76..3db448ccdfe1 100644
--- a/arch/arm/mach-aspeed/Makefile
+++ b/arch/arm/mach-aspeed/Makefile
@@ -3,3 +3,4 @@
 # Copyright IBM Corp.
 
 obj-$(CONFIG_SMP) += platsmp.o
+obj-$(CONFIG_DEBUG_FS) += debugfs.o
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Joel Stanley]

This reads out the status of the secure boot controller and exposes it
in debugfs.

An example on a AST2600A3 QEMU model:

 # grep -r . /sys/kernel/debug/aspeed/*
 /sys/kernel/debug/aspeed/sbc/abr_image:0
 /sys/kernel/debug/aspeed/sbc/low_security_key:0
 /sys/kernel/debug/aspeed/sbc/otp_protected:0
 /sys/kernel/debug/aspeed/sbc/secure_boot:1
 /sys/kernel/debug/aspeed/sbc/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

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
  - Place files in aspeed/sbc
  - Check for error when creating directory
  - Print secure boot message even if debugfs is disabled
---
 drivers/soc/aspeed/aspeed-sbc.c | 73 +++++++++++++++++++++++++++++++++
 drivers/soc/aspeed/Kconfig      |  7 ++++
 drivers/soc/aspeed/Makefile     |  1 +
 3 files changed, 81 insertions(+)
 create mode 100644 drivers/soc/aspeed/aspeed-sbc.c

diff --git a/drivers/soc/aspeed/aspeed-sbc.c b/drivers/soc/aspeed/aspeed-sbc.c
new file mode 100644
index 000000000000..be4497b418c4
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-sbc.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/* Copyright 2022 IBM Corp. */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/debugfs.h>
+
+#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)
+
+struct sbe {
+	u8 abr_image;
+	u8 low_security_key;
+	u8 otp_protected;
+	u8 secure_boot;
+	u8 invert;
+	u8 uart_boot;
+};
+
+static struct sbe sbe;
+
+static int __init aspeed_sbc_init(void)
+{
+	struct device_node *np;
+	void __iomem *base;
+	struct dentry *sbc_dir;
+	u32 security_status;
+
+	/* 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);
+
+	sbe.abr_image = !!(security_status & ABR_IMAGE_SOURCE);
+	sbe.low_security_key = !!(security_status & LOW_SEC_KEY);
+	sbe.otp_protected = !!(security_status & OTP_PROTECTED);
+	sbe.secure_boot = !!(security_status & SECURE_BOOT);
+	/* Invert the bit, as 1 is boot from SPI/eMMC */
+	sbe.uart_boot =  !(security_status & UART_BOOT);
+
+	pr_info("AST2600 secure boot %s\n", sbe.secure_boot ? "enabled" : "disabled");
+
+	sbc_dir = debugfs_create_dir("sbc", arch_debugfs_dir);
+	if (IS_ERR(sbc_dir))
+		return PTR_ERR(sbc_dir);
+
+	debugfs_create_u8("abr_image", 0444, sbc_dir, &sbe.abr_image);
+	debugfs_create_u8("low_security_key", 0444, sbc_dir, &sbe.low_security_key);
+	debugfs_create_u8("otp_protected", 0444, sbc_dir, &sbe.otp_protected);
+	debugfs_create_u8("uart_boot", 0444, sbc_dir, &sbe.uart_boot);
+	debugfs_create_u8("secure_boot", 0444, sbc_dir, &sbe.secure_boot);
+
+	return 0;
+}
+
+subsys_initcall(aspeed_sbc_init);
diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f941c41b84dc..aaf4596ae4f9 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -62,6 +62,13 @@ config ASPEED_XDMA
 	  SoCs. The XDMA engine can perform PCIe DMA operations between the BMC
 	  and a host processor.
 
+config ASPEED_SBC
+	bool "ASPEED Secure Boot Controller driver"
+	default MACH_ASPEED_G6
+	help
+	  Say yes to provide information about the secure boot controller in
+	  debugfs.
+
 endmenu
 
 endif
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index 8fb73cede4bf..9e275fd1d54d 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP)		+= aspeed-lpc-snoop.o
 obj-$(CONFIG_ASPEED_UART_ROUTING)	+= aspeed-uart-routing.o
 obj-$(CONFIG_ASPEED_P2A_CTRL)		+= aspeed-p2a-ctrl.o
 obj-$(CONFIG_ASPEED_SOCINFO)		+= aspeed-socinfo.o
+obj-$(CONFIG_ASPEED_SBC)		+= aspeed-sbc.o
 obj-$(CONFIG_ASPEED_XDMA)	+= aspeed-xdma.o
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Arnd Bergmann]

On Thu, Mar 10, 2022 at 1:06 AM Joel Stanley <joel@jms.id.au> wrote:
>
> This reads out the status of the secure boot controller and exposes it
> in debugfs.
>
> An example on a AST2600A3 QEMU model:
>
>  # grep -r . /sys/kernel/debug/aspeed/*
>  /sys/kernel/debug/aspeed/sbc/abr_image:0
>  /sys/kernel/debug/aspeed/sbc/low_security_key:0
>  /sys/kernel/debug/aspeed/sbc/otp_protected:0
>  /sys/kernel/debug/aspeed/sbc/secure_boot:1
>  /sys/kernel/debug/aspeed/sbc/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
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>   - Place files in aspeed/sbc
>   - Check for error when creating directory
>   - Print secure boot message even if debugfs is disabled

The implementation looks fine to me, but I think the changelog needs to
explain why you picked debugfs over a sysfs interface, and how the
interface is going to be used.

As a rule, sysfs interfaces need to be documented and kept stable
so that user space can rely on it, while debugfs interfaces should only
be used for development and never be accessed by applications
that need to work across kernel versions. If no stable user space
is allowed to access these files, what is accessing them?

      Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Joel Stanley]

On Thu, 10 Mar 2022 at 08:03, Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Mar 10, 2022 at 1:06 AM Joel Stanley <joel@jms.id.au> wrote:
> >
> > This reads out the status of the secure boot controller and exposes it
> > in debugfs.
> >
> > An example on a AST2600A3 QEMU model:
> >
> >  # grep -r . /sys/kernel/debug/aspeed/*
> >  /sys/kernel/debug/aspeed/sbc/abr_image:0
> >  /sys/kernel/debug/aspeed/sbc/low_security_key:0
> >  /sys/kernel/debug/aspeed/sbc/otp_protected:0
> >  /sys/kernel/debug/aspeed/sbc/secure_boot:1
> >  /sys/kernel/debug/aspeed/sbc/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
> >
> > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > ---
> > v2:
> >   - Place files in aspeed/sbc
> >   - Check for error when creating directory
> >   - Print secure boot message even if debugfs is disabled
>
> The implementation looks fine to me, but I think the changelog needs to
> explain why you picked debugfs over a sysfs interface, and how the
> interface is going to be used.
>
> As a rule, sysfs interfaces need to be documented and kept stable
> so that user space can rely on it, while debugfs interfaces should only
> be used for development and never be accessed by applications
> that need to work across kernel versions. If no stable user space
> is allowed to access these files, what is accessing them?

I hear what you're saying, but we're now going around in circles. The
first proposal added a soc-specific sysfs interface, which was
rejected in favor of creating a common interface. The common interface
didn't get any support, and with the feedback being the files were too
soc-specific. You rightly point out that the debugfs API is not
supposed to be relied on as a stable userspace API.

Do you think we should revisit the soc-specific sysfs?

The userspace that accesses the files comes in two forms: a runtime
application that checks the system state, and some manufacturing line
scripts that checks if the machine was correctly provisioned. The
application source can be viewed here:

 https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-state-manager/+/51766

The discussion has lost momentum for me, as in practice we needed to
ship the hardware, so the openbmc kernel will support the version of
the patches that were merged there for the lifetime of the product.
This isn't a threat, but an observation that the mainline kernel
process has failed in this instance, despite the best intentions of
everyone involved.

Cheers,

Joel

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel