All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Add support for SEV Launch Secret Injection
@ 2020-05-28 20:51 Tobin Feldman-Fitzthum
  2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
                   ` (6 more replies)
  0 siblings, 7 replies; 13+ messages in thread
From: Tobin Feldman-Fitzthum @ 2020-05-28 20:51 UTC (permalink / raw)
  To: jejb, qemu-devel; +Cc: Tobin Feldman-Fitzthum, tobin

This patchset contains two patches. The first enables QEMU
to facilitate the injection of a secret blob into the guest
memory.

The second enables QEMU to parse the guest ROM to determine
the address at which the secret should be injected.

Tobin Feldman-Fitzthum (2):
  sev: add sev-inject-launch-secret
  sev: scan guest ROM for launch secret address

 include/sysemu/sev.h       |   2 +
 qapi/misc-target.json      |  20 +++++++
 target/i386/monitor.c      |   8 +++
 target/i386/sev-stub.c     |   5 ++
 target/i386/sev.c          | 113 +++++++++++++++++++++++++++++++++++++
 target/i386/sev_i386.h     |  16 ++++++
 target/i386/trace-events   |   1 +
 tests/qtest/qmp-cmd-test.c |   6 +-
 8 files changed, 168 insertions(+), 3 deletions(-)

-- 
2.20.1 (Apple Git-117)



^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/2] sev: add sev-inject-launch-secret
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
@ 2020-05-28 20:51 ` Tobin Feldman-Fitzthum
  2020-05-28 21:00   ` James Bottomley
  2020-05-28 21:42   ` Eric Blake
  2020-05-28 20:51 ` [PATCH 2/2] sev: scan guest ROM for launch secret address Tobin Feldman-Fitzthum
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 13+ messages in thread
From: Tobin Feldman-Fitzthum @ 2020-05-28 20:51 UTC (permalink / raw)
  To: jejb, qemu-devel; +Cc: Tobin Feldman-Fitzthum, tobin

From: Tobin Feldman-Fitzthum <tobin@ibm.com>

AMD SEV allows a guest owner to inject a secret blob
into the memory of a virtual machine. The secret is
encrypted with the SEV Transport Encryption Key and
integrity is guaranteed with the Transport Integrity
Key. Although QEMU faciliates the injection of the
launch secret, it cannot access the secret.

Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com>
---
 include/sysemu/sev.h       |  2 +
 qapi/misc-target.json      | 20 +++++++++
 target/i386/monitor.c      |  8 ++++
 target/i386/sev-stub.c     |  5 +++
 target/i386/sev.c          | 83 ++++++++++++++++++++++++++++++++++++++
 target/i386/trace-events   |  1 +
 tests/qtest/qmp-cmd-test.c |  6 +--
 7 files changed, 122 insertions(+), 3 deletions(-)

diff --git a/include/sysemu/sev.h b/include/sysemu/sev.h
index 98c1ec8d38..313ee30fc8 100644
--- a/include/sysemu/sev.h
+++ b/include/sysemu/sev.h
@@ -18,4 +18,6 @@
 
 void *sev_guest_init(const char *id);
 int sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len);
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+		             uint64_t gpa);
 #endif
diff --git a/qapi/misc-target.json b/qapi/misc-target.json
index dee3b45930..27458b765b 100644
--- a/qapi/misc-target.json
+++ b/qapi/misc-target.json
@@ -200,6 +200,26 @@
 { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
   'if': 'defined(TARGET_I386)' }
 
+##
+# @sev-inject-launch-secret:
+#
+# This command injects a secret blob into memory of SEV guest.
+#
+# @packet-header: the launch secret packet header encoded in base64
+#
+# @secret: the launch secret data to be injected encoded in base64
+#
+# @gpa: the guest physical address where secret will be injected.
+        GPA provided here will be ignored if guest ROM specifies 
+        the a launch secret GPA.
+#
+# Since: 5.0.0
+#
+##
+{ 'command': 'sev-inject-launch-secret',
+  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },
+  'if': 'defined(TARGET_I386)' }
+
 ##
 # @dump-skeys:
 #
diff --git a/target/i386/monitor.c b/target/i386/monitor.c
index 27ebfa3ad2..5c2b7d2c17 100644
--- a/target/i386/monitor.c
+++ b/target/i386/monitor.c
@@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error **errp)
 
     return data;
 }
+
+void qmp_sev_inject_launch_secret(const char *packet_hdr,
+                                  const char *secret, uint64_t gpa,
+                                  Error **errp)
+{
+    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
+      error_setg(errp, "SEV inject secret failed");
+}
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index e5ee13309c..2b8c5f1f53 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
 {
     return NULL;
 }
+int sev_inject_launch_secret(const char *hdr, const char *secret,
+		                             uint64_t gpa)
+{
+	    return 1;
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 846018a12d..774e47d9d1 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -28,6 +28,7 @@
 #include "sysemu/runstate.h"
 #include "trace.h"
 #include "migration/blocker.h"
+#include "exec/address-spaces.h"
 
 #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
@@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
     return 0;
 }
 
+
+static void *
+gpa2hva(hwaddr addr, uint64_t size)
+{
+    MemoryRegionSection mrs = memory_region_find(get_system_memory(),
+                                                 addr, size);
+
+    if (!mrs.mr) {
+        error_report("No memory is mapped at address 0x%" HWADDR_PRIx, addr);
+        return NULL;
+    }
+
+    if (!memory_region_is_ram(mrs.mr) && !memory_region_is_romd(mrs.mr)) {
+        error_report("Memory at address 0x%" HWADDR_PRIx "is not RAM", addr);
+        memory_region_unref(mrs.mr);
+        return NULL;
+    }
+
+    return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region);
+}
+
+int sev_inject_launch_secret(const char *packet_hdr,
+                             const char *secret, uint64_t gpa)
+{
+    struct kvm_sev_launch_secret *input = NULL;
+    guchar *data = NULL, *hdr = NULL;
+    int error, ret = 1;
+    void *hva;
+    gsize hdr_sz = 0, data_sz = 0;
+
+    /* secret can be inject only in this state */
+    if (!sev_check_state(SEV_STATE_LAUNCH_SECRET)) {
+	error_report("Not in correct state. %x",sev_state->state);
+	return 1;
+    }
+
+    hdr = g_base64_decode(packet_hdr, &hdr_sz);
+    if (!hdr || !hdr_sz) {
+        error_report("SEV: Failed to decode sequence header");
+        return 1;
+    }
+
+    data = g_base64_decode(secret, &data_sz);
+    if (!data || !data_sz) {
+        error_report("SEV: Failed to decode data");
+        goto err;
+    }
+
+    hva = gpa2hva(gpa, data_sz);
+    if (!hva) {
+        goto err;
+    }
+    input = g_new0(struct kvm_sev_launch_secret, 1);
+
+    input->hdr_uaddr = (unsigned long)hdr;
+    input->hdr_len = hdr_sz;
+
+    input->trans_uaddr = (unsigned long)data;
+    input->trans_len = data_sz;
+
+    input->guest_uaddr = (unsigned long)hva;
+    input->guest_len = data_sz;
+
+    trace_kvm_sev_launch_secret(gpa, input->guest_uaddr,
+                                input->trans_uaddr, input->trans_len);
+
+    ret = sev_ioctl(sev_state->sev_fd,KVM_SEV_LAUNCH_SECRET, input, &error);
+    if (ret) {
+        error_report("SEV: failed to inject secret ret=%d fw_error=%d '%s'",
+                     ret, error, fw_error_to_str(error));
+        goto err;
+    }
+
+    ret = 0;
+
+err:
+    g_free(data);
+    g_free(hdr);
+    g_free(input);
+    return ret;
+}
+
 static void
 sev_register_types(void)
 {
diff --git a/target/i386/trace-events b/target/i386/trace-events
index 789c700d4a..9f299e94a2 100644
--- a/target/i386/trace-events
+++ b/target/i386/trace-events
@@ -15,3 +15,4 @@ kvm_sev_launch_start(int policy, void *session, void *pdh) "policy 0x%x session
 kvm_sev_launch_update_data(void *addr, uint64_t len) "addr %p len 0x%" PRIu64
 kvm_sev_launch_measurement(const char *value) "data %s"
 kvm_sev_launch_finish(void) ""
+kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d"
diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c
index 9f5228cd99..50b2b42830 100644
--- a/tests/qtest/qmp-cmd-test.c
+++ b/tests/qtest/qmp-cmd-test.c
@@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd)
         /* Success depends on target-specific build configuration: */
         "query-pci",              /* CONFIG_PCI */
         /* Success depends on launching SEV guest */
-        "query-sev-launch-measure",
+        // "query-sev-launch-measure",
         /* Success depends on Host or Hypervisor SEV support */
-        "query-sev",
-        "query-sev-capabilities",
+        // "query-sev",
+        // "query-sev-capabilities",
         NULL
     };
     int i;
-- 
2.20.1 (Apple Git-117)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] sev: scan guest ROM for launch secret address
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
  2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
@ 2020-05-28 20:51 ` Tobin Feldman-Fitzthum
  2020-05-29 19:08   ` Tom Lendacky
  2020-05-29  3:32 ` [PATCH 0/2] Add support for SEV Launch Secret Injection no-reply
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 13+ messages in thread
From: Tobin Feldman-Fitzthum @ 2020-05-28 20:51 UTC (permalink / raw)
  To: jejb, qemu-devel; +Cc: Tobin Feldman-Fitzthum, tobin

From: Tobin Feldman-Fitzthum <tobin@ibm.com>

In addition to using QMP to provide the guest memory address
that the launch secret blob will be injected into, the
secret address can also be specified in the guest ROM. This
patch adds sev_find_secret_gpa, which scans the ROM page by
page to find a launch secret table identified by a GUID. If
the table is found, the address it contains will be used
in place of any address specified via QMP.

Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com>
---
 target/i386/sev.c      | 34 ++++++++++++++++++++++++++++++++--
 target/i386/sev_i386.h | 16 ++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 774e47d9d1..4adc56d7e3 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -706,6 +706,8 @@ sev_guest_init(const char *id)
     s->api_major = status.api_major;
     s->api_minor = status.api_minor;
 
+    s->secret_gpa = 0;
+
     trace_kvm_sev_init();
     ret = sev_ioctl(s->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
     if (ret) {
@@ -731,6 +733,28 @@ err:
     return NULL;
 }
 
+static void
+sev_find_secret_gpa(uint8_t *ptr, uint64_t len)
+{
+    uint64_t offset;
+
+    SevROMSecretTable *secret_table;
+    QemuUUID secret_table_guid;
+
+    qemu_uuid_parse(SEV_ROM_SECRET_GUID,&secret_table_guid);
+    secret_table_guid = qemu_uuid_bswap(secret_table_guid);
+
+    offset = len - 0x1000;
+    while(offset > 0) {
+        secret_table = (SevROMSecretTable *)(ptr + offset);
+        if(qemu_uuid_is_equal(&secret_table_guid, (QemuUUID *) secret_table)){
+            sev_state->secret_gpa = (long unsigned int) secret_table->base;
+            break;
+        }
+        offset -= 0x1000;
+    }
+}
+
 int
 sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 {
@@ -738,6 +762,9 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
 
     /* if SEV is in update state then encrypt the data else do nothing */
     if (sev_check_state(SEV_STATE_LAUNCH_UPDATE)) {
+        if(!sev_state->secret_gpa) {
+            sev_find_secret_gpa(ptr, len);
+	    }
         return sev_launch_update_data(ptr, len);
     }
 
@@ -776,8 +803,8 @@ int sev_inject_launch_secret(const char *packet_hdr,
 
     /* secret can be inject only in this state */
     if (!sev_check_state(SEV_STATE_LAUNCH_SECRET)) {
-	error_report("Not in correct state. %x",sev_state->state);
-	return 1;
+        error_report("Not in correct state. %x",sev_state->state);
+        return 1;
     }
 
     hdr = g_base64_decode(packet_hdr, &hdr_sz);
@@ -792,6 +819,9 @@ int sev_inject_launch_secret(const char *packet_hdr,
         goto err;
     }
 
+    if(sev_state->secret_gpa)
+        gpa = sev_state->secret_gpa;
+
     hva = gpa2hva(gpa, data_sz);
     if (!hva) {
         goto err;
diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index 8ada9d385d..b1f9ab93bb 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -19,6 +19,7 @@
 #include "sysemu/kvm.h"
 #include "sysemu/sev.h"
 #include "qemu/error-report.h"
+#include "qemu/uuid.h"
 #include "qapi/qapi-types-misc-target.h"
 
 #define SEV_POLICY_NODBG        0x1
@@ -28,6 +29,8 @@
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
+#define SEV_ROM_SECRET_GUID "adf956ad-e98c-484c-ae11-b51c7d336447"
+
 #define TYPE_QSEV_GUEST_INFO "sev-guest"
 #define QSEV_GUEST_INFO(obj)                  \
     OBJECT_CHECK(QSevGuestInfo, (obj), TYPE_QSEV_GUEST_INFO)
@@ -42,6 +45,18 @@ extern SevCapability *sev_get_capabilities(void);
 
 typedef struct QSevGuestInfo QSevGuestInfo;
 typedef struct QSevGuestInfoClass QSevGuestInfoClass;
+typedef struct SevROMSecretTable SevROMSecretTable;
+
+/**
+ * If guest physical address for the launch secret is
+ * provided in the ROM, it should be in the following
+ * page-aligned structure.
+ */
+struct SevROMSecretTable {
+    QemuUUID guid;
+    unsigned int base;
+    unsigned int size;
+};
 
 /**
  * QSevGuestInfo:
@@ -78,6 +93,7 @@ struct SEVState {
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
     uint32_t handle;
+    uint64_t secret_gpa;
     int sev_fd;
     SevState state;
     gchar *measurement;
-- 
2.20.1 (Apple Git-117)



^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] sev: add sev-inject-launch-secret
  2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
@ 2020-05-28 21:00   ` James Bottomley
  2020-05-29 18:09     ` tobin
  2020-05-28 21:42   ` Eric Blake
  1 sibling, 1 reply; 13+ messages in thread
From: James Bottomley @ 2020-05-28 21:00 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, qemu-devel; +Cc: tobin

On Thu, 2020-05-28 at 16:51 -0400, Tobin Feldman-Fitzthum wrote:
> --- a/qapi/misc-target.json
> +++ b/qapi/misc-target.json
> @@ -200,6 +200,26 @@
>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>    'if': 'defined(TARGET_I386)' }
>  
> +##
> +# @sev-inject-launch-secret:
> +#
> +# This command injects a secret blob into memory of SEV guest.
> +#
> +# @packet-header: the launch secret packet header encoded in base64
> +#
> +# @secret: the launch secret data to be injected encoded in base64
> +#
> +# @gpa: the guest physical address where secret will be injected.
> +        GPA provided here will be ignored if guest ROM specifies 
> +        the a launch secret GPA.

Shouldn't we eliminate the gpa argument to this now the gpa is
extracted from OVMF?  You add it here but don't take it out in the next
patch.

> +# Since: 5.0.0
> +#
> +##
> +{ 'command': 'sev-inject-launch-secret',
> +  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },

Java (i.e. Json) people hate underscores and abbreviations.  I bet
they'll want this to be 'packet-header'

> +  'if': 'defined(TARGET_I386)' }
> +
>  ##
>  # @dump-skeys:
>  #
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 27ebfa3ad2..5c2b7d2c17 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error
> **errp)
>  
>      return data;
>  }
> +
> +void qmp_sev_inject_launch_secret(const char *packet_hdr,
> +                                  const char *secret, uint64_t gpa,
> +                                  Error **errp)
> +{
> +    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
> +      error_setg(errp, "SEV inject secret failed");
> +}
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index e5ee13309c..2b8c5f1f53 100644
> --- a/target/i386/sev-stub.c
> +++ b/target/i386/sev-stub.c
> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
>  {
>      return NULL;
>  }
> +int sev_inject_launch_secret(const char *hdr, const char *secret,
> +		                             uint64_t gpa)
> +{
> +	    return 1;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 846018a12d..774e47d9d1 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/runstate.h"
>  #include "trace.h"
>  #include "migration/blocker.h"
> +#include "exec/address-spaces.h"
>  
>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
> @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr,
> uint64_t len)
>      return 0;
>  }
>  
> +
> +static void *
> +gpa2hva(hwaddr addr, uint64_t size)
> +{
> +    MemoryRegionSection mrs =
> memory_region_find(get_system_memory(),
> +                                                 addr, size);
> +
> +    if (!mrs.mr) {
> +        error_report("No memory is mapped at address 0x%"
> HWADDR_PRIx, addr);
> +        return NULL;
> +    }
> +
> +    if (!memory_region_is_ram(mrs.mr) &&
> !memory_region_is_romd(mrs.mr)) {
> +        error_report("Memory at address 0x%" HWADDR_PRIx "is not
> RAM", addr);
> +        memory_region_unref(mrs.mr);
> +        return NULL;
> +    }

We can still check this, but it should be like an assertion failure. 
Since the GPA is selected by the OVMF build there should be no way it
can't be mapped into the host.

[...]
> --- a/tests/qtest/qmp-cmd-test.c
> +++ b/tests/qtest/qmp-cmd-test.c
> @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd)
>          /* Success depends on target-specific build configuration:
> */
>          "query-pci",              /* CONFIG_PCI */
>          /* Success depends on launching SEV guest */
> -        "query-sev-launch-measure",
> +        // "query-sev-launch-measure",
>          /* Success depends on Host or Hypervisor SEV support */
> -        "query-sev",
> -        "query-sev-capabilities",
> +        // "query-sev",
> +        // "query-sev-capabilities",

We're eliminating existing tests ... is that just a stray hunk that you
forgot to remove?

James



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] sev: add sev-inject-launch-secret
  2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
  2020-05-28 21:00   ` James Bottomley
@ 2020-05-28 21:42   ` Eric Blake
  2020-05-29 18:04     ` tobin
  1 sibling, 1 reply; 13+ messages in thread
From: Eric Blake @ 2020-05-28 21:42 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, jejb, qemu-devel; +Cc: tobin

On 5/28/20 3:51 PM, Tobin Feldman-Fitzthum wrote:
> From: Tobin Feldman-Fitzthum <tobin@ibm.com>
> 
> AMD SEV allows a guest owner to inject a secret blob
> into the memory of a virtual machine. The secret is
> encrypted with the SEV Transport Encryption Key and
> integrity is guaranteed with the Transport Integrity
> Key. Although QEMU faciliates the injection of the
> launch secret, it cannot access the secret.
> 
> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com>
> ---

> +++ b/qapi/misc-target.json
> @@ -200,6 +200,26 @@
>   { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>     'if': 'defined(TARGET_I386)' }
>   
> +##
> +# @sev-inject-launch-secret:
> +#
> +# This command injects a secret blob into memory of SEV guest.
> +#
> +# @packet-header: the launch secret packet header encoded in base64
> +#
> +# @secret: the launch secret data to be injected encoded in base64
> +#
> +# @gpa: the guest physical address where secret will be injected.
> +        GPA provided here will be ignored if guest ROM specifies
> +        the a launch secret GPA.

Missing # on the wrapped lines.

> +#
> +# Since: 5.0.0

You've missed 5.0, and more sites tend to use x.y instead of x.y.z 
(although we aren't consistent); this should be 'Since: 5.1'

> +#
> +##
> +{ 'command': 'sev-inject-launch-secret',
> +  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },

This does not match your documentation above, which named it 
'packet-header'.  Should 'gpa' be optional, to account for the case 
where ROM specifies it?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] Add support for SEV Launch Secret Injection
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
  2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
  2020-05-28 20:51 ` [PATCH 2/2] sev: scan guest ROM for launch secret address Tobin Feldman-Fitzthum
@ 2020-05-29  3:32 ` no-reply
  2020-05-29  3:35 ` no-reply
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2020-05-29  3:32 UTC (permalink / raw)
  To: tobin; +Cc: tobin, jejb, tobin, qemu-devel

Patchew URL: https://patchew.org/QEMU/20200528205114.42078-1-tobin@linux.vnet.ibm.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN     ui/input-keymap-qnum-to-qcode.c
In file included from /tmp/qemu-test/src/qapi/qapi-schema.json:85:
/tmp/qemu-test/src/qapi/misc-target.json:213:9: stray 'GPA'
make: *** [qapi-gen-timestamp] Error 1
make: *** Waiting for unfinished jobs....
  CC      /tmp/qemu-test/build/slirp/src/slirp.o
  CC      /tmp/qemu-test/build/slirp/src/vmstate.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=6e1594b856a84baabe3c89fab85fce17', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-yd1xv0uz/src/docker-src.2020-05-28-23.30.04.14959:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=6e1594b856a84baabe3c89fab85fce17
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-yd1xv0uz/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    1m59.216s
user    0m7.852s


The full log is available at
http://patchew.org/logs/20200528205114.42078-1-tobin@linux.vnet.ibm.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] Add support for SEV Launch Secret Injection
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
                   ` (2 preceding siblings ...)
  2020-05-29  3:32 ` [PATCH 0/2] Add support for SEV Launch Secret Injection no-reply
@ 2020-05-29  3:35 ` no-reply
  2020-05-29  3:36 ` no-reply
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2020-05-29  3:35 UTC (permalink / raw)
  To: tobin; +Cc: tobin, jejb, tobin, qemu-devel

Patchew URL: https://patchew.org/QEMU/20200528205114.42078-1-tobin@linux.vnet.ibm.com/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN     scsi/trace.h
  GEN     audio/trace.h
  CC      /tmp/qemu-test/build/slirp/src/tcp_output.o
make: *** [Makefile:666: qapi-gen-timestamp] Error 1
make: *** Waiting for unfinished jobs....
  CC      /tmp/qemu-test/build/slirp/src/ndp_table.o
  CC      /tmp/qemu-test/build/slirp/src/bootp.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=93d79e62908146289998366473c102a3', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-bnxinu3b/src/docker-src.2020-05-28-23.32.39.19459:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=93d79e62908146289998366473c102a3
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-bnxinu3b/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    3m13.106s
user    0m8.085s


The full log is available at
http://patchew.org/logs/20200528205114.42078-1-tobin@linux.vnet.ibm.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] Add support for SEV Launch Secret Injection
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
                   ` (3 preceding siblings ...)
  2020-05-29  3:35 ` no-reply
@ 2020-05-29  3:36 ` no-reply
  2020-05-29  3:39 ` no-reply
  2020-06-01 18:51 ` Dr. David Alan Gilbert
  6 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2020-05-29  3:36 UTC (permalink / raw)
  To: tobin; +Cc: tobin, jejb, tobin, qemu-devel

Patchew URL: https://patchew.org/QEMU/20200528205114.42078-1-tobin@linux.vnet.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200528205114.42078-1-tobin@linux.vnet.ibm.com
Subject: [PATCH 0/2] Add support for SEV Launch Secret Injection
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
fefbf6f sev: scan guest ROM for launch secret address
94d7e7b sev: add sev-inject-launch-secret

=== OUTPUT BEGIN ===
1/2 Checking commit 94d7e7bc7c3c (sev: add sev-inject-launch-secret)
ERROR: code indent should never use tabs
#26: FILE: include/sysemu/sev.h:22:
+^I^I             uint64_t gpa);$

ERROR: trailing whitespace
#45: FILE: qapi/misc-target.json:213:
+        GPA provided here will be ignored if guest ROM specifies $

ERROR: suspect code indent for conditional statements (4, 6)
#72: FILE: target/i386/monitor.c:744:
+    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
+      error_setg(errp, "SEV inject secret failed");

ERROR: space required after that ',' (ctx:VxV)
#72: FILE: target/i386/monitor.c:744:
+    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
                                            ^

ERROR: space required after that ',' (ctx:VxV)
#72: FILE: target/i386/monitor.c:744:
+    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
                                                   ^

ERROR: braces {} are necessary for all arms of this statement
#72: FILE: target/i386/monitor.c:744:
+    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
[...]

ERROR: code indent should never use tabs
#84: FILE: target/i386/sev-stub.c:52:
+^I^I                             uint64_t gpa)$

ERROR: code indent should never use tabs
#86: FILE: target/i386/sev-stub.c:54:
+^I    return 1;$

ERROR: code indent should never use tabs
#136: FILE: target/i386/sev.c:776:
+^Ierror_report("Not in correct state. %x",sev_state->state);$

ERROR: space required after that ',' (ctx:VxV)
#136: FILE: target/i386/sev.c:776:
+       error_report("Not in correct state. %x",sev_state->state);
                                               ^

ERROR: code indent should never use tabs
#137: FILE: target/i386/sev.c:777:
+^Ireturn 1;$

ERROR: space required after that ',' (ctx:VxV)
#170: FILE: target/i386/sev.c:810:
+    ret = sev_ioctl(sev_state->sev_fd,KVM_SEV_LAUNCH_SECRET, input, &error);
                                      ^

ERROR: do not use C99 // comments
#207: FILE: tests/qtest/qmp-cmd-test.c:96:
+        // "query-sev-launch-measure",

ERROR: do not use C99 // comments
#211: FILE: tests/qtest/qmp-cmd-test.c:98:
+        // "query-sev",

ERROR: do not use C99 // comments
#212: FILE: tests/qtest/qmp-cmd-test.c:99:
+        // "query-sev-capabilities",

total: 15 errors, 0 warnings, 163 lines checked

Patch 1/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/2 Checking commit fefbf6f8855c (sev: scan guest ROM for launch secret address)
ERROR: space required after that ',' (ctx:VxO)
#43: FILE: target/i386/sev.c:741:
+    qemu_uuid_parse(SEV_ROM_SECRET_GUID,&secret_table_guid);
                                        ^

ERROR: space required before that '&' (ctx:OxV)
#43: FILE: target/i386/sev.c:741:
+    qemu_uuid_parse(SEV_ROM_SECRET_GUID,&secret_table_guid);
                                         ^

ERROR: space required before the open parenthesis '('
#47: FILE: target/i386/sev.c:745:
+    while(offset > 0) {

ERROR: space required before the open brace '{'
#49: FILE: target/i386/sev.c:747:
+        if(qemu_uuid_is_equal(&secret_table_guid, (QemuUUID *) secret_table)){

ERROR: space required before the open parenthesis '('
#49: FILE: target/i386/sev.c:747:
+        if(qemu_uuid_is_equal(&secret_table_guid, (QemuUUID *) secret_table)){

ERROR: space required before the open parenthesis '('
#64: FILE: target/i386/sev.c:762:
+        if(!sev_state->secret_gpa) {

ERROR: code indent should never use tabs
#66: FILE: target/i386/sev.c:764:
+^I    }$

ERROR: space required after that ',' (ctx:VxV)
#76: FILE: target/i386/sev.c:803:
+        error_report("Not in correct state. %x",sev_state->state);
                                                ^

ERROR: space required before the open parenthesis '('
#85: FILE: target/i386/sev.c:819:
+    if(sev_state->secret_gpa)

ERROR: braces {} are necessary for all arms of this statement
#85: FILE: target/i386/sev.c:819:
+    if(sev_state->secret_gpa)
[...]

total: 10 errors, 0 warnings, 104 lines checked

Patch 2/2 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200528205114.42078-1-tobin@linux.vnet.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] Add support for SEV Launch Secret Injection
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
                   ` (4 preceding siblings ...)
  2020-05-29  3:36 ` no-reply
@ 2020-05-29  3:39 ` no-reply
  2020-06-01 18:51 ` Dr. David Alan Gilbert
  6 siblings, 0 replies; 13+ messages in thread
From: no-reply @ 2020-05-29  3:39 UTC (permalink / raw)
  To: tobin; +Cc: tobin, jejb, tobin, qemu-devel

Patchew URL: https://patchew.org/QEMU/20200528205114.42078-1-tobin@linux.vnet.ibm.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  GEN     ui/input-keymap-xorgxquartz-to-qcode.c
In file included from /tmp/qemu-test/src/qapi/qapi-schema.json:85:
/tmp/qemu-test/src/qapi/misc-target.json:213:9: stray 'GPA'
make: *** [Makefile:666: qapi-gen-timestamp] Error 1
make: *** Waiting for unfinished jobs....
  CC      /tmp/qemu-test/build/slirp/src/ip6_icmp.o
  CC      /tmp/qemu-test/build/slirp/src/slirp.o
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=cb62fe08a707401d8f3632cb951681ac', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-h6j9yyx9/src/docker-src.2020-05-28-23.37.24.24496:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=cb62fe08a707401d8f3632cb951681ac
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-h6j9yyx9/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m8.174s
user    0m8.497s


The full log is available at
http://patchew.org/logs/20200528205114.42078-1-tobin@linux.vnet.ibm.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] sev: add sev-inject-launch-secret
  2020-05-28 21:42   ` Eric Blake
@ 2020-05-29 18:04     ` tobin
  0 siblings, 0 replies; 13+ messages in thread
From: tobin @ 2020-05-29 18:04 UTC (permalink / raw)
  To: Eric Blake; +Cc: jejb, tobin, qemu-devel

On 2020-05-28 17:42, Eric Blake wrote:
> On 5/28/20 3:51 PM, Tobin Feldman-Fitzthum wrote:
>> From: Tobin Feldman-Fitzthum <tobin@ibm.com>
>> 
>> AMD SEV allows a guest owner to inject a secret blob
>> into the memory of a virtual machine. The secret is
>> encrypted with the SEV Transport Encryption Key and
>> integrity is guaranteed with the Transport Integrity
>> Key. Although QEMU faciliates the injection of the
>> launch secret, it cannot access the secret.
>> 
>> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com>
>> ---
> 
>> +++ b/qapi/misc-target.json
>> @@ -200,6 +200,26 @@
>>   { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>>     'if': 'defined(TARGET_I386)' }
>>   +##
>> +# @sev-inject-launch-secret:
>> +#
>> +# This command injects a secret blob into memory of SEV guest.
>> +#
>> +# @packet-header: the launch secret packet header encoded in base64
>> +#
>> +# @secret: the launch secret data to be injected encoded in base64
>> +#
>> +# @gpa: the guest physical address where secret will be injected.
>> +        GPA provided here will be ignored if guest ROM specifies
>> +        the a launch secret GPA.
> 
> Missing # on the wrapped lines.
> 
>> +#
>> +# Since: 5.0.0
> 
> You've missed 5.0, and more sites tend to use x.y instead of x.y.z
> (although we aren't consistent); this should be 'Since: 5.1'
> 
>> +#
>> +##
>> +{ 'command': 'sev-inject-launch-secret',
>> +  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },
> 
> This does not match your documentation above, which named it
> 'packet-header'.  Should 'gpa' be optional, to account for the case
> where ROM specifies it?

My bad on the syntax issues. I think making GPA optional makes sense.
In the first patch we can have it be required and in the second
we add the option to scan the ROM.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] sev: add sev-inject-launch-secret
  2020-05-28 21:00   ` James Bottomley
@ 2020-05-29 18:09     ` tobin
  0 siblings, 0 replies; 13+ messages in thread
From: tobin @ 2020-05-29 18:09 UTC (permalink / raw)
  To: jejb; +Cc: tobin, qemu-devel

On 2020-05-28 17:00, James Bottomley wrote:
> On Thu, 2020-05-28 at 16:51 -0400, Tobin Feldman-Fitzthum wrote:
>> --- a/qapi/misc-target.json
>> +++ b/qapi/misc-target.json
>> @@ -200,6 +200,26 @@
>>  { 'command': 'query-sev-capabilities', 'returns': 'SevCapability',
>>    'if': 'defined(TARGET_I386)' }
>> 
>> +##
>> +# @sev-inject-launch-secret:
>> +#
>> +# This command injects a secret blob into memory of SEV guest.
>> +#
>> +# @packet-header: the launch secret packet header encoded in base64
>> +#
>> +# @secret: the launch secret data to be injected encoded in base64
>> +#
>> +# @gpa: the guest physical address where secret will be injected.
>> +        GPA provided here will be ignored if guest ROM specifies
>> +        the a launch secret GPA.
> 
> Shouldn't we eliminate the gpa argument to this now the gpa is
> extracted from OVMF?  You add it here but don't take it out in the next
> patch.
> 
I think having GPA as an optional argument might make the most sense.
Users may or may not know how to use the argument, but it is probably
a good idea to give another option besides sticking the GPA into the 
ROM.

>> +# Since: 5.0.0
>> +#
>> +##
>> +{ 'command': 'sev-inject-launch-secret',
>> +  'data': { 'packet_hdr': 'str', 'secret': 'str', 'gpa': 'uint64' },
> 
> Java (i.e. Json) people hate underscores and abbreviations.  I bet
> they'll want this to be 'packet-header'
> 
Happy to change this.

>> +  'if': 'defined(TARGET_I386)' }
>> +
>>  ##
>>  # @dump-skeys:
>>  #
>> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
>> index 27ebfa3ad2..5c2b7d2c17 100644
>> --- a/target/i386/monitor.c
>> +++ b/target/i386/monitor.c
>> @@ -736,3 +736,11 @@ SevCapability *qmp_query_sev_capabilities(Error
>> **errp)
>> 
>>      return data;
>>  }
>> +
>> +void qmp_sev_inject_launch_secret(const char *packet_hdr,
>> +                                  const char *secret, uint64_t gpa,
>> +                                  Error **errp)
>> +{
>> +    if (sev_inject_launch_secret(packet_hdr,secret,gpa) != 0)
>> +      error_setg(errp, "SEV inject secret failed");
>> +}
>> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
>> index e5ee13309c..2b8c5f1f53 100644
>> --- a/target/i386/sev-stub.c
>> +++ b/target/i386/sev-stub.c
>> @@ -48,3 +48,8 @@ SevCapability *sev_get_capabilities(void)
>>  {
>>      return NULL;
>>  }
>> +int sev_inject_launch_secret(const char *hdr, const char *secret,
>> +		                             uint64_t gpa)
>> +{
>> +	    return 1;
>> +}
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 846018a12d..774e47d9d1 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -28,6 +28,7 @@
>>  #include "sysemu/runstate.h"
>>  #include "trace.h"
>>  #include "migration/blocker.h"
>> +#include "exec/address-spaces.h"
>> 
>>  #define DEFAULT_GUEST_POLICY    0x1 /* disable debug */
>>  #define DEFAULT_SEV_DEVICE      "/dev/sev"
>> @@ -743,6 +744,88 @@ sev_encrypt_data(void *handle, uint8_t *ptr,
>> uint64_t len)
>>      return 0;
>>  }
>> 
>> +
>> +static void *
>> +gpa2hva(hwaddr addr, uint64_t size)
>> +{
>> +    MemoryRegionSection mrs =
>> memory_region_find(get_system_memory(),
>> +                                                 addr, size);
>> +
>> +    if (!mrs.mr) {
>> +        error_report("No memory is mapped at address 0x%"
>> HWADDR_PRIx, addr);
>> +        return NULL;
>> +    }
>> +
>> +    if (!memory_region_is_ram(mrs.mr) &&
>> !memory_region_is_romd(mrs.mr)) {
>> +        error_report("Memory at address 0x%" HWADDR_PRIx "is not
>> RAM", addr);
>> +        memory_region_unref(mrs.mr);
>> +        return NULL;
>> +    }
> 
> We can still check this, but it should be like an assertion failure.
> Since the GPA is selected by the OVMF build there should be no way it
> can't be mapped into the host.
> 
> [...]
>> --- a/tests/qtest/qmp-cmd-test.c
>> +++ b/tests/qtest/qmp-cmd-test.c
>> @@ -93,10 +93,10 @@ static bool query_is_blacklisted(const char *cmd)
>>          /* Success depends on target-specific build configuration:
>> */
>>          "query-pci",              /* CONFIG_PCI */
>>          /* Success depends on launching SEV guest */
>> -        "query-sev-launch-measure",
>> +        // "query-sev-launch-measure",
>>          /* Success depends on Host or Hypervisor SEV support */
>> -        "query-sev",
>> -        "query-sev-capabilities",
>> +        // "query-sev",
>> +        // "query-sev-capabilities",
> 
> We're eliminating existing tests ... is that just a stray hunk that you
> forgot to remove?
> 
Yes.
> James


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] sev: scan guest ROM for launch secret address
  2020-05-28 20:51 ` [PATCH 2/2] sev: scan guest ROM for launch secret address Tobin Feldman-Fitzthum
@ 2020-05-29 19:08   ` Tom Lendacky
  0 siblings, 0 replies; 13+ messages in thread
From: Tom Lendacky @ 2020-05-29 19:08 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, jejb, qemu-devel; +Cc: Brijesh Singh, tobin

On 5/28/20 3:51 PM, Tobin Feldman-Fitzthum wrote:
> From: Tobin Feldman-Fitzthum <tobin@ibm.com>
> 
> In addition to using QMP to provide the guest memory address
> that the launch secret blob will be injected into, the
> secret address can also be specified in the guest ROM. This
> patch adds sev_find_secret_gpa, which scans the ROM page by
> page to find a launch secret table identified by a GUID. If
> the table is found, the address it contains will be used
> in place of any address specified via QMP.

I'm working on something similar for SEV-ES support in OVMF (see 
https://www.mail-archive.com/devel@edk2.groups.io/msg20716.html). The GUID 
is placed at a fixed location from the end of the ROM. One of the OVMF 
maintainers recommended the approach and I think we should work to support 
the guest LAUNCH SECRET GPA using the same GUID. This particular patch 
should be delayed until an OVMF method is accepted, so that it doesn't 
have to be reworked.

Thanks,
Tom

> 
> Signed-off-by: Tobin Feldman-Fitzthum <tobin@linux.vnet.ibm.com>
> ---
>   target/i386/sev.c      | 34 ++++++++++++++++++++++++++++++++--
>   target/i386/sev_i386.h | 16 ++++++++++++++++
>   2 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 774e47d9d1..4adc56d7e3 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -706,6 +706,8 @@ sev_guest_init(const char *id)
>       s->api_major = status.api_major;
>       s->api_minor = status.api_minor;
>   
> +    s->secret_gpa = 0;
> +
>       trace_kvm_sev_init();
>       ret = sev_ioctl(s->sev_fd, KVM_SEV_INIT, NULL, &fw_error);
>       if (ret) {
> @@ -731,6 +733,28 @@ err:
>       return NULL;
>   }
>   
> +static void
> +sev_find_secret_gpa(uint8_t *ptr, uint64_t len)
> +{
> +    uint64_t offset;
> +
> +    SevROMSecretTable *secret_table;
> +    QemuUUID secret_table_guid;
> +
> +    qemu_uuid_parse(SEV_ROM_SECRET_GUID,&secret_table_guid);
> +    secret_table_guid = qemu_uuid_bswap(secret_table_guid);
> +
> +    offset = len - 0x1000;
> +    while(offset > 0) {
> +        secret_table = (SevROMSecretTable *)(ptr + offset);
> +        if(qemu_uuid_is_equal(&secret_table_guid, (QemuUUID *) secret_table)){
> +            sev_state->secret_gpa = (long unsigned int) secret_table->base;
> +            break;
> +        }
> +        offset -= 0x1000;
> +    }
> +}
> +
>   int
>   sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
>   {
> @@ -738,6 +762,9 @@ sev_encrypt_data(void *handle, uint8_t *ptr, uint64_t len)
>   
>       /* if SEV is in update state then encrypt the data else do nothing */
>       if (sev_check_state(SEV_STATE_LAUNCH_UPDATE)) {
> +        if(!sev_state->secret_gpa) {
> +            sev_find_secret_gpa(ptr, len);
> +	    }
>           return sev_launch_update_data(ptr, len);
>       }
>   
> @@ -776,8 +803,8 @@ int sev_inject_launch_secret(const char *packet_hdr,
>   
>       /* secret can be inject only in this state */
>       if (!sev_check_state(SEV_STATE_LAUNCH_SECRET)) {
> -	error_report("Not in correct state. %x",sev_state->state);
> -	return 1;
> +        error_report("Not in correct state. %x",sev_state->state);
> +        return 1;
>       }
>   
>       hdr = g_base64_decode(packet_hdr, &hdr_sz);
> @@ -792,6 +819,9 @@ int sev_inject_launch_secret(const char *packet_hdr,
>           goto err;
>       }
>   
> +    if(sev_state->secret_gpa)
> +        gpa = sev_state->secret_gpa;
> +
>       hva = gpa2hva(gpa, data_sz);
>       if (!hva) {
>           goto err;
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index 8ada9d385d..b1f9ab93bb 100644
> --- a/target/i386/sev_i386.h
> +++ b/target/i386/sev_i386.h
> @@ -19,6 +19,7 @@
>   #include "sysemu/kvm.h"
>   #include "sysemu/sev.h"
>   #include "qemu/error-report.h"
> +#include "qemu/uuid.h"
>   #include "qapi/qapi-types-misc-target.h"
>   
>   #define SEV_POLICY_NODBG        0x1
> @@ -28,6 +29,8 @@
>   #define SEV_POLICY_DOMAIN       0x10
>   #define SEV_POLICY_SEV          0x20
>   
> +#define SEV_ROM_SECRET_GUID "adf956ad-e98c-484c-ae11-b51c7d336447"
> +
>   #define TYPE_QSEV_GUEST_INFO "sev-guest"
>   #define QSEV_GUEST_INFO(obj)                  \
>       OBJECT_CHECK(QSevGuestInfo, (obj), TYPE_QSEV_GUEST_INFO)
> @@ -42,6 +45,18 @@ extern SevCapability *sev_get_capabilities(void);
>   
>   typedef struct QSevGuestInfo QSevGuestInfo;
>   typedef struct QSevGuestInfoClass QSevGuestInfoClass;
> +typedef struct SevROMSecretTable SevROMSecretTable;
> +
> +/**
> + * If guest physical address for the launch secret is
> + * provided in the ROM, it should be in the following
> + * page-aligned structure.
> + */
> +struct SevROMSecretTable {
> +    QemuUUID guid;
> +    unsigned int base;
> +    unsigned int size;
> +};
>   
>   /**
>    * QSevGuestInfo:
> @@ -78,6 +93,7 @@ struct SEVState {
>       uint32_t cbitpos;
>       uint32_t reduced_phys_bits;
>       uint32_t handle;
> +    uint64_t secret_gpa;
>       int sev_fd;
>       SevState state;
>       gchar *measurement;
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/2] Add support for SEV Launch Secret Injection
  2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
                   ` (5 preceding siblings ...)
  2020-05-29  3:39 ` no-reply
@ 2020-06-01 18:51 ` Dr. David Alan Gilbert
  6 siblings, 0 replies; 13+ messages in thread
From: Dr. David Alan Gilbert @ 2020-06-01 18:51 UTC (permalink / raw)
  To: Tobin Feldman-Fitzthum, brijesh.singh, pbonzini; +Cc: jejb, tobin, qemu-devel

cc'ing in Brijesh for SEV stuff, and also Paolo.

* Tobin Feldman-Fitzthum (tobin@linux.vnet.ibm.com) wrote:
> This patchset contains two patches. The first enables QEMU
> to facilitate the injection of a secret blob into the guest
> memory.
> 
> The second enables QEMU to parse the guest ROM to determine
> the address at which the secret should be injected.
> 
> Tobin Feldman-Fitzthum (2):
>   sev: add sev-inject-launch-secret
>   sev: scan guest ROM for launch secret address
> 
>  include/sysemu/sev.h       |   2 +
>  qapi/misc-target.json      |  20 +++++++
>  target/i386/monitor.c      |   8 +++
>  target/i386/sev-stub.c     |   5 ++
>  target/i386/sev.c          | 113 +++++++++++++++++++++++++++++++++++++
>  target/i386/sev_i386.h     |  16 ++++++
>  target/i386/trace-events   |   1 +
>  tests/qtest/qmp-cmd-test.c |   6 +-
>  8 files changed, 168 insertions(+), 3 deletions(-)
> 
> -- 
> 2.20.1 (Apple Git-117)
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-06-01 18:52 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28 20:51 [PATCH 0/2] Add support for SEV Launch Secret Injection Tobin Feldman-Fitzthum
2020-05-28 20:51 ` [PATCH 1/2] sev: add sev-inject-launch-secret Tobin Feldman-Fitzthum
2020-05-28 21:00   ` James Bottomley
2020-05-29 18:09     ` tobin
2020-05-28 21:42   ` Eric Blake
2020-05-29 18:04     ` tobin
2020-05-28 20:51 ` [PATCH 2/2] sev: scan guest ROM for launch secret address Tobin Feldman-Fitzthum
2020-05-29 19:08   ` Tom Lendacky
2020-05-29  3:32 ` [PATCH 0/2] Add support for SEV Launch Secret Injection no-reply
2020-05-29  3:35 ` no-reply
2020-05-29  3:36 ` no-reply
2020-05-29  3:39 ` no-reply
2020-06-01 18:51 ` Dr. David Alan Gilbert

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.