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

[Zev Weiss]

On Thu, Apr 14, 2022 at 01:56:49AM PDT, Joel Stanley wrote:
>On Thu, 14 Apr 2022 at 08:41, Zev Weiss <zev at 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 at 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 at 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