Re: [PATCH QEMU HCI-8.1 v6 14/34] q35: Fix migration of SMRAM state

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] q35: pre-negotiation writes to SMBASE reg can lock it and enable SMRAM blackhole
--

commit e599128e44cfa63b5f0a0828dcf6845ef880af50
Author: Igor Mammedov <imammedo@redhat.com>

q35: Fix migration of SMRAM state

This change drops the wmask == 0xff discard in mch_update_smbase_smram() and
resyncs wmask from the SMBASE config register value, so a locked SMRAM state
survives migration. It also removes the pre-negotiation junk-write check
from test_smram_smbase_lock().

Link: https://patchew.org/QEMU/20251203180851.6390-1-arbn@yandex-team.com/

> diff --git a/hw/pci-host/q35.c b/hw/pci-host/q35.c
> --- a/hw/pci-host/q35.c
> +++ b/hw/pci-host/q35.c

[ ... ]

> @@ -432,30 +432,27 @@ static void mch_update_smbase_smram(MCHPCIState *mch)
>      }
>  
>      if (*reg == MCH_HOST_BRIDGE_F_SMBASE_QUERY) {
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] =
> -            MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_IN_RAM;
>          return;
>      }
>  
>      /*
> -     * default/reset state, discard written value
> -     * which will disable SMRAM balackhole at SMBASE
> +     * reg value can come from register write/reset/migration source,
> +     * update wmask to be in sync with it regardless of source
>       */
> -    if (pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] == 0xff) {
> -        *reg = 0x00;
> +    if (*reg == MCH_HOST_BRIDGE_F_SMBASE_IN_RAM) {
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        return;
>      }
> -
> -    memory_region_transaction_begin();
>      if (*reg & MCH_HOST_BRIDGE_F_SMBASE_LCK) {
> -        /* disable all writes */
> -        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] &=
> -            ~MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +        /* lock register at 0x2 and disable all writes */
> +        pd->wmask[MCH_HOST_BRIDGE_F_SMBASE] = 0;
>          *reg = MCH_HOST_BRIDGE_F_SMBASE_LCK;
> -        lck = true;
> -    } else {
> -        lck = false;
>      }
> +
> +    lck = *reg & MCH_HOST_BRIDGE_F_SMBASE_LCK;
> +    memory_region_transaction_begin();
>      memory_region_set_enabled(&mch->smbase_blackhole, lck);
>      memory_region_set_enabled(&mch->smbase_window, lck);
>      memory_region_transaction_commit();

[Severity: Low]
Can a guest lock the SMBASE register without negotiating first?

mch_reset() sets config[MCH_HOST_BRIDGE_F_SMBASE] to 0 and
wmask[MCH_HOST_BRIDGE_F_SMBASE] to 0xff, so after reset every bit of the
register is writable.

A guest config write to offset 0x9c then goes through this path:

pci_default_write_config()
  mch_write_config()
    mch_update_smbase_smram()

The old "wmask == 0xff" check used to set *reg = 0x00 here.  With that
check removed, a written value with bit 0x2 set reaches the
MCH_HOST_BRIDGE_F_SMBASE_LCK branch with no 0xff query step first.  That
branch sets wmask to 0 and *reg to 0x02, and then enables smbase_blackhole
and smbase_window.

A write of exactly 0x01 reaches the MCH_HOST_BRIDGE_F_SMBASE_IN_RAM branch
and sets wmask to MCH_HOST_BRIDGE_F_SMBASE_LCK, also without negotiation.

Can this make the 128K of RAM at the default SMBASE become inaccessible to
the guest CPU address space until the next reset, for a guest that never
negotiated the protocol?  For example, a stray dword config write covering
0x9c could trigger it.

The commit message acknowledges junk writes are accepted ("it would let
guest write junk values in register ... until it's stumbles upon reserved
0x1 [|] 0x2 values").  Is this behavior change intended, given that the
migration fix needs wmask to be derived from the register value?  No later
patch in the series changes mch_update_smbase_smram() or the SMBASE wmask
handling.

> diff --git a/tests/qtest/q35-test.c b/tests/qtest/q35-test.c
> --- a/tests/qtest/q35-test.c
> +++ b/tests/qtest/q35-test.c
> @@ -206,12 +206,6 @@ static void test_smram_smbase_lock(void)
>      qtest_writeb(qts, SMBASE, SMRAM_TEST_PATTERN);
>      g_assert_cmpint(qtest_readb(qts, SMBASE), ==, SMRAM_TEST_PATTERN);
>  
> -    /* check that writing junk to 0x9c before before negotiating is ignored */
> -    for (i = 0; i < 0xff; i++) {
> -        qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, i);
> -        g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0);
> -    }
> -

Since this junk-write check was removed from test_smram_smbase_lock(), is
there any test that covers the junk-then-lock path, where a pre-negotiation
write with bit 0x2 set enables the SMRAM blackhole?

>      /* enable SMRAM at SMBASE */
>      qpci_config_writeb(pcidev, MCH_HOST_BRIDGE_F_SMBASE, 0xff);
>      g_assert(qpci_config_readb(pcidev, MCH_HOST_BRIDGE_F_SMBASE) == 0x01);

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261102-vfio_v1-v6-0-9bc7cabc850d@virtuozzo.com?part=14