diff mbox series

[v11,6/6] tpm: add ACPI memory clear interface

Message ID 20180906032949.13753-7-marcandre.lureau@redhat.com
State New
Headers show
Series Add support for TPM Physical Presence interface | expand

Commit Message

Marc-André Lureau Sept. 6, 2018, 3:29 a.m. UTC
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.lureau@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(+)

Comments

Stefan Berger Sept. 6, 2018, 8:09 p.m. UTC | #1
On 09/05/2018 11:29 PM, Marc-André Lureau 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.lureau@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 c2ab2ed300..b8f67962c7 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 f2f07f895e..46ca8ea3ea 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);
> +
> +    if (ptr[0x15a] & 0x1) {
> +        GuestPhysBlockList guest_phys_blocks;
> +        GuestPhysBlock *block;
> +
> +        guest_phys_blocks_init(&guest_phys_blocks);
> +        guest_phys_blocks_append(&guest_phys_blocks);
> +        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
> +            trace_tpm_ppi_memset(block->host_addr,
> +                             block->target_end - block->target_start);
> +            memset(block->host_addr, 0,
> +                   block->target_end - block->target_start);

Does this also clear the PPI memory? If so, could we create a backup of 
the few relevant bytes the firmware will look at and restore them after 
the loop? The PPI device likely contains the only data that may need to 
be preserved across a rebbot, and we are running this loop here in the 
ppi module itself, so should be able to do that.

     Stefan
Marc-André Lureau Sept. 7, 2018, 6:01 a.m. UTC | #2
Hi

On Fri, Sep 7, 2018 at 12:09 AM, Stefan Berger
<stefanb@linux.vnet.ibm.com> wrote:
> On 09/05/2018 11:29 PM, Marc-André Lureau 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.lureau@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 c2ab2ed300..b8f67962c7 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 f2f07f895e..46ca8ea3ea 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);
>> +
>> +    if (ptr[0x15a] & 0x1) {
>> +        GuestPhysBlockList guest_phys_blocks;
>> +        GuestPhysBlock *block;
>> +
>> +        guest_phys_blocks_init(&guest_phys_blocks);
>> +        guest_phys_blocks_append(&guest_phys_blocks);
>> +        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
>> +            trace_tpm_ppi_memset(block->host_addr,
>> +                             block->target_end - block->target_start);
>> +            memset(block->host_addr, 0,
>> +                   block->target_end - block->target_start);
>
>
> Does this also clear the PPI memory? If so, could we create a backup of the
> few relevant bytes the firmware will look at and restore them after the
> loop? The PPI device likely contains the only data that may need to be
> preserved across a rebbot, and we are running this loop here in the ppi
> module itself, so should be able to do that.

guest_phys_blocks_append() doesn't gather device ram, so it will skip
TPM PPI buf.

thanks
diff mbox series

Patch

diff --git a/hw/tpm/tpm_ppi.h b/hw/tpm/tpm_ppi.h
index c2ab2ed300..b8f67962c7 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 f2f07f895e..46ca8ea3ea 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);
+
+    if (ptr[0x15a] & 0x1) {
+        GuestPhysBlockList guest_phys_blocks;
+        GuestPhysBlock *block;
+
+        guest_phys_blocks_init(&guest_phys_blocks);
+        guest_phys_blocks_append(&guest_phys_blocks);
+        QTAILQ_FOREACH(block, &guest_phys_blocks.head, next) {
+            trace_tpm_ppi_memset(block->host_addr,
+                             block->target_end - block->target_start);
+            memset(block->host_addr, 0,
+                   block->target_end - block->target_start);
+        }
+        guest_phys_blocks_free(&guest_phys_blocks);
+    }
+}
 
 bool tpm_ppi_init(TPMPPI *tpmppi, struct MemoryRegion *m,
                   hwaddr addr, Object *obj, Error **errp)
@@ -28,5 +50,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"