[RFC PATCH v2 0/2] i386/sev: Support measured direct kernel boot on SNP

[RFC PATCH v2 0/2] i386/sev: Support measured direct kernel boot on SNP

[Dov Murik]

This RFC patch series is based on AMD's RFC upmv10-snpv3 tree [1].


In order to enable measured direct kernel boot on SNP, QEMU needs to
fill the hashes page when kernel-hashes=on.  This relies on several
changes to the SNP metadata published by OVMF (See [2] for proposed
OVMF patches).

Patch 1 pulls the 'kernel-hashes' property from the SEV guest settings
to the common settings to make it available for both SEV and SNP.

Patch 2 adds the hashes table for SNP guests (or validates the page as a
zero page if kernel-hashes=off).

This patch series is also available at [3].


[1] https://github.com/mdroth/qemu/commits/upmv10-snpv3
[2] https://edk2.groups.io/g/devel/message/100286
[3] https://github.com/confidential-containers-demo/qemu/tree/snp-kernel-hashes-v2

v2 changes:
* Rebase on top of upmv10-snpv3 which includes kernel-hashes.

v1: https://lore.kernel.org/qemu-devel/20220329064038.96006-1-dovmurik%40linux.ibm.com/

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Eduardo Habkost <eduardo@habkost.net>
Cc: Eric Blake <eblake@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Mario Smarduch <mario.smarduch@amd.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>

Dov Murik (2):
  qapi, i386: Move kernel-hashes to SevCommonProperties
  i386/sev: Allow measured direct kernel boot on SNP

 qapi/qom.json     | 12 +++---
 target/i386/sev.c | 95 +++++++++++++++++++++++++++++------------------
 2 files changed, 65 insertions(+), 42 deletions(-)

-- 
2.25.1

[RFC PATCH v2 1/2] qapi, i386: Move kernel-hashes to SevCommonProperties

[Dov Murik]

In order to enable kernel-hashes for SNP, pull it from
SevGuestProperties to its parent SevCommonProperties so
it will be available for both SEV and SNP.
---
 qapi/qom.json     | 12 ++++++------
 target/i386/sev.c | 44 ++++++++++++++++++--------------------------
 2 files changed, 24 insertions(+), 32 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index 33abba0e04..9b2897d54c 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -836,6 +836,10 @@
 # @reduced-phys-bits: number of bits in physical addresses that become
 #                     unavailable when SEV is enabled
 #
+# @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a
+#                 designated guest firmware page for measured boot
+#                 with -kernel (default: false) (since 6.2)
+#
 # @upm-mode: configure Unmapped Private Memory mode
 #
 # @discard: configure how discarding is handled for memory after
@@ -848,6 +852,7 @@
   'data': { '*sev-device': 'str',
             '*cbitpos': 'uint32',
             'reduced-phys-bits': 'uint32',
+            '*kernel-hashes': 'bool',
             '*upm-mode': 'bool',
             '*discard': 'str' } }
 
@@ -864,10 +869,6 @@
 #
 # @handle: SEV firmware handle (default: 0)
 #
-# @kernel-hashes: if true, add hashes of kernel/initrd/cmdline to a
-#                 designated guest firmware page for measured boot
-#                 with -kernel (default: false) (since 6.2)
-#
 # Since: 2.12
 ##
 { 'struct': 'SevGuestProperties',
@@ -875,8 +876,7 @@
   'data': { '*dh-cert-file': 'str',
             '*session-file': 'str',
             '*policy': 'uint32',
-            '*handle': 'uint32',
-            '*kernel-hashes': 'bool' } }
+            '*handle': 'uint32' } }
 
 ##
 # @SevSnpGuestProperties:
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 758e8225c2..6b8e85888f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -64,6 +64,7 @@ struct SevCommonState {
     char *sev_device;
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
+    bool kernel_hashes;
     bool upm_mode;
     char *discard;
 
@@ -88,7 +89,6 @@ struct SevGuestState {
     uint32_t policy;
     char *dh_cert_file;
     char *session_file;
-    bool kernel_hashes;
 };
 
 struct SevSnpGuestState {
@@ -390,6 +390,16 @@ sev_common_set_sev_device(Object *obj, const char *value, Error **errp)
     SEV_COMMON(obj)->sev_device = g_strdup(value);
 }
 
+static bool sev_common_get_kernel_hashes(Object *obj, Error **errp)
+{
+    return SEV_COMMON(obj)->kernel_hashes;
+}
+
+static void sev_common_set_kernel_hashes(Object *obj, bool value, Error **errp)
+{
+    SEV_COMMON(obj)->kernel_hashes = value;
+}
+
 static bool sev_common_get_upm_mode(Object *obj, Error **errp)
 {
     return SEV_COMMON(obj)->upm_mode;
@@ -420,6 +430,11 @@ sev_common_class_init(ObjectClass *oc, void *data)
                                   sev_common_set_sev_device);
     object_class_property_set_description(oc, "sev-device",
             "SEV device to use");
+    object_class_property_add_bool(oc, "kernel-hashes",
+                                   sev_common_get_kernel_hashes,
+                                   sev_common_set_kernel_hashes);
+    object_class_property_set_description(oc, "kernel-hashes",
+            "add kernel hashes to guest firmware for measured Linux boot");
     object_class_property_add_bool(oc, "upm-mode",
                                    sev_common_get_upm_mode,
                                    sev_common_set_upm_mode);
@@ -484,20 +499,6 @@ sev_guest_set_session_file(Object *obj, const char *value, Error **errp)
     SEV_GUEST(obj)->session_file = g_strdup(value);
 }
 
-static bool sev_guest_get_kernel_hashes(Object *obj, Error **errp)
-{
-    SevGuestState *sev_guest = SEV_GUEST(obj);
-
-    return sev_guest->kernel_hashes;
-}
-
-static void sev_guest_set_kernel_hashes(Object *obj, bool value, Error **errp)
-{
-    SevGuestState *sev = SEV_GUEST(obj);
-
-    sev->kernel_hashes = value;
-}
-
 static void
 sev_guest_class_init(ObjectClass *oc, void *data)
 {
@@ -511,11 +512,6 @@ sev_guest_class_init(ObjectClass *oc, void *data)
                                   sev_guest_set_session_file);
     object_class_property_set_description(oc, "session-file",
             "guest owners session parameters (encoded with base64)");
-    object_class_property_add_bool(oc, "kernel-hashes",
-                                   sev_guest_get_kernel_hashes,
-                                   sev_guest_set_kernel_hashes);
-    object_class_property_set_description(oc, "kernel-hashes",
-            "add kernel hashes to guest firmware for measured Linux boot");
 }
 
 static void
@@ -2088,16 +2084,12 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     MemTxAttrs attrs = { 0 };
     bool ret = true;
     SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
-    SevGuestState *sev_guest =
-        (SevGuestState *)object_dynamic_cast(OBJECT(sev_common),
-                                             TYPE_SEV_GUEST);
 
     /*
      * Only add the kernel hashes if the sev-guest configuration explicitly
-     * stated kernel-hashes=on. Currently only enabled for SEV/SEV-ES guests,
-     * so check for TYPE_SEV_GUEST as well.
+     * stated kernel-hashes=on.
      */
-    if (sev_guest && !sev_guest->kernel_hashes) {
+    if (!sev_common->kernel_hashes) {
         return false;
     }
 
-- 
2.25.1

Re: [RFC PATCH v2 1/2] qapi, i386: Move kernel-hashes to SevCommonProperties

[Markus Armbruster]

Dov Murik <dovmurik@linux.ibm.com> writes:

> In order to enable kernel-hashes for SNP, pull it from
> SevGuestProperties to its parent SevCommonProperties so
> it will be available for both SEV and SNP.

Missing

  Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Patch does not apply for me.

Re: [RFC PATCH v2 1/2] qapi, i386: Move kernel-hashes to SevCommonProperties

[Dov Murik]

Hello Markus,

On 16/02/2023 11:24, Markus Armbruster wrote:
> Dov Murik <dovmurik@linux.ibm.com> writes:
> 
>> In order to enable kernel-hashes for SNP, pull it from
>> SevGuestProperties to its parent SevCommonProperties so
>> it will be available for both SEV and SNP.
> 
> Missing
> 
>   Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> 

Oops, thanks. I'll fix.

> Patch does not apply for me.
> 

This patch series is based on AMD's upmv10-snpv3 tree:

  https://github.com/mdroth/qemu/tree/upmv10-snpv3

Have you tried to apply it on top of that tree?

-Dov

Re: [RFC PATCH v2 1/2] qapi, i386: Move kernel-hashes to SevCommonProperties

[Markus Armbruster]

Dov Murik <dovmurik@linux.ibm.com> writes:

> Hello Markus,
>
> On 16/02/2023 11:24, Markus Armbruster wrote:
>> Dov Murik <dovmurik@linux.ibm.com> writes:
>> 
>>> In order to enable kernel-hashes for SNP, pull it from
>>> SevGuestProperties to its parent SevCommonProperties so
>>> it will be available for both SEV and SNP.
>> 
>> Missing
>> 
>>   Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> 
>
> Oops, thanks. I'll fix.
>
>> Patch does not apply for me.
>> 
>
> This patch series is based on AMD's upmv10-snpv3 tree:
>
>   https://github.com/mdroth/qemu/tree/upmv10-snpv3
>
> Have you tried to apply it on top of that tree?

Missed that part in the cover letter, oops :)

Recommend to also express it like

    Based-on: <message-id>

so machines like Patchew can apply it correctly.

However, that upmv10-snpv3 branch is a bit over 1700 commits behind
upstream master.  I'm afraid you guys need to rebase :)

[RFC PATCH v2 2/2] i386/sev: Allow measured direct kernel boot on SNP

[Dov Murik]

In SNP, the hashes page is not included in the ranges to pre-validate
that appear in the SNP metadata published by AmdSev OVMF.

Therefore, if the user enabled kernel hashes (for measured direct boot),
QEMU should fill hashes table and encrypt the page.  Note that in SNP
(unlike SEV and SEV-ES) the measurements is done in whole 4KB pages.
Therefore QEMU zeros the whole page that includes the hashes table, and
fills in the kernel hashes area in that page, and then encrypts the
whole page.  The rest of the page is reserved for SEV launch secrets
which are not usable anyway on SNP.

If the user disabled kernel hashes, QEMU pre-validates the page as a
zero page.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 target/i386/sev.c | 51 +++++++++++++++++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 10 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 6b8e85888f..c36ba9a541 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -2079,8 +2079,11 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t initrd_hash[HASH_SIZE];
     uint8_t kernel_hash[HASH_SIZE];
     uint8_t *hashp;
+    hwaddr mapped_gpa, mapped_offset, mapped_len, expected_mapped_len;
+    uint8_t *mapped_area = NULL;
+    MemoryRegion *mr = NULL;
+    void *hva;
     size_t hash_len = HASH_SIZE;
-    hwaddr mapped_len = sizeof(*padded_ht);
     MemTxAttrs attrs = { 0 };
     bool ret = true;
     SevCommonState *sev_common = SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
@@ -2090,6 +2093,25 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
      * stated kernel-hashes=on.
      */
     if (!sev_common->kernel_hashes) {
+        if (sev_snp_enabled()) {
+            /* Mark the hashes page (if defined) as a zero page */
+            if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
+                return false;
+            }
+
+            area = (SevHashTableDescriptor *)data;
+            if (!area->base || area->size < sizeof(PaddedSevHashTable)) {
+                return false;
+            }
+
+            mapped_gpa = area->base & TARGET_PAGE_MASK;
+            hva = gpa2hva(&mr, mapped_gpa, TARGET_PAGE_SIZE, NULL);
+            if (sev_snp_launch_update(SEV_SNP_GUEST(sev_common), mapped_gpa, hva,
+                                      TARGET_PAGE_SIZE, KVM_SEV_SNP_PAGE_TYPE_ZERO)) {
+                error_setg(errp, "SEV: error marking kernel hashes page as zero");
+            }
+            return false;
+        }
         return false;
     }
 
@@ -2099,10 +2121,6 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
         return false;
     }
 
-    if (sev_snp_enabled()) {
-        return false;
-    }
-
     area = (SevHashTableDescriptor *)data;
     if (!area->base || area->size < sizeof(PaddedSevHashTable)) {
         error_setg(errp, "SEV: guest firmware hashes table area is invalid "
@@ -2149,12 +2167,25 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
      * Populate the hashes table in the guest's memory at the OVMF-designated
      * area for the SEV hashes table
      */
-    padded_ht = address_space_map(&address_space_memory, area->base,
-                                  &mapped_len, true, attrs);
-    if (!padded_ht || mapped_len != sizeof(*padded_ht)) {
+    if (sev_snp_enabled()) {
+        /* SNP encrypts and measures memory in whole pages */
+        mapped_gpa = area->base & TARGET_PAGE_MASK;
+        mapped_offset = area->base & ~TARGET_PAGE_MASK;
+        mapped_len = TARGET_PAGE_SIZE;
+    } else {
+        mapped_gpa = area->base;
+        mapped_offset = 0;
+        mapped_len = sizeof(*padded_ht);
+    }
+    expected_mapped_len = mapped_len;
+    mapped_area = address_space_map(&address_space_memory, mapped_gpa,
+                                    &mapped_len, true, attrs);
+    if (!mapped_area || mapped_len != expected_mapped_len) {
         error_setg(errp, "SEV: cannot map hashes table guest memory area");
         return false;
     }
+    memset(mapped_area, 0, mapped_len);
+    padded_ht = (PaddedSevHashTable *)(mapped_area + mapped_offset);
     ht = &padded_ht->ht;
 
     ht->guid = sev_hash_table_header_guid;
@@ -2175,11 +2206,11 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     /* zero the excess data so the measurement can be reliably calculated */
     memset(padded_ht->padding, 0, sizeof(padded_ht->padding));
 
-    if (sev_encrypt_flash(area->base, (uint8_t *)padded_ht, sizeof(*padded_ht), errp) < 0) {
+    if (sev_encrypt_flash(mapped_gpa, mapped_area, mapped_len, errp) < 0) {
         ret = false;
     }
 
-    address_space_unmap(&address_space_memory, padded_ht,
+    address_space_unmap(&address_space_memory, mapped_area,
                         mapped_len, true, mapped_len);
 
     return ret;
-- 
2.25.1

Re: [RFC PATCH v2 0/2] i386/sev: Support measured direct kernel boot on SNP

[Dov Murik]

On 16/02/2023 10:49, Dov Murik wrote:
> This RFC patch series is based on AMD's RFC upmv10-snpv3 tree [1].
> 

Note that in order to test this you must use '-machine pc-q35-7.1'
to circumvent the SETUP_RNG_SEED bug [1] that interferes with the
measured kernel.

[1] https://lore.kernel.org/qemu-devel/20230208211212.41951-1-mst@redhat.com/


-Dov


> 
> In order to enable measured direct kernel boot on SNP, QEMU needs to
> fill the hashes page when kernel-hashes=on.  This relies on several
> changes to the SNP metadata published by OVMF (See [2] for proposed
> OVMF patches).
> 
> Patch 1 pulls the 'kernel-hashes' property from the SEV guest settings
> to the common settings to make it available for both SEV and SNP.
> 
> Patch 2 adds the hashes table for SNP guests (or validates the page as a
> zero page if kernel-hashes=off).
> 
> This patch series is also available at [3].
> 
> 
> [1] https://github.com/mdroth/qemu/commits/upmv10-snpv3
> [2] https://edk2.groups.io/g/devel/message/100286
> [3] https://github.com/confidential-containers-demo/qemu/tree/snp-kernel-hashes-v2
> 
> v2 changes:
> * Rebase on top of upmv10-snpv3 which includes kernel-hashes.
> 
> v1: https://lore.kernel.org/qemu-devel/20220329064038.96006-1-dovmurik%40linux.ibm.com/
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Eduardo Habkost <eduardo@habkost.net>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Mario Smarduch <mario.smarduch@amd.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> 
> Dov Murik (2):
>   qapi, i386: Move kernel-hashes to SevCommonProperties
>   i386/sev: Allow measured direct kernel boot on SNP
> 
>  qapi/qom.json     | 12 +++---
>  target/i386/sev.c | 95 +++++++++++++++++++++++++++++------------------
>  2 files changed, 65 insertions(+), 42 deletions(-)
> 

Re: [RFC PATCH v2 0/2] i386/sev: Support measured direct kernel boot on SNP

[Daniel P. Berrangé]

On Thu, Feb 16, 2023 at 08:49:11AM +0000, Dov Murik wrote:
> This RFC patch series is based on AMD's RFC upmv10-snpv3 tree [1].

I've seen postings of the kernel patches for SNP using the kernel
UPM support, but I don't recall ever seeing these QEMU pieces
posted for review. The code in that QEMU branch looks different
from the last posting of SNP to qemu-devel years ago.

IMHO it would be very desirable if that QEMU UPM tree was submitted
to qemu-devel for review feedback, before requesting review of patches
that build on top of it.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [RFC PATCH v2 0/2] i386/sev: Support measured direct kernel boot on SNP

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Feb 16, 2023 at 08:49:11AM +0000, Dov Murik wrote:
> > This RFC patch series is based on AMD's RFC upmv10-snpv3 tree [1].
> 
> I've seen postings of the kernel patches for SNP using the kernel
> UPM support, but I don't recall ever seeing these QEMU pieces
> posted for review. The code in that QEMU branch looks different
> from the last posting of SNP to qemu-devel years ago.
> 
> IMHO it would be very desirable if that QEMU UPM tree was submitted
> to qemu-devel for review feedback

Some of the patches in there look like they're not dependent on SNP
or the UPM interface; (eg some CPU model updates).  It's probably worth
posting those separately so that they can be reviewed and merged and out
of the way.

> before requesting review of patches
> that build on top of it.

But at the same time it seems right for Dov to send these patches for
review.

Dave

> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK