Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC

From: Joel Stanley <joel@jms.id.au>
To: Zev Weiss <zev@bewilderbeest.net>
Cc: Andrew Jeffery <andrew@aj.id.au>,
	OpenBMC Maillist <openbmc@lists.ozlabs.org>,
	Ryan Chen <ryan_chen@aspeedtech.com>
Subject: Re: [PATCH u-boot v2019.04-aspeed-openbmc] aspeed: add CONFIG_ASPEED_ISOLATE_BMC
Date: Thu, 14 Apr 2022 08:56:49 +0000	[thread overview]
Message-ID: <CACPK8Xd3HnX1angrhtb1UC5Zaa08n8emKp5Og9QEsm9yp-wpTA@mail.gmail.com> (raw)
In-Reply-To: <YlfePCrv0TBYtNHJ@hatter.bewilderbeest.net>

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.