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

Properly report if the SuperIO is ready or busy.

Signed-off-by: Deb McLemore <debmc@linux.ibm.com>
---
 hw/ast-bmc/ast-io.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

On Fri, Oct 18, 2019 at 8:57 AM Deb McLemore <debmc@linux.ibm.com> wrote:
>
> Properly report if the SuperIO is ready or busy.
>
> Signed-off-by: Deb McLemore <debmc@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/

Hi Deb,

On Fri, 18 Oct 2019, at 08:53, Deb McLemore wrote:
> Properly report if the SuperIO is ready or busy.

You're conflating SuperIO with the iLPC2AHB backdoor here. The iLPC2AHB
is a SuperIO device, so they're related but different entities: SuperIO is a
byte-based indexed-IO interface to devices, the iLPC2AHB maps read and
write operations from the host onto the BMC's physical address space.

Also the bit being read doesn't determine whether SuperIO is ready or busy,
but whether the iLPC2AHB bridge has been "disabled" (i.e. writes have no
effect). SuperIO and its devices have no concept of being busy, SuperIO
relies on the firmware to protect against concurrent access. If SuperIO is
enabled we can always read the state of the "disable" bit via the iLPC2AHB
device as the disable bit only impacts the device's ability to write.

Without the ability to write via the iLPC2AHB bridge we have no means to
drive the flash controller in the event that HIOMAP support isn't present
(e.g. older P8 firmwares).

> 
> Signed-off-by: Deb McLemore <debmc@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)
>  {
> -	return !(ast_ahb_readl(LPC_HICRB) & LPC_HICRB_ILPC_DISABLE);
> +	int rc;
> +	rc = ast_ahb_readl(LPC_HICRB);
> +	prlog(PR_NOTICE, "LPC: SuperIO Ready/Busy=%s rc=0x%x LPC_HICRB 0x%x\n",
> +		((rc & LPC_HICRB_ILPC_DISABLE) ? "Ready" : "Busy"),
> +		rc,
> +		(rc & LPC_HICRB_ILPC_DISABLE));
> +	return (rc & LPC_HICRB_ILPC_DISABLE);

As Oliver points out your change inverts the condition - this means the function
will indicate to the caller that the iLPC2AHB bridge is RW capable when in fact
it is configured as RO (and RO when it is RW). This isn't what we want :)

Andrew

On Mon, 21 Oct 2019, at 17:54, Oliver O'Halloran wrote:
> On Fri, Oct 18, 2019 at 8:57 AM Deb McLemore <debmc@linux.ibm.com> wrote:
> >
> > Properly report if the SuperIO is ready or busy.
> >
> > Signed-off-by: Deb McLemore <debmc@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] 

Yep, the description in the ASPEED datasheets isn't very helpful.