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

[Zev Weiss]

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).

>> 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