* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
>
> On Thu, Sep 6, 2018 at 12:59 PM Dr. David Alan Gilbert
> <dgilb...@redhat.com> wrote:
> >
> > * Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> > > Hi
> > >
> > > On Thu, Sep 6, 2018 at 11:58 AM Igor Mammedov <imamm...@redhat.com> wrote:
> > > >
> > > > On Thu, 6 Sep 2018 07:50:09 +0400
> > > > Marc-André Lureau <marcandre.lur...@gmail.com> wrote:
> > > >
> > > > > Hi
> > > > >
> > > > > On Tue, Sep 4, 2018 at 10:47 AM Igor Mammedov <imamm...@redhat.com>
> > > > > wrote:
> > > > > >
> > > > > > On Fri, 31 Aug 2018 19:24:24 +0200
> > > > > > Marc-André Lureau <marcandre.lur...@redhat.com> wrote:
> > > > > >
> > > > > > > This allows to pass the last failing test from the Windows HLK
> > > > > > > TPM 2.0
> > > > > > > TCG PPI 1.3 tests.
> > > > > > >
> > > > > > > The interface is described in the "TCG Platform Reset Attack
> > > > > > > Mitigation Specification", chapter 6 "ACPI _DSM Function".
> > > > > > > According
> > > > > > > to Laszlo, it's not so easy to implement in OVMF, he suggested to
> > > > > > > do
> > > > > > > it in qemu instead.
> > > > > > >
> > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > > > > > > ---
> > > > > > > hw/tpm/tpm_ppi.h | 2 ++
> > > > > > > hw/i386/acpi-build.c | 46
> > > > > > > ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > hw/tpm/tpm_crb.c | 1 +
> > > > > > > hw/tpm/tpm_ppi.c | 23 ++++++++++++++++++++++
> > > > > > > hw/tpm/tpm_tis.c | 1 +
> > > > > > > docs/specs/tpm.txt | 2 ++
> > > > > > > hw/tpm/trace-events | 3 +++
> > > > > > > 7 files changed, 78 insertions(+)
> > > > > > >
> > > > > > > diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
> > > > > > > index f6458bf87e..3239751e9f 100644
> > > > > > > --- a/hw/tpm/tpm_ppi.h
> > > > > > > +++ b/hw/tpm/tpm_ppi.h
> > > > > > > @@ -23,4 +23,6 @@ typedef struct TPMPPI {
> > > > > > > bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > hwaddr addr, Object *obj, Error **errp);
> > > > > > >
> > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi);
> > > > > > > +
> > > > > > > #endif /* TPM_TPM_PPI_H */
> > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > index c5e9a6e11d..2ab3e8fae7 100644
> > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > @@ -1824,6 +1824,13 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > pprq = aml_name("PPRQ");
> > > > > > > pprm = aml_name("PPRM");
> > > > > > >
> > > > > > > + aml_append(dev,
> > > > > > > + aml_operation_region("TPP3", AML_SYSTEM_MEMORY,
> > > > > > > + aml_int(TPM_PPI_ADDR_BASE +
> > > > > > > 0x15a),
> > > > > > > + 0x1));
> > > > > > > + field = aml_field("TPP3", AML_BYTE_ACC, AML_NOLOCK,
> > > > > > > AML_PRESERVE);
> > > > > > > + aml_append(field, aml_named_field("MOVV", 8));
> > > > > > > + aml_append(dev, field);
> > > > > > > /*
> > > > > > > * DerefOf in Windows is broken with SYSTEM_MEMORY. Use a
> > > > > > > dynamic
> > > > > > > * operation region inside of a method for getting FUNC[op].
> > > > > > > @@ -2166,7 +2173,46 @@ build_tpm_ppi(TPMIf *tpm, Aml *dev)
> > > > > > > aml_append(ifctx, aml_return(aml_buffer(1,
> > > > > > > zerobyte)));
> > > > > > > }
> > > > > > > aml_append(method, ifctx);
> > > > > > > +
> > > > > > > + ifctx = aml_if(
> > > > > > > + aml_equal(uuid,
> > > > > > > +
> > > > > > > aml_touuid("376054ED-CC13-4675-901C-4756D7F2D45D")));
> > > > > > > + {
> > > > > > > + /* standard DSM query function */
> > > > > > > + ifctx2 = aml_if(aml_equal(function, zero));
> > > > > > > + {
> > > > > > > + uint8_t byte_list[1] = { 0x03 };
> > > > > > > + aml_append(ifctx2, aml_return(aml_buffer(1,
> > > > > > > byte_list)));
> > > > > > > + }
> > > > > > > + aml_append(ifctx, ifctx2);
> > > > > > > +
> > > > > > > + /*
> > > > > > > + * TCG Platform Reset Attack Mitigation
> > > > > > > Specification 1.0 Ch.6
> > > > > > > + *
> > > > > > > + * Arg 2 (Integer): Function Index = 1
> > > > > > > + * Arg 3 (Package): Arguments = Package: Type:
> > > > > > > Integer
> > > > > > > + * Operation Value of the Request
> > > > > > > + * Returns: Type: Integer
> > > > > > > + * 0: Success
> > > > > > > + * 1: General Failure
> > > > > > > + */
> > > > > > > + ifctx2 = aml_if(aml_equal(function, one));
> > > > > > > + {
> > > > > > > + aml_append(ifctx2,
> > > > > > > +
> > > > > > > aml_store(aml_derefof(aml_index(arguments, zero)),
> > > > > > > + op));
> > > > > > > + {
> > > > > > > + aml_append(ifctx2, aml_store(op,
> > > > > > > aml_name("MOVV")));
> > > > > > > +
> > > > > > > + /* 0: success */
> > > > > > > + aml_append(ifctx2, aml_return(zero));
> > > > > > > + }
> > > > > > > + }
> > > > > > > + aml_append(ifctx, ifctx2);
> > > > > > > + }
> > > > > > > + aml_append(method, ifctx);
> > > > > > > }
> > > > > > > +
> > > > > > > aml_append(dev, method);
> > > > > > > }
> > > > > > >
> > > > > > > diff --git a/hw/tpm/tpm_crb.c b/hw/tpm/tpm_crb.c
> > > > > > > index b243222fd6..48f6a716ad 100644
> > > > > > > --- a/hw/tpm/tpm_crb.c
> > > > > > > +++ b/hw/tpm/tpm_crb.c
> > > > > > > @@ -233,6 +233,7 @@ static void tpm_crb_reset(void *dev)
> > > > > > > {
> > > > > > > CRBState *s = CRB(dev);
> > > > > > >
> > > > > > > + tpm_ppi_reset(&s->ppi);
> > > > > > > tpm_backend_reset(s->tpmbe);
> > > > > > >
> > > > > > > memset(s->regs, 0, sizeof(s->regs));
> > > > > > > diff --git a/hw/tpm/tpm_ppi.c b/hw/tpm/tpm_ppi.c
> > > > > > > index 8b46b9dd4b..ce43bc5729 100644
> > > > > > > --- a/hw/tpm/tpm_ppi.c
> > > > > > > +++ b/hw/tpm/tpm_ppi.c
> > > > > > > @@ -16,8 +16,30 @@
> > > > > > > #include "qapi/error.h"
> > > > > > > #include "cpu.h"
> > > > > > > #include "sysemu/memory_mapping.h"
> > > > > > > +#include "sysemu/reset.h"
> > > > > > > #include "migration/vmstate.h"
> > > > > > > #include "tpm_ppi.h"
> > > > > > > +#include "trace.h"
> > > > > > > +
> > > > > > > +void tpm_ppi_reset(TPMPPI *tpmppi)
> > > > > > > +{
> > > > > >
> > > > > >
> > > > > > > + char *ptr = memory_region_get_ram_ptr(&tpmppi->ram);
> > > > > > nvdimm seems to use cpu_physical_memory_read() to access guest
> > > > > > accessible memory, so question is what's difference?
> > > > >
> > > > > cpu_physical_memory_read() is higher level, doing dispatch on address
> > > > > and length checks.
> > > > >
> > > > > This is a bit unnecessary, as ppi->buf could be accessed directly.
> > > > [...]
> > > > > > > + memset(block->host_addr, 0,
> > > > > > > + block->target_end - block->target_start);
> > > > > > > + }
> > > > my concern here is that if we directly touch guest memory here
> > > > we might get in trouble on migration without dirtying modified
> > > > ranges
> > >
> > > It is a read-only of one byte.
> > > by the time the reset handler is called, the memory must have been
> > > already migrated.
> >
> > Looks like a write to me?
>
> the PPI RAM memory is read for the "memory clear" byte
> The whole guest RAM is reset to 0 if set.
Oh, I see; hmm.
How do you avoid zeroing things like persistent memory? Or ROMs? Or EFI
pflash?
> > Also, don't forget that a guest reset can happen during a migration.
>
> Hmm, does cpu_physical_memory_read() guarantee the memory has been migrated?
> Is there a way to wait for migration to be completed in a reset handler?
No; remember that migration can take a significant amount of time (many
minutes) as you stuff many GB of RAM down a network.
So you can be in the situation where:
a) Migration starts
b) Migration sends a copy of most of RAM across
c) Guest dirties lots of RAM in parallel with b
d) migration sends some of the RAM again
e) guest reboots
f) migration keeps sending ram across
g) Migration finally completes and starts on destination
a-f are all happening on the source side as the guest is still running
and doing whatever it wants (including reboots).
Given something like acpi-build.c's acpi_ram_update's call to
memory_region_set_dirty, would that work for you?
Dave
> >
> > Dave
> >
> > > >
> > > > PS:
> > > > feel free it ignore since I don't have a clue what I'm talking about :)
> > > >
> > > > > > > + guest_phys_blocks_free(&guest_phys_blocks);
> > > > > > > + }
> > > > > > > +}
> > > > > > >
> > > > > > > bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
> > > > > > > hwaddr addr, Object *obj, Error **errp)
> > > > > > > @@ -27,5 +49,6 @@ bool tpm_ppi_init(TPMPPI *tpmppi, struct
> > > > > > > MemoryRegion *m,
> > > > > > > vmstate_register_ram(&tpmppi->ram, DEVICE(obj));
> > > > > > >
> > > > > > > memory_region_add_subregion(m, addr, &tpmppi->ram);
> > > > > > > +
> > > > > > > return true;
> > > > > > > }
> > > > > > > diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
> > > > > > > index 70432ffe8b..d9bfa956cc 100644
> > > > > > > --- a/hw/tpm/tpm_tis.c
> > > > > > > +++ b/hw/tpm/tpm_tis.c
> > > > > > > @@ -868,6 +868,7 @@ static void tpm_tis_reset(DeviceState *dev)
> > > > > > > s->be_buffer_size =
> > > > > > > MIN(tpm_backend_get_buffer_size(s->be_driver),
> > > > > > > TPM_TIS_BUFFER_MAX);
> > > > > > >
> > > > > > > + tpm_ppi_reset(&s->ppi);
> > > > > > > tpm_backend_reset(s->be_driver);
> > > > > > >
> > > > > > > s->active_locty = TPM_TIS_NO_LOCALITY;
> > > > > > > diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
> > > > > > > index 332c2ae597..ce9bda3c89 100644
> > > > > > > --- a/docs/specs/tpm.txt
> > > > > > > +++ b/docs/specs/tpm.txt
> > > > > > > @@ -121,6 +121,8 @@ layout:
> > > > > > >
> > > > > > > +----------+--------+--------+-------------------------------------------+
> > > > > > > | next_step| 0x1 | 0x159 | Operation to execute after
> > > > > > > reboot by |
> > > > > > > | | | | firmware. Used by firmware.
> > > > > > > |
> > > > > > > +
> > > > > > > +----------+--------+--------+-------------------------------------------+
> > > > > > > + | movv | 0x1 | 0x15a | Memory overwrite variable
> > > > > > > |
> > > > > > >
> > > > > > > +----------+--------+--------+-------------------------------------------+
> > > > > > >
> > > > > > > The following values are supported for the 'func' field. They
> > > > > > > correspond
> > > > > > > diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
> > > > > > > index 25bee0cecf..920d32ad55 100644
> > > > > > > --- a/hw/tpm/trace-events
> > > > > > > +++ b/hw/tpm/trace-events
> > > > > > > @@ -51,3 +51,6 @@ tpm_tis_mmio_write_init_abort(void) "Initiating
> > > > > > > abort"
> > > > > > > tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
> > > > > > > tpm_tis_mmio_write_data2send(uint32_t value, unsigned size)
> > > > > > > "Data to send to TPM: 0x%08x (size=%d)"
> > > > > > > tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d,
> > > > > > > rw_offset = %u"
> > > > > > > +
> > > > > > > +# hw/tpm/tpm_ppi.c
> > > > > > > +tpm_ppi_memset(uint8_t *ptr, size_t size) "memset: %p %zu"
> > > > > >
> > > > > >
> > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Marc-André Lureau
> > --
> > Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
>
>
>
> --
> Marc-André Lureau
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
