Re: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable backdoor interfaces

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

On Wed, 20 Apr 2022 at 03:35, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Wed, 20 Apr 2022, at 12:36, Ryan Chen wrote:
> >> -----Original Message-----
> >> From: Zev Weiss <zev@bewilderbeest.net>
> >> Sent: Wednesday, April 20, 2022 7:42 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>; Ian Woloschin
> >> <ian.woloschin@akamai.com>; Lei Yu <yulei.sh@bytedance.com>
> >> Subject: [PATCH u-boot v2019.04-aspeed-openbmc v3] aspeed: Disable
> >> backdoor interfaces
> >>
> >> On ast2400 and ast2500 we now disable the various hardware backdoor
> >> interfaces as is done on ast2600.  Two Kconfig options can selectively
> >> re-enable some of these interfaces: CONFIG_ASPEED_ENABLE_SUPERIO leaves
> >> the ast2x00 built-in Super I/O device enabled, as it is required for some
> >> systems, and CONFIG_ASPEED_ENABLE_DEBUG_UART leaves the hardware
> >> debug UART enabled, since it provides a relatively high ratio of utility to
> >> security risk during development.
> >>
> >> 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>
> >> ---
> >>
> >> Tested on ast2500 and (hostless, BMC-only) ast2400.
> >>
> >> Ryan, are you OK with having an option (off by default) to leave the debug
> >> UART enabled as in this version of the patch?
> >>
> > Thanks your submit.
> > Again, my opinion still keep the direct patch to disable it.
> > Not have config to enable it.
> >
>
> Ideally yes, but as Ian mentioned he has at least one system where the
> SuperIO AHB bridge must be enabled to allow their BIOS to configure the
> UARTs. So we need an option to cater to that.

Agreed.

Ideally these backdoors would be controlled by strapping, so
development systems and platforms that chose to open them can
configure the system appropriately. But the hardware does not have
this ability, so the next best thing we can do is provide an option in
the firmware.

Note that before Zev sent this patch, the backdoors were left enabled
with the current SDK. Having them disabled by default, behind an
option, is a welcome improvement.

I'm happy with this patch, but I'll give others time to respond before merging.

Cheers,

Joel


>
> I don't want people to have to patch the code to allow use of the
> backdoors, that will just lead to other problems (e.g. reverting this
> patch is the simplest thing, and opens up all the backdoors instead of
> a targeted one).
>
> Andrew