* [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
@ 2022-04-14 4:04 Zev Weiss
2022-04-14 8:13 ` Joel Stanley
0 siblings, 1 reply; 6+ messages in thread
From: Zev Weiss @ 2022-04-14 4:04 UTC (permalink / raw)
To: openbmc, Joel Stanley; +Cc: Andrew Jeffery, Zev Weiss
This provides the functionality of the OpenBMC df-isolate-bmc distro
feature flag, and is very directly derived from Andrew Jeffery's patch
in the OpenBMC tree for the v2016.07 u-boot branch. The
implementation currently only supports ast2500, though ast2400 and
ast2600 support should be fairly simple extensions.
Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
---
This is meant more as something of an RFC to see if this seems like
approximately the right way of going about this (since as far as I can
see the existing df-isolate-bmc implementation only supports the old
2016 u-boot branch), but if it looks OK I suppose it could potentially
go in as-is.
arch/arm/include/asm/arch-aspeed/platform.h | 3 +
.../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
arch/arm/mach-aspeed/Kconfig | 12 +++
arch/arm/mach-aspeed/ast2500/board_common.c | 75 +++++++++++++++++++
4 files changed, 98 insertions(+)
diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
index f016bdaba3e7..be7e9087a782 100644
--- a/arch/arm/include/asm/arch-aspeed/platform.h
+++ b/arch/arm/include/asm/arch-aspeed/platform.h
@@ -25,14 +25,17 @@
#define ASPEED_FMC_CS0_BASE 0x20000000
#elif defined(CONFIG_ASPEED_AST2500)
#define ASPEED_MAC_COUNT 2
+#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_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
index 8fe4028e4ff0..2d54d915dfed 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_DISABLE (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..2b51f87e0732 100644
--- a/arch/arm/mach-aspeed/Kconfig
+++ b/arch/arm/mach-aspeed/Kconfig
@@ -45,6 +45,18 @@ config ASPEED_AST2600
which is enabled by support of LPC and eSPI peripherals.
endchoice
+config ASPEED_ISOLATE_BMC
+ bool "Disable hardware features that provide unnecessary access to the BMC"
+ depends on ASPEED_AST2500
+ default n
+ help
+ Aspeed BMC hardware provides a number of hardware features
+ that, in their default configuration, allow access to BMC
+ internals that may be undesirable in production systems for
+ security reasons (iLPC2AHB, P2A, PCIe, debug UART, X-DMA,
+ LPC2AHB). This disables these features so as to provide
+ stronger security isolation for the BMC.
+
config ASPEED_PALLADIUM
bool "Aspeed palladium for simulation"
default n
diff --git a/arch/arm/mach-aspeed/ast2500/board_common.c b/arch/arm/mach-aspeed/ast2500/board_common.c
index ce541e88fb8e..bd73fe1c1070 100644
--- a/arch/arm/mach-aspeed/ast2500/board_common.c
+++ b/arch/arm/mach-aspeed/ast2500/board_common.c
@@ -7,18 +7,93 @@
#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;
+#if CONFIG_ASPEED_ISOLATE_BMC
+#define AST_LPC_BASE 0x1e789000
+# define AST_LPC_HICR5 0x080
+# define LPC_HICR5_ENFWH BIT(10)
+# define AST_LPC_HICRB 0x100
+# define LPC_HICRB_ILPC2AHB BIT(6)
+
+#define AST_SDMC_BASE 0x1e6e0000
+# 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 */
+ val = readl(ASPEED_HW_STRAP1);
+ val |= SCU_HWSTRAP_LPC_DISABLE;
+ writel(val, ASPEED_HW_STRAP1);
+
+ val = readl(AST_LPC_BASE + AST_LPC_HICRB);
+ val |= LPC_HICRB_ILPC2AHB;
+ writel(val, AST_LPC_BASE + 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 */
+ val = readl(ASPEED_MISC1_CTRL);
+ val |= SCU_MISC_DEBUG_UART_DISABLE;
+ writel(val, ASPEED_MISC1_CTRL);
+
+ /* X-DMA */
+ sdmc_unlocked = readl(AST_SDMC_BASE + AST_SDMC_PROTECT);
+ if (!sdmc_unlocked)
+ writel(SDRAM_UNLOCK_KEY, AST_SDMC_BASE + AST_SDMC_PROTECT);
+
+ val = readl(AST_SDMC_BASE + 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, AST_SDMC_BASE + AST_SDMC_GFX_PROT);
+
+ if (!sdmc_unlocked)
+ writel(~SDRAM_UNLOCK_KEY, AST_SDMC_BASE + AST_SDMC_PROTECT);
+
+ /* LPC2AHB */
+ val = readl(AST_LPC_BASE + AST_LPC_HICR5);
+ val &= ~LPC_HICR5_ENFWH;
+ writel(val, AST_LPC_BASE + AST_LPC_HICR5);
+}
+#endif
+
__weak int board_init(void)
{
struct udevice *dev;
int i;
int ret;
+#if CONFIG_ASPEED_ISOLATE_BMC
+ isolate_bmc();
+#endif
+
gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
/*
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
2022-04-14 4:04 [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC Zev Weiss
@ 2022-04-14 8:13 ` Joel Stanley
2022-04-14 8:41 ` Zev Weiss
0 siblings, 1 reply; 6+ messages in thread
From: Joel Stanley @ 2022-04-14 8:13 UTC (permalink / raw)
To: Zev Weiss, Ryan Chen; +Cc: Andrew Jeffery, OpenBMC Maillist
On Thu, 14 Apr 2022 at 04:05, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> This provides the functionality of the OpenBMC df-isolate-bmc distro
> feature flag, and is very directly derived from Andrew Jeffery's patch
> in the OpenBMC tree for the v2016.07 u-boot branch. The
> implementation currently only supports ast2500, though ast2400 and
> ast2600 support should be fairly simple extensions.
>
> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> ---
>
> This is meant more as something of an RFC to see if this seems like
> approximately the right way of going about this (since as far as I can
> see the existing df-isolate-bmc implementation only supports the old
> 2016 u-boot branch), but if it looks OK I suppose it could potentially
> go in as-is.
Thanks for doing this. The only potential change I can suggest is we
make each bit of hardware a different option (or we allow it to be
configured in the device tree). That assumes someone has a use case
for leaving one of the backdoorts open but closing the others.
>
> arch/arm/include/asm/arch-aspeed/platform.h | 3 +
> .../arm/include/asm/arch-aspeed/scu_ast2500.h | 8 ++
> arch/arm/mach-aspeed/Kconfig | 12 +++
> arch/arm/mach-aspeed/ast2500/board_common.c | 75 +++++++++++++++++++
> 4 files changed, 98 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-aspeed/platform.h b/arch/arm/include/asm/arch-aspeed/platform.h
> index f016bdaba3e7..be7e9087a782 100644
> --- a/arch/arm/include/asm/arch-aspeed/platform.h
> +++ b/arch/arm/include/asm/arch-aspeed/platform.h
> @@ -25,14 +25,17 @@
> #define ASPEED_FMC_CS0_BASE 0x20000000
> #elif defined(CONFIG_ASPEED_AST2500)
> #define ASPEED_MAC_COUNT 2
> +#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_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h
> index 8fe4028e4ff0..2d54d915dfed 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_DISABLE (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..2b51f87e0732 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -45,6 +45,18 @@ config ASPEED_AST2600
> which is enabled by support of LPC and eSPI peripherals.
> endchoice
>
> +config ASPEED_ISOLATE_BMC
> + bool "Disable hardware features that provide unnecessary access to the BMC"
> + depends on ASPEED_AST2500
> + default n
all kconfig are "default n", so you can omit this.
I suggest we invert the meaning of the option. The default should be
turn off the backdoors, and someone can optionally re-enable them by
selecting the option.
config ASPEED_ALLOW_BACKDOORS?
> + help
> + Aspeed BMC hardware provides a number of hardware features
> + that, in their default configuration, allow access to BMC
> + internals that may be undesirable in production systems for
> + security reasons (iLPC2AHB, P2A, PCIe, debug UART, X-DMA,
> + LPC2AHB). This disables these features so as to provide
> + stronger security isolation for the BMC.
> +
> config ASPEED_PALLADIUM
> bool "Aspeed palladium for simulation"
> default n
> diff --git a/arch/arm/mach-aspeed/ast2500/board_common.c b/arch/arm/mach-aspeed/ast2500/board_common.c
> index ce541e88fb8e..bd73fe1c1070 100644
> --- a/arch/arm/mach-aspeed/ast2500/board_common.c
> +++ b/arch/arm/mach-aspeed/ast2500/board_common.c
> @@ -7,18 +7,93 @@
> #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;
>
> +#if CONFIG_ASPEED_ISOLATE_BMC
> +#define AST_LPC_BASE 0x1e789000
> +# define AST_LPC_HICR5 0x080
> +# define LPC_HICR5_ENFWH BIT(10)
> +# define AST_LPC_HICRB 0x100
> +# define LPC_HICRB_ILPC2AHB BIT(6)
> +
> +#define AST_SDMC_BASE 0x1e6e0000
> +# 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 */
> + val = readl(ASPEED_HW_STRAP1);
> + val |= SCU_HWSTRAP_LPC_DISABLE;
> + writel(val, ASPEED_HW_STRAP1);
> +
> + val = readl(AST_LPC_BASE + AST_LPC_HICRB);
> + val |= LPC_HICRB_ILPC2AHB;
> + writel(val, AST_LPC_BASE + 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 */
> + val = readl(ASPEED_MISC1_CTRL);
> + val |= SCU_MISC_DEBUG_UART_DISABLE;
> + writel(val, ASPEED_MISC1_CTRL);
> +
> + /* X-DMA */
> + sdmc_unlocked = readl(AST_SDMC_BASE + AST_SDMC_PROTECT);
> + if (!sdmc_unlocked)
> + writel(SDRAM_UNLOCK_KEY, AST_SDMC_BASE + AST_SDMC_PROTECT);
> +
> + val = readl(AST_SDMC_BASE + 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, AST_SDMC_BASE + AST_SDMC_GFX_PROT);
> +
> + if (!sdmc_unlocked)
> + writel(~SDRAM_UNLOCK_KEY, AST_SDMC_BASE + AST_SDMC_PROTECT);
> +
> + /* LPC2AHB */
> + val = readl(AST_LPC_BASE + AST_LPC_HICR5);
> + val &= ~LPC_HICR5_ENFWH;
> + writel(val, AST_LPC_BASE + AST_LPC_HICR5);
> +}
> +#endif
> +
> __weak int board_init(void)
> {
> struct udevice *dev;
> int i;
> int ret;
>
> +#if CONFIG_ASPEED_ISOLATE_BMC
> + isolate_bmc();
> +#endif
> +
> gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100;
>
> /*
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
2022-04-14 8:13 ` Joel Stanley
@ 2022-04-14 8:41 ` Zev Weiss
2022-04-14 8:56 ` Joel Stanley
0 siblings, 1 reply; 6+ messages in thread
From: Zev Weiss @ 2022-04-14 8:41 UTC (permalink / raw)
To: Joel Stanley; +Cc: Andrew Jeffery, OpenBMC Maillist, Ryan Chen
On Thu, Apr 14, 2022 at 01:13:37AM PDT, Joel Stanley wrote:
>On Thu, 14 Apr 2022 at 04:05, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> This provides the functionality of the OpenBMC df-isolate-bmc distro
>> feature flag, and is very directly derived from Andrew Jeffery's patch
>> in the OpenBMC tree for the v2016.07 u-boot branch. The
>> implementation currently only supports ast2500, though ast2400 and
>> ast2600 support should be fairly simple extensions.
>>
>> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> ---
>>
>> This is meant more as something of an RFC to see if this seems like
>> approximately the right way of going about this (since as far as I can
>> see the existing df-isolate-bmc implementation only supports the old
>> 2016 u-boot branch), but if it looks OK I suppose it could potentially
>> go in as-is.
>
>Thanks for doing this. The only potential change I can suggest is we
>make each bit of hardware a different option (or we allow it to be
>configured in the device tree). That assumes someone has a use case
>for leaving one of the backdoorts open but closing the others.
>
This possibility came up on Discord with Andrew earlier -- I agree it'd
be nice to have somewhat more fine-grained control over it, though I'm
not aware of any platforms that really need it at the moment. I'm
certainly not as well-versed as Andrew in the precise details of exactly
how all the various busses interact in different circumstances (this was
just a fairly mechanical translation of his patch), so I'm not 100%
confident I wouldn't unwittingly introduce screwy combinations with a
more fine-grained set of config options (mostly w.r.t. to the LPC & iLPC
stuff).
>> diff --git a/arch/arm/mach-aspeed/Kconfig
>> b/arch/arm/mach-aspeed/Kconfig
>> index 579a547df61e..2b51f87e0732 100644
>> --- a/arch/arm/mach-aspeed/Kconfig
>> +++ b/arch/arm/mach-aspeed/Kconfig
>> @@ -45,6 +45,18 @@ config ASPEED_AST2600
>> which is enabled by support of LPC and eSPI peripherals.
>> endchoice
>>
>> +config ASPEED_ISOLATE_BMC
>> + bool "Disable hardware features that provide unnecessary access to the BMC"
>> + depends on ASPEED_AST2500
>> + default n
>
>all kconfig are "default n", so you can omit this.
>
Ack, thanks.
>I suggest we invert the meaning of the option. The default should be
>turn off the backdoors, and someone can optionally re-enable them by
>selecting the option.
>
I was tempted to make it 'default y' (i.e. secure-by-default), but the
possibility of compatibility breaks with existing systems that might
depend on the current insecure-by-default arrangement gave me pause. If
we don't think that's a big concern though I'm happy to flip the sense
of it and have the more aggressive default.
>config ASPEED_ALLOW_BACKDOORS?
Sounds reasonable to me, though maybe s/ALLOW/ENABLE/?
Thanks,
Zev
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
2022-04-14 8:41 ` Zev Weiss
@ 2022-04-14 8:56 ` Joel Stanley
2022-04-14 9:59 ` Zev Weiss
0 siblings, 1 reply; 6+ messages in thread
From: Joel Stanley @ 2022-04-14 8:56 UTC (permalink / raw)
To: Zev Weiss; +Cc: Andrew Jeffery, OpenBMC Maillist, Ryan Chen
On Thu, 14 Apr 2022 at 08:41, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Thu, Apr 14, 2022 at 01:13:37AM PDT, Joel Stanley wrote:
> >On Thu, 14 Apr 2022 at 04:05, Zev Weiss <zev@bewilderbeest.net> wrote:
> >>
> >> This provides the functionality of the OpenBMC df-isolate-bmc distro
> >> feature flag, and is very directly derived from Andrew Jeffery's patch
> >> in the OpenBMC tree for the v2016.07 u-boot branch. The
> >> implementation currently only supports ast2500, though ast2400 and
> >> ast2600 support should be fairly simple extensions.
> >>
> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> ---
> >>
> >> This is meant more as something of an RFC to see if this seems like
> >> approximately the right way of going about this (since as far as I can
> >> see the existing df-isolate-bmc implementation only supports the old
> >> 2016 u-boot branch), but if it looks OK I suppose it could potentially
> >> go in as-is.
> >
> >Thanks for doing this. The only potential change I can suggest is we
> >make each bit of hardware a different option (or we allow it to be
> >configured in the device tree). That assumes someone has a use case
> >for leaving one of the backdoorts open but closing the others.
> >
>
> This possibility came up on Discord with Andrew earlier -- I agree it'd
> be nice to have somewhat more fine-grained control over it, though I'm
> not aware of any platforms that really need it at the moment. I'm
> certainly not as well-versed as Andrew in the precise details of exactly
> how all the various busses interact in different circumstances (this was
> just a fairly mechanical translation of his patch), so I'm not 100%
> confident I wouldn't unwittingly introduce screwy combinations with a
> more fine-grained set of config options (mostly w.r.t. to the LPC & iLPC
> stuff).
Okay. Let this thread be a guide for the person who wants to follow up
with that work.
> >I suggest we invert the meaning of the option. The default should be
> >turn off the backdoors, and someone can optionally re-enable them by
> >selecting the option.
> >
>
> I was tempted to make it 'default y' (i.e. secure-by-default), but the
> possibility of compatibility breaks with existing systems that might
> depend on the current insecure-by-default arrangement gave me pause. If
> we don't think that's a big concern though I'm happy to flip the sense
> of it and have the more aggressive default.
Given the impact of having these left accidentally open, I think we're
doing the industry a favour by closing them off.
This aligns the 2500 with the 2600 which defaults to the backdoors
closed from A3 in silicon, and for all systems running their u-boot
SDK (which the openbmc tree is based on):
https://github.com/AspeedTech-BMC/u-boot/blob/v00.04.10/arch/arm/mach-aspeed/ast2600/platform.S#L307
>
> >config ASPEED_ALLOW_BACKDOORS?
>
> Sounds reasonable to me, though maybe s/ALLOW/ENABLE/?
Yep, go for it.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
2022-04-14 8:56 ` Joel Stanley
@ 2022-04-14 9:59 ` Zev Weiss
2022-04-14 10:32 ` Joel Stanley
0 siblings, 1 reply; 6+ messages in thread
From: Zev Weiss @ 2022-04-14 9:59 UTC (permalink / raw)
To: Joel Stanley; +Cc: Andrew Jeffery, OpenBMC Maillist, Ryan Chen
On Thu, Apr 14, 2022 at 01:56:49AM PDT, Joel Stanley wrote:
>On Thu, 14 Apr 2022 at 08:41, Zev Weiss <zev@bewilderbeest.net> wrote:
>>
>> On Thu, Apr 14, 2022 at 01:13:37AM PDT, Joel Stanley wrote:
>> >On Thu, 14 Apr 2022 at 04:05, Zev Weiss <zev@bewilderbeest.net> wrote:
>> >>
>> >> This provides the functionality of the OpenBMC df-isolate-bmc distro
>> >> feature flag, and is very directly derived from Andrew Jeffery's patch
>> >> in the OpenBMC tree for the v2016.07 u-boot branch. The
>> >> implementation currently only supports ast2500, though ast2400 and
>> >> ast2600 support should be fairly simple extensions.
>> >>
>> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
>> >> ---
>> >>
>> >> This is meant more as something of an RFC to see if this seems like
>> >> approximately the right way of going about this (since as far as I can
>> >> see the existing df-isolate-bmc implementation only supports the old
>> >> 2016 u-boot branch), but if it looks OK I suppose it could potentially
>> >> go in as-is.
>> >
>> >Thanks for doing this. The only potential change I can suggest is we
>> >make each bit of hardware a different option (or we allow it to be
>> >configured in the device tree). That assumes someone has a use case
>> >for leaving one of the backdoorts open but closing the others.
>> >
>>
>> This possibility came up on Discord with Andrew earlier -- I agree it'd
>> be nice to have somewhat more fine-grained control over it, though I'm
>> not aware of any platforms that really need it at the moment. I'm
>> certainly not as well-versed as Andrew in the precise details of exactly
>> how all the various busses interact in different circumstances (this was
>> just a fairly mechanical translation of his patch), so I'm not 100%
>> confident I wouldn't unwittingly introduce screwy combinations with a
>> more fine-grained set of config options (mostly w.r.t. to the LPC & iLPC
>> stuff).
>
>Okay. Let this thread be a guide for the person who wants to follow up
>with that work.
>
>> >I suggest we invert the meaning of the option. The default should be
>> >turn off the backdoors, and someone can optionally re-enable them by
>> >selecting the option.
>> >
>>
>> I was tempted to make it 'default y' (i.e. secure-by-default), but the
>> possibility of compatibility breaks with existing systems that might
>> depend on the current insecure-by-default arrangement gave me pause. If
>> we don't think that's a big concern though I'm happy to flip the sense
>> of it and have the more aggressive default.
>
>Given the impact of having these left accidentally open, I think we're
>doing the industry a favour by closing them off.
>
>This aligns the 2500 with the 2600 which defaults to the backdoors
>closed from A3 in silicon, and for all systems running their u-boot
>SDK (which the openbmc tree is based on):
>
>https://github.com/AspeedTech-BMC/u-boot/blob/v00.04.10/arch/arm/mach-aspeed/ast2600/platform.S#L307
>
>>
>> >config ASPEED_ALLOW_BACKDOORS?
>>
>> Sounds reasonable to me, though maybe s/ALLOW/ENABLE/?
>
>Yep, go for it.
Hmm, though now that I've drafted up a version of the patch with that
change incorporated, one other thing that's occurred to me is the
potential for confusion on ast2[46]00 systems -- since the patch as it
stands doesn't cover them, those will still have the backdoors enabled
regardless, but it seems like the Kconfig text could easily leave people
with the incorrect impression that they'll have the secure configuration
unless they explicitly opt out of it.
I don't have any g6 hardware to test on, but I think I'll expand it to
cover the ast2400 at least, and add a note to the Kconfig help text
clarifying that pre-A3 ast2600s will still be insecure. Though I guess
people doing a 'make menuconfig' for an ast2600 probably won't see it
anyway given the dependencies...not sure if there's a better way of
handling that.
Thanks,
Zev
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
2022-04-14 9:59 ` Zev Weiss
@ 2022-04-14 10:32 ` Joel Stanley
0 siblings, 0 replies; 6+ messages in thread
From: Joel Stanley @ 2022-04-14 10:32 UTC (permalink / raw)
To: Zev Weiss; +Cc: Andrew Jeffery, OpenBMC Maillist, Ryan Chen
On Thu, 14 Apr 2022 at 09:59, Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Thu, Apr 14, 2022 at 01:56:49AM PDT, Joel Stanley wrote:
> >On Thu, 14 Apr 2022 at 08:41, Zev Weiss <zev@bewilderbeest.net> wrote:
> >>
> >> On Thu, Apr 14, 2022 at 01:13:37AM PDT, Joel Stanley wrote:
> >> >On Thu, 14 Apr 2022 at 04:05, Zev Weiss <zev@bewilderbeest.net> wrote:
> >> >>
> >> >> This provides the functionality of the OpenBMC df-isolate-bmc distro
> >> >> feature flag, and is very directly derived from Andrew Jeffery's patch
> >> >> in the OpenBMC tree for the v2016.07 u-boot branch. The
> >> >> implementation currently only supports ast2500, though ast2400 and
> >> >> ast2600 support should be fairly simple extensions.
> >> >>
> >> >> Signed-off-by: Zev Weiss <zev@bewilderbeest.net>
> >> >> ---
> >> >>
> >> >> This is meant more as something of an RFC to see if this seems like
> >> >> approximately the right way of going about this (since as far as I can
> >> >> see the existing df-isolate-bmc implementation only supports the old
> >> >> 2016 u-boot branch), but if it looks OK I suppose it could potentially
> >> >> go in as-is.
> >> >
> >> >Thanks for doing this. The only potential change I can suggest is we
> >> >make each bit of hardware a different option (or we allow it to be
> >> >configured in the device tree). That assumes someone has a use case
> >> >for leaving one of the backdoorts open but closing the others.
> >> >
> >>
> >> This possibility came up on Discord with Andrew earlier -- I agree it'd
> >> be nice to have somewhat more fine-grained control over it, though I'm
> >> not aware of any platforms that really need it at the moment. I'm
> >> certainly not as well-versed as Andrew in the precise details of exactly
> >> how all the various busses interact in different circumstances (this was
> >> just a fairly mechanical translation of his patch), so I'm not 100%
> >> confident I wouldn't unwittingly introduce screwy combinations with a
> >> more fine-grained set of config options (mostly w.r.t. to the LPC & iLPC
> >> stuff).
> >
> >Okay. Let this thread be a guide for the person who wants to follow up
> >with that work.
> >
> >> >I suggest we invert the meaning of the option. The default should be
> >> >turn off the backdoors, and someone can optionally re-enable them by
> >> >selecting the option.
> >> >
> >>
> >> I was tempted to make it 'default y' (i.e. secure-by-default), but the
> >> possibility of compatibility breaks with existing systems that might
> >> depend on the current insecure-by-default arrangement gave me pause. If
> >> we don't think that's a big concern though I'm happy to flip the sense
> >> of it and have the more aggressive default.
> >
> >Given the impact of having these left accidentally open, I think we're
> >doing the industry a favour by closing them off.
> >
> >This aligns the 2500 with the 2600 which defaults to the backdoors
> >closed from A3 in silicon, and for all systems running their u-boot
> >SDK (which the openbmc tree is based on):
> >
> >https://github.com/AspeedTech-BMC/u-boot/blob/v00.04.10/arch/arm/mach-aspeed/ast2600/platform.S#L307
> >
> >>
> >> >config ASPEED_ALLOW_BACKDOORS?
> >>
> >> Sounds reasonable to me, though maybe s/ALLOW/ENABLE/?
> >
> >Yep, go for it.
>
> Hmm, though now that I've drafted up a version of the patch with that
> change incorporated, one other thing that's occurred to me is the
> potential for confusion on ast2[46]00 systems -- since the patch as it
> stands doesn't cover them, those will still have the backdoors enabled
> regardless, but it seems like the Kconfig text could easily leave people
> with the incorrect impression that they'll have the secure configuration
> unless they explicitly opt out of it.
The 2600 is covered, see the link above. It unconditionally sets the
"disable backdoor" bits on the 2600A1/A2, and those bits default to
"disable backdoor" on the A3.
> I don't have any g6 hardware to test on, but I think I'll expand it to
> cover the ast2400 at least, and add a note to the Kconfig help text
> clarifying that pre-A3 ast2600s will still be insecure. Though I guess
> people doing a 'make menuconfig' for an ast2600 probably won't see it
> anyway given the dependencies...not sure if there's a better way of
> handling that.
It would be good to cover the 2400 for completeness.
You could add an ifdef to remove the "disable backdoor" code on the
2600. I think it would be better to leave it as-is, and clarify that
the 2600 doesn't support the option as it disables backdoorts by
default.
It would be good to document the existence of these settings and the
defaults we have in place.
Cheers,
Joel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-14 10:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-14 4:04 [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC Zev Weiss
2022-04-14 8:13 ` Joel Stanley
2022-04-14 8:41 ` Zev Weiss
2022-04-14 8:56 ` Joel Stanley
2022-04-14 9:59 ` Zev Weiss
2022-04-14 10:32 ` Joel Stanley
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.