Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On Sun, Jul 04, 2021 at 09:16:59AM +0300, Dov Murik wrote: > Hi Michael, > > [+cc Connor, Dave] > > On 03/07/2021 19:42, Michael S. Tsirkin wrote: > > On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote: > >> From: James Bottomley > >> > >> If the VM is using memory encryption and also specifies a kernel/initrd > >> or appended command line, calculate the hashes and add them to the > >> encrypted data. For this to work, OVMF must support an encrypted area > >> to place the data which is advertised via a special GUID in the OVMF > >> reset table (if the GUID doesn't exist, the user isn't allowed to pass > >> in the kernel/initrd/cmdline via the fw_cfg interface). > > > > Sorry about asking basic questions so late in the game. > > No worries. Please noice there's a newer version: > > https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmu...@linux.ibm.com/ > > > > I'm a bit curious why this feature makes sense. If someone can play > > with a Linux kernel command line isn't it pretty much game over security > > wise? What protections does Linux have against malicious actors > > manipulating the command line? > > > > You're right -- if the host can modify the kernel command-line it's a game > over. > > This is why this patch (together with the corresponding OVMF patches; still > under review) measures and verifies the content of the kernel blob and > the initrd blob *and* the command-line blob. > > Any modification/omission of any of them by the host will make the expected > SEV PSP measurement invalid, which should then indicate to the Guest Owner > that > something is wrong with this guest. At that point the Guest Owner should > refuse to inject secrets into the guest (and also complain to the Cloud > Service Provider). > > -Dov Got it, thanks! -- MST
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Hi Michael, [+cc Connor, Dave] On 03/07/2021 19:42, Michael S. Tsirkin wrote: > On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote: >> From: James Bottomley >> >> If the VM is using memory encryption and also specifies a kernel/initrd >> or appended command line, calculate the hashes and add them to the >> encrypted data. For this to work, OVMF must support an encrypted area >> to place the data which is advertised via a special GUID in the OVMF >> reset table (if the GUID doesn't exist, the user isn't allowed to pass >> in the kernel/initrd/cmdline via the fw_cfg interface). > > Sorry about asking basic questions so late in the game. No worries. Please noice there's a newer version: https://lore.kernel.org/qemu-devel/20210624102040.2015280-1-dovmu...@linux.ibm.com/ > I'm a bit curious why this feature makes sense. If someone can play > with a Linux kernel command line isn't it pretty much game over security > wise? What protections does Linux have against malicious actors > manipulating the command line? > You're right -- if the host can modify the kernel command-line it's a game over. This is why this patch (together with the corresponding OVMF patches; still under review) measures and verifies the content of the kernel blob and the initrd blob *and* the command-line blob. Any modification/omission of any of them by the host will make the expected SEV PSP measurement invalid, which should then indicate to the Guest Owner that something is wrong with this guest. At that point the Guest Owner should refuse to inject secrets into the guest (and also complain to the Cloud Service Provider). -Dov
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
> From: James Bottomley
>
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data. For this to work, OVMF must support an encrypted area
> to place the data which is advertised via a special GUID in the OVMF
> reset table (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
Sorry about asking basic questions so late in the game.
I'm a bit curious why this feature makes sense. If someone can play
with a Linux kernel command line isn't it pretty much game over security
wise? What protections does Linux have against malicious actors
manipulating the command line?
>
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included). Each entry in the hashes
> table is GUID identified and since they're passed through the memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
>
> Signed-off-by: James Bottomley
> Signed-off-by: Dov Murik
> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
>
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
>
> ---
> hw/i386/x86.c | 120 +-
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..d8e77b99b4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -37,12 +37,16 @@
> #include "sysemu/replay.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/cpu-timers.h"
> +#include "sysemu/sev.h"
> +#include "exec/confidential-guest-support.h"
> #include "trace.h"
> +#include "crypto/hash.h"
>
> #include "hw/i386/x86.h"
> #include "target/i386/cpu.h"
> #include "hw/i386/topology.h"
> #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
> #include "hw/intc/i8259.h"
> #include "hw/rtc/mc146818rtc.h"
>
> @@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
> return true;
> }
>
> +struct sev_hash_table_descriptor {
> +uint32_t base;
> +uint32_t size;
> +};
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +struct sev_hash_table_entry {
> +uint8_t guid[16];
> +uint16_t len;
> +uint8_t hash[HASH_SIZE];
> +} __attribute__ ((packed));
> +
> +struct sev_hash_table {
> +uint8_t guid[16];
> +uint16_t len;
> +struct sev_hash_table_entry entries[];
> +} __attribute__((packed));
> +
> +#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +
> +static const uint8_t sev_hash_table_header_guid[] =
> +UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
> +0xd4, 0x11, 0xfd, 0x21);
> +
> +static const uint8_t sev_kernel_entry_guid[] =
> +UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
> +0x72, 0xd2, 0x04, 0x5b);
> +static const uint8_t sev_initrd_entry_guid[] =
> +UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
> +0x91, 0x69, 0x78, 0x1d);
> +static const uint8_t sev_cmdline_entry_guid[] =
> +UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
> +0x4d, 0x36, 0xab, 0x2a);
> +
> void x86_load_linux(X86MachineState *x86ms,
> FWCfgState *fw_cfg,
> int acpi_data_size,
> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *initrd_filename = machine->initrd_filename;
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> +uint8_t buf[HASH_SIZE];
> +uint8_t *hash = buf;
> +size_t hash_len = sizeof(buf);
> +struct sev_hash_table *sev_ht = NULL;
> +int sev_ht_index = 0;
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
> exit(1);
> }
>
> +if (machine->cgs && machine->cgs->ready) {
> +uint8_t *data;
> +struct sev_hash_table_descriptor *area;
> +
> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, , NULL))
> {
> +fprintf(stderr, "qemu: kernel command line specified but OVMF
> has "
> +"no hash table guid\n");
> +exit(1);
> +}
> +area = (struct sev_hash_table_descriptor *)data;
> +
> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
> sizeof(sev_ht->guid));
> +sev_ht->len = sizeof(*sev_ht);
> +}
> +
> /* kernel protocol version */
>
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 6/21/21 11:15 AM, Philippe Mathieu-Daudé wrote:
> On 6/21/21 10:44 AM, Thomas Huth wrote:
>> On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
>> [...]
>>> This works, but I'd rather use:
>>>
>>> if (sev_enabled()) {
>>> sev_kernel_loader_calc_cmdline_hash(_loader_context,
>>> kernel_cmdline);
>>> }
>>>
>>> And have sev_enabled() defined as:
>>>
>>> #ifdef CONFIG_SEV
>>> bool sev_enabled(void);
>>> #else
>>> #define sev_enabled() false
>>> #endif
>>>
>>> So the compiler could elide the statement if SEV is disabled,
>>> and stub is not necessary.
>>>
>>> But that means we'd need to add "#include CONFIG_DEVICES" in
>>> a sysemu/ header, which looks like an anti-pattern.
>>>
>>> Thomas / Paolo, what do you think?
>>
>> I'd only do that if you are very, very sure that the header file is
>> only included from target-specific files. Otherwise this will of course
>> cause more trouble than benefit.
Back to Paolo, I think the problem is we started to use target-specific
features in Kconfig, which was designed for devices (not
target-specific). We have the same problem (another thread) with the
semihosting architectural feature.
> Hmm it could be clearer to rearrange the target-specific sysemu/
> headers. For this example, eventually sysemu/i386/sev.h?
>
> Phil.
>
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 6/21/21 10:44 AM, Thomas Huth wrote:
> On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
> [...]
>> This works, but I'd rather use:
>>
>> if (sev_enabled()) {
>> sev_kernel_loader_calc_cmdline_hash(_loader_context,
>> kernel_cmdline);
>> }
>>
>> And have sev_enabled() defined as:
>>
>> #ifdef CONFIG_SEV
>> bool sev_enabled(void);
>> #else
>> #define sev_enabled() false
>> #endif
>>
>> So the compiler could elide the statement if SEV is disabled,
>> and stub is not necessary.
>>
>> But that means we'd need to add "#include CONFIG_DEVICES" in
>> a sysemu/ header, which looks like an anti-pattern.
>>
>> Thomas / Paolo, what do you think?
>
> I'd only do that if you are very, very sure that the header file is
> only included from target-specific files. Otherwise this will of course
> cause more trouble than benefit.
Hmm it could be clearer to rearrange the target-specific sysemu/
headers. For this example, eventually sysemu/i386/sev.h?
Phil.
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 17/06/2021 17.48, Philippe Mathieu-Daudé wrote:
[...]
This works, but I'd rather use:
if (sev_enabled()) {
sev_kernel_loader_calc_cmdline_hash(_loader_context,
kernel_cmdline);
}
And have sev_enabled() defined as:
#ifdef CONFIG_SEV
bool sev_enabled(void);
#else
#define sev_enabled() false
#endif
So the compiler could elide the statement if SEV is disabled,
and stub is not necessary.
But that means we'd need to add "#include CONFIG_DEVICES" in
a sysemu/ header, which looks like an anti-pattern.
Thomas / Paolo, what do you think?
I'd only do that if you are very, very sure that the header file is only
included from target-specific files. Otherwise this will of course cause
more trouble than benefit.
Thomas
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On Thu, Jun 17, 2021 at 3:17 PM Dov Murik wrote:
>
>
>
> On 17/06/2021 20:22, Eduardo Habkost wrote:
> > On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
> >>
> >>
> >> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> >>> Hi Dov, James,
> >>>
> >>> +Connor who asked to be reviewer.
> >>>
> >>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
> > From: James Bottomley
> >
> > If the VM is using memory encryption and also specifies a kernel/initrd
> > or appended command line, calculate the hashes and add them to the
> > encrypted data. For this to work, OVMF must support an encrypted area
> > to place the data which is advertised via a special GUID in the OVMF
> > reset table (if the GUID doesn't exist, the user isn't allowed to pass
> > in the kernel/initrd/cmdline via the fw_cfg interface).
> >
> > The hashes of each of the files is calculated (or the string in the case
> > of the cmdline with trailing '\0' included). Each entry in the hashes
> > table is GUID identified and since they're passed through the memcrypt
> > interface, the hash of the encrypted data will be accumulated by the
> > PSP.
> >
> > Signed-off-by: James Bottomley
> > Signed-off-by: Dov Murik
> > [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> > strings, remove GCC pragma, fix checkpatch errors]
> > ---
> >
> > OVMF support for handling the table of hashes (verifying that the
> > kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> > to the measured hashes in the table) will be posted soon to edk2-devel.
> >
> > ---
> > hw/i386/x86.c | 120 +-
> > 1 file changed, 119 insertions(+), 1 deletion(-)
> >
>
> This is not an objection to the patch itself, but: can we do
> something to move all sev-related code to sev.c? It would make
> the process of assigning a maintainer and reviewing/merging
> future patches much simpler.
>
> I am not familiar with SEV internals, so my only question is
> about configurations where SEV is disabled:
>
> [...]
> > diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> > @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> > const char *initrd_filename = machine->initrd_filename;
> > const char *dtb_filename = machine->dtb;
> > const char *kernel_cmdline = machine->kernel_cmdline;
> > +uint8_t buf[HASH_SIZE];
> > +uint8_t *hash = buf;
> > +size_t hash_len = sizeof(buf);
> > +struct sev_hash_table *sev_ht = NULL;
> > +int sev_ht_index = 0;
> >>>
> >>> Can you move all these variable into a structure, and use it as a
> >>> SEV loader context?
> >>>
> >>> Then each block of code you added can be moved to its own function,
> >>> self-described, working with the previous context.
> >>>
> >>> The functions can be declared in sev_i386.h and defined in sev.c as
> >>> Eduardo suggested.
> >>>
> >>
> >> Thanks Philippe, I'll try this approach.
> >>
> >>
> >> I assume you mean that an addition like this:
> >>
> >> +if (sev_ht) {
> >> +struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
> >> +
> >> +qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char
> >> *)kernel_cmdline,
> >> + strlen(kernel_cmdline) + 1,
> >> + , _len, _fatal);
> >> +memcpy(e->hash, hash, hash_len);
> >> +e->len = sizeof(*e);
> >> +memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> >> +}
> >>
> >> will be extracted to a function, and here (in x86_load_linux()) replaced
> >> with a single call:
> >>
> >> sev_kernel_loader_calc_cmdline_hash(_loader_context,
> >> kernel_cmdline);
> >>
> >> and that function will have an empty stub in non-SEV builds, and a do-
> >> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> >> will do the actual work in SEV VMs.
> >>
> >> Right?
> >
> > I would suggest a generic notification mechanism instead, where
> > SEV code could register to be notified after the kernel/initrd is
> > loaded.
> >
> > Maybe the existing qemu_add_machine_init_done_notifier()
> > mechanism would be enough for this? Is there a reason the hash
> > calculation needs to be done inside x86_load_linux(),
> > specifically?
> >
>
> SEV already registers that callback - sev_machine_done_notify, which
> calls sev_launch_get_measure. We want the hashes page filled and
> encrypted *before* that measurement is taken by the PSP. We can squeeze
> in the hashes and page encryption before the measurement *if* we can
> calculate the hashes at this point.
>
> x86_load_linux already deals with kernel/initrd/cmdline, so that was the
> easiest place to add the hash calculation.
>
> It's also a bit weird
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 17/06/2021 20:22, Eduardo Habkost wrote:
> On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
>>
>>
>> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
>>> Hi Dov, James,
>>>
>>> +Connor who asked to be reviewer.
>>>
>>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
> From: James Bottomley
>
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data. For this to work, OVMF must support an encrypted area
> to place the data which is advertised via a special GUID in the OVMF
> reset table (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
>
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included). Each entry in the hashes
> table is GUID identified and since they're passed through the memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
>
> Signed-off-by: James Bottomley
> Signed-off-by: Dov Murik
> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
>
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
>
> ---
> hw/i386/x86.c | 120 +-
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
This is not an objection to the patch itself, but: can we do
something to move all sev-related code to sev.c? It would make
the process of assigning a maintainer and reviewing/merging
future patches much simpler.
I am not familiar with SEV internals, so my only question is
about configurations where SEV is disabled:
[...]
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *initrd_filename = machine->initrd_filename;
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> +uint8_t buf[HASH_SIZE];
> +uint8_t *hash = buf;
> +size_t hash_len = sizeof(buf);
> +struct sev_hash_table *sev_ht = NULL;
> +int sev_ht_index = 0;
>>>
>>> Can you move all these variable into a structure, and use it as a
>>> SEV loader context?
>>>
>>> Then each block of code you added can be moved to its own function,
>>> self-described, working with the previous context.
>>>
>>> The functions can be declared in sev_i386.h and defined in sev.c as
>>> Eduardo suggested.
>>>
>>
>> Thanks Philippe, I'll try this approach.
>>
>>
>> I assume you mean that an addition like this:
>>
>> +if (sev_ht) {
>> +struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
>> +
>> +qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
>> + strlen(kernel_cmdline) + 1,
>> + , _len, _fatal);
>> +memcpy(e->hash, hash, hash_len);
>> +e->len = sizeof(*e);
>> +memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
>> +}
>>
>> will be extracted to a function, and here (in x86_load_linux()) replaced
>> with a single call:
>>
>> sev_kernel_loader_calc_cmdline_hash(_loader_context, kernel_cmdline);
>>
>> and that function will have an empty stub in non-SEV builds, and a do-
>> nothing condition (at the beginning) if it's an SEV-disabled VM, and
>> will do the actual work in SEV VMs.
>>
>> Right?
>
> I would suggest a generic notification mechanism instead, where
> SEV code could register to be notified after the kernel/initrd is
> loaded.
>
> Maybe the existing qemu_add_machine_init_done_notifier()
> mechanism would be enough for this? Is there a reason the hash
> calculation needs to be done inside x86_load_linux(),
> specifically?
>
SEV already registers that callback - sev_machine_done_notify, which
calls sev_launch_get_measure. We want the hashes page filled and
encrypted *before* that measurement is taken by the PSP. We can squeeze
in the hashes and page encryption before the measurement *if* we can
calculate the hashes at this point.
x86_load_linux already deals with kernel/initrd/cmdline, so that was the
easiest place to add the hash calculation.
It's also a bit weird (semantically) to modify the guest RAM after
pc_memory_init has finished.
We can add a new notifier towards the end of x86_load_linux, but can we
avoid passing all the different pointers+sizes of kernel/initrd/cmdline
to that notifier? Do you envision other uses for registering on that event?
-Dov
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Hi Dov,
+Thomas
On 6/17/21 2:48 PM, Dov Murik wrote:
> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
>> Hi Dov, James,
>>
>> +Connor who asked to be reviewer.
>>
>> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>>> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
From: James Bottomley
If the VM is using memory encryption and also specifies a kernel/initrd
or appended command line, calculate the hashes and add them to the
encrypted data. For this to work, OVMF must support an encrypted area
to place the data which is advertised via a special GUID in the OVMF
reset table (if the GUID doesn't exist, the user isn't allowed to pass
in the kernel/initrd/cmdline via the fw_cfg interface).
The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included). Each entry in the hashes
table is GUID identified and since they're passed through the memcrypt
interface, the hash of the encrypted data will be accumulated by the
PSP.
Signed-off-by: James Bottomley
Signed-off-by: Dov Murik
[dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
strings, remove GCC pragma, fix checkpatch errors]
---
OVMF support for handling the table of hashes (verifying that the
kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
to the measured hashes in the table) will be posted soon to edk2-devel.
---
hw/i386/x86.c | 120 +-
1 file changed, 119 insertions(+), 1 deletion(-)
>>>
>>> This is not an objection to the patch itself, but: can we do
>>> something to move all sev-related code to sev.c? It would make
>>> the process of assigning a maintainer and reviewing/merging
>>> future patches much simpler.
>>>
>>> I am not familiar with SEV internals, so my only question is
>>> about configurations where SEV is disabled:
>>>
>>> [...]
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
@@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
const char *initrd_filename = machine->initrd_filename;
const char *dtb_filename = machine->dtb;
const char *kernel_cmdline = machine->kernel_cmdline;
+uint8_t buf[HASH_SIZE];
+uint8_t *hash = buf;
+size_t hash_len = sizeof(buf);
+struct sev_hash_table *sev_ht = NULL;
+int sev_ht_index = 0;
>>
>> Can you move all these variable into a structure, and use it as a
>> SEV loader context?
>>
>> Then each block of code you added can be moved to its own function,
>> self-described, working with the previous context.
>>
>> The functions can be declared in sev_i386.h and defined in sev.c as
>> Eduardo suggested.
>>
>
> Thanks Philippe, I'll try this approach.
>
>
> I assume you mean that an addition like this:
>
> +if (sev_ht) {
> +struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
> +
> +qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> + strlen(kernel_cmdline) + 1,
> + , _len, _fatal);
> +memcpy(e->hash, hash, hash_len);
> +e->len = sizeof(*e);
> +memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +}
>
> will be extracted to a function, and here (in x86_load_linux()) replaced
> with a single call:
>
> sev_kernel_loader_calc_cmdline_hash(_loader_context, kernel_cmdline);
>
> and that function will have an empty stub in non-SEV builds, and a do-
> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> will do the actual work in SEV VMs.
This works, but I'd rather use:
if (sev_enabled()) {
sev_kernel_loader_calc_cmdline_hash(_loader_context,
kernel_cmdline);
}
And have sev_enabled() defined as:
#ifdef CONFIG_SEV
bool sev_enabled(void);
#else
#define sev_enabled() false
#endif
So the compiler could elide the statement if SEV is disabled,
and stub is not necessary.
But that means we'd need to add "#include CONFIG_DEVICES" in
a sysemu/ header, which looks like an anti-pattern.
Thomas / Paolo, what do you think?
> Also, should I base my next version on top of the current master, or on
> top of your SEV-Housekeeping patch series, or on top of some other tree?
It depends its shape :> If the maintainers disagree with it, you better
base your patch on Eduardo's tree. Indeed the code will be different.
If you think the housekeeping is worthwhile, you could also review it
to increase the odds to get it queued ;)
Regards,
Phil.
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On Thu, Jun 17, 2021 at 03:48:52PM +0300, Dov Murik wrote:
>
>
> On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> > Hi Dov, James,
> >
> > +Connor who asked to be reviewer.
> >
> > On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> >> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
> >>> From: James Bottomley
> >>>
> >>> If the VM is using memory encryption and also specifies a kernel/initrd
> >>> or appended command line, calculate the hashes and add them to the
> >>> encrypted data. For this to work, OVMF must support an encrypted area
> >>> to place the data which is advertised via a special GUID in the OVMF
> >>> reset table (if the GUID doesn't exist, the user isn't allowed to pass
> >>> in the kernel/initrd/cmdline via the fw_cfg interface).
> >>>
> >>> The hashes of each of the files is calculated (or the string in the case
> >>> of the cmdline with trailing '\0' included). Each entry in the hashes
> >>> table is GUID identified and since they're passed through the memcrypt
> >>> interface, the hash of the encrypted data will be accumulated by the
> >>> PSP.
> >>>
> >>> Signed-off-by: James Bottomley
> >>> Signed-off-by: Dov Murik
> >>> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> >>> strings, remove GCC pragma, fix checkpatch errors]
> >>> ---
> >>>
> >>> OVMF support for handling the table of hashes (verifying that the
> >>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> >>> to the measured hashes in the table) will be posted soon to edk2-devel.
> >>>
> >>> ---
> >>> hw/i386/x86.c | 120 +-
> >>> 1 file changed, 119 insertions(+), 1 deletion(-)
> >>>
> >>
> >> This is not an objection to the patch itself, but: can we do
> >> something to move all sev-related code to sev.c? It would make
> >> the process of assigning a maintainer and reviewing/merging
> >> future patches much simpler.
> >>
> >> I am not familiar with SEV internals, so my only question is
> >> about configurations where SEV is disabled:
> >>
> >> [...]
> >>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> >>> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> >>> const char *initrd_filename = machine->initrd_filename;
> >>> const char *dtb_filename = machine->dtb;
> >>> const char *kernel_cmdline = machine->kernel_cmdline;
> >>> +uint8_t buf[HASH_SIZE];
> >>> +uint8_t *hash = buf;
> >>> +size_t hash_len = sizeof(buf);
> >>> +struct sev_hash_table *sev_ht = NULL;
> >>> +int sev_ht_index = 0;
> >
> > Can you move all these variable into a structure, and use it as a
> > SEV loader context?
> >
> > Then each block of code you added can be moved to its own function,
> > self-described, working with the previous context.
> >
> > The functions can be declared in sev_i386.h and defined in sev.c as
> > Eduardo suggested.
> >
>
> Thanks Philippe, I'll try this approach.
>
>
> I assume you mean that an addition like this:
>
> +if (sev_ht) {
> +struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
> +
> +qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
> + strlen(kernel_cmdline) + 1,
> + , _len, _fatal);
> +memcpy(e->hash, hash, hash_len);
> +e->len = sizeof(*e);
> +memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
> +}
>
> will be extracted to a function, and here (in x86_load_linux()) replaced
> with a single call:
>
> sev_kernel_loader_calc_cmdline_hash(_loader_context, kernel_cmdline);
>
> and that function will have an empty stub in non-SEV builds, and a do-
> nothing condition (at the beginning) if it's an SEV-disabled VM, and
> will do the actual work in SEV VMs.
>
> Right?
I would suggest a generic notification mechanism instead, where
SEV code could register to be notified after the kernel/initrd is
loaded.
Maybe the existing qemu_add_machine_init_done_notifier()
mechanism would be enough for this? Is there a reason the hash
calculation needs to be done inside x86_load_linux(),
specifically?
>
>
> Also, should I base my next version on top of the current master, or on
> top of your SEV-Housekeeping patch series, or on top of some other tree?
>
>
> -Dov
>
> >>>
> >>> /* Align to 16 bytes as a paranoia measure */
> >>> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> >>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
> >>> exit(1);
> >>> }
> >>>
> >>> +if (machine->cgs && machine->cgs->ready) {
> >>
> >> machine->cgs doesn't seem to be a SEV-specific field.
> >> What if machine->cgs->ready is set but SEV is disabled?
> >>
> >>> +uint8_t *data;
> >>> +struct sev_hash_table_descriptor *area;
> >>> +
> >>> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, ,
> >>> NULL)) {
> >>> +fprintf(stderr, "qemu: kernel command line
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 15/06/2021 22:53, Philippe Mathieu-Daudé wrote:
> Hi Dov, James,
>
> +Connor who asked to be reviewer.
>
> On 6/15/21 5:20 PM, Eduardo Habkost wrote:
>> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
>>> From: James Bottomley
>>>
>>> If the VM is using memory encryption and also specifies a kernel/initrd
>>> or appended command line, calculate the hashes and add them to the
>>> encrypted data. For this to work, OVMF must support an encrypted area
>>> to place the data which is advertised via a special GUID in the OVMF
>>> reset table (if the GUID doesn't exist, the user isn't allowed to pass
>>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>>
>>> The hashes of each of the files is calculated (or the string in the case
>>> of the cmdline with trailing '\0' included). Each entry in the hashes
>>> table is GUID identified and since they're passed through the memcrypt
>>> interface, the hash of the encrypted data will be accumulated by the
>>> PSP.
>>>
>>> Signed-off-by: James Bottomley
>>> Signed-off-by: Dov Murik
>>> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
>>> strings, remove GCC pragma, fix checkpatch errors]
>>> ---
>>>
>>> OVMF support for handling the table of hashes (verifying that the
>>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>>
>>> ---
>>> hw/i386/x86.c | 120 +-
>>> 1 file changed, 119 insertions(+), 1 deletion(-)
>>>
>>
>> This is not an objection to the patch itself, but: can we do
>> something to move all sev-related code to sev.c? It would make
>> the process of assigning a maintainer and reviewing/merging
>> future patches much simpler.
>>
>> I am not familiar with SEV internals, so my only question is
>> about configurations where SEV is disabled:
>>
>> [...]
>>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>>> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
>>> const char *initrd_filename = machine->initrd_filename;
>>> const char *dtb_filename = machine->dtb;
>>> const char *kernel_cmdline = machine->kernel_cmdline;
>>> +uint8_t buf[HASH_SIZE];
>>> +uint8_t *hash = buf;
>>> +size_t hash_len = sizeof(buf);
>>> +struct sev_hash_table *sev_ht = NULL;
>>> +int sev_ht_index = 0;
>
> Can you move all these variable into a structure, and use it as a
> SEV loader context?
>
> Then each block of code you added can be moved to its own function,
> self-described, working with the previous context.
>
> The functions can be declared in sev_i386.h and defined in sev.c as
> Eduardo suggested.
>
Thanks Philippe, I'll try this approach.
I assume you mean that an addition like this:
+if (sev_ht) {
+struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
+
+qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
+ strlen(kernel_cmdline) + 1,
+ , _len, _fatal);
+memcpy(e->hash, hash, hash_len);
+e->len = sizeof(*e);
+memcpy(e->guid, sev_cmdline_entry_guid, sizeof(e->guid));
+}
will be extracted to a function, and here (in x86_load_linux()) replaced
with a single call:
sev_kernel_loader_calc_cmdline_hash(_loader_context, kernel_cmdline);
and that function will have an empty stub in non-SEV builds, and a do-
nothing condition (at the beginning) if it's an SEV-disabled VM, and
will do the actual work in SEV VMs.
Right?
Also, should I base my next version on top of the current master, or on
top of your SEV-Housekeeping patch series, or on top of some other tree?
-Dov
>>>
>>> /* Align to 16 bytes as a paranoia measure */
>>> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>>> exit(1);
>>> }
>>>
>>> +if (machine->cgs && machine->cgs->ready) {
>>
>> machine->cgs doesn't seem to be a SEV-specific field.
>> What if machine->cgs->ready is set but SEV is disabled?
>>
>>> +uint8_t *data;
>>> +struct sev_hash_table_descriptor *area;
>>> +
>>> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, ,
>>> NULL)) {
>>> +fprintf(stderr, "qemu: kernel command line specified but OVMF
>>> has "
>>> +"no hash table guid\n");
>>> +exit(1);
>>> +}
>>> +area = (struct sev_hash_table_descriptor *)data;
>>> +
>>> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
>>> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
>>> sizeof(sev_ht->guid));
>>> +sev_ht->len = sizeof(*sev_ht);
>>> +}
>>> +
>>> /* kernel protocol version */
>>> if (ldl_p(header + 0x202) == 0x53726448) {
>>> protocol = lduw_p(header + 0x206);
>> [...]
>>
>
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Hi Eduardo,
On 15/06/2021 18:20, Eduardo Habkost wrote:
> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
>> From: James Bottomley
>>
>> If the VM is using memory encryption and also specifies a kernel/initrd
>> or appended command line, calculate the hashes and add them to the
>> encrypted data. For this to work, OVMF must support an encrypted area
>> to place the data which is advertised via a special GUID in the OVMF
>> reset table (if the GUID doesn't exist, the user isn't allowed to pass
>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>
>> The hashes of each of the files is calculated (or the string in the case
>> of the cmdline with trailing '\0' included). Each entry in the hashes
>> table is GUID identified and since they're passed through the memcrypt
>> interface, the hash of the encrypted data will be accumulated by the
>> PSP.
>>
>> Signed-off-by: James Bottomley
>> Signed-off-by: Dov Murik
>> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
>> strings, remove GCC pragma, fix checkpatch errors]
>> ---
>>
>> OVMF support for handling the table of hashes (verifying that the
>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>
>> ---
>> hw/i386/x86.c | 120 +-
>> 1 file changed, 119 insertions(+), 1 deletion(-)
>>
>
> This is not an objection to the patch itself, but: can we do
> something to move all sev-related code to sev.c? It would make
> the process of assigning a maintainer and reviewing/merging
> future patches much simpler.
>
I'll look into this following Philippe's suggestions.
> I am not familiar with SEV internals, so my only question is
> about configurations where SEV is disabled:
>
> [...]
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
>> const char *initrd_filename = machine->initrd_filename;
>> const char *dtb_filename = machine->dtb;
>> const char *kernel_cmdline = machine->kernel_cmdline;
>> +uint8_t buf[HASH_SIZE];
>> +uint8_t *hash = buf;
>> +size_t hash_len = sizeof(buf);
>> +struct sev_hash_table *sev_ht = NULL;
>> +int sev_ht_index = 0;
>>
>> /* Align to 16 bytes as a paranoia measure */
>> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>> exit(1);
>> }
>>
>> +if (machine->cgs && machine->cgs->ready) {
>
> machine->cgs doesn't seem to be a SEV-specific field.
> What if machine->cgs->ready is set but SEV is disabled?
>
You're right; I'll change this to sev_enabled() like in
hw/i386/pc_sysfw.c .
-Dov
>> +uint8_t *data;
>> +struct sev_hash_table_descriptor *area;
>> +
>> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, ,
>> NULL)) {
>> +fprintf(stderr, "qemu: kernel command line specified but OVMF
>> has "
>> +"no hash table guid\n");
>> +exit(1);
>> +}
>> +area = (struct sev_hash_table_descriptor *)data;
>> +
>> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
>> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
>> sizeof(sev_ht->guid));
>> +sev_ht->len = sizeof(*sev_ht);
>> +}
>> +
>> /* kernel protocol version */
>> if (ldl_p(header + 0x202) == 0x53726448) {
>> protocol = lduw_p(header + 0x206);
> [...]
>
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
Hi Dov, James,
+Connor who asked to be reviewer.
On 6/15/21 5:20 PM, Eduardo Habkost wrote:
> On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
>> From: James Bottomley
>>
>> If the VM is using memory encryption and also specifies a kernel/initrd
>> or appended command line, calculate the hashes and add them to the
>> encrypted data. For this to work, OVMF must support an encrypted area
>> to place the data which is advertised via a special GUID in the OVMF
>> reset table (if the GUID doesn't exist, the user isn't allowed to pass
>> in the kernel/initrd/cmdline via the fw_cfg interface).
>>
>> The hashes of each of the files is calculated (or the string in the case
>> of the cmdline with trailing '\0' included). Each entry in the hashes
>> table is GUID identified and since they're passed through the memcrypt
>> interface, the hash of the encrypted data will be accumulated by the
>> PSP.
>>
>> Signed-off-by: James Bottomley
>> Signed-off-by: Dov Murik
>> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
>> strings, remove GCC pragma, fix checkpatch errors]
>> ---
>>
>> OVMF support for handling the table of hashes (verifying that the
>> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
>> to the measured hashes in the table) will be posted soon to edk2-devel.
>>
>> ---
>> hw/i386/x86.c | 120 +-
>> 1 file changed, 119 insertions(+), 1 deletion(-)
>>
>
> This is not an objection to the patch itself, but: can we do
> something to move all sev-related code to sev.c? It would make
> the process of assigning a maintainer and reviewing/merging
> future patches much simpler.
>
> I am not familiar with SEV internals, so my only question is
> about configurations where SEV is disabled:
>
> [...]
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
>> const char *initrd_filename = machine->initrd_filename;
>> const char *dtb_filename = machine->dtb;
>> const char *kernel_cmdline = machine->kernel_cmdline;
>> +uint8_t buf[HASH_SIZE];
>> +uint8_t *hash = buf;
>> +size_t hash_len = sizeof(buf);
>> +struct sev_hash_table *sev_ht = NULL;
>> +int sev_ht_index = 0;
Can you move all these variable into a structure, and use it as a
SEV loader context?
Then each block of code you added can be moved to its own function,
self-described, working with the previous context.
The functions can be declared in sev_i386.h and defined in sev.c as
Eduardo suggested.
>>
>> /* Align to 16 bytes as a paranoia measure */
>> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
>> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
>> exit(1);
>> }
>>
>> +if (machine->cgs && machine->cgs->ready) {
>
> machine->cgs doesn't seem to be a SEV-specific field.
> What if machine->cgs->ready is set but SEV is disabled?
>
>> +uint8_t *data;
>> +struct sev_hash_table_descriptor *area;
>> +
>> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, ,
>> NULL)) {
>> +fprintf(stderr, "qemu: kernel command line specified but OVMF
>> has "
>> +"no hash table guid\n");
>> +exit(1);
>> +}
>> +area = (struct sev_hash_table_descriptor *)data;
>> +
>> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
>> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
>> sizeof(sev_ht->guid));
>> +sev_ht->len = sizeof(*sev_ht);
>> +}
>> +
>> /* kernel protocol version */
>> if (ldl_p(header + 0x202) == 0x53726448) {
>> protocol = lduw_p(header + 0x206);
> [...]
>
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On Tue, May 25, 2021 at 06:59:31AM +, Dov Murik wrote:
> From: James Bottomley
>
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data. For this to work, OVMF must support an encrypted area
> to place the data which is advertised via a special GUID in the OVMF
> reset table (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
>
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included). Each entry in the hashes
> table is GUID identified and since they're passed through the memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
>
> Signed-off-by: James Bottomley
> Signed-off-by: Dov Murik
> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
>
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
>
> ---
> hw/i386/x86.c | 120 +-
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
This is not an objection to the patch itself, but: can we do
something to move all sev-related code to sev.c? It would make
the process of assigning a maintainer and reviewing/merging
future patches much simpler.
I am not familiar with SEV internals, so my only question is
about configurations where SEV is disabled:
[...]
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *initrd_filename = machine->initrd_filename;
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> +uint8_t buf[HASH_SIZE];
> +uint8_t *hash = buf;
> +size_t hash_len = sizeof(buf);
> +struct sev_hash_table *sev_ht = NULL;
> +int sev_ht_index = 0;
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
> exit(1);
> }
>
> +if (machine->cgs && machine->cgs->ready) {
machine->cgs doesn't seem to be a SEV-specific field.
What if machine->cgs->ready is set but SEV is disabled?
> +uint8_t *data;
> +struct sev_hash_table_descriptor *area;
> +
> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, , NULL))
> {
> +fprintf(stderr, "qemu: kernel command line specified but OVMF
> has "
> +"no hash table guid\n");
> +exit(1);
> +}
> +area = (struct sev_hash_table_descriptor *)data;
> +
> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
> sizeof(sev_ht->guid));
> +sev_ht->len = sizeof(*sev_ht);
> +}
> +
> /* kernel protocol version */
> if (ldl_p(header + 0x202) == 0x53726448) {
> protocol = lduw_p(header + 0x206);
[...]
--
Eduardo
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
ping
Reminder: this is to support secure (measured) boot with AMD SEV with
QEMU's -kernel/-initrd/-append switches.
The OVMF side of the implementation is under review (with some changes
requested), but so far no functional changes are exepcted from the QEMU
side, on top of this proposed patch.
(+Cc: Dave)
-Dov
On 25/05/2021 9:59, Dov Murik wrote:
> From: James Bottomley
>
> If the VM is using memory encryption and also specifies a kernel/initrd
> or appended command line, calculate the hashes and add them to the
> encrypted data. For this to work, OVMF must support an encrypted area
> to place the data which is advertised via a special GUID in the OVMF
> reset table (if the GUID doesn't exist, the user isn't allowed to pass
> in the kernel/initrd/cmdline via the fw_cfg interface).
>
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included). Each entry in the hashes
> table is GUID identified and since they're passed through the memcrypt
> interface, the hash of the encrypted data will be accumulated by the
> PSP.
>
> Signed-off-by: James Bottomley
> Signed-off-by: Dov Murik
> [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
> strings, remove GCC pragma, fix checkpatch errors]
> ---
>
> OVMF support for handling the table of hashes (verifying that the
> kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
> to the measured hashes in the table) will be posted soon to edk2-devel.
>
> ---
> hw/i386/x86.c | 120 +-
> 1 file changed, 119 insertions(+), 1 deletion(-)
>
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index ed796fe6ba..d8e77b99b4 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -37,12 +37,16 @@
> #include "sysemu/replay.h"
> #include "sysemu/sysemu.h"
> #include "sysemu/cpu-timers.h"
> +#include "sysemu/sev.h"
> +#include "exec/confidential-guest-support.h"
> #include "trace.h"
> +#include "crypto/hash.h"
>
> #include "hw/i386/x86.h"
> #include "target/i386/cpu.h"
> #include "hw/i386/topology.h"
> #include "hw/i386/fw_cfg.h"
> +#include "hw/i386/pc.h"
> #include "hw/intc/i8259.h"
> #include "hw/rtc/mc146818rtc.h"
>
> @@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
> return true;
> }
>
> +struct sev_hash_table_descriptor {
> +uint32_t base;
> +uint32_t size;
> +};
> +
> +/* hard code sha256 digest size */
> +#define HASH_SIZE 32
> +
> +struct sev_hash_table_entry {
> +uint8_t guid[16];
> +uint16_t len;
> +uint8_t hash[HASH_SIZE];
> +} __attribute__ ((packed));
> +
> +struct sev_hash_table {
> +uint8_t guid[16];
> +uint16_t len;
> +struct sev_hash_table_entry entries[];
> +} __attribute__((packed));
> +
> +#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +
> +static const uint8_t sev_hash_table_header_guid[] =
> +UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
> +0xd4, 0x11, 0xfd, 0x21);
> +
> +static const uint8_t sev_kernel_entry_guid[] =
> +UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
> +0x72, 0xd2, 0x04, 0x5b);
> +static const uint8_t sev_initrd_entry_guid[] =
> +UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
> +0x91, 0x69, 0x78, 0x1d);
> +static const uint8_t sev_cmdline_entry_guid[] =
> +UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
> +0x4d, 0x36, 0xab, 0x2a);
> +
> void x86_load_linux(X86MachineState *x86ms,
> FWCfgState *fw_cfg,
> int acpi_data_size,
> @@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
> const char *initrd_filename = machine->initrd_filename;
> const char *dtb_filename = machine->dtb;
> const char *kernel_cmdline = machine->kernel_cmdline;
> +uint8_t buf[HASH_SIZE];
> +uint8_t *hash = buf;
> +size_t hash_len = sizeof(buf);
> +struct sev_hash_table *sev_ht = NULL;
> +int sev_ht_index = 0;
>
> /* Align to 16 bytes as a paranoia measure */
> cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
> @@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
> exit(1);
> }
>
> +if (machine->cgs && machine->cgs->ready) {
> +uint8_t *data;
> +struct sev_hash_table_descriptor *area;
> +
> +if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, , NULL))
> {
> +fprintf(stderr, "qemu: kernel command line specified but OVMF
> has "
> +"no hash table guid\n");
> +exit(1);
> +}
> +area = (struct sev_hash_table_descriptor *)data;
> +
> +sev_ht = qemu_map_ram_ptr(NULL, area->base);
> +memcpy(sev_ht->guid, sev_hash_table_header_guid,
> sizeof(sev_ht->guid));
> +sev_ht->len = sizeof(*sev_ht);
> +}
> +
> /* kernel protocol version
Re: [PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
On 25/05/2021 9:59, Dov Murik wrote: > From: James Bottomley > > If the VM is using memory encryption and also specifies a kernel/initrd > or appended command line, calculate the hashes and add them to the > encrypted data. For this to work, OVMF must support an encrypted area > to place the data which is advertised via a special GUID in the OVMF > reset table (if the GUID doesn't exist, the user isn't allowed to pass > in the kernel/initrd/cmdline via the fw_cfg interface). > > The hashes of each of the files is calculated (or the string in the case > of the cmdline with trailing '\0' included). Each entry in the hashes > table is GUID identified and since they're passed through the memcrypt > interface, the hash of the encrypted data will be accumulated by the > PSP. > > Signed-off-by: James Bottomley > Signed-off-by: Dov Murik > [dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID > strings, remove GCC pragma, fix checkpatch errors] > --- > > OVMF support for handling the table of hashes (verifying that the > kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond > to the measured hashes in the table) will be posted soon to edk2-devel. > OVMF support was submitted to edk2-devel (patch series "Measured SEV boot with kernel/initrd/cmdline"), which starts here: https://edk2.groups.io/g/devel/topic/patch_v1_0_8_measured_sev/83074450 -Dov
[PATCH] x86: add SEV hashing to fw_cfg for kernel/initrd/cmdline
From: James Bottomley
If the VM is using memory encryption and also specifies a kernel/initrd
or appended command line, calculate the hashes and add them to the
encrypted data. For this to work, OVMF must support an encrypted area
to place the data which is advertised via a special GUID in the OVMF
reset table (if the GUID doesn't exist, the user isn't allowed to pass
in the kernel/initrd/cmdline via the fw_cfg interface).
The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included). Each entry in the hashes
table is GUID identified and since they're passed through the memcrypt
interface, the hash of the encrypted data will be accumulated by the
PSP.
Signed-off-by: James Bottomley
Signed-off-by: Dov Murik
[dovmu...@linux.ibm.com: use machine->cgs, remove parsing of GUID
strings, remove GCC pragma, fix checkpatch errors]
---
OVMF support for handling the table of hashes (verifying that the
kernel/initrd/cmdline passed via the fw_cfg interface indeed correspond
to the measured hashes in the table) will be posted soon to edk2-devel.
---
hw/i386/x86.c | 120 +-
1 file changed, 119 insertions(+), 1 deletion(-)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index ed796fe6ba..d8e77b99b4 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -37,12 +37,16 @@
#include "sysemu/replay.h"
#include "sysemu/sysemu.h"
#include "sysemu/cpu-timers.h"
+#include "sysemu/sev.h"
+#include "exec/confidential-guest-support.h"
#include "trace.h"
+#include "crypto/hash.h"
#include "hw/i386/x86.h"
#include "target/i386/cpu.h"
#include "hw/i386/topology.h"
#include "hw/i386/fw_cfg.h"
+#include "hw/i386/pc.h"
#include "hw/intc/i8259.h"
#include "hw/rtc/mc146818rtc.h"
@@ -758,6 +762,42 @@ static bool load_elfboot(const char *kernel_filename,
return true;
}
+struct sev_hash_table_descriptor {
+uint32_t base;
+uint32_t size;
+};
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+struct sev_hash_table_entry {
+uint8_t guid[16];
+uint16_t len;
+uint8_t hash[HASH_SIZE];
+} __attribute__ ((packed));
+
+struct sev_hash_table {
+uint8_t guid[16];
+uint16_t len;
+struct sev_hash_table_entry entries[];
+} __attribute__((packed));
+
+#define SEV_HASH_TABLE_RV_GUID "7255371f-3a3b-4b04-927b-1da6efa8d454"
+
+static const uint8_t sev_hash_table_header_guid[] =
+UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
+0xd4, 0x11, 0xfd, 0x21);
+
+static const uint8_t sev_kernel_entry_guid[] =
+UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
+0x72, 0xd2, 0x04, 0x5b);
+static const uint8_t sev_initrd_entry_guid[] =
+UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
+0x91, 0x69, 0x78, 0x1d);
+static const uint8_t sev_cmdline_entry_guid[] =
+UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
+0x4d, 0x36, 0xab, 0x2a);
+
void x86_load_linux(X86MachineState *x86ms,
FWCfgState *fw_cfg,
int acpi_data_size,
@@ -778,6 +818,11 @@ void x86_load_linux(X86MachineState *x86ms,
const char *initrd_filename = machine->initrd_filename;
const char *dtb_filename = machine->dtb;
const char *kernel_cmdline = machine->kernel_cmdline;
+uint8_t buf[HASH_SIZE];
+uint8_t *hash = buf;
+size_t hash_len = sizeof(buf);
+struct sev_hash_table *sev_ht = NULL;
+int sev_ht_index = 0;
/* Align to 16 bytes as a paranoia measure */
cmdline_size = (strlen(kernel_cmdline) + 16) & ~15;
@@ -799,6 +844,22 @@ void x86_load_linux(X86MachineState *x86ms,
exit(1);
}
+if (machine->cgs && machine->cgs->ready) {
+uint8_t *data;
+struct sev_hash_table_descriptor *area;
+
+if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, , NULL)) {
+fprintf(stderr, "qemu: kernel command line specified but OVMF has "
+"no hash table guid\n");
+exit(1);
+}
+area = (struct sev_hash_table_descriptor *)data;
+
+sev_ht = qemu_map_ram_ptr(NULL, area->base);
+memcpy(sev_ht->guid, sev_hash_table_header_guid, sizeof(sev_ht->guid));
+sev_ht->len = sizeof(*sev_ht);
+}
+
/* kernel protocol version */
if (ldl_p(header + 0x202) == 0x53726448) {
protocol = lduw_p(header + 0x206);
@@ -925,6 +986,17 @@ void x86_load_linux(X86MachineState *x86ms,
fw_cfg_add_i32(fw_cfg, FW_CFG_CMDLINE_SIZE, strlen(kernel_cmdline) + 1);
fw_cfg_add_string(fw_cfg, FW_CFG_CMDLINE_DATA, kernel_cmdline);
+if (sev_ht) {
+struct sev_hash_table_entry *e = _ht->entries[sev_ht_index++];
+
+qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, (char *)kernel_cmdline,
+ strlen(kernel_cmdline) + 1,
+ , _len, _fatal);
+memcpy(e->hash, hash,
