diff mbox series

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

Message ID 20211108134840.2757206-2-dovmurik@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series SEV: add kernel-hashes=on for measured -kernel launch | expand

Commit Message

Dov Murik Nov. 8, 2021, 1:48 p.m. UTC
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(-)

Comments

Markus Armbruster Nov. 8, 2021, 3:51 p.m. UTC | #1
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:

[...]
Dov Murik Nov. 8, 2021, 6:20 p.m. UTC | #2
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:
> 
> [...]
>
Daniel P. Berrangé Nov. 11, 2021, 9:26 a.m. UTC | #3
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
Daniel P. Berrangé Nov. 11, 2021, 9:27 a.m. UTC | #4
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
Dov Murik Nov. 11, 2021, 9:38 a.m. UTC | #5
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
diff mbox series

Patch

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