* [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
@ 2022-04-19 23:42 Zev Weiss
2022-04-20 3:06 ` Ryan Chen
2022-05-02 8:11 ` Joel Stanley
0 siblings, 2 replies; 7+ messages in thread
From: Zev Weiss @ 2022-04-19 23:42 UTC (permalink / raw)
To: Joel Stanley, openbmc
Cc: Andrew Jeffery, Lei Yu, Ryan Chen, Zev Weiss, Ian Woloschin
On ast2400 and ast2500 we now disable the various hardware backdoor
interfaces as is done on ast2600. Two Kconfig options can selectively
re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO
leaves the ast2x00 built-in Super I/O device enabled, as it is
required for some systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves
the hardware debug UART enabled, since it provides a relatively high
ratio of utility to security risk during development.
This patch is based on a patch by Andrew Jeffery for an older u-boot
branch in the OpenBMC tree for the df-isolate-bmc distro feature flag.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
Tested on ast2500 and (hostless, BMC-only) ast2400.
Ryan, are you OK with having an option (off by default) to leave the
debug UART enabled as in this version of the patch?
Ian, if you could test this out with CONFIG_ASPEED_ENABLE_SUPERIO=y on
one of your systems and confirm that that setting works as intended
that would be great.
Changes since v2 [1]:
- made most of the changes unconditional/unconfigurable, but added
Kconfig options to leave Super I/O and debug UART enabled
Changes since v1 [0]:
- extended to cover ast2400
- inverted sense of Kconfig option, default (n) is now secure mode
- renamed some register/bit macros more appropriately
[0] https://lore.kernel.org/openbmc/20220414040448.27100-1-zev@bewilderbeest.net/
[1] https://lore.kernel.org/openbmc/20220414224004.29703-1-zev@bewilderbeest.net/
arch/arm/include/asm/arch-aspeed/platform.h | 7 ++
.../arm/include/asm/arch-aspeed/scu_ast2400.h | 7 ++
.../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
arch/arm/mach-aspeed/Kconfig | 22 ++++++
arch/arm/mach-aspeed/ast2400/board_common.c | 66 +++++++++++++++++
arch/arm/mach-aspeed/ast2500/board_common.c | 73 +++++++++++++++++++
6 files changed, 183 insertions(+)
diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
index f016bdaba3e7..f05747642f38 100644
--- a/arch/arm/include/asm/arch-aspeed/platform.h
+++ b/arch/arm/include/asm/arch-aspeed/platform.h
@@ -15,24 +15,31 @@
/*********************************************************************************/
#if defined(CONFIG_ASPEED_AST2400)
#define ASPEED_MAC_COUNT 2
+#define ASPEED_SDRAM_CTRL 0x1e6e0000
#define ASPEED_HW_STRAP1 0x1e6e2070
#define ASPEED_REVISION_ID 0x1e6e207C
#define ASPEED_SYS_RESET_CTRL 0x1e6e203C
#define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
+#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
#define ASPEED_DRAM_BASE 0x40000000
#define ASPEED_SRAM_BASE 0x1E720000
+#define ASPEED_LPC_CTRL 0x1e789000
#define ASPEED_SRAM_SIZE 0x8000
#define ASPEED_FMC_CS0_BASE 0x20000000
#elif defined(CONFIG_ASPEED_AST2500)
#define ASPEED_MAC_COUNT 2
+#define ASPEED_SDRAM_CTRL 0x1e6e0000
+#define ASPEED_MISC1_CTRL 0x1e6e202C
#define ASPEED_HW_STRAP1 0x1e6e2070
#define ASPEED_HW_STRAP2 0x1e6e20D0
#define ASPEED_REVISION_ID 0x1e6e207C
#define ASPEED_SYS_RESET_CTRL 0x1e6e203C
#define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
+#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
#define ASPEED_MAC_COUNT 2
#define ASPEED_DRAM_BASE 0x80000000
#define ASPEED_SRAM_BASE 0x1E720000
+#define ASPEED_LPC_CTRL 0x1e789000
#define ASPEED_SRAM_SIZE 0x9000
#define ASPEED_FMC_CS0_BASE 0x20000000
#elif defined(CONFIG_ASPEED_AST2600)
diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
index 9c5d96ae84b9..55875fd8312f 100644
--- a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
+++ b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
@@ -8,6 +8,7 @@
#define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
#define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
#define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
+#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
#define SCU_HWSTRAP_DDR4 (1 << 24)
#define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
@@ -104,6 +105,12 @@
#define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
#define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
+#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
+#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
+#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
+#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
+
+
struct ast2400_clk_priv {
struct ast2400_scu *scu;
};
diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
index 8fe4028e4ff0..06dc998afaa8 100644
--- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
+++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
@@ -11,6 +11,7 @@
#define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
#define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
#define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
+#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
#define SCU_HWSTRAP_DDR4 (1 << 24)
#define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
@@ -107,6 +108,13 @@
#define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
#define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
+#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
+#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
+#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
+#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
+
+#define SCU_MISC_DEBUG_UART_DISABLE (1 << 10)
+
struct ast2500_clk_priv {
struct ast2500_scu *scu;
};
diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
index 579a547df61e..fc565e0da830 100644
--- a/arch/arm/mach-aspeed/Kconfig
+++ b/arch/arm/mach-aspeed/Kconfig
@@ -45,6 +45,28 @@ config ASPEED_AST2600
which is enabled by support of LPC and eSPI peripherals.
endchoice
+config ASPEED_ENABLE_SUPERIO
+ bool "Enable built-in AST2x00 Super I/O hardware"
+ depends on ASPEED_AST2400 || ASPEED_AST2500
+ help
+ The Aspeed AST2400 and AST2500 include a built-in Super I/O
+ device that is normally disabled; say Y here to enable it.
+ Note that this has security implications: it grants the host
+ read access to the BMC's entire address space. This should
+ thus be left disabled unless required by a specific system.
+
+config ASPEED_ENABLE_DEBUG_UART
+ bool "Enable AST2500 hardware debug UART"
+ depends on ASPEED_AST2500
+ help
+ The Aspeed AST2500 include a hardware-supported, UART-based
+ debug interface that is normally disabled; say Y here to
+ enable it. Note that this has security implications: the
+ debug UART provide read/write access to the BMC's entire
+ address space. This should thus be left disabled on
+ production systems, but may be useful to enable for
+ debugging during development.
+
config ASPEED_PALLADIUM
bool "Aspeed palladium for simulation"
default n
diff --git a/arch/arm/mach-aspeed/ast2400/board_common.c b/arch/arm/mach-aspeed/ast2400/board_common.c
index 3829b069342e..7134105232cb 100644
--- a/arch/arm/mach-aspeed/ast2400/board_common.c
+++ b/arch/arm/mach-aspeed/ast2400/board_common.c
@@ -4,14 +4,80 @@
#include <ram.h>
#include <timer.h>
#include <asm/io.h>
+#include <asm/arch/platform.h>
+#include <asm/arch/scu_ast2400.h>
#include <asm/arch/timer.h>
#include <linux/err.h>
#include <dm/uclass.h>
DECLARE_GLOBAL_DATA_PTR;
+#define AST_LPC_HICR5 0x080
+# define LPC_HICR5_ENFWH BIT(10)
+#define AST_LPC_HICRB 0x100
+# define LPC_HICRB_SIO_ILPC2AHB_DIS BIT(6)
+
+#define AST_SDMC_PROTECT 0x00
+# define SDRAM_UNLOCK_KEY 0xfc600309
+#define AST_SDMC_GFX_PROT 0x08
+# define SDMC_GFX_PROT_VGA_CURSOR BIT(0)
+# define SDMC_GFX_PROT_VGA_CG_READ BIT(1)
+# define SDMC_GFX_PROT_VGA_ASCII_READ BIT(2)
+# define SDMC_GFX_PROT_VGA_CRT BIT(3)
+# define SDMC_GFX_PROT_PCIE BIT(16)
+# define SDMC_GFX_PROT_XDMA BIT(17)
+
+static void isolate_bmc(void)
+{
+ bool sdmc_unlocked;
+ u32 val;
+
+ /* iLPC2AHB */
+#if !defined(CONFIG_ASPEED_ENABLE_SUPERIO)
+ val = readl(ASPEED_HW_STRAP1);
+ val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS;
+ writel(val, ASPEED_HW_STRAP1);
+#endif
+
+ val = readl(ASPEED_LPC_CTRL + AST_LPC_HICRB);
+ val |= LPC_HICRB_SIO_ILPC2AHB_DIS;
+ writel(val, ASPEED_LPC_CTRL + AST_LPC_HICRB);
+
+ /* P2A, PCIe BMC */
+ val = readl(ASPEED_PCIE_CONFIG_SET);
+ val &= ~(SCU_PCIE_CONFIG_SET_BMC_DMA
+ | SCU_PCIE_CONFIG_SET_BMC_MMIO
+ | SCU_PCIE_CONFIG_SET_BMC_EN
+ | SCU_PCIE_CONFIG_SET_VGA_MMIO);
+ writel(val, ASPEED_PCIE_CONFIG_SET);
+
+ /* X-DMA */
+ sdmc_unlocked = readl(ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+ if (!sdmc_unlocked)
+ writel(SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+
+ val = readl(ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
+ val |= (SDMC_GFX_PROT_VGA_CURSOR
+ | SDMC_GFX_PROT_VGA_CG_READ
+ | SDMC_GFX_PROT_VGA_ASCII_READ
+ | SDMC_GFX_PROT_VGA_CRT
+ | SDMC_GFX_PROT_PCIE
+ | SDMC_GFX_PROT_XDMA);
+ writel(val, ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
+
+ if (!sdmc_unlocked)
+ writel(~SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+
+ /* LPC2AHB */
+ val = readl(ASPEED_LPC_CTRL + AST_LPC_HICR5);
+ val &= ~LPC_HICR5_ENFWH;
+ writel(val, ASPEED_LPC_CTRL + AST_LPC_HICR5);
+}
+
__weak int board_init(void)
{
+ isolate_bmc();
+
gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
return 0;
diff --git a/arch/arm/mach-aspeed/ast2500/board_common.c b/arch/arm/mach-aspeed/ast2500/board_common.c
index ce541e88fb8e..c63fe466eb4b 100644
--- a/arch/arm/mach-aspeed/ast2500/board_common.c
+++ b/arch/arm/mach-aspeed/ast2500/board_common.c
@@ -7,18 +7,91 @@
#include <ram.h>
#include <timer.h>
#include <asm/io.h>
+#include <asm/arch/platform.h>
+#include <asm/arch/scu_ast2500.h>
+#include <asm/arch/sdram_ast2500.h>
#include <asm/arch/timer.h>
#include <linux/err.h>
#include <dm/uclass.h>
DECLARE_GLOBAL_DATA_PTR;
+#define AST_LPC_HICR5 0x080
+# define LPC_HICR5_ENFWH BIT(10)
+#define AST_LPC_HICRB 0x100
+# define LPC_HICRB_SIO_ILPC2AHB_DIS BIT(6)
+
+# define AST_SDMC_PROTECT 0x00
+# define AST_SDMC_GFX_PROT 0x08
+# define SDMC_GFX_PROT_VGA_CURSOR BIT(0)
+# define SDMC_GFX_PROT_VGA_CG_READ BIT(1)
+# define SDMC_GFX_PROT_VGA_ASCII_READ BIT(2)
+# define SDMC_GFX_PROT_VGA_CRT BIT(3)
+# define SDMC_GFX_PROT_PCIE BIT(16)
+# define SDMC_GFX_PROT_XDMA BIT(17)
+
+static void isolate_bmc(void)
+{
+ bool sdmc_unlocked;
+ u32 val;
+
+ /* iLPC2AHB */
+#if !defined(CONFIG_ASPEED_ENABLE_SUPERIO)
+ val = readl(ASPEED_HW_STRAP1);
+ val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS;
+ writel(val, ASPEED_HW_STRAP1);
+#endif
+
+ val = readl(ASPEED_LPC_CTRL + AST_LPC_HICRB);
+ val |= LPC_HICRB_SIO_ILPC2AHB_DIS;
+ writel(val, ASPEED_LPC_CTRL + AST_LPC_HICRB);
+
+ /* P2A, PCIe BMC */
+ val = readl(ASPEED_PCIE_CONFIG_SET);
+ val &= ~(SCU_PCIE_CONFIG_SET_BMC_DMA
+ | SCU_PCIE_CONFIG_SET_BMC_MMIO
+ | SCU_PCIE_CONFIG_SET_BMC_EN
+ | SCU_PCIE_CONFIG_SET_VGA_MMIO);
+ writel(val, ASPEED_PCIE_CONFIG_SET);
+
+ /* Debug UART */
+#if !defined(CONFIG_ASPEED_ENABLE_DEBUG_UART)
+ val = readl(ASPEED_MISC1_CTRL);
+ val |= SCU_MISC_DEBUG_UART_DISABLE;
+ writel(val, ASPEED_MISC1_CTRL);
+#endif
+
+ /* X-DMA */
+ sdmc_unlocked = readl(ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+ if (!sdmc_unlocked)
+ writel(SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+
+ val = readl(ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
+ val |= (SDMC_GFX_PROT_VGA_CURSOR
+ | SDMC_GFX_PROT_VGA_CG_READ
+ | SDMC_GFX_PROT_VGA_ASCII_READ
+ | SDMC_GFX_PROT_VGA_CRT
+ | SDMC_GFX_PROT_PCIE
+ | SDMC_GFX_PROT_XDMA);
+ writel(val, ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
+
+ if (!sdmc_unlocked)
+ writel(~SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
+
+ /* LPC2AHB */
+ val = readl(ASPEED_LPC_CTRL + AST_LPC_HICR5);
+ val &= ~LPC_HICR5_ENFWH;
+ writel(val, ASPEED_LPC_CTRL + AST_LPC_HICR5);
+}
+
__weak int board_init(void)
{
struct udevice *dev;
int i;
int ret;
+ isolate_bmc();
+
gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
/*
--
2.35.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-04-19 23:42 [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces Zev Weiss
@ 2022-04-20 3:06 ` Ryan Chen
2022-04-20 3:34 ` Andrew Jeffery
2022-05-02 8:11 ` Joel Stanley
1 sibling, 1 reply; 7+ messages in thread
From: Ryan Chen @ 2022-04-20 3:06 UTC (permalink / raw)
To: Zev Weiss, Joel Stanley, openbmc; +Cc: Andrew Jeffery, Lei Yu, Ian Woloschin
> -----Original Message-----
> From: Zev Weiss <zev@bewilderbeest.net>
> Sent: Wednesday, April 20, 2022 7:42 AM
> To: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org
> Cc: Zev Weiss <zev@bewilderbeest.net>; Andrew Jeffery <andrew@aj.id.au>;
> Ryan Chen <ryan_chen@aspeedtech.com>; Ian Woloschin
> <ian.woloschin@akamai.com>; Lei Yu <yulei.sh@bytedance.com>
> Subject: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable
> backdoor interfaces
>
> On ast2400 and ast2500 we now disable the various hardware backdoor
> interfaces as is done on ast2600. Two Kconfig options can selectively
> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO leaves
> the ast2x00 built-in Super I/O device enabled, as it is required for some
> systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves the hardware
> debug UART enabled, since it provides a relatively high ratio of utility to
> security risk during development.
>
> This patch is based on a patch by Andrew Jeffery for an older u-boot branch in
> the OpenBMC tree for the df-isolate-bmc distro feature flag.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>
> Tested on ast2500 and (hostless, BMC-only) ast2400.
>
> Ryan, are you OK with having an option (off by default) to leave the debug
> UART enabled as in this version of the patch?
>
Thanks your submit.
Again, my opinion still keep the direct patch to disable it.
Not have config to enable it.
Ryan
> Ian, if you could test this out with CONFIG_ASPEED_ENABLE_SUPERIO=y on
> one of your systems and confirm that that setting works as intended that
> would be great.
>
> Changes since v2 [1]:
> - made most of the changes unconditional/unconfigurable, but added
> Kconfig options to leave Super I/O and debug UART enabled
>
> Changes since v1 [0]:
> - extended to cover ast2400
> - inverted sense of Kconfig option, default (n) is now secure mode
> - renamed some register/bit macros more appropriately
>
> [0]
> https://lore.kernel.org/openbmc/20220414040448.27100-1-zev@bewilderbees
> t.net/
> [1]
> https://lore.kernel.org/openbmc/20220414224004.29703-1-zev@bewilderbees
> t.net/
>
> arch/arm/include/asm/arch-aspeed/platform.h | 7 ++
> .../arm/include/asm/arch-aspeed/scu_ast2400.h | 7
> ++ .../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
> arch/arm/mach-aspeed/Kconfig | 22 ++++++
> arch/arm/mach-aspeed/ast2400/board_common.c | 66
> +++++++++++++++++
> arch/arm/mach-aspeed/ast2500/board_common.c | 73
> +++++++++++++++++++
> 6 files changed, 183 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/platform.h
> b/arch/arm/include/asm/arch-aspeed/platform.h
> index f016bdaba3e7..f05747642f38 100644
> --- a/arch/arm/include/asm/arch-aspeed/platform.h
> +++ b/arch/arm/include/asm/arch-aspeed/platform.h
> @@ -15,24 +15,31 @@
>
> /***************************************************************
> ******************/
> #if defined(CONFIG_ASPEED_AST2400)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction
> handshake register */
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_DRAM_BASE 0x40000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x8000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2500)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> +#define ASPEED_MISC1_CTRL 0x1e6e202C
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_HW_STRAP2 0x1e6e20D0
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction
> handshake register */
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_MAC_COUNT 2
> #define ASPEED_DRAM_BASE 0x80000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x9000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2600)
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> index 9c5d96ae84b9..55875fd8312f 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> @@ -8,6 +8,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 <<
> SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -104,6 +105,12 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f <<
> SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +
> struct ast2400_clk_priv {
> struct ast2400_scu *scu;
> };
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> index 8fe4028e4ff0..06dc998afaa8 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> @@ -11,6 +11,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 <<
> SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -107,6 +108,13 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f <<
> SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +#define SCU_MISC_DEBUG_UART_DISABLE (1 << 10)
> +
> struct ast2500_clk_priv {
> struct ast2500_scu *scu;
> };
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 579a547df61e..fc565e0da830 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -45,6 +45,28 @@ config ASPEED_AST2600
> which is enabled by support of LPC and eSPI peripherals.
> endchoice
>
> +config ASPEED_ENABLE_SUPERIO
> + bool "Enable built-in AST2x00 Super I/O hardware"
> + depends on ASPEED_AST2400 || ASPEED_AST2500
> + help
> + The Aspeed AST2400 and AST2500 include a built-in Super I/O
> + device that is normally disabled; say Y here to enable it.
> + Note that this has security implications: it grants the host
> + read access to the BMC's entire address space. This should
> + thus be left disabled unless required by a specific system.
> +
> +config ASPEED_ENABLE_DEBUG_UART
> + bool "Enable AST2500 hardware debug UART"
> + depends on ASPEED_AST2500
> + help
> + The Aspeed AST2500 include a hardware-supported, UART-based
> + debug interface that is normally disabled; say Y here to
> + enable it. Note that this has security implications: the
> + debug UART provide read/write access to the BMC's entire
> + address space. This should thus be left disabled on
> + production systems, but may be useful to enable for
> + debugging during development.
> +
> config ASPEED_PALLADIUM
> bool "Aspeed palladium for simulation"
> default n
> diff --git a/arch/arm/mach-aspeed/ast2400/board_common.c
> b/arch/arm/mach-aspeed/ast2400/board_common.c
> index 3829b069342e..7134105232cb 100644
> --- a/arch/arm/mach-aspeed/ast2400/board_common.c
> +++ b/arch/arm/mach-aspeed/ast2400/board_common.c
> @@ -4,14 +4,80 @@
> #include <ram.h>
> #include <timer.h>
> #include <asm/io.h>
> +#include <asm/arch/platform.h>
> +#include <asm/arch/scu_ast2400.h>
> #include <asm/arch/timer.h>
> #include <linux/err.h>
> #include <dm/uclass.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +#define AST_LPC_HICR5 0x080
> +# define LPC_HICR5_ENFWH BIT(10)
> +#define AST_LPC_HICRB 0x100
> +# define LPC_HICRB_SIO_ILPC2AHB_DIS BIT(6)
> +
> +#define AST_SDMC_PROTECT 0x00
> +# define SDRAM_UNLOCK_KEY 0xfc600309
> +#define AST_SDMC_GFX_PROT 0x08
> +# define SDMC_GFX_PROT_VGA_CURSOR BIT(0) # define
> +SDMC_GFX_PROT_VGA_CG_READ BIT(1) # define
> SDMC_GFX_PROT_VGA_ASCII_READ
> +BIT(2) # define SDMC_GFX_PROT_VGA_CRT BIT(3) # define
> +SDMC_GFX_PROT_PCIE BIT(16) # define SDMC_GFX_PROT_XDMA BIT(17)
> +
> +static void isolate_bmc(void)
> +{
> + bool sdmc_unlocked;
> + u32 val;
> +
> + /* iLPC2AHB */
> +#if !defined(CONFIG_ASPEED_ENABLE_SUPERIO)
> + val = readl(ASPEED_HW_STRAP1);
> + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS;
> + writel(val, ASPEED_HW_STRAP1);
> +#endif
> +
> + val = readl(ASPEED_LPC_CTRL + AST_LPC_HICRB);
> + val |= LPC_HICRB_SIO_ILPC2AHB_DIS;
> + writel(val, ASPEED_LPC_CTRL + AST_LPC_HICRB);
> +
> + /* P2A, PCIe BMC */
> + val = readl(ASPEED_PCIE_CONFIG_SET);
> + val &= ~(SCU_PCIE_CONFIG_SET_BMC_DMA
> + | SCU_PCIE_CONFIG_SET_BMC_MMIO
> + | SCU_PCIE_CONFIG_SET_BMC_EN
> + | SCU_PCIE_CONFIG_SET_VGA_MMIO);
> + writel(val, ASPEED_PCIE_CONFIG_SET);
> +
> + /* X-DMA */
> + sdmc_unlocked = readl(ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
> + if (!sdmc_unlocked)
> + writel(SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL +
> AST_SDMC_PROTECT);
> +
> + val = readl(ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
> + val |= (SDMC_GFX_PROT_VGA_CURSOR
> + | SDMC_GFX_PROT_VGA_CG_READ
> + | SDMC_GFX_PROT_VGA_ASCII_READ
> + | SDMC_GFX_PROT_VGA_CRT
> + | SDMC_GFX_PROT_PCIE
> + | SDMC_GFX_PROT_XDMA);
> + writel(val, ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
> +
> + if (!sdmc_unlocked)
> + writel(~SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL +
> AST_SDMC_PROTECT);
> +
> + /* LPC2AHB */
> + val = readl(ASPEED_LPC_CTRL + AST_LPC_HICR5);
> + val &= ~LPC_HICR5_ENFWH;
> + writel(val, ASPEED_LPC_CTRL + AST_LPC_HICR5); }
> +
> __weak int board_init(void)
> {
> + isolate_bmc();
> +
> gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>
> return 0;
> diff --git a/arch/arm/mach-aspeed/ast2500/board_common.c
> b/arch/arm/mach-aspeed/ast2500/board_common.c
> index ce541e88fb8e..c63fe466eb4b 100644
> --- a/arch/arm/mach-aspeed/ast2500/board_common.c
> +++ b/arch/arm/mach-aspeed/ast2500/board_common.c
> @@ -7,18 +7,91 @@
> #include <ram.h>
> #include <timer.h>
> #include <asm/io.h>
> +#include <asm/arch/platform.h>
> +#include <asm/arch/scu_ast2500.h>
> +#include <asm/arch/sdram_ast2500.h>
> #include <asm/arch/timer.h>
> #include <linux/err.h>
> #include <dm/uclass.h>
>
> DECLARE_GLOBAL_DATA_PTR;
>
> +#define AST_LPC_HICR5 0x080
> +# define LPC_HICR5_ENFWH BIT(10)
> +#define AST_LPC_HICRB 0x100
> +# define LPC_HICRB_SIO_ILPC2AHB_DIS BIT(6)
> +
> +# define AST_SDMC_PROTECT 0x00
> +# define AST_SDMC_GFX_PROT 0x08
> +# define SDMC_GFX_PROT_VGA_CURSOR BIT(0) # define
> +SDMC_GFX_PROT_VGA_CG_READ BIT(1) # define
> SDMC_GFX_PROT_VGA_ASCII_READ
> +BIT(2) # define SDMC_GFX_PROT_VGA_CRT BIT(3) # define
> +SDMC_GFX_PROT_PCIE BIT(16) # define SDMC_GFX_PROT_XDMA BIT(17)
> +
> +static void isolate_bmc(void)
> +{
> + bool sdmc_unlocked;
> + u32 val;
> +
> + /* iLPC2AHB */
> +#if !defined(CONFIG_ASPEED_ENABLE_SUPERIO)
> + val = readl(ASPEED_HW_STRAP1);
> + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS;
> + writel(val, ASPEED_HW_STRAP1);
> +#endif
> +
> + val = readl(ASPEED_LPC_CTRL + AST_LPC_HICRB);
> + val |= LPC_HICRB_SIO_ILPC2AHB_DIS;
> + writel(val, ASPEED_LPC_CTRL + AST_LPC_HICRB);
> +
> + /* P2A, PCIe BMC */
> + val = readl(ASPEED_PCIE_CONFIG_SET);
> + val &= ~(SCU_PCIE_CONFIG_SET_BMC_DMA
> + | SCU_PCIE_CONFIG_SET_BMC_MMIO
> + | SCU_PCIE_CONFIG_SET_BMC_EN
> + | SCU_PCIE_CONFIG_SET_VGA_MMIO);
> + writel(val, ASPEED_PCIE_CONFIG_SET);
> +
> + /* Debug UART */
> +#if !defined(CONFIG_ASPEED_ENABLE_DEBUG_UART)
> + val = readl(ASPEED_MISC1_CTRL);
> + val |= SCU_MISC_DEBUG_UART_DISABLE;
> + writel(val, ASPEED_MISC1_CTRL);
> +#endif
> +
> + /* X-DMA */
> + sdmc_unlocked = readl(ASPEED_SDRAM_CTRL + AST_SDMC_PROTECT);
> + if (!sdmc_unlocked)
> + writel(SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL +
> AST_SDMC_PROTECT);
> +
> + val = readl(ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
> + val |= (SDMC_GFX_PROT_VGA_CURSOR
> + | SDMC_GFX_PROT_VGA_CG_READ
> + | SDMC_GFX_PROT_VGA_ASCII_READ
> + | SDMC_GFX_PROT_VGA_CRT
> + | SDMC_GFX_PROT_PCIE
> + | SDMC_GFX_PROT_XDMA);
> + writel(val, ASPEED_SDRAM_CTRL + AST_SDMC_GFX_PROT);
> +
> + if (!sdmc_unlocked)
> + writel(~SDRAM_UNLOCK_KEY, ASPEED_SDRAM_CTRL +
> AST_SDMC_PROTECT);
> +
> + /* LPC2AHB */
> + val = readl(ASPEED_LPC_CTRL + AST_LPC_HICR5);
> + val &= ~LPC_HICR5_ENFWH;
> + writel(val, ASPEED_LPC_CTRL + AST_LPC_HICR5); }
> +
> __weak int board_init(void)
> {
> struct udevice *dev;
> int i;
> int ret;
>
> + isolate_bmc();
> +
> gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>
> /*
> --
> 2.35.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-04-20 3:06 ` Ryan Chen
@ 2022-04-20 3:34 ` Andrew Jeffery
2022-04-21 6:20 ` Joel Stanley
0 siblings, 1 reply; 7+ messages in thread
From: Andrew Jeffery @ 2022-04-20 3:34 UTC (permalink / raw)
To: Ryan Chen, Zev Weiss, Joel Stanley, openbmc; +Cc: Lei Yu, Ian Woloschin
On Wed, 20 Apr 2022, at 12:36, Ryan Chen wrote:
>> -----Original Message-----
>> From: Zev Weiss <zev@bewilderbeest.net>
>> Sent: Wednesday, April 20, 2022 7:42 AM
>> To: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org
>> Cc: Zev Weiss <zev@bewilderbeest.net>; Andrew Jeffery <andrew@aj.id.au>;
>> Ryan Chen <ryan_chen@aspeedtech.com>; Ian Woloschin
>> <ian.woloschin@akamai.com>; Lei Yu <yulei.sh@bytedance.com>
>> Subject: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable
>> backdoor interfaces
>>
>> On ast2400 and ast2500 we now disable the various hardware backdoor
>> interfaces as is done on ast2600. Two Kconfig options can selectively
>> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO leaves
>> the ast2x00 built-in Super I/O device enabled, as it is required for some
>> systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves the hardware
>> debug UART enabled, since it provides a relatively high ratio of utility to
>> security risk during development.
>>
>> This patch is based on a patch by Andrew Jeffery for an older u-boot branch in
>> the OpenBMC tree for the df-isolate-bmc distro feature flag.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>
>> Tested on ast2500 and (hostless, BMC-only) ast2400.
>>
>> Ryan, are you OK with having an option (off by default) to leave the debug
>> UART enabled as in this version of the patch?
>>
> Thanks your submit.
> Again, my opinion still keep the direct patch to disable it.
> Not have config to enable it.
>
Ideally yes, but as Ian mentioned he has at least one system where the
SuperIO AHB bridge must be enabled to allow their BIOS to configure the
UARTs. So we need an option to cater to that.
I don't want people to have to patch the code to allow use of the
backdoors, that will just lead to other problems (e.g. reverting this
patch is the simplest thing, and opens up all the backdoors instead of
a targeted one).
Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-04-20 3:34 ` Andrew Jeffery
@ 2022-04-21 6:20 ` Joel Stanley
0 siblings, 0 replies; 7+ messages in thread
From: Joel Stanley @ 2022-04-21 6:20 UTC (permalink / raw)
To: Andrew Jeffery; +Cc: openbmc, Lei Yu, Ryan Chen, Zev Weiss, Ian Woloschin
On Wed, 20 Apr 2022 at 03:35, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 20 Apr 2022, at 12:36, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: Zev Weiss <zev@bewilderbeest.net>
> >> Sent: Wednesday, April 20, 2022 7:42 AM
> >> To: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org
> >> Cc: Zev Weiss <zev@bewilderbeest.net>; Andrew Jeffery <andrew@aj.id.au>;
> >> Ryan Chen <ryan_chen@aspeedtech.com>; Ian Woloschin
> >> <ian.woloschin@akamai.com>; Lei Yu <yulei.sh@bytedance.com>
> >> Subject: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable
> >> backdoor interfaces
> >>
> >> On ast2400 and ast2500 we now disable the various hardware backdoor
> >> interfaces as is done on ast2600. Two Kconfig options can selectively
> >> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO leaves
> >> the ast2x00 built-in Super I/O device enabled, as it is required for some
> >> systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves the hardware
> >> debug UART enabled, since it provides a relatively high ratio of utility to
> >> security risk during development.
> >>
> >> This patch is based on a patch by Andrew Jeffery for an older u-boot branch in
> >> the OpenBMC tree for the df-isolate-bmc distro feature flag.
> >>
> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> ---
> >>
> >> Tested on ast2500 and (hostless, BMC-only) ast2400.
> >>
> >> Ryan, are you OK with having an option (off by default) to leave the debug
> >> UART enabled as in this version of the patch?
> >>
> > Thanks your submit.
> > Again, my opinion still keep the direct patch to disable it.
> > Not have config to enable it.
> >
>
> Ideally yes, but as Ian mentioned he has at least one system where the
> SuperIO AHB bridge must be enabled to allow their BIOS to configure the
> UARTs. So we need an option to cater to that.
Agreed.
Ideally these backdoors would be controlled by strapping, so
development systems and platforms that chose to open them can
configure the system appropriately. But the hardware does not have
this ability, so the next best thing we can do is provide an option in
the firmware.
Note that before Zev sent this patch, the backdoors were left enabled
with the current SDK. Having them disabled by default, behind an
option, is a welcome improvement.
I'm happy with this patch, but I'll give others time to respond before merging.
Cheers,
Joel
>
> I don't want people to have to patch the code to allow use of the
> backdoors, that will just lead to other problems (e.g. reverting this
> patch is the simplest thing, and opens up all the backdoors instead of
> a targeted one).
>
> Andrew
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-04-19 23:42 [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces Zev Weiss
2022-04-20 3:06 ` Ryan Chen
@ 2022-05-02 8:11 ` Joel Stanley
2022-05-02 14:37 ` Woloschin, Ian
2022-05-04 0:21 ` Zev Weiss
1 sibling, 2 replies; 7+ messages in thread
From: Joel Stanley @ 2022-05-02 8:11 UTC (permalink / raw)
To: Zev Weiss
Cc: Andrew Jeffery, OpenBMC Maillist, Lei Yu, Ryan Chen, Ian Woloschin
On Tue, 19 Apr 2022 at 23:43, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On ast2400 and ast2500 we now disable the various hardware backdoor
> interfaces as is done on ast2600. Two Kconfig options can selectively
> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO
> leaves the ast2x00 built-in Super I/O device enabled, as it is
> required for some systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves
> the hardware debug UART enabled, since it provides a relatively high
> ratio of utility to security risk during development.
>
> This patch is based on a patch by Andrew Jeffery for an older u-boot
> branch in the OpenBMC tree for the df-isolate-bmc distro feature flag.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>
> Tested on ast2500 and (hostless, BMC-only) ast2400.
>
> Ryan, are you OK with having an option (off by default) to leave the
> debug UART enabled as in this version of the patch?
>
> Ian, if you could test this out with CONFIG_ASPEED_ENABLE_SUPERIO=y on
> one of your systems and confirm that that setting works as intended
> that would be great.
Ian, did you get around to testing this?
I've given it a careful review.
>
> Changes since v2 [1]:
> - made most of the changes unconditional/unconfigurable, but added
> Kconfig options to leave Super I/O and debug UART enabled
>
> Changes since v1 [0]:
> - extended to cover ast2400
> - inverted sense of Kconfig option, default (n) is now secure mode
> - renamed some register/bit macros more appropriately
>
> [0] https://lore.kernel.org/openbmc/20220414040448.27100-1-zev@bewilderbeest.net/
> [1] https://lore.kernel.org/openbmc/20220414224004.29703-1-zev@bewilderbeest.net/
>
> arch/arm/include/asm/arch-aspeed/platform.h | 7 ++
> .../arm/include/asm/arch-aspeed/scu_ast2400.h | 7 ++
> .../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
> arch/arm/mach-aspeed/Kconfig | 22 ++++++
> arch/arm/mach-aspeed/ast2400/board_common.c | 66 +++++++++++++++++
> arch/arm/mach-aspeed/ast2500/board_common.c | 73 +++++++++++++++++++
> 6 files changed, 183 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
> index f016bdaba3e7..f05747642f38 100644
> --- a/arch/arm/include/asm/arch-aspeed/platform.h
> +++ b/arch/arm/include/asm/arch-aspeed/platform.h
> @@ -15,24 +15,31 @@
> /*********************************************************************************/
> #if defined(CONFIG_ASPEED_AST2400)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
sp: function
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_DRAM_BASE 0x40000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x8000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2500)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> +#define ASPEED_MISC1_CTRL 0x1e6e202C
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_HW_STRAP2 0x1e6e20D0
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
sp: function
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_MAC_COUNT 2
> #define ASPEED_DRAM_BASE 0x80000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x9000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2600)
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> index 9c5d96ae84b9..55875fd8312f 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> @@ -8,6 +8,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -104,6 +105,12 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +
> struct ast2400_clk_priv {
> struct ast2400_scu *scu;
> };
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> index 8fe4028e4ff0..06dc998afaa8 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> @@ -11,6 +11,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -107,6 +108,13 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +#define SCU_MISC_DEBUG_UART_DISABLE (1 << 10)
> +
> struct ast2500_clk_priv {
> struct ast2500_scu *scu;
> };
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 579a547df61e..fc565e0da830 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -45,6 +45,28 @@ config ASPEED_AST2600
> which is enabled by support of LPC and eSPI peripherals.
> endchoice
>
> +config ASPEED_ENABLE_SUPERIO
> + bool "Enable built-in AST2x00 Super I/O hardware"
> + depends on ASPEED_AST2400 || ASPEED_AST2500
> + help
> + The Aspeed AST2400 and AST2500 include a built-in Super I/O
> + device that is normally disabled; say Y here to enable it.
Can you add a line break here.
> + Note that this has security implications: it grants the host
> + read access to the BMC's entire address space. This should
> + thus be left disabled unless required by a specific system.
Change "note" to WARNING or DANGER or an all caps label of your choosing.
Do you think we should put these behind a config
ASPEED_SHOW_DANGEROUS_BACKDOOR_FEATURE so users have to acknowledge
that option before enabling these?
> +
> +config ASPEED_ENABLE_DEBUG_UART
> + bool "Enable AST2500 hardware debug UART"
> + depends on ASPEED_AST2500
> + help
> + The Aspeed AST2500 include a hardware-supported, UART-based
> + debug interface that is normally disabled; say Y here to
> + enable it.
line break
> Note that this has security implications: the
> + debug UART provide read/write access to the BMC's entire
provides
> + address space. This should thus be left disabled on
> + production systems, but may be useful to enable for
> + debugging during development.
> +
I did a build as follows:
$ cat build-ast2500.sh
#!/bin/bash
set -e
OBJ=ast2500-obj
CONFIG=evb-ast2500_defconfig
IMG="$OBJ/test.img"
make -j8 O="$OBJ" -s clean
make -j8 O="$OBJ" -j8 -s $CONFIG
CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm make -j8 O="$OBJ" -j8
DEVICE_TREE=ast2500-evb -s
size "$OBJ/u-boot"
cp "$OBJ/u-boot.bin" "$OBJ/test.img"
truncate -s 32M "$OBJ/test.img"
Testing on a qemu instance, before your patch:
$ qemu-system-arm -M ast2500-evb -drive
file=ast2500-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Enabled
iLPC2AHB Bridge: Read-write
VGA PCIe device: Enabled
MMIO on VGA device: Enabled
P2A write filter state:
0x00000000-0x0fffffff (Firmware): Read-write
0x10000000-0x1fffffff (SoC IO): Read-write
0x20000000-0x2fffffff (BMC Flash): Read-write
0x30000000-0x3fffffff (Host Flash): Read-write
0x40000000-0x5fffffff (Reserved): Read-write
0x60000000-0x7fffffff (LPC Host): Read-write
0x80000000-0xffffffff (DRAM): Read-write
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: Yes
Debug UART: Enabled
Debug UART enabled on: UART5
After applying your patch:
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Disabled
VGA PCIe device: Enabled
MMIO on VGA device: Disabled
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: No
Debug UART: Disabled
Similarly with the ast2400:
qemu-system-arm -M palmetto-bmc -drive
file=ast2400-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Enabled
iLPC2AHB Bridge: Read-write
VGA PCIe device: Enabled
MMIO on VGA device: Enabled
P2A write filter state:
0x00000000-0x17ffffff (Firmware): Read-write
0x18000000-0x1fffffff (SoC IO): Read-write
0x20000000-0x2fffffff (BMC Flash): Read-write
0x30000000-0x3fffffff (Host Flash): Read-write
0x40000000-0x5fffffff (DRAM): Read-write
0x60000000-0x7fffffff (LPC Host): Read-write
0x80000000-0xffffffff (Reserved): Read-write
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: Yes
Debug UART: Absent
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Disabled
VGA PCIe device: Enabled
MMIO on VGA device: Disabled
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: No
Debug UART: Absent
These "after" settings look good. I didn't check that qemu models the
default state of these bits in detail, but the fact that they change
from "insecure" to "secure" indicates your patch is setting the
correct things.
Tested-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-05-02 8:11 ` Joel Stanley
@ 2022-05-02 14:37 ` Woloschin, Ian
2022-05-04 0:21 ` Zev Weiss
1 sibling, 0 replies; 7+ messages in thread
From: Woloschin, Ian @ 2022-05-02 14:37 UTC (permalink / raw)
To: Joel Stanley, Zev Weiss
Cc: Andrew Jeffery, OpenBMC Maillist, Lei Yu, Ryan Chen
No, I have not tested it yet. I'm in the office today, might be able to give it a quick test.
-Ian
On 5/2/22, 4:12 AM, "Joel Stanley" <joel@jms.id.au> wrote:
On Tue, 19 Apr 2022 at 23:43, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On ast2400 and ast2500 we now disable the various hardware backdoor
> interfaces as is done on ast2600. Two Kconfig options can selectively
> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO
> leaves the ast2x00 built-in Super I/O device enabled, as it is
> required for some systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves
> the hardware debug UART enabled, since it provides a relatively high
> ratio of utility to security risk during development.
>
> This patch is based on a patch by Andrew Jeffery for an older u-boot
> branch in the OpenBMC tree for the df-isolate-bmc distro feature flag.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>
> Tested on ast2500 and (hostless, BMC-only) ast2400.
>
> Ryan, are you OK with having an option (off by default) to leave the
> debug UART enabled as in this version of the patch?
>
> Ian, if you could test this out with CONFIG_ASPEED_ENABLE_SUPERIO=y on
> one of your systems and confirm that that setting works as intended
> that would be great.
Ian, did you get around to testing this?
I've given it a careful review.
>
> Changes since v2 [1]:
> - made most of the changes unconditional/unconfigurable, but added
> Kconfig options to leave Super I/O and debug UART enabled
>
> Changes since v1 [0]:
> - extended to cover ast2400
> - inverted sense of Kconfig option, default (n) is now secure mode
> - renamed some register/bit macros more appropriately
>
> [0] 20220414040448.27100-1-zev@bewilderbeest.net <https://urldefense.com/v3/__https://lore.kernel.org/openbmc/<a href=>/__;!!GjvTz_vk!Wmxbf3_uRf7Cfqkyb2m2HoP9OfItgOj7v2B8KzOdqjdlSqscdSDyMmQdt1_sJgazuiBqc5S8sOcn27tvHw$">https://urldefense.com/v3/__https://lore.kernel.org/openbmc/20220414040448.27100-1-zev@bewilderbeest.net/__;!!GjvTz_vk!Wmxbf3_uRf7Cfqkyb2m2HoP9OfItgOj7v2B8KzOdqjdlSqscdSDyMmQdt1_sJgazuiBqc5S8sOcn27tvHw$
> [1] 20220414224004.29703-1-zev@bewilderbeest.net <https://urldefense.com/v3/__https://lore.kernel.org/openbmc/<a href=>/__;!!GjvTz_vk!Wmxbf3_uRf7Cfqkyb2m2HoP9OfItgOj7v2B8KzOdqjdlSqscdSDyMmQdt1_sJgazuiBqc5S8sOdO5rNOsQ$">https://urldefense.com/v3/__https://lore.kernel.org/openbmc/20220414224004.29703-1-zev@bewilderbeest.net/__;!!GjvTz_vk!Wmxbf3_uRf7Cfqkyb2m2HoP9OfItgOj7v2B8KzOdqjdlSqscdSDyMmQdt1_sJgazuiBqc5S8sOdO5rNOsQ$
>
> arch/arm/include/asm/arch-aspeed/platform.h | 7 ++
> .../arm/include/asm/arch-aspeed/scu_ast2400.h | 7 ++
> .../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
> arch/arm/mach-aspeed/Kconfig | 22 ++++++
> arch/arm/mach-aspeed/ast2400/board_common.c | 66 +++++++++++++++++
> arch/arm/mach-aspeed/ast2500/board_common.c | 73 +++++++++++++++++++
> 6 files changed, 183 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
> index f016bdaba3e7..f05747642f38 100644
> --- a/arch/arm/include/asm/arch-aspeed/platform.h
> +++ b/arch/arm/include/asm/arch-aspeed/platform.h
> @@ -15,24 +15,31 @@
> /*********************************************************************************/
> #if defined(CONFIG_ASPEED_AST2400)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
sp: function
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_DRAM_BASE 0x40000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x8000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2500)
> #define ASPEED_MAC_COUNT 2
> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
> +#define ASPEED_MISC1_CTRL 0x1e6e202C
> #define ASPEED_HW_STRAP1 0x1e6e2070
> #define ASPEED_HW_STRAP2 0x1e6e20D0
> #define ASPEED_REVISION_ID 0x1e6e207C
> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
sp: function
> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
> #define ASPEED_MAC_COUNT 2
> #define ASPEED_DRAM_BASE 0x80000000
> #define ASPEED_SRAM_BASE 0x1E720000
> +#define ASPEED_LPC_CTRL 0x1e789000
> #define ASPEED_SRAM_SIZE 0x9000
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2600)
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> index 9c5d96ae84b9..55875fd8312f 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
> @@ -8,6 +8,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -104,6 +105,12 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +
> struct ast2400_clk_priv {
> struct ast2400_scu *scu;
> };
> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> index 8fe4028e4ff0..06dc998afaa8 100644
> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> @@ -11,6 +11,7 @@
> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
> #define SCU_HWSTRAP_DDR4 (1 << 24)
> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>
> @@ -107,6 +108,13 @@
> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>
> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
> +
> +#define SCU_MISC_DEBUG_UART_DISABLE (1 << 10)
> +
> struct ast2500_clk_priv {
> struct ast2500_scu *scu;
> };
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 579a547df61e..fc565e0da830 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -45,6 +45,28 @@ config ASPEED_AST2600
> which is enabled by support of LPC and eSPI peripherals.
> endchoice
>
> +config ASPEED_ENABLE_SUPERIO
> + bool "Enable built-in AST2x00 Super I/O hardware"
> + depends on ASPEED_AST2400 || ASPEED_AST2500
> + help
> + The Aspeed AST2400 and AST2500 include a built-in Super I/O
> + device that is normally disabled; say Y here to enable it.
Can you add a line break here.
> + Note that this has security implications: it grants the host
> + read access to the BMC's entire address space. This should
> + thus be left disabled unless required by a specific system.
Change "note" to WARNING or DANGER or an all caps label of your choosing.
Do you think we should put these behind a config
ASPEED_SHOW_DANGEROUS_BACKDOOR_FEATURE so users have to acknowledge
that option before enabling these?
> +
> +config ASPEED_ENABLE_DEBUG_UART
> + bool "Enable AST2500 hardware debug UART"
> + depends on ASPEED_AST2500
> + help
> + The Aspeed AST2500 include a hardware-supported, UART-based
> + debug interface that is normally disabled; say Y here to
> + enable it.
line break
> Note that this has security implications: the
> + debug UART provide read/write access to the BMC's entire
provides
> + address space. This should thus be left disabled on
> + production systems, but may be useful to enable for
> + debugging during development.
> +
I did a build as follows:
$ cat build-ast2500.sh
#!/bin/bash
set -e
OBJ=ast2500-obj
CONFIG=evb-ast2500_defconfig
IMG="$OBJ/test.img"
make -j8 O="$OBJ" -s clean
make -j8 O="$OBJ" -j8 -s $CONFIG
CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm make -j8 O="$OBJ" -j8
DEVICE_TREE=ast2500-evb -s
size "$OBJ/u-boot"
cp "$OBJ/u-boot.bin" "$OBJ/test.img"
truncate -s 32M "$OBJ/test.img"
Testing on a qemu instance, before your patch:
$ qemu-system-arm -M ast2500-evb -drive
file=ast2500-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Enabled
iLPC2AHB Bridge: Read-write
VGA PCIe device: Enabled
MMIO on VGA device: Enabled
P2A write filter state:
0x00000000-0x0fffffff (Firmware): Read-write
0x10000000-0x1fffffff (SoC IO): Read-write
0x20000000-0x2fffffff (BMC Flash): Read-write
0x30000000-0x3fffffff (Host Flash): Read-write
0x40000000-0x5fffffff (Reserved): Read-write
0x60000000-0x7fffffff (LPC Host): Read-write
0x80000000-0xffffffff (DRAM): Read-write
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: Yes
Debug UART: Enabled
Debug UART enabled on: UART5
After applying your patch:
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Disabled
VGA PCIe device: Enabled
MMIO on VGA device: Disabled
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: No
Debug UART: Disabled
Similarly with the ast2400:
qemu-system-arm -M palmetto-bmc -drive
file=ast2400-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Enabled
iLPC2AHB Bridge: Read-write
VGA PCIe device: Enabled
MMIO on VGA device: Enabled
P2A write filter state:
0x00000000-0x17ffffff (Firmware): Read-write
0x18000000-0x1fffffff (SoC IO): Read-write
0x20000000-0x2fffffff (BMC Flash): Read-write
0x30000000-0x3fffffff (Host Flash): Read-write
0x40000000-0x5fffffff (DRAM): Read-write
0x60000000-0x7fffffff (LPC Host): Read-write
0x80000000-0xffffffff (Reserved): Read-write
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: Yes
Debug UART: Absent
# ./culvert probe
[*] Probing AHB interfaces
[*] Performing interface discovery via devmem
SuperIO: Disabled
VGA PCIe device: Enabled
MMIO on VGA device: Disabled
X-DMA on VGA device: Enabled
BMC PCIe device: Disabled
X-DMA is unconstrained: No
Debug UART: Absent
These "after" settings look good. I didn't check that qemu models the
default state of these bits in detail, but the fact that they change
from "insecure" to "secure" indicates your patch is setting the
correct things.
Tested-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Cheers,
Joel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces
2022-05-02 8:11 ` Joel Stanley
2022-05-02 14:37 ` Woloschin, Ian
@ 2022-05-04 0:21 ` Zev Weiss
1 sibling, 0 replies; 7+ messages in thread
From: Zev Weiss @ 2022-05-04 0:21 UTC (permalink / raw)
To: Joel Stanley
Cc: Andrew Jeffery, OpenBMC Maillist, Lei Yu, Ryan Chen, Ian Woloschin
On Mon, May 02, 2022 at 01:11:00AM PDT, Joel Stanley wrote:
>On Tue, 19 Apr 2022 at 23:43, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> On ast2400 and ast2500 we now disable the various hardware backdoor
>> interfaces as is done on ast2600. Two Kconfig options can selectively
>> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO
>> leaves the ast2x00 built-in Super I/O device enabled, as it is
>> required for some systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves
>> the hardware debug UART enabled, since it provides a relatively high
>> ratio of utility to security risk during development.
>>
>> This patch is based on a patch by Andrew Jeffery for an older u-boot
>> branch in the OpenBMC tree for the df-isolate-bmc distro feature flag.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>
>> Tested on ast2500 and (hostless, BMC-only) ast2400.
>>
>> Ryan, are you OK with having an option (off by default) to leave the
>> debug UART enabled as in this version of the patch?
>>
>> Ian, if you could test this out with CONFIG_ASPEED_ENABLE_SUPERIO=y on
>> one of your systems and confirm that that setting works as intended
>> that would be great.
>
>Ian, did you get around to testing this?
>
>I've given it a careful review.
>
Thanks Joel!
>>
>> Changes since v2 [1]:
>> - made most of the changes unconditional/unconfigurable, but added
>> Kconfig options to leave Super I/O and debug UART enabled
>>
>> Changes since v1 [0]:
>> - extended to cover ast2400
>> - inverted sense of Kconfig option, default (n) is now secure mode
>> - renamed some register/bit macros more appropriately
>>
>> [0] https://lore.kernel.org/openbmc/20220414040448.27100-1-zev@bewilderbeest.net/
>> [1] https://lore.kernel.org/openbmc/20220414224004.29703-1-zev@bewilderbeest.net/
>>
>> arch/arm/include/asm/arch-aspeed/platform.h | 7 ++
>> .../arm/include/asm/arch-aspeed/scu_ast2400.h | 7 ++
>> .../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
>> arch/arm/mach-aspeed/Kconfig | 22 ++++++
>> arch/arm/mach-aspeed/ast2400/board_common.c | 66 +++++++++++++++++
>> arch/arm/mach-aspeed/ast2500/board_common.c | 73 +++++++++++++++++++
>> 6 files changed, 183 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
>> index f016bdaba3e7..f05747642f38 100644
>> --- a/arch/arm/include/asm/arch-aspeed/platform.h
>> +++ b/arch/arm/include/asm/arch-aspeed/platform.h
>> @@ -15,24 +15,31 @@
>> /*********************************************************************************/
>> #if defined(CONFIG_ASPEED_AST2400)
>> #define ASPEED_MAC_COUNT 2
>> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
>> #define ASPEED_HW_STRAP1 0x1e6e2070
>> #define ASPEED_REVISION_ID 0x1e6e207C
>> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
>> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
>
>sp: function
>
I'm guessing we don't want to bundle unrelated spelling fixes in
existing code into the same patch, but I can post a separate one to
address this?
>> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
>> #define ASPEED_DRAM_BASE 0x40000000
>> #define ASPEED_SRAM_BASE 0x1E720000
>> +#define ASPEED_LPC_CTRL 0x1e789000
>> #define ASPEED_SRAM_SIZE 0x8000
>> #define ASPEED_FMC_CS0_BASE 0x20000000
>> #elif defined(CONFIG_ASPEED_AST2500)
>> #define ASPEED_MAC_COUNT 2
>> +#define ASPEED_SDRAM_CTRL 0x1e6e0000
>> +#define ASPEED_MISC1_CTRL 0x1e6e202C
>> #define ASPEED_HW_STRAP1 0x1e6e2070
>> #define ASPEED_HW_STRAP2 0x1e6e20D0
>> #define ASPEED_REVISION_ID 0x1e6e207C
>> #define ASPEED_SYS_RESET_CTRL 0x1e6e203C
>> #define ASPEED_VGA_HANDSHAKE0 0x1e6e2040 /* VGA fuction handshake register */
>
>sp: function
>
(As above.)
>> +#define ASPEED_PCIE_CONFIG_SET 0x1e6e2180
>> #define ASPEED_MAC_COUNT 2
>> #define ASPEED_DRAM_BASE 0x80000000
>> #define ASPEED_SRAM_BASE 0x1E720000
>> +#define ASPEED_LPC_CTRL 0x1e789000
>> #define ASPEED_SRAM_SIZE 0x9000
>> #define ASPEED_FMC_CS0_BASE 0x20000000
>> #elif defined(CONFIG_ASPEED_AST2600)
>> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
>> index 9c5d96ae84b9..55875fd8312f 100644
>> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
>> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2400.h
>> @@ -8,6 +8,7 @@
>> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
>> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
>> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
>> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
>> #define SCU_HWSTRAP_DDR4 (1 << 24)
>> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>>
>> @@ -104,6 +105,12 @@
>> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
>> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>>
>> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
>> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
>> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
>> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
>> +
>> +
>> struct ast2400_clk_priv {
>> struct ast2400_scu *scu;
>> };
>> diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
>> index 8fe4028e4ff0..06dc998afaa8 100644
>> --- a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
>> +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
>> @@ -11,6 +11,7 @@
>> #define SCU_HWSTRAP_VGAMEM_MASK (3 << SCU_HWSTRAP_VGAMEM_SHIFT)
>> #define SCU_HWSTRAP_MAC1_RGMII (1 << 6)
>> #define SCU_HWSTRAP_MAC2_RGMII (1 << 7)
>> +#define SCU_HWSTRAP_LPC_SIO_DEC_DIS (1 << 20)
>> #define SCU_HWSTRAP_DDR4 (1 << 24)
>> #define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)
>>
>> @@ -107,6 +108,13 @@
>> #define SCU_CLKDUTY_RGMII2TXCK_SHIFT 16
>> #define SCU_CLKDUTY_RGMII2TXCK_MASK (0x7f << SCU_CLKDUTY_RGMII2TXCK_SHIFT)
>>
>> +#define SCU_PCIE_CONFIG_SET_VGA_MMIO (1 << 1)
>> +#define SCU_PCIE_CONFIG_SET_BMC_EN (1 << 8)
>> +#define SCU_PCIE_CONFIG_SET_BMC_MMIO (1 << 9)
>> +#define SCU_PCIE_CONFIG_SET_BMC_DMA (1 << 14)
>> +
>> +#define SCU_MISC_DEBUG_UART_DISABLE (1 << 10)
>> +
>> struct ast2500_clk_priv {
>> struct ast2500_scu *scu;
>> };
>> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
>> index 579a547df61e..fc565e0da830 100644
>> --- a/arch/arm/mach-aspeed/Kconfig
>> +++ b/arch/arm/mach-aspeed/Kconfig
>> @@ -45,6 +45,28 @@ config ASPEED_AST2600
>> which is enabled by support of LPC and eSPI peripherals.
>> endchoice
>>
>> +config ASPEED_ENABLE_SUPERIO
>> + bool "Enable built-in AST2x00 Super I/O hardware"
>> + depends on ASPEED_AST2400 || ASPEED_AST2500
>> + help
>> + The Aspeed AST2400 and AST2500 include a built-in Super I/O
>> + device that is normally disabled; say Y here to enable it.
>
>Can you add a line break here.
>
Sure.
>> + Note that this has security implications: it grants the host
>> + read access to the BMC's entire address space. This should
>> + thus be left disabled unless required by a specific system.
>
>Change "note" to WARNING or DANGER or an all caps label of your choosing.
>
>Do you think we should put these behind a config
>ASPEED_SHOW_DANGEROUS_BACKDOOR_FEATURE so users have to acknowledge
>that option before enabling these?
>
My first thought was that this seemed like it might be overkill given
the emphasis in the description, but then remembered that depending on
the particular Kconfig UI it'd be fairly easy to only see the one-line
summary, so yeah, that seems like a reasonable extra seatbelt to add.
>
>> +
>> +config ASPEED_ENABLE_DEBUG_UART
>> + bool "Enable AST2500 hardware debug UART"
>> + depends on ASPEED_AST2500
>> + help
>> + The Aspeed AST2500 include a hardware-supported, UART-based
>> + debug interface that is normally disabled; say Y here to
>> + enable it.
>
>line break
>
Ack.
>> Note that this has security implications: the
>> + debug UART provide read/write access to the BMC's entire
>
>provides
>
Ack.
>> + address space. This should thus be left disabled on
>> + production systems, but may be useful to enable for
>> + debugging during development.
>> +
>
>I did a build as follows:
>
>$ cat build-ast2500.sh
>#!/bin/bash
>
>set -e
>
>OBJ=ast2500-obj
>CONFIG=evb-ast2500_defconfig
>IMG="$OBJ/test.img"
>
>make -j8 O="$OBJ" -s clean
>make -j8 O="$OBJ" -j8 -s $CONFIG
>CROSS_COMPILE=arm-linux-gnueabi- ARCH=arm make -j8 O="$OBJ" -j8
>DEVICE_TREE=ast2500-evb -s
>size "$OBJ/u-boot"
>
>cp "$OBJ/u-boot.bin" "$OBJ/test.img"
>truncate -s 32M "$OBJ/test.img"
>
>Testing on a qemu instance, before your patch:
>
>$ qemu-system-arm -M ast2500-evb -drive
>file=ast2500-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
>
># ./culvert probe
>[*] Probing AHB interfaces
>[*] Performing interface discovery via devmem
>SuperIO: Enabled
>iLPC2AHB Bridge: Read-write
>VGA PCIe device: Enabled
>MMIO on VGA device: Enabled
>P2A write filter state:
>0x00000000-0x0fffffff (Firmware): Read-write
>0x10000000-0x1fffffff (SoC IO): Read-write
>0x20000000-0x2fffffff (BMC Flash): Read-write
>0x30000000-0x3fffffff (Host Flash): Read-write
>0x40000000-0x5fffffff (Reserved): Read-write
>0x60000000-0x7fffffff (LPC Host): Read-write
>0x80000000-0xffffffff (DRAM): Read-write
>X-DMA on VGA device: Enabled
>BMC PCIe device: Disabled
>X-DMA is unconstrained: Yes
>Debug UART: Enabled
>Debug UART enabled on: UART5
>
>After applying your patch:
>
># ./culvert probe
>[*] Probing AHB interfaces
>[*] Performing interface discovery via devmem
>SuperIO: Disabled
>VGA PCIe device: Enabled
>MMIO on VGA device: Disabled
>X-DMA on VGA device: Enabled
>BMC PCIe device: Disabled
>X-DMA is unconstrained: No
>Debug UART: Disabled
>
>
>Similarly with the ast2400:
>
>qemu-system-arm -M palmetto-bmc -drive
>file=ast2400-obj/test.img,if=mtd -nographic -nic user,tftp=/srv/tftp/
>
># ./culvert probe
>[*] Probing AHB interfaces
>[*] Performing interface discovery via devmem
>SuperIO: Enabled
>iLPC2AHB Bridge: Read-write
>VGA PCIe device: Enabled
>MMIO on VGA device: Enabled
>P2A write filter state:
>0x00000000-0x17ffffff (Firmware): Read-write
>0x18000000-0x1fffffff (SoC IO): Read-write
>0x20000000-0x2fffffff (BMC Flash): Read-write
>0x30000000-0x3fffffff (Host Flash): Read-write
>0x40000000-0x5fffffff (DRAM): Read-write
>0x60000000-0x7fffffff (LPC Host): Read-write
>0x80000000-0xffffffff (Reserved): Read-write
>X-DMA on VGA device: Enabled
>BMC PCIe device: Disabled
>X-DMA is unconstrained: Yes
>Debug UART: Absent
>
># ./culvert probe
>[*] Probing AHB interfaces
>[*] Performing interface discovery via devmem
>SuperIO: Disabled
>VGA PCIe device: Enabled
>MMIO on VGA device: Disabled
>X-DMA on VGA device: Enabled
>BMC PCIe device: Disabled
>X-DMA is unconstrained: No
>Debug UART: Absent
>
>These "after" settings look good. I didn't check that qemu models the
>default state of these bits in detail, but the fact that they change
>from "insecure" to "secure" indicates your patch is setting the
>correct things.
>
>Tested-by: Joel Stanley <joel@jms.id.au>
>Reviewed-by: Joel Stanley <joel@jms.id.au>
>
Great, thanks -- I'll add those tags and post v4 with the above changes
soon.
Zev
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-04 0:21 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-19 23:42 [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces Zev Weiss
2022-04-20 3:06 ` Ryan Chen
2022-04-20 3:34 ` Andrew Jeffery
2022-04-21 6:20 ` Joel Stanley
2022-05-02 8:11 ` Joel Stanley
2022-05-02 14:37 ` Woloschin, Ian
2022-05-04 0:21 ` Zev Weiss
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.