On Wed, Oct 26, 2022 at 02:57:33PM +0300, Dov Murik wrote: > (sorry in advance for missing CCs, I tried to download the mbox from > https://listman.redhat.com/archives/libvir-list/ but it doesn't include > the To and Cc lines of the messages.) > > > On 19/10/2022 13:17, berrange at redhat.com (Daniel P. Berrangé) wrote: > > When doing direct kernel boot we need to include the kernel, initrd and > > cmdline in the measurement. > > > > Signed-off-by: Daniel P. Berrang? <berrange at redhat.com> > > --- > > docs/manpages/virt-qemu-sev-validate.rst | 43 ++++++++++ > > tools/virt-qemu-sev-validate | 102 ++++++++++++++++++++++- > > 2 files changed, 144 insertions(+), 1 deletion(-) > > > > diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst > > index 2c02a27103..da804ae6a0 100644 > > --- a/docs/manpages/virt-qemu-sev-validate.rst > > +++ b/docs/manpages/virt-qemu-sev-validate.rst > > @@ -102,6 +102,20 @@ initialize AMD SEV. For the validation to be trustworthy it important that the > > firmware build used has no support for loading non-volatile variables from > > NVRAM, even if NVRAM is expose to the guest. > > > > +``-k PATH``, ``--kernel=PATH`` > > + > > +Path to the kernel binary if doing direct kernel boot. > > + > > +``-r PATH``, ``--initrd=PATH`` > > + > > +Path to the initrd binary if doing direct kernel boot. Defaults to zero length > > +content if omitted. > > + > > +``-e STRING``, ``--cmdline=STRING`` > > + > > +String containing any kernel command line parameters used during boot of the > > +domain. Defaults to the empty string if omitted. > > + > > ``--tik PATH`` > > > > TIK file for domain. This file must be exactly 16 bytes in size and contains the > > @@ -180,6 +194,22 @@ Validate the measurement of a SEV guest booting from disk: > > --build-id 13 \ > > --policy 3 > > > > +Validate the measurement of a SEV guest with direct kernel boot: > > + > > +:: > > + > > + # virt-dom-sev-validate \ > > + --firmware OVMF.sev.fd \ > > + --kernel vmlinuz-5.11.12 \ > > + --initrd initramfs-5.11.12 \ > > + --cmdline "root=/dev/vda1" \ > > + --tk this-guest-tk.bin \ > > + --measurement Zs2pf19ubFSafpZ2WKkwquXvACx9Wt/BV+eJwQ/taO8jhyIj/F8swFrybR1fZ2ID \ > > + --api-major 0 \ > > + --api-minor 24 \ > > + --build-id 13 \ > > + --policy 3 > > + > > Fetch from remote libvirt > > ------------------------- > > > > @@ -200,6 +230,19 @@ Validate the measurement of a SEV guest booting from disk: > > --tk this-guest-tk.bin \ > > --domain fedora34x86_64 > > > > +Validate the measurement of a SEV guest with direct kernel boot: > > + > > +:: > > + > > + # virt-dom-sev-validate \ > > + --connect qemu+ssh://root at some.remote.host/system \ > > + --firmware OVMF.sev.fd \ > > + --kernel vmlinuz-5.11.12 \ > > + --initrd initramfs-5.11.12 \ > > + --cmdline "root=/dev/vda1" \ > > + --tk this-guest-tk.bin \ > > + --domain fedora34x86_64 > > + > > Fetch from local libvirt > > ------------------------ > > > > diff --git a/tools/virt-qemu-sev-validate b/tools/virt-qemu-sev-validate > > index eb9485c6ed..062f9545f8 100755 > > --- a/tools/virt-qemu-sev-validate > > +++ b/tools/virt-qemu-sev-validate > > @@ -34,6 +34,7 @@ > > # firmware versions with known flaws. > > # > > > > +import abc > > import argparse > > from base64 import b64decode > > from hashlib import sha256 > > @@ -43,6 +44,7 @@ import re > > import socket > > import sys > > import traceback > > +from uuid import UUID > > > > from lxml import etree > > import libvirt > > @@ -70,6 +72,85 @@ class InvalidStateException(Exception): > > pass > > > > > > +class GUIDTable(abc.ABC): > > + GUID_LEN = 16 > > + > > + def __init__(self, guid, lenlen=2): > > + self.guid = guid > > + self.lenlen = lenlen > > + > > + @abc.abstractmethod > > + def entries(self): > > + pass > > + > > + def build_entry(self, guid, payload, lenlen): > > + dummylen = int(0).to_bytes(lenlen, 'little') > > + entry = bytearray(guid + dummylen + payload) > > + > > + lenle = len(entry).to_bytes(lenlen, 'little') > > + entry[self.GUID_LEN:(self.GUID_LEN + lenlen)] = lenle > > + > > + return bytes(entry) > > + > > + def build(self): > > + payload = self.entries() > > + > > + if len(payload) == 0: > > + return bytes([]) > > + > > + dummylen = int(0).to_bytes(self.lenlen, 'little') > > + table = bytearray(self.guid + dummylen + payload) > > + > > + guidlen = len(table).to_bytes(self.lenlen, 'little') > > + table[self.GUID_LEN:(self.GUID_LEN + self.lenlen)] = guidlen > > + > > + pad = 16 - (len(table) % 16) > > + table += bytes([0]) * pad > > + > > + log.debug("Table: %s", bytes(table).hex()) > > + return bytes(table) > > + > > + > > +class KernelTable(GUIDTable): > > + > > + TABLE_GUID = UUID('{9438d606-4f22-4cc9-b479-a793-d411fd21}').bytes_le > > + KERNEL_GUID = UUID('{4de79437-abd2-427f-b835-d5b1-72d2045b}').bytes_le > > + INITRD_GUID = UUID('{44baf731-3a2f-4bd7-9af1-41e2-9169781d}').bytes_le > > + CMDLINE_GUID = UUID('{97d02dd8-bd20-4c94-aa78-e771-4d36ab2a}').bytes_le > > + > > + def __init__(self): > > + super().__init__(guid=self.TABLE_GUID, > > + lenlen=2) > > + > > + self.kernel = None > > + self.initrd = None > > + self.cmdline = None > > + > > + def load_kernel(self, path): > > + with open(path, "rb") as fh: > > + self.kernel = sha256(fh.read()).digest() > > + log.debug("Kernel: %s", self.kernel.hex()) > > + > > + def load_initrd(self, path): > > + with open(path, "rb") as fh: > > + self.initrd = sha256(fh.read()).digest() > > + log.debug("Initrd: %s", self.initrd.hex()) > > + > > + def load_cmdline(self, val): > > + self.cmdline = sha256(val.encode("utf8") + bytes([0])).digest() > > + log.debug("Cmdline: %s", self.cmdline.hex()) > > + > > + def entries(self): > > + entries = bytes([]) > > + if self.cmdline is not None: > > + entries += self.build_entry(self.CMDLINE_GUID, self.cmdline, 2) > > + if self.initrd is not None: > > + entries += self.build_entry(self.INITRD_GUID, self.initrd, 2) > > I think this will not work correctly if cmdline and/or initrd are not > supplied. The QEMU behaviour is to always include all three entries in > the table. > > If initrd is not supplied then its entry should be sha256(""). > > If cmdline is not supplied then its entry should be sha256(bytes[0]). Ah yes, I had known that, but completely forgot to address it before sending. I wonder if we should fully describe the SEV hashes table in the docs/specs/sev-guest-firmware.rst file, to avoid others making the same mistake. > In any case, for direct boot kernel must be supplied. It doesn't make > sense to pass initrd or cmdline without kernel. Good point, will make that combination report an error. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
