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

[Joel Stanley <joel@jms.id.au>]

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