| Message ID | 20220414224004.29703-1-zev@bewilderbeest.net |
|---|---|
| State | New |
| Headers | show |
| Series | [u-boot,v2019.04-aspeed-openbmc,v2] aspeed: add CONFIG_ASPEED_ENABLE_BACKDOORS | expand |
Hello Zev, I don't think it is good to send a patch to enable security backdoor. It should not be enabled, even it user aware it. That will cause big issues in BMC. Ryan > -----Original Message----- > From: Zev Weiss <zev@bewilderbeest.net> > Sent: Friday, April 15, 2022 6:40 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> > Subject: [PATCH u-boot v2019.04-aspeed-openbmc v2] aspeed: add > CONFIG_ASPEED_ENABLE_BACKDOORS > > On ast2400 and ast2500 we now default to disabling the various hardware > backdoor interfaces as is done on ast2600, though the Kconfig option can be > set to y to re-enable them if desired for debugging. > > 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> > --- > > I've tested this on both ast2500 and ast2400, with the slight caveat that the > only ast2400 hardware I have is a hostless (BMC-only) system. > > 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/ > > 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 | 13 ++++ > arch/arm/mach-aspeed/ast2400/board_common.c | 68 > +++++++++++++++++ > arch/arm/mach-aspeed/ast2500/board_common.c | 73 > +++++++++++++++++++ > 6 files changed, 176 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..4bbf6fff326d 100644 > --- a/arch/arm/mach-aspeed/Kconfig > +++ b/arch/arm/mach-aspeed/Kconfig > @@ -45,6 +45,19 @@ config ASPEED_AST2600 > which is enabled by support of LPC and eSPI peripherals. > endchoice > > +config ASPEED_ENABLE_BACKDOORS > + bool "Enable hardware features that provide back-door access to the > BMC" > + depends on ASPEED_AST2400 || ASPEED_AST2500 > + help > + Aspeed BMCs include a number of hardware features that > + provide access to BMC internals that is undesirable in > + production systems for security reasons (iLPC2AHB, P2A, > + PCIe, debug UART, X-DMA, LPC2AHB), but may be useful for > + debugging. Say Y here to enable these features for a debug > + (insecure) build. (This option is not available for the > + ast2600, on which the backdoors are disabled > + unconditionally.) > + > 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..10ce7af8c108 100644 > --- a/arch/arm/mach-aspeed/ast2400/board_common.c > +++ b/arch/arm/mach-aspeed/ast2400/board_common.c > @@ -4,14 +4,82 @@ > #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; > > +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) > +#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 */ > + val = readl(ASPEED_HW_STRAP1); > + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS; > + writel(val, ASPEED_HW_STRAP1); > + > + 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); } #endif > + > __weak int board_init(void) > { > +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) > + isolate_bmc(); > +#endif > + > 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..29554011eb38 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; > > +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) > +#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 */ > + val = readl(ASPEED_HW_STRAP1); > + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS; > + writel(val, ASPEED_HW_STRAP1); > + > + 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 */ > + val = readl(ASPEED_MISC1_CTRL); > + val |= SCU_MISC_DEBUG_UART_DISABLE; > + writel(val, ASPEED_MISC1_CTRL); > + > + /* 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); } #endif > + > __weak int board_init(void) > { > struct udevice *dev; > int i; > int ret; > > +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) > + isolate_bmc(); > +#endif > + > gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100; > > /* > -- > 2.35.1
On Thu, Apr 14, 2022 at 08:21:00PM PDT, Ryan Chen wrote: >Hello Zev, > I don't think it is good to send a patch to enable security backdoor. > It should not be enabled, even it user aware it. > That will cause big issues in BMC. > Hi Ryan, To clarify, the current state of the code leaves the backdoors enabled on ast2400 and ast2500 (insecure/debug mode), with no easy way to turn them off. With this patch they'll be turned off by default (secure/production mode), but a user that wants to turn them back on can still do so if they explicitly request it via the new Kconfig option. The name and description of the option I think make it pretty clear that it's for debugging only and shouldn't be enabled on production systems. Is your opinion that we should apply something like this patch, but without any configurability at all? I think having the option available to leave the backdoors on could be worthwhile (I've found the debug UART useful now and then during my own development work, for example) as long as the security implications are clearly indicated. It wouldn't be the first Kconfig option that's really only appropriate for development and shouldn't be enabled in a production build (e.g. ASPEED_PALLADIUM). Thanks, Zev
Hello, Thanks your response. And yes, I prefer apply patch without any config to disable it. Ryan > -----Original Message----- > From: Zev Weiss <zev@bewilderbeest.net> > Sent: Friday, April 15, 2022 4:04 PM > To: Ryan Chen <ryan_chen@aspeedtech.com> > Cc: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org; Andrew Jeffery > <andrew@aj.id.au> > Subject: Re: [PATCH u-boot v2019.04-aspeed-openbmc v2] aspeed: add > CONFIG_ASPEED_ENABLE_BACKDOORS > > On Thu, Apr 14, 2022 at 08:21:00PM PDT, Ryan Chen wrote: > >Hello Zev, > > I don't think it is good to send a patch to enable security backdoor. > > It should not be enabled, even it user aware it. > > That will cause big issues in BMC. > > > > Hi Ryan, > > To clarify, the current state of the code leaves the backdoors enabled on > ast2400 and ast2500 (insecure/debug mode), with no easy way to turn them > off. > > With this patch they'll be turned off by default (secure/production mode), but a > user that wants to turn them back on can still do so if they explicitly request it > via the new Kconfig option. The name and description of the option I think > make it pretty clear that it's for debugging only and shouldn't be enabled on > production systems. > > Is your opinion that we should apply something like this patch, but without any > configurability at all? I think having the option available to leave the > backdoors on could be worthwhile (I've found the debug UART useful now and > then during my own development work, for example) as long as the security > implications are clearly indicated. It wouldn't be the first Kconfig option > that's really only appropriate for development and shouldn't be enabled in a > production build (e.g. ASPEED_PALLADIUM). > > > Thanks, > Zev
On Fri, Apr 15, 2022 at 01:11:09AM PDT, Ryan Chen wrote: >Hello, > Thanks your response. > And yes, I prefer apply patch without any config to disable it. > >Ryan > After thinking about this a bit more, I remembered that Ian Woloschin (CCed) had mentioned at some point that the systems he works with do in fact use the AST2500's built-in Super-IO, and hence would presumably be broken by a patch that unconditionally disabled that. And in contrast, the ASRock boards I've been working with require the AST2500 Super-IO to be disabled for the host to boot properly, so it seems like we'll need *some* minimal amount of configurability to support at least those two classes of systems (i.e. a Kconfig boolean that determines whether the Super-IO should be enabled or disabled). I don't know offhand what the interactions between SCU70[20], HICRB[6], and HICR5[10] are though, and I don't have any hardware that actually uses the AST2500 Super-IO to test with. Would leaving SCU70[20]=0 to enable the Super-IO while leaving HICRB[6]=1 and HICR5[10]=0 work for systems like Ian's to enable the Super-IO while keeping everything else locked down as much as possible? Zev
Hello, Yes, leave SCU70[20] =0, set HICR5[8] = 0, HICRB[6] = 1 is enough to disable LPC2AHB. HICR5[6] is LPC fw cycle it is allowed. Ryan > -----Original Message----- > From: Zev Weiss <zev@bewilderbeest.net> > Sent: Tuesday, April 19, 2022 9:00 AM > To: Ryan Chen <ryan_chen@aspeedtech.com> > Cc: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org; Andrew Jeffery > <andrew@aj.id.au>; Ian Woloschin <ian.woloschin@akamai.com> > Subject: Re: [PATCH u-boot v2019.04-aspeed-openbmc v2] aspeed: add > CONFIG_ASPEED_ENABLE_BACKDOORS > > On Fri, Apr 15, 2022 at 01:11:09AM PDT, Ryan Chen wrote: > >Hello, > > Thanks your response. > > And yes, I prefer apply patch without any config to disable it. > > > >Ryan > > > > After thinking about this a bit more, I remembered that Ian Woloschin > (CCed) had mentioned at some point that the systems he works with do in fact > use the AST2500's built-in Super-IO, and hence would presumably be broken by > a patch that unconditionally disabled that. And in contrast, the ASRock boards > I've been working with require the AST2500 Super-IO to be disabled for the > host to boot properly, so it seems like we'll need > *some* minimal amount of configurability to support at least those two > classes of systems (i.e. a Kconfig boolean that determines whether the > Super-IO should be enabled or disabled). > > I don't know offhand what the interactions between SCU70[20], HICRB[6], and > HICR5[10] are though, and I don't have any hardware that actually uses the > AST2500 Super-IO to test with. Would leaving SCU70[20]=0 to enable the > Super-IO while leaving HICRB[6]=1 and HICR5[10]=0 work for systems like > Ian's to enable the Super-IO while keeping everything else locked down as > much as possible? > > > Zev
Yup, I'm pretty sure my systems need SuperIO to configure the serial ports which we do use, though I'll be the first to admit I do not entirely understand how any of this is actually configured.
-Ian
On 4/18/22, 10:33 PM, "Ryan Chen" <ryan_chen@aspeedtech.com> wrote:
Hello,
Yes, leave SCU70[20] =0, set HICR5[8] = 0, HICRB[6] = 1 is enough to disable LPC2AHB.
HICR5[6] is LPC fw cycle it is allowed.
Ryan
> -----Original Message-----
> From: Zev Weiss <zev@bewilderbeest.net>
> Sent: Tuesday, April 19, 2022 9:00 AM
> To: Ryan Chen <ryan_chen@aspeedtech.com>
> Cc: Joel Stanley <joel@jms.id.au>; openbmc@lists.ozlabs.org; Andrew Jeffery
> <andrew@aj.id.au>; Ian Woloschin <ian.woloschin@akamai.com>
> Subject: Re: [PATCH u-boot v2019.04-aspeed-openbmc v2] aspeed: add
> CONFIG_ASPEED_ENABLE_BACKDOORS
>
> On Fri, Apr 15, 2022 at 01:11:09AM PDT, Ryan Chen wrote:
> >Hello,
> > Thanks your response.
> > And yes, I prefer apply patch without any config to disable it.
> >
> >Ryan
> >
>
> After thinking about this a bit more, I remembered that Ian Woloschin
> (CCed) had mentioned at some point that the systems he works with do in fact
> use the AST2500's built-in Super-IO, and hence would presumably be broken by
> a patch that unconditionally disabled that. And in contrast, the ASRock boards
> I've been working with require the AST2500 Super-IO to be disabled for the
> host to boot properly, so it seems like we'll need
> *some* minimal amount of configurability to support at least those two
> classes of systems (i.e. a Kconfig boolean that determines whether the
> Super-IO should be enabled or disabled).
>
> I don't know offhand what the interactions between SCU70[20], HICRB[6], and
> HICR5[10] are though, and I don't have any hardware that actually uses the
> AST2500 Super-IO to test with. Would leaving SCU70[20]=0 to enable the
> Super-IO while leaving HICRB[6]=1 and HICR5[10]=0 work for systems like
> Ian's to enable the Super-IO while keeping everything else locked down as
> much as possible?
>
>
> Zev
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..4bbf6fff326d 100644 --- a/arch/arm/mach-aspeed/Kconfig +++ b/arch/arm/mach-aspeed/Kconfig @@ -45,6 +45,19 @@ config ASPEED_AST2600 which is enabled by support of LPC and eSPI peripherals. endchoice +config ASPEED_ENABLE_BACKDOORS + bool "Enable hardware features that provide back-door access to the BMC" + depends on ASPEED_AST2400 || ASPEED_AST2500 + help + Aspeed BMCs include a number of hardware features that + provide access to BMC internals that is undesirable in + production systems for security reasons (iLPC2AHB, P2A, + PCIe, debug UART, X-DMA, LPC2AHB), but may be useful for + debugging. Say Y here to enable these features for a debug + (insecure) build. (This option is not available for the + ast2600, on which the backdoors are disabled + unconditionally.) + 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..10ce7af8c108 100644 --- a/arch/arm/mach-aspeed/ast2400/board_common.c +++ b/arch/arm/mach-aspeed/ast2400/board_common.c @@ -4,14 +4,82 @@ #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; +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) +#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 */ + val = readl(ASPEED_HW_STRAP1); + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS; + writel(val, ASPEED_HW_STRAP1); + + 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); +} +#endif + __weak int board_init(void) { +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) + isolate_bmc(); +#endif + 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..29554011eb38 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; +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) +#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 */ + val = readl(ASPEED_HW_STRAP1); + val |= SCU_HWSTRAP_LPC_SIO_DEC_DIS; + writel(val, ASPEED_HW_STRAP1); + + 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 */ + val = readl(ASPEED_MISC1_CTRL); + val |= SCU_MISC_DEBUG_UART_DISABLE; + writel(val, ASPEED_MISC1_CTRL); + + /* 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); +} +#endif + __weak int board_init(void) { struct udevice *dev; int i; int ret; +#if !defined(CONFIG_ASPEED_ENABLE_BACKDOORS) + isolate_bmc(); +#endif + gd->bd->bi_boot_params = CONFIG_SYS_SDRAM_BASE + 0x100; /*
On ast2400 and ast2500 we now default to disabling the various hardware backdoor interfaces as is done on ast2600, though the Kconfig option can be set to y to re-enable them if desired for debugging. 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> --- I've tested this on both ast2500 and ast2400, with the slight caveat that the only ast2400 hardware I have is a hostless (BMC-only) system. 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/ 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 | 13 ++++ arch/arm/mach-aspeed/ast2400/board_common.c | 68 +++++++++++++++++ arch/arm/mach-aspeed/ast2500/board_common.c | 73 +++++++++++++++++++ 6 files changed, 176 insertions(+)