[Skiboot] [PATCH 08/12] ast-io: Fix Ready/Busy indication

[Oliver O'Halloran]

On Fri, Oct 18, 2019 at 8:57 AM Deb McLemore <debmc at linux.ibm.com> wrote:
>
> Properly report if the SuperIO is ready or busy.
>
> Signed-off-by: Deb McLemore <debmc at linux.ibm.com>
> ---
>  hw/ast-bmc/ast-io.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/ast-bmc/ast-io.c b/hw/ast-bmc/ast-io.c
> index 171942a..22fb59f 100644
> --- a/hw/ast-bmc/ast-io.c
> +++ b/hw/ast-bmc/ast-io.c
> @@ -361,7 +361,13 @@ bool ast_sio_init(void)
>
>  bool ast_io_is_rw(void)
>  {

With the prlog removed:

> -       return !(ast_ahb_readl(LPC_HICRB) & LPC_HICRB_ILPC_DISABLE);
> +       int rc;
> +       rc = ast_ahb_readl(LPC_HICRB);
> +       return (rc & LPC_HICRB_ILPC_DISABLE);

So, you're flipping the polarity of the test.

Now the only real documentation for the ILPC_DISABLE bit I could find
was Stewart's blog on pantsdown[0] which says:

> iLPC2AHB bridge Pt I
> State: Enabled at cold start
> Description: A SuperIO device is exposed that provides access to the BMC’s address-space
> Impact: Arbitrary reads and writes to the BMC address-space
> Risk: High – known vulnerability and explicitly used as a feature in some platform designs
> Mitigation: Can be disabled by configuring a bit in the BMC’s LPC controller, however see Pt II.
>
> iLPC2AHB bridge Pt II
> State: Enabled at cold start
> Description: The bit disabling the iLPC2AHB bridge only removes write access – reads are still possible.
> Impact: Arbitrary reads of the BMC address-space
> Risk: High – we expect the capability and mitigation are not well known, and the mitigation has side-effects
> Mitigation: Disable SuperIO decoding on the LPC bus (0x2E/0x4E decode). Decoding is controlled via hardware strapping and can be turned off at runtime, however disabling SuperIO decoding also removes the host’s ability to configure SUARTs, System wakeups, GPIOs and the BMC/Host mailbox

Based on that the existing test seems correct to me.

>From the commit message you're really interested in the state of the
SuperIO rather than the RW state of the iLPC <-> AHB bridge. That
said, I don't really understand the intention behind the change or how
it helps detecting the state of the SuperIO. I'm guessing that the
patch changes something and as a secondary effect it happened to fix
whatever bug you were chasing, but I can't tell from the change alone.
Can you elaborate a bit on what the intention is here, the bug it
fixes, and what platforms require it?

Oliver

[0] https://www.flamingspork.com/blog/2019/01/23/cve-2019-6260:-gaining-control-of-bmc-from-the-host-processor/