[PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch

[PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch

[Dov Murik]

Tom Lendacky and Brijesh Singh reported two issues with launching SEV
guests with the -kernel QEMU option when an old [1] or wrongly configured [2]
OVMF images are used.

To fix these issues, these series "hides" the whole kernel hashes
additions behind a kernel-hashes=on option (with default value of
"off").  This allows existing scenarios to work without change, and
explicitly forces kernel hashes additions for guests that require that.

Patch 1 introduces a new boolean option "kernel-hashes" on the sev-guest
object, and patch 2 causes QEMU to add kernel hashes only if its
explicitly set to "on".  This will mitigate both experienced issues
because the default of the new setting is off, and therefore is backward
compatible with older OVMF images (which don't have a designated hashes
table area) or with guests that don't wish to measure the kernel/initrd.

Patch 3 fixes the wording on the error message displayed when no hashes
table is found in the guest firmware.

Patch 4 detects incorrect address and length of the guest firmware
hashes table area and fails the boot.

Patch 5 is a refactoring of parts of the same function
sev_add_kernel_loader_hashes() to calculate all padding sizes at
compile-time.  Patch 6 also changes the same function and replaces the
call to qemu_map_ram_ptr() with address_space_map() to allow for error
detection.  Patches 5-6 are not required to fix the issues above, but
are suggested as an improvement (no functional change intended).

To enable addition of kernel/initrd/cmdline hashes into the SEV guest at
launch time, specify:

    qemu-system-x86_64 ... -object sev-guest,...,kernel-hashes=on


[1] https://lore.kernel.org/qemu-devel/3b9d10d9-5d9c-da52-f18c-cd93c1931706@amd.com/
[2] https://lore.kernel.org/qemu-devel/001dd81a-282d-c307-a657-e228480d4af3@amd.com/



Changes in v2:
 - Instead of trying to figure out whether to add hashes or not,
   explicity declare an option (kernel-hashes=on) for that.  When that
   option is turned on, fail if the hashes cannot be added.
 - Rephrase error message when no hashes table GUID is found.
 - Replace qemu_map_ram_ptr with address_space_map

v1: https://lore.kernel.org/qemu-devel/20211101102136.1706421-1-dovmurik@linux.ibm.com/


Dov Murik (6):
  qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option
  target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on
  target/i386/sev: Rephrase error message when no hashes table in guest
    firmware
  target/i386/sev: Fail when invalid hashes table area detected
  target/i386/sev: Perform padding calculations at compile-time
  target/i386/sev: Replace qemu_map_ram_ptr with address_space_map

 qapi/qom.json     |  7 ++++-
 target/i386/sev.c | 77 +++++++++++++++++++++++++++++++++++++++--------
 qemu-options.hx   |  6 +++-
 3 files changed, 75 insertions(+), 15 deletions(-)


base-commit: af531756d25541a1b3b3d9a14e72e7fedd941a2e
-- 
2.25.1

[PATCH v2 1/6] qapi/qom, target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Dov Murik]

Introduce new boolean 'kernel-hashes' option on the sev-guest object.
It will be used to to decide whether to add the hashes of
kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
The default value is 'off'.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 qapi/qom.json     |  7 ++++++-
 target/i386/sev.c | 20 ++++++++++++++++++++
 qemu-options.hx   |  6 +++++-
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index ccd1167808..4fd5d1716b 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -769,6 +769,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: 2.12
 ##
 { 'struct': 'SevGuestProperties',
@@ -778,7 +782,8 @@
             '*policy': 'uint32',
             '*handle': 'uint32',
             '*cbitpos': 'uint32',
-            'reduced-phys-bits': 'uint32' } }
+            'reduced-phys-bits': 'uint32',
+            '*kernel-hashes': 'bool' } }
 
 ##
 # @ObjectType:
diff --git a/target/i386/sev.c b/target/i386/sev.c
index eede07f11d..cad32812f5 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -62,6 +62,7 @@ struct SevGuestState {
     char *session_file;
     uint32_t cbitpos;
     uint32_t reduced_phys_bits;
+    bool kernel_hashes;
 
     /* runtime state */
     uint32_t handle;
@@ -327,6 +328,20 @@ sev_guest_set_sev_device(Object *obj, const char *value, Error **errp)
     sev->sev_device = g_strdup(value);
 }
 
+static bool sev_guest_get_kernel_hashes(Object *obj, Error **errp)
+{
+    SevGuestState *sev = SEV_GUEST(obj);
+
+    return sev->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)
 {
@@ -345,6 +360,11 @@ 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
diff --git a/qemu-options.hx b/qemu-options.hx
index f051536b63..f50fdc3e47 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -5189,7 +5189,7 @@ SRST
                  -object secret,id=sec0,keyid=secmaster0,format=base64,\\
                      data=$SECRET,iv=$(<iv.b64)
 
-    ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file]``
+    ``-object sev-guest,id=id,cbitpos=cbitpos,reduced-phys-bits=val,[sev-device=string,policy=policy,handle=handle,dh-cert-file=file,session-file=file,kernel-hashes=on|off]``
         Create a Secure Encrypted Virtualization (SEV) guest object,
         which can be used to provide the guest memory encryption support
         on AMD processors.
@@ -5229,6 +5229,10 @@ SRST
         session with the guest owner to negotiate keys used for
         attestation. The file must be encoded in base64.
 
+        The ``kernel-hashes`` adds the hashes of given kernel/initrd/
+        cmdline to a designated guest firmware page for measured Linux
+        boot with -kernel. The default is off.
+
         e.g to launch a SEV guest
 
         .. parsed-literal::
-- 
2.25.1

Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Markus Armbruster]

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

> Introduce new boolean 'kernel-hashes' option on the sev-guest object.
> It will be used to to decide whether to add the hashes of
> kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
> The default value is 'off'.
>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  qapi/qom.json     |  7 ++++++-
>  target/i386/sev.c | 20 ++++++++++++++++++++
>  qemu-options.hx   |  6 +++++-
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index ccd1167808..4fd5d1716b 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -769,6 +769,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)

Missing: (since 7.0)

> +#
>  # Since: 2.12
>  ##
>  { 'struct': 'SevGuestProperties',
> @@ -778,7 +782,8 @@
>              '*policy': 'uint32',
>              '*handle': 'uint32',
>              '*cbitpos': 'uint32',
> -            'reduced-phys-bits': 'uint32' } }
> +            'reduced-phys-bits': 'uint32',
> +            '*kernel-hashes': 'bool' } }
>  
>  ##
>  # @ObjectType:

[...]

Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Dov Murik]

On 08/11/2021 17:51, Markus Armbruster wrote:
> Dov Murik <dovmurik@linux.ibm.com> writes:
> 
>> Introduce new boolean 'kernel-hashes' option on the sev-guest object.
>> It will be used to to decide whether to add the hashes of
>> kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
>> The default value is 'off'.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  qapi/qom.json     |  7 ++++++-
>>  target/i386/sev.c | 20 ++++++++++++++++++++
>>  qemu-options.hx   |  6 +++++-
>>  3 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index ccd1167808..4fd5d1716b 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -769,6 +769,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)
> 
> Missing: (since 7.0)
> 

I agree the "since" clause is missing, but I think this series (at least
patches 1-4) should be considered a bug fix (because booting with
-kernel will break in 6.2 for older OVMF which doesn't have guest
firmware area for hashes).

I think it should be added for 6.2.

Paolo?


If agreed, the hunk should be:



--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -769,6 +769,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)
+#
 # Since: 2.12
 ##
 { 'struct': 'SevGuestProperties',




-Dov


>> +#
>>  # Since: 2.12
>>  ##
>>  { 'struct': 'SevGuestProperties',
>> @@ -778,7 +782,8 @@
>>              '*policy': 'uint32',
>>              '*handle': 'uint32',
>>              '*cbitpos': 'uint32',
>> -            'reduced-phys-bits': 'uint32' } }
>> +            'reduced-phys-bits': 'uint32',
>> +            '*kernel-hashes': 'bool' } }
>>  
>>  ##
>>  # @ObjectType:
> 
> [...]
> 

Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 08:20:48PM +0200, Dov Murik wrote:
> 
> 
> On 08/11/2021 17:51, Markus Armbruster wrote:
> > Dov Murik <dovmurik@linux.ibm.com> writes:
> > 
> >> Introduce new boolean 'kernel-hashes' option on the sev-guest object.
> >> It will be used to to decide whether to add the hashes of
> >> kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
> >> The default value is 'off'.
> >>
> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> >> ---
> >>  qapi/qom.json     |  7 ++++++-
> >>  target/i386/sev.c | 20 ++++++++++++++++++++
> >>  qemu-options.hx   |  6 +++++-
> >>  3 files changed, 31 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/qapi/qom.json b/qapi/qom.json
> >> index ccd1167808..4fd5d1716b 100644
> >> --- a/qapi/qom.json
> >> +++ b/qapi/qom.json
> >> @@ -769,6 +769,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)
> > 
> > Missing: (since 7.0)
> > 
> 
> I agree the "since" clause is missing, but I think this series (at least
> patches 1-4) should be considered a bug fix (because booting with
> -kernel will break in 6.2 for older OVMF which doesn't have guest
> firmware area for hashes).
> 
> I think it should be added for 6.2.
> 
> Paolo?
> 
> 
> If agreed, the hunk should be:

Yes, the kernel hashes feature was introduced in this 6.2 dev
cycle, and this patch is fixing a significant behavioural
problem with it. We need this included in the 6.2 release


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: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Dov Murik]

On 11/11/2021 11:26, Daniel P. Berrangé wrote:
> On Mon, Nov 08, 2021 at 08:20:48PM +0200, Dov Murik wrote:
>>
>>
>> On 08/11/2021 17:51, Markus Armbruster wrote:
>>> Dov Murik <dovmurik@linux.ibm.com> writes:
>>>
>>>> Introduce new boolean 'kernel-hashes' option on the sev-guest object.
>>>> It will be used to to decide whether to add the hashes of
>>>> kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
>>>> The default value is 'off'.
>>>>
>>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>>> ---
>>>>  qapi/qom.json     |  7 ++++++-
>>>>  target/i386/sev.c | 20 ++++++++++++++++++++
>>>>  qemu-options.hx   |  6 +++++-
>>>>  3 files changed, 31 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>> index ccd1167808..4fd5d1716b 100644
>>>> --- a/qapi/qom.json
>>>> +++ b/qapi/qom.json
>>>> @@ -769,6 +769,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)
>>>
>>> Missing: (since 7.0)
>>>
>>
>> I agree the "since" clause is missing, but I think this series (at least
>> patches 1-4) should be considered a bug fix (because booting with
>> -kernel will break in 6.2 for older OVMF which doesn't have guest
>> firmware area for hashes).
>>
>> I think it should be added for 6.2.
>>
>> Paolo?
>>
>>
>> If agreed, the hunk should be:
> 
> Yes, the kernel hashes feature was introduced in this 6.2 dev
> cycle, and this patch is fixing a significant behavioural
> problem with it. We need this included in the 6.2 release
> 

Thanks for reviewing.

I'll shortly send a v3 with the minor doc/string changes suggested here
(patches 1 and 3).

-Dov

Re: [PATCH v2 1/6] qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:35PM +0000, Dov Murik wrote:
> Introduce new boolean 'kernel-hashes' option on the sev-guest object.
> It will be used to to decide whether to add the hashes of
> kernel/initrd/cmdline to SEV guest memory when booting with -kernel.
> The default value is 'off'.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  qapi/qom.json     |  7 ++++++-
>  target/i386/sev.c | 20 ++++++++++++++++++++
>  qemu-options.hx   |  6 +++++-
>  3 files changed, 31 insertions(+), 2 deletions(-)


Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

(Assuming the version tag is added as discussed)
 

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 :|

[PATCH v2 2/6] target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on

[Dov Murik]

Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
for measured linux boot", 2021-09-30) introduced measured direct boot
with -kernel, using an OVMF-designated hashes table which QEMU fills.

However, if OVMF doesn't designate such an area, QEMU would completely
abort the VM launch.  This breaks launching with -kernel using older
OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.

Fix that so QEMU will only look for the hashes table if the sev-guest
kernel-hashes option is set to on.  Otherwise, QEMU won't look for the
designated area in OVMF and won't fill that area.

To enable addition of kernel hashes, launch the guest with:

    -object sev-guest,...,kernel-hashes=on

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 target/i386/sev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index cad32812f5..e3abbeef68 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1223,6 +1223,14 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     size_t hash_len = HASH_SIZE;
     int aligned_len;
 
+    /*
+     * Only add the kernel hashes if the sev-guest configuration explicitly
+     * stated kernel-hashes=on.
+     */
+    if (!sev_guest->kernel_hashes) {
+        return false;
+    }
+
     if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
         error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
         return false;
-- 
2.25.1

Re: [PATCH v2 2/6] target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:36PM +0000, Dov Murik wrote:
> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> for measured linux boot", 2021-09-30) introduced measured direct boot
> with -kernel, using an OVMF-designated hashes table which QEMU fills.
> 
> However, if OVMF doesn't designate such an area, QEMU would completely
> abort the VM launch.  This breaks launching with -kernel using older
> OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.
> 
> Fix that so QEMU will only look for the hashes table if the sev-guest
> kernel-hashes option is set to on.  Otherwise, QEMU won't look for the
> designated area in OVMF and won't fill that area.
> 
> To enable addition of kernel hashes, launch the guest with:
> 
>     -object sev-guest,...,kernel-hashes=on
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  target/i386/sev.c | 8 ++++++++
>  1 file changed, 8 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH v2 3/6] target/i386/sev: Rephrase error message when no hashes table in guest firmware

[Dov Murik]

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

diff --git a/target/i386/sev.c b/target/i386/sev.c
index e3abbeef68..c71d23654f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1232,7 +1232,8 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     }
 
     if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
-        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
+        error_setg(errp, "SEV: -kernel specified but guest firmware "
+                         "has no hashes table GUID");
         return false;
     }
     area = (SevHashTableDescriptor *)data;
-- 
2.25.1

Re: [PATCH v2 3/6] target/i386/sev: Rephrase error message when no hashes table in guest firmware

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:37PM +0000, Dov Murik wrote:
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  target/i386/sev.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index e3abbeef68..c71d23654f 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1232,7 +1232,8 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
>      }
>  
>      if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> -        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
> +        error_setg(errp, "SEV: -kernel specified but guest firmware "
> +                         "has no hashes table GUID");

Don't refer to "-kernel" as that's just one way to specifying it. The
user might have used

   -machine ....,kernel=/path/to/vmlinux

Simply "kernel" as the original text has, is fine.

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: [PATCH v2 3/6] target/i386/sev: Rephrase error message when no hashes table in guest firmware

[Dov Murik]

On 08/11/2021 15:53, Daniel P. Berrangé wrote:
> On Mon, Nov 08, 2021 at 01:48:37PM +0000, Dov Murik wrote:
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  target/i386/sev.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index e3abbeef68..c71d23654f 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -1232,7 +1232,8 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
>>      }
>>  
>>      if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
>> -        error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid");
>> +        error_setg(errp, "SEV: -kernel specified but guest firmware "
>> +                         "has no hashes table GUID");
> 
> Don't refer to "-kernel" as that's just one way to specifying it. The
> user might have used
> 
>    -machine ....,kernel=/path/to/vmlinux
> 
> Simply "kernel" as the original text has, is fine.
> 

OK, good point. Thanks.

-Dov

[PATCH v2 4/6] target/i386/sev: Fail when invalid hashes table area detected

[Dov Murik]

Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
for measured linux boot", 2021-09-30) introduced measured direct boot
with -kernel, using an OVMF-designated hashes table which QEMU fills.

However, no checks are performed on the validity of the hashes area
designated by OVMF.  Specifically, if OVMF publishes the
SEV_HASH_TABLE_RV_GUID entry but it is filled with zeroes, this will
cause QEMU to write the hashes entries over the first page of the
guest's memory (GPA 0).

Add validity checks to the published area.  If the hashes table area's
base address is zero, or its size is too small to fit the aligned hashes
table, display an error and stop the guest launch.  In such case, the
following error will be displayed:

    qemu-system-x86_64: SEV: guest firmware hashes table area is invalid (base=0x0 size=0x0)

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reported-by: Brijesh Singh <brijesh.singh@amd.com>
---
 target/i386/sev.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index c71d23654f..2588bd623f 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -1221,7 +1221,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t kernel_hash[HASH_SIZE];
     uint8_t *hashp;
     size_t hash_len = HASH_SIZE;
-    int aligned_len;
+    int aligned_len = ROUND_UP(sizeof(SevHashTable), 16);
 
     /*
      * Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1237,6 +1237,11 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
         return false;
     }
     area = (SevHashTableDescriptor *)data;
+    if (!area->base || area->size < aligned_len) {
+        error_setg(errp, "SEV: guest firmware hashes table area is invalid "
+                         "(base=0x%x size=0x%x)", area->base, area->size);
+        return false;
+    }
 
     /*
      * Calculate hash of kernel command-line with the terminating null byte. If
@@ -1295,7 +1300,6 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
 
     /* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
-    aligned_len = ROUND_UP(ht->len, 16);
     if (aligned_len != ht->len) {
         /* zero the excess data so the measurement can be reliably calculated */
         memset(ht->padding, 0, aligned_len - ht->len);
-- 
2.25.1

Re: [PATCH v2 4/6] target/i386/sev: Fail when invalid hashes table area detected

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:38PM +0000, Dov Murik wrote:
> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> for measured linux boot", 2021-09-30) introduced measured direct boot
> with -kernel, using an OVMF-designated hashes table which QEMU fills.
> 
> However, no checks are performed on the validity of the hashes area
> designated by OVMF.  Specifically, if OVMF publishes the
> SEV_HASH_TABLE_RV_GUID entry but it is filled with zeroes, this will
> cause QEMU to write the hashes entries over the first page of the
> guest's memory (GPA 0).
> 
> Add validity checks to the published area.  If the hashes table area's
> base address is zero, or its size is too small to fit the aligned hashes
> table, display an error and stop the guest launch.  In such case, the
> following error will be displayed:
> 
>     qemu-system-x86_64: SEV: guest firmware hashes table area is invalid (base=0x0 size=0x0)
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  target/i386/sev.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH v2 5/6] target/i386/sev: Perform padding calculations at compile-time

[Dov Murik]

In sev_add_kernel_loader_hashes, the sizes of structs are known at
compile-time, so calculate needed padding at compile-time.

No functional change intended.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 target/i386/sev.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2588bd623f..2e3a6e8ff8 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -110,9 +110,19 @@ typedef struct QEMU_PACKED SevHashTable {
     SevHashTableEntry cmdline;
     SevHashTableEntry initrd;
     SevHashTableEntry kernel;
-    uint8_t padding[];
 } SevHashTable;
 
+/*
+ * Data encrypted by sev_encrypt_flash() must be padded to a multiple of
+ * 16 bytes.
+ */
+typedef struct QEMU_PACKED PaddedSevHashTable {
+    SevHashTable ht;
+    uint8_t padding[ROUND_UP(sizeof(SevHashTable), 16) - sizeof(SevHashTable)];
+} PaddedSevHashTable;
+
+QEMU_BUILD_BUG_ON(sizeof(PaddedSevHashTable) % 16 != 0);
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1216,12 +1226,12 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t *data;
     SevHashTableDescriptor *area;
     SevHashTable *ht;
+    PaddedSevHashTable *padded_ht;
     uint8_t cmdline_hash[HASH_SIZE];
     uint8_t initrd_hash[HASH_SIZE];
     uint8_t kernel_hash[HASH_SIZE];
     uint8_t *hashp;
     size_t hash_len = HASH_SIZE;
-    int aligned_len = ROUND_UP(sizeof(SevHashTable), 16);
 
     /*
      * Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1237,7 +1247,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
         return false;
     }
     area = (SevHashTableDescriptor *)data;
-    if (!area->base || area->size < aligned_len) {
+    if (!area->base || area->size < sizeof(PaddedSevHashTable)) {
         error_setg(errp, "SEV: guest firmware hashes table area is invalid "
                          "(base=0x%x size=0x%x)", area->base, area->size);
         return false;
@@ -1282,7 +1292,8 @@ 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
      */
-    ht = qemu_map_ram_ptr(NULL, area->base);
+    padded_ht = qemu_map_ram_ptr(NULL, area->base);
+    ht = &padded_ht->ht;
 
     ht->guid = sev_hash_table_header_guid;
     ht->len = sizeof(*ht);
@@ -1299,13 +1310,10 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     ht->kernel.len = sizeof(ht->kernel);
     memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
 
-    /* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
-    if (aligned_len != ht->len) {
-        /* zero the excess data so the measurement can be reliably calculated */
-        memset(ht->padding, 0, aligned_len - ht->len);
-    }
+    /* zero the excess data so the measurement can be reliably calculated */
+    memset(padded_ht->padding, 0, sizeof(padded_ht->padding));
 
-    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
+    if (sev_encrypt_flash((uint8_t *)padded_ht, sizeof(*padded_ht), errp) < 0) {
         return false;
     }
 
-- 
2.25.1

Re: [PATCH v2 5/6] target/i386/sev: Perform padding calculations at compile-time

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:39PM +0000, Dov Murik wrote:
> In sev_add_kernel_loader_hashes, the sizes of structs are known at
> compile-time, so calculate needed padding at compile-time.
> 
> No functional change intended.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  target/i386/sev.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

[PATCH v2 6/6] target/i386/sev: Replace qemu_map_ram_ptr with address_space_map

[Dov Murik]

Use address_space_map/unmap and check for errors.

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

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 2e3a6e8ff8..12f28e878c 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -37,6 +37,7 @@
 #include "qapi/qmp/qerror.h"
 #include "exec/confidential-guest-support.h"
 #include "hw/i386/pc.h"
+#include "exec/address-spaces.h"
 
 #define TYPE_SEV_GUEST "sev-guest"
 OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
@@ -1232,6 +1233,9 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     uint8_t kernel_hash[HASH_SIZE];
     uint8_t *hashp;
     size_t hash_len = HASH_SIZE;
+    hwaddr mapped_len = sizeof(*padded_ht);
+    MemTxAttrs attrs = { 0 };
+    bool ret = true;
 
     /*
      * Only add the kernel hashes if the sev-guest configuration explicitly
@@ -1292,7 +1296,11 @@ 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 = qemu_map_ram_ptr(NULL, area->base);
+    padded_ht = address_space_map(&address_space_memory, area->base, &mapped_len, true, attrs);
+    if (!padded_ht || mapped_len != sizeof(*padded_ht)) {
+        error_setg(errp, "SEV: cannot map hashes table guest memory area");
+        return false;
+    }
     ht = &padded_ht->ht;
 
     ht->guid = sev_hash_table_header_guid;
@@ -1314,10 +1322,12 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
     memset(padded_ht->padding, 0, sizeof(padded_ht->padding));
 
     if (sev_encrypt_flash((uint8_t *)padded_ht, sizeof(*padded_ht), errp) < 0) {
-        return false;
+        ret = false;
     }
 
-    return true;
+    address_space_unmap(&address_space_memory, padded_ht, mapped_len, true, mapped_len);
+
+    return ret;
 }
 
 static void
-- 
2.25.1

Re: [PATCH v2 6/6] target/i386/sev: Replace qemu_map_ram_ptr with address_space_map

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:40PM +0000, Dov Murik wrote:
> Use address_space_map/unmap and check for errors.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  target/i386/sev.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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: [PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch

[Brijesh Singh]

On 11/8/21 7:48 AM, Dov Murik wrote:
> Tom Lendacky and Brijesh Singh reported two issues with launching SEV
> guests with the -kernel QEMU option when an old [1] or wrongly configured [2]
> OVMF images are used.
> 
> To fix these issues, these series "hides" the whole kernel hashes
> additions behind a kernel-hashes=on option (with default value of
> "off").  This allows existing scenarios to work without change, and
> explicitly forces kernel hashes additions for guests that require that.
> 
> Patch 1 introduces a new boolean option "kernel-hashes" on the sev-guest
> object, and patch 2 causes QEMU to add kernel hashes only if its
> explicitly set to "on".  This will mitigate both experienced issues
> because the default of the new setting is off, and therefore is backward
> compatible with older OVMF images (which don't have a designated hashes
> table area) or with guests that don't wish to measure the kernel/initrd.
> 
> Patch 3 fixes the wording on the error message displayed when no hashes
> table is found in the guest firmware.
> 
> Patch 4 detects incorrect address and length of the guest firmware
> hashes table area and fails the boot.
> 
> Patch 5 is a refactoring of parts of the same function
> sev_add_kernel_loader_hashes() to calculate all padding sizes at
> compile-time.  Patch 6 also changes the same function and replaces the
> call to qemu_map_ram_ptr() with address_space_map() to allow for error
> detection.  Patches 5-6 are not required to fix the issues above, but
> are suggested as an improvement (no functional change intended).
> 
> To enable addition of kernel/initrd/cmdline hashes into the SEV guest at
> launch time, specify:
> 
>      qemu-system-x86_64 ... -object sev-guest,...,kernel-hashes=on
> 
> 
> [1] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F3b9d10d9-5d9c-da52-f18c-cd93c1931706%40amd.com%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7C908b739400a747e1b22308d9a2be7e07%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719761315906327%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cMoOlNU2faGwRk6dXVmOE1SuNrg3VvySAC1Ds8fcaFQ%3D&reserved=0
> [2] https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F001dd81a-282d-c307-a657-e228480d4af3%40amd.com%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7C908b739400a747e1b22308d9a2be7e07%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719761315916323%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7IZ1%2B%2Fh%2B88xWDlHd%2FMKPN0fJfI6dmSX%2F1TbK8aL8bAs%3D&reserved=0
> 
> 
> 
> Changes in v2:
>   - Instead of trying to figure out whether to add hashes or not,
>     explicity declare an option (kernel-hashes=on) for that.  When that
>     option is turned on, fail if the hashes cannot be added.
>   - Rephrase error message when no hashes table GUID is found.
>   - Replace qemu_map_ram_ptr with address_space_map
> 
> v1: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fqemu-devel%2F20211101102136.1706421-1-dovmurik%40linux.ibm.com%2F&data=04%7C01%7Cbrijesh.singh%40amd.com%7C908b739400a747e1b22308d9a2be7e07%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637719761315916323%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SrE9kYP0Qdhx0WqIXbnwHgeX%2BjBVT9BsK6I0OLU3naI%3D&reserved=0
> 
> 
> Dov Murik (6):
>    qapi/qom,target/i386: sev-guest: Introduce kernel-hashes=on|off option
>    target/i386/sev: Add kernel hashes only if sev-guest.kernel-hashes=on
>    target/i386/sev: Rephrase error message when no hashes table in guest
>      firmware
>    target/i386/sev: Fail when invalid hashes table area detected
>    target/i386/sev: Perform padding calculations at compile-time
>    target/i386/sev: Replace qemu_map_ram_ptr with address_space_map
> 
>   qapi/qom.json     |  7 ++++-
>   target/i386/sev.c | 77 +++++++++++++++++++++++++++++++++++++++--------
>   qemu-options.hx   |  6 +++-
>   3 files changed, 75 insertions(+), 15 deletions(-)
> 
> 

Thanks for the fixing it Dov.

Acked-by: Brijesh Singh <brijesh.singh@amd.com>

thanks

Re: [PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch

[Daniel P. Berrangé]

On Mon, Nov 08, 2021 at 01:48:34PM +0000, Dov Murik wrote:
> Tom Lendacky and Brijesh Singh reported two issues with launching SEV
> guests with the -kernel QEMU option when an old [1] or wrongly configured [2]
> OVMF images are used.
> 
> To fix these issues, these series "hides" the whole kernel hashes
> additions behind a kernel-hashes=on option (with default value of
> "off").  This allows existing scenarios to work without change, and
> explicitly forces kernel hashes additions for guests that require that.

We need to to get this into 6.2 to adress the regression vs  previous
QEMU releases.

There's just a couple of small review points. If you can repost
with the easy fixes, then we can get this queued.

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: [PATCH v2 0/6] SEV: add kernel-hashes=on for measured -kernel launch

[Dov Murik]

On 11/11/2021 11:39, Daniel P. Berrangé wrote:
> On Mon, Nov 08, 2021 at 01:48:34PM +0000, Dov Murik wrote:
>> Tom Lendacky and Brijesh Singh reported two issues with launching SEV
>> guests with the -kernel QEMU option when an old [1] or wrongly configured [2]
>> OVMF images are used.
>>
>> To fix these issues, these series "hides" the whole kernel hashes
>> additions behind a kernel-hashes=on option (with default value of
>> "off").  This allows existing scenarios to work without change, and
>> explicitly forces kernel hashes additions for guests that require that.
> 
> We need to to get this into 6.2 to adress the regression vs  previous
> QEMU releases.
> 
> There's just a couple of small review points. If you can repost
> with the easy fixes, then we can get this queued.
> 

Sent v3 now.

Patch 3/6 (error message rephrase) still misses Reviewed-by.

Thanks,
-Dov