This function is fine to use in other languages Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- build-aux/syntax-check.mk | 1 + 1 file changed, 1 insertion(+) diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index 649eb91acb..41970d31a1 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -203,6 +203,7 @@ sc_prohibit_readlink: sc_prohibit_gethostname: @prohibit='gethostname *\(' \ + in_vc_files='\.[ch]$$' \ halt='use virGetHostname, not gethostname' \ $(_sc_search_regexp) -- 2.37.3

Accept information about a connection to libvirt and a guest on the command line. Talk to libvirt to obtain the running guest state and automatically detect as much configuration as possible. It will refuse to use a libvirt connection that is thought to be local to the current machine, as running this tool on the hypervisor itself is not considered secure. This can be overridden using the --insecure flag. When querying the guest, it will also analyse the XML configuration in an attempt to detect any options that are liable to be mistakes. For example the NVRAM being measured should not have a persistent varstore. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- docs/manpages/virt-qemu-sev-validate.rst | 109 +++++++++++- tools/virt-qemu-sev-validate.py | 201 +++++++++++++++++++++-- 2 files changed, 296 insertions(+), 14 deletions(-) diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst index 36de9becfd..2c02a27103 100644 --- a/docs/manpages/virt-qemu-sev-validate.rst +++ b/docs/manpages/virt-qemu-sev-validate.rst @@ -37,6 +37,12 @@ another virtual machine protected by AMD SEV that has already had its launch measurement validated. Running this program on the virtualization host will not produce an answer that can be trusted. +If told to connect to libvirt, it will refuse to use a libvirt connection that +is local to the machine, since that cannot be trusted. For the sake of testing +or demonstration purposes, however, it can be forced to run in this scenario +using the ``--insecure`` flag. The result will, of course, still not be +trustworthy. + OPTIONS ======= @@ -115,6 +121,41 @@ size, with the first 16 bytes containing the TEK and the last 16 bytes containing the TIK. This is mutually exclusive with the ``--tik`` and ``--tek`` arguments. +Libvirt options +--------------- + +These options are used when connecting to libvirt to automatically obtain +state and configuration information about the domain to be attested. + +``-c``, ``--connect URI`` + +Libvirt connection URI. For the validation to be trustworthy this must be a URI +resolving to a remote virtualization host. This requirement can be overridden +using the ``--insecure`` argument + +``-o``, ``--domain ID|NAME|UUID`` + +Domain ID, or domain name or domain UUID. Used to identify which libvirt domain +is to have its launch measured. The domain must be running, and would usually +have been started in a paused state, to allow validation to be performed before +guest CPUs begin execution. + +``-i``, ``--insecure`` + +Proceed even if usage scenario is known to be insecure. This allows the program +to connect to a local libvirt hypervisor and rely on file content from the +virtualization host. The result of the validation must not be trusted. + +``-g``, ``--ignore-config`` + +Do not attempt to sanity check the domain config. The default behaviour is to +print out errors if identifying configuration elements in the guest XML that +would invalidate the launch measurement. This can help the guest owner to +understand any configuration mistakes that have been made. If the +``--ignore-config`` argument is given, this sanity checking of configuration +will be skipped. The result is that the validation will likely be reported as +failed. + EXAMPLES ======== @@ -139,6 +180,46 @@ Validate the measurement of a SEV guest booting from disk: --build-id 13 \ --policy 3 +Fetch from remote libvirt +------------------------- + +This scenario allows fetching certain data from a remote hypervisor via a +connection to libvirt. It will aid in debugging by analysing the guest +configuration and reporting anything that could invalidate the measurement +of the guest. This usage model is considered secure, because the limited +information obtained from the untrusted hypervisor cannot be used to change +the result. + +Validate the measurement of a SEV guest booting from disk: + +:: + + # virt-qemu-sev-validate \ + --connect qemu+ssh://root@some.remote.host/system \ + --firmware OVMF.sev.fd \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 + +Fetch from local libvirt +------------------------ + +This scenario allows fetching all data from the local hypervisor via a +connection to libvirt. It is only to be used for the purpose of testing, +debugging, or demonstrations, because running on the local hypervisor is not +a secure scenario. To enable this usage, the ``--insecure`` flag must be +specified. Given a pointer to the libvirt guest to validate, all information +needed to perform a validation, except the TIK/TEK pair can be acquired +automatically. + +Validate the measurement of a SEV guest booting from disk: + +:: + + # virt-qemu-sev-validate \ + --insecure \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 + EXIT STATUS =========== @@ -158,7 +239,33 @@ be set: The way in which this program has been invoked prevent it from being able to validate the launch measurement. -* **3** - *unexpected error occurred in the code* +* **3** - *Usage scenario is not secure* + + The way in which this program has been invoked means that the result of any + launch measurement validation will not be secure. + + The program can be reinvoked with ``--insecure`` argument to force a + validation, however, the results of this should not be trusted. This should + only be used for testing, debugging or demonstration purposes, never in a + production deployment. + +* **4** - *Domain has incorrect configuration to be measured* + + The way in which the guest has been configured prevent this program from being + able to validate the launch measurement. Note that in general the guest + configuration reported by the hypervisor is not trustworthy, so it is + possible this error could be a false positive designed to cause a denial of + service. + + This program can be reinvoked with the ``--ignore-config`` argument to skip + the sanity checks on the domain XML. This will likely result in it failing + with an exit code of **1** indicating the measurement is invalid + +* **5** - *Domain is in incorrect state to be measured* + + The domain has to be running in order to validate a launch measurement. + +* **6** - *unexpected error occurred in the code* A logic flaw in this program means it is unable to complete the validation of the measurement. This is a bug which should be reported to the maintainers. diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py index e206b3befa..c47e565224 100755 --- a/tools/virt-qemu-sev-validate.py +++ b/tools/virt-qemu-sev-validate.py @@ -38,7 +38,11 @@ import argparse from base64 import b64decode from hashlib import sha256 import hmac +import libvirt import logging +from lxml import etree +import re +import socket import sys import traceback @@ -53,6 +57,18 @@ class UnsupportedUsageException(Exception): pass +class InsecureUsageException(Exception): + pass + + +class IncorrectConfigException(Exception): + pass + + +class InvalidStateException(Exception): + pass + + class ConfidentialVM(object): def __init__(self, @@ -159,6 +175,103 @@ class ConfidentialVM(object): "Measurement does not match, VM is not trustworthy") +class LibvirtConfidentialVM(ConfidentialVM): + def __init__(self, **kwargs): + super().__init__(**kwargs) + + self.conn = None + self.dom = None + + def check_domain(self, doc, secure): + ls = doc.xpath("/domain/launchSecurity[@type='sev']") + if len(ls) != 1: + raise IncorrectConfigException( + "Domain is not configured with SEV launch security") + + dh = doc.xpath("/domain/launchSecurity[@type='sev']/dhCert") + if len(dh) != 1: + raise IncorrectConfigException( + "Domain must have SEV owner cert to validate measurement") + + session = doc.xpath("/domain/launchSecurity[@type='sev']/session") + if len(session) != 1: + raise IncorrectConfigException( + "Domain must have SEV session data to validate measurement") + + nvramnodes = doc.xpath("/domain/os/nvram") + if len(nvramnodes) != 0 and secure: + raise InsecureUsageException( + "Domain firmware with NVRAM cannot be securely measured") + + loadernodes = doc.xpath("/domain/os/loader") + if len(loadernodes) != 1: + raise IncorrectConfigException( + "Domain must have one firmware path") + + def load_domain(self, uri, id_name_uuid, secure, ignore_config): + self.conn = libvirt.open(uri) + + remote = socket.gethostname() != self.conn.getHostname() + if not remote and secure: + raise InsecureUsageException( + "running locally on the hypervisor host is not secure") + + if re.match(r'^\d+$', id_name_uuid): + self.dom = self.conn.lookupByID(int(id_name_uuid)) + elif re.match(r'^[-a-f0-9]+$', id_name_uuid): + self.dom = self.conn.lookupByUUIDString(id_name_uuid) + else: + self.dom = self.conn.lookupByName(id_name_uuid) + + log.debug("VM: id=%d name=%s uuid=%s" % ( + self.dom.ID(), self.dom.name(), self.dom.UUIDString())) + + if not self.dom.isActive(): + raise InvalidStateException( + "Domain must be running to validate measurement") + + xml = self.dom.XMLDesc() + + doc = etree.fromstring(xml) + if not ignore_config: + self.check_domain(doc, secure) + + # See comments at top of file wrt why we are OK to trust the + # sev-api-major, sev-api-minor, sev-build-id and sev-policy data + # reported by the host + sevinfo = self.dom.launchSecurityInfo() + + if "sev-api-major" not in sevinfo: + raise UnsupportedUsageException( + "'api-major' not reported in domain launch security info") + + if self.measurement is None: + self.measurement = sevinfo["sev-measurement"] + if self.api_major is None: + self.api_major = sevinfo["sev-api-major"] + if self.api_minor is None: + self.api_minor = sevinfo["sev-api-minor"] + if self.build_id is None: + self.build_id = sevinfo["sev-build-id"] + if self.policy is None: + self.policy = sevinfo["sev-policy"] + + if self.firmware is None: + if remote: + raise UnsupportedUsageException( + "Cannot access loader path remotely") + if secure: + raise InsecureUsageException( + "Using loader path from XML is not secure") + + loadernodes = doc.xpath("/domain/os/loader") + if len(loadernodes) == 0: + raise UnsupportedUsageException( + "--firmware not specified and not loader path found") + + self.load_firmware(loadernodes[0].text) + + def parse_command_line(): parser = argparse.ArgumentParser( description='Validate guest AMD SEV launch measurement') @@ -169,20 +282,20 @@ def parse_command_line(): # Arguments related to the state of the launched guest vmstate = parser.add_argument_group("Virtual machine launch state") - vmstate.add_argument('--measurement', '-m', required=True, + vmstate.add_argument('--measurement', '-m', help='Measurement for the running domain') - vmstate.add_argument('--api-major', type=int, required=True, + vmstate.add_argument('--api-major', type=int, help='SEV API major version for the running domain') - vmstate.add_argument('--api-minor', type=int, required=True, + vmstate.add_argument('--api-minor', type=int, help='SEV API major version for the running domain') - vmstate.add_argument('--build-id', type=int, required=True, + vmstate.add_argument('--build-id', type=int, help='SEV build ID for the running domain') - vmstate.add_argument('--policy', type=int, required=True, + vmstate.add_argument('--policy', type=int, help='SEV policy for the running domain') # Arguments related to calculation of the expected launch measurement vmconfig = parser.add_argument_group("Virtual machine config") - vmconfig.add_argument('--firmware', '-f', required=True, + vmconfig.add_argument('--firmware', '-f', help='Path to the firmware binary') vmconfig.add_argument('--tik', help='TIK file for domain') @@ -191,6 +304,17 @@ def parse_command_line(): vmconfig.add_argument('--tk', help='TEK/TIK combined file for domain') + # Arguments related to the connection to libvirt + vmconn = parser.add_argument_group("Libvirt guest connection") + vmconn.add_argument('--connect', '-c', default="qemu:///system", + help='libvirt connection URI') + vmconn.add_argument('--domain', '-o', + help='domain ID / Name / UUID') + vmconn.add_argument('--insecure', '-i', action='store_true', + help='Proceed even if usage scenario is insecure') + vmconn.add_argument('--ignore-config', '-g', action='store_true', + help='Do not attempt to sanity check the guest config') + return parser.parse_args() @@ -206,21 +330,60 @@ def check_usage(args): raise UnsupportedUsageException( "Either --tk or both of --tek/--tik are required") + if args.domain is None: + if args.measurement is None: + raise UnsupportedUsageException( + "Either --measurement or --domain is required") + + if args.api_major is None: + raise UnsupportedUsageException( + "Either --api-major or --domain is required") + + if args.api_minor is None: + raise UnsupportedUsageException( + "Either --api-minor or --domain is required") + + if args.build_id is None: + raise UnsupportedUsageException( + "Either --build-id or --domain is required") + + if args.policy is None: + raise UnsupportedUsageException( + "Either --policy or --domain is required") + + if args.firmware is None: + raise UnsupportedUsageException( + "Either --firmware or --domain is required") + def attest(args): - cvm = ConfidentialVM(measurement=args.measurement, - api_major=args.api_major, - api_minor=args.api_minor, - build_id=args.build_id, - policy=args.policy) + if args.domain is None: + cvm = ConfidentialVM(measurement=args.measurement, + api_major=args.api_major, + api_minor=args.api_minor, + build_id=args.build_id, + policy=args.policy) + else: + cvm = LibvirtConfidentialVM(measurement=args.measurement, + api_major=args.api_major, + api_minor=args.api_minor, + build_id=args.build_id, + policy=args.policy) - cvm.load_firmware(args.firmware) + if args.firmware is not None: + cvm.load_firmware(args.firmware) if args.tk is not None: cvm.load_tk(args.tk) else: cvm.load_tik_tek(args.tik, args.tek) + if args.domain is not None: + cvm.load_domain(args.connect, + args.domain, + not args.insecure, + args.ignore_config) + cvm.attest() if not args.quiet: @@ -249,9 +412,21 @@ if __name__ == "__main__": if not args.quiet: print("ERROR: %s" % e, file=sys.stderr) sys.exit(2) + except InsecureUsageException as e: + if not args.quiet: + print("ERROR: %s" % e, file=sys.stderr) + sys.exit(3) + except IncorrectConfigException as e: + if not args.quiet: + print("ERROR: %s" % e, file=sys.stderr) + sys.exit(4) + except InvalidStateException as e: + if not args.quiet: + print("ERROR: %s" % e, file=sys.stderr) + sys.exit(5) except Exception as e: if args.debug: traceback.print_tb(e.__traceback__) if not args.quiet: print("ERROR: %s" % e, file=sys.stderr) - sys.exit(3) + sys.exit(6) -- 2.37.3

When connected to libvirt we can validate that the guest configuration has the kernel hashes property enabled, otherwise including the kernel GUID table in our expected measurements is not likely to match the actual measurement. When running locally we can also automatically detect the kernel/initrd paths, along with the cmdline string from the XML. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- docs/manpages/virt-qemu-sev-validate.rst | 9 ++++ tools/virt-qemu-sev-validate.py | 59 ++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst index da804ae6a0..beb40383be 100644 --- a/docs/manpages/virt-qemu-sev-validate.rst +++ b/docs/manpages/virt-qemu-sev-validate.rst @@ -263,6 +263,15 @@ 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 \ + --insecure \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 + EXIT STATUS =========== diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py index 4bc3b925f7..f2eeabac0d 100755 --- a/tools/virt-qemu-sev-validate.py +++ b/tools/virt-qemu-sev-validate.py @@ -293,6 +293,35 @@ class LibvirtConfidentialVM(ConfidentialVM): raise IncorrectConfigException( "Domain must have one firmware path") + measure_kernel_nodes = doc.xpath( + "/domain/launchSecurity[@type='sev']/@kernelHashes") + measure_kernel = False + if len(measure_kernel_nodes) == 1: + if measure_kernel_nodes[0] == "yes": + measure_kernel = True + + xp_kernel = "/domain/os/kernel" + xp_initrd = "/domain/os/initrd" + xp_cmdline = "/domain/os/cmdline" + kern_nodes = (doc.xpath(xp_kernel) + + doc.xpath(xp_initrd) + + doc.xpath(xp_cmdline)) + if not measure_kernel: + if len(self.kernel_table.entries()) != 0: + raise UnsupportedUsageException( + "kernel/initrd/cmdline provided but kernel " + "measurement not enabled") + + # Check for an insecure scenario + if len(kern_nodes) != 0 and secure: + raise InsecureUsageException( + "direct kernel boot present without measurement") + else: + if len(kern_nodes) == 0: + raise IncorrectConfigException( + "kernel/initrd/cmdline not provided but kernel " + "measurement is enabled") + def load_domain(self, uri, id_name_uuid, secure, ignore_config): self.conn = libvirt.open(uri) @@ -356,6 +385,36 @@ class LibvirtConfidentialVM(ConfidentialVM): self.load_firmware(loadernodes[0].text) + if self.kernel_table.kernel is None: + kernelnodes = doc.xpath("/domain/os/kernel") + if len(kernelnodes) != 0: + if remote: + raise UnsupportedUsageException( + "Cannot access kernel path remotely") + if secure: + raise InsecureUsageException( + "Using kernel path from XML is not secure") + self.kernel_table.load_kernel(kernelnodes[0].text) + + if self.kernel_table.initrd is None: + initrdnodes = doc.xpath("/domain/os/initrd") + if len(initrdnodes) != 0: + if remote: + raise UnsupportedUsageException( + "Cannot access initrd path remotely") + if secure: + raise InsecureUsageException( + "Using initrd path from XML is not secure") + self.kernel_table.load_initrd(initrdnodes[0].text) + + if self.kernel_table.cmdline is None: + cmdlinenodes = doc.xpath("/domain/os/cmdline") + if len(cmdlinenodes) != 0: + if secure: + raise InsecureUsageException( + "Using cmdline string from XML is not secure") + self.kernel_table.load_cmdline(cmdlinenodes[0].text) + def parse_command_line(): parser = argparse.ArgumentParser( -- 2.37.3

The VMSA files contain the expected CPU register state for the VM. Their content varies based on a few pieces of the stack - AMD CPU architectural initial state - KVM hypervisor VM CPU initialization - QEMU userspace VM CPU initialization - AMD CPU SKU (family/model/stepping) The first three pieces of information we can obtain through code inspection. The last piece of information we can take on the command line. This allows a user to validate a SEV-ES guest merely by providing the CPU SKU information, using --cpu-family, --cpu-model, --cpu-stepping. This avoids the need to obtain or construct VMSA files directly. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- docs/manpages/virt-qemu-sev-validate.rst | 45 +++ tools/virt-qemu-sev-validate.py | 475 +++++++++++++++++++++++ 2 files changed, 520 insertions(+) diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst index 24bca98d28..7ba7323e13 100644 --- a/docs/manpages/virt-qemu-sev-validate.rst +++ b/docs/manpages/virt-qemu-sev-validate.rst @@ -243,6 +243,24 @@ Validate the measurement of a SEV-ES SMP guest booting from disk: --build-id 13 \ --policy 7 +Validate the measurement of a SEV-ES SMP guest booting from disk, with +automatically constructed VMSA: + +:: + + # virt-dom-sev-validate \ + --firmware OVMF.sev.fd \ + --num-cpus 2 \ + --cpu-family 23 \ + --cpu-model 49 \ + --cpu-stepping 0 \ + --tk this-guest-tk.bin \ + --measurement Zs2pf19ubFSafpZ2WKkwquXvACx9Wt/BV+eJwQ/taO8jhyIj/F8swFrybR1fZ2ID \ + --api-major 0 \ + --api-minor 24 \ + --build-id 13 \ + --policy 7 + Fetch from remote libvirt ------------------------- @@ -289,6 +307,20 @@ Validate the measurement of a SEV-ES SMP guest booting from disk: --tk this-guest-tk.bin \ --domain fedora34x86_64 +Validate the measurement of a SEV-ES SMP guest booting from disk, with +automatically constructed VMSA: + +:: + + # virt-dom-sev-validate \ + --connect qemu+ssh://root@some.remote.host/system \ + --firmware OVMF.sev.fd \ + --cpu-family 23 \ + --cpu-model 49 \ + --cpu-stepping 0 \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 + Fetch from local libvirt ------------------------ @@ -330,6 +362,19 @@ Validate the measurement of a SEV-ES SMP guest booting from disk: --tk this-guest-tk.bin \ --domain fedora34x86_64 +Validate the measurement of a SEV-ES SMP guest booting from disk, with +automatically constructed VMSA: + +:: + + # virt-dom-sev-validate \ + --insecure \ + --cpu-family 23 \ + --cpu-model 49 \ + --cpu-stepping 0 \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 + EXIT STATUS =========== diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py index ea5be80129..2505aff07f 100755 --- a/tools/virt-qemu-sev-validate.py +++ b/tools/virt-qemu-sev-validate.py @@ -44,6 +44,7 @@ import logging from lxml import etree import re import socket +from struct import pack import sys import traceback from uuid import UUID @@ -71,6 +72,435 @@ class InvalidStateException(Exception): pass +class Field(object): + U8 = 0 + U16 = 2 + U32 = 4 + U64 = 8 + + SCALAR = 0 + BITMASK = 1 + ARRAY = 2 + + def __init__(self, name, size, format, value, order): + self.name = name + self.size = size + self.value = value + self.format = format + self.order = order + + +class Struct(object): + def __init__(self, size): + self._fields = {} + self.size = size + + def register_field(self, name, size, format=Field.SCALAR, defvalue=0): + self._fields[name] = Field(name, size, format, + defvalue, len(self.fields)) + + @property + def fields(self): + return sorted(self._fields.values(), key=lambda f: f.order) + + def __getattr__(self, name): + return self._fields[name] + + def __setattr__(self, name, value): + if name in ["_fields", "size"]: + super().__setattr__(name, value) + else: + self._fields[name].value = value + + def binary_format(self): + fmt = ["<"] + datalen = 0 + for field in self.fields: + if field.size == Field.U8: + if field.format == Field.ARRAY: + datalen += len(field.value) + fmt += ["%dB" % len(field.value)] + else: + datalen += 1 + fmt += ["B"] + elif field.size == Field.U16: + datalen += 2 + fmt += ["H"] + elif field.size == Field.U32: + datalen += 4 + fmt += ["L"] + elif field.size == Field.U64: + datalen += 8 + fmt += ["Q"] + + pad = self.size - datalen + assert self.size >= 1 + fmt += ["%dB" % pad] + + return "".join(fmt), pad + + def pack(self): + fmt, pad = self.binary_format() + + values = [] + for field in self.fields: + if field.size == Field.U8 and field.format == Field.ARRAY: + for k in range(len(field.value)): + values.append(field.value[k]) + else: + values.append(field.value) + values.extend([0] * pad) + + return pack(fmt, *values) + + +class VMSA(Struct): + ATTR_G_SHIFT = 23 + ATTR_G_MASK = (1 << ATTR_G_SHIFT) + ATTR_B_SHIFT = 22 + ATTR_B_MASK = (1 << ATTR_B_SHIFT) + ATTR_L_SHIFT = 21 + ATTR_L_MASK = (1 << ATTR_L_SHIFT) + ATTR_AVL_SHIFT = 20 + ATTR_AVL_MASK = (1 << ATTR_AVL_SHIFT) + ATTR_P_SHIFT = 15 + ATTR_P_MASK = (1 << ATTR_P_SHIFT) + ATTR_DPL_SHIFT = 13 + ATTR_DPL_MASK = (3 << ATTR_DPL_SHIFT) + ATTR_S_SHIFT = 12 + ATTR_S_MASK = (1 << ATTR_S_SHIFT) + ATTR_TYPE_SHIFT = 8 + ATTR_TYPE_MASK = (15 << ATTR_TYPE_SHIFT) + ATTR_A_MASK = (1 << 8) + + ATTR_CS_MASK = (1 << 11) + ATTR_C_MASK = (1 << 10) + ATTR_R_MASK = (1 << 9) + + ATTR_E_MASK = (1 << 10) + ATTR_W_MASK = (1 << 9) + + def __init__(self): + super().__init__(4096) + + # From Linux arch/x86/include/asm/svm.h, we're unpacking the + # struct vmcb_save_area + + self.register_field("es_selector", Field.U16) + self.register_field("es_attrib", Field.U16, Field.BITMASK) + self.register_field("es_limit", Field.U32) + self.register_field("es_base", Field.U64) + + self.register_field("cs_selector", Field.U16) + self.register_field("cs_attrib", Field.U16, Field.BITMASK) + self.register_field("cs_limit", Field.U32) + self.register_field("cs_base", Field.U64) + + self.register_field("ss_selector", Field.U16) + self.register_field("ss_attrib", Field.U16, Field.BITMASK) + self.register_field("ss_limit", Field.U32) + self.register_field("ss_base", Field.U64) + + self.register_field("ds_selector", Field.U16) + self.register_field("ds_attrib", Field.U16, Field.BITMASK) + self.register_field("ds_limit", Field.U32) + self.register_field("ds_base", Field.U64) + + self.register_field("fs_selector", Field.U16) + self.register_field("fs_attrib", Field.U16, Field.BITMASK) + self.register_field("fs_limit", Field.U32) + self.register_field("fs_base", Field.U64) + + self.register_field("gs_selector", Field.U16) + self.register_field("gs_attrib", Field.U16, Field.BITMASK) + self.register_field("gs_limit", Field.U32) + self.register_field("gs_base", Field.U64) + + self.register_field("gdtr_selector", Field.U16) + self.register_field("gdtr_attrib", Field.U16, Field.BITMASK) + self.register_field("gdtr_limit", Field.U32) + self.register_field("gdtr_base", Field.U64) + + self.register_field("ldtr_selector", Field.U16) + self.register_field("ldtr_attrib", Field.U16, Field.BITMASK) + self.register_field("ldtr_limit", Field.U32) + self.register_field("ldtr_base", Field.U64) + + self.register_field("idtr_selector", Field.U16) + self.register_field("idtr_attrib", Field.U16, Field.BITMASK) + self.register_field("idtr_limit", Field.U32) + self.register_field("idtr_base", Field.U64) + + self.register_field("tr_selector", Field.U16) + self.register_field("tr_attrib", Field.U16, Field.BITMASK) + self.register_field("tr_limit", Field.U32) + self.register_field("tr_base", Field.U64) + + self.register_field("reserved_1", + Field.U8, Field.ARRAY, bytearray([0] * 43)) + + self.register_field("cpl", Field.U8) + + self.register_field("reserved_2", + Field.U8, Field.ARRAY, bytearray([0] * 4)) + + self.register_field("efer", Field.U64) + + self.register_field("reserved_3", + Field.U8, Field.ARRAY, bytearray([0] * 104)) + + self.register_field("xss", Field.U64) + self.register_field("cr4", Field.U64) + self.register_field("cr3", Field.U64) + self.register_field("cr0", Field.U64) + self.register_field("dr7", Field.U64) + self.register_field("dr6", Field.U64) + self.register_field("rflags", Field.U64) + self.register_field("rip", Field.U64) + + self.register_field("reserved_4", + Field.U8, Field.ARRAY, bytearray([0] * 88)) + + self.register_field("rsp", Field.U64) + + self.register_field("reserved_5", + Field.U8, Field.ARRAY, bytearray([0] * 24)) + + self.register_field("rax", Field.U64) + self.register_field("star", Field.U64) + self.register_field("lstar", Field.U64) + self.register_field("cstar", Field.U64) + self.register_field("sfmask", Field.U64) + self.register_field("kernel_gs_base", Field.U64) + self.register_field("sysenter_cs", Field.U64) + self.register_field("sysenter_esp", Field.U64) + self.register_field("sysenter_eip", Field.U64) + self.register_field("cr2", Field.U64) + + self.register_field("reserved_6", + Field.U8, Field.ARRAY, bytearray([0] * 32)) + + self.register_field("g_pat", Field.U64) + self.register_field("dbgctl", Field.U64) + self.register_field("br_from", Field.U64) + self.register_field("br_to", Field.U64) + self.register_field("last_excp_from", Field.U64) + self.register_field("last_excp_to", Field.U64) + + self.register_field("reserved_7", + Field.U8, Field.ARRAY, bytearray([0] * 72)) + + self.register_field("spec_ctrl", Field.U32) + + self.register_field("reserved_7b", + Field.U8, Field.ARRAY, bytearray([0] * 4)) + + self.register_field("pkru", Field.U32) + + self.register_field("reserved_7a", + Field.U8, Field.ARRAY, bytearray([0] * 20)) + + self.register_field("reserved_8", Field.U64) # rax duplicate + + self.register_field("rcx", Field.U64) + self.register_field("rdx", Field.U64, Field.BITMASK) + self.register_field("rbx", Field.U64) + + self.register_field("reserved_9", Field.U64) # rsp duplicate + + self.register_field("rbp", Field.U64) + self.register_field("rsi", Field.U64) + self.register_field("rdi", Field.U64) + self.register_field("r8", Field.U64) + self.register_field("r9", Field.U64) + self.register_field("r10", Field.U64) + self.register_field("r11", Field.U64) + self.register_field("r12", Field.U64) + self.register_field("r13", Field.U64) + self.register_field("r14", Field.U64) + self.register_field("r15", Field.U64) + + self.register_field("reserved_10", + Field.U8, Field.ARRAY, bytearray([0] * 16)) + + self.register_field("sw_exit_code", Field.U64) + self.register_field("sw_exit_info_1", Field.U64) + self.register_field("sw_exit_info_2", Field.U64) + self.register_field("sw_scratch", Field.U64) + + self.register_field("reserved_11", + Field.U8, Field.ARRAY, bytearray([0] * 56)) + + self.register_field("xcr0", Field.U64) + self.register_field("valid_bitmap", + Field.U8, Field.ARRAY, bytearray([0] * 16)) + self.register_field("x87_state_gpa", + Field.U64) + + def amd64_cpu_init(self): + # AMD64 Architecture Programmer’s Manual + # Volume 2: System Programming. + # + # 14.1.3 Processor Initialization State + # + # Values after INIT + + self.cr0 = (1 << 4) + self.rip = 0xfff0 + + self.cs_selector = 0xf000 + self.cs_base = 0xffff0000 + self.cs_limit = 0xffff + + self.ds_limit = 0xffff + + self.es_limit = 0xffff + self.fs_limit = 0xffff + self.gs_limit = 0xffff + self.ss_limit = 0xffff + + self.gdtr_limit = 0xffff + self.idtr_limit = 0xffff + + self.ldtr_limit = 0xffff + self.tr_limit = 0xffff + + self.dr6 = 0xffff0ff0 + self.dr7 = 0x0400 + self.rflags = 0x2 + self.xcr0 = 0x1 + + def kvm_cpu_init(self): + # svm_set_cr4() sets guest X86_CR4_MCE bit if host + # has X86_CR4_MCE enabled + self.cr4 = 0x40 + + # svm_set_efer sets guest EFER_SVME (Secure Virtual Machine enable) + self.efer = 0x1000 + + # init_vmcb + init_sys_seg() sets + # SVM_SELECTOR_P_MASK | SEG_TYPE_LDT + self.ldtr_attrib = 0x0082 + + # init_vmcb + init_sys_seg() sets + # SVM_SELECTOR_P_MASK | SEG_TYPE_BUSY_TSS16 + self.tr_attrib = 0x0083 + + # kvm_arch_vcpu_create() in arch/x86/kvm/x86.c + self.g_pat = 0x0007040600070406 + + def qemu_cpu_init(self): + # Based on logic in x86_cpu_reset() + # + # file target/i386/cpu.c + + def attr(mask): + return (mask >> VMSA.ATTR_TYPE_SHIFT) + + self.ldtr_attrib = attr(VMSA.ATTR_P_MASK | + (2 << VMSA.ATTR_TYPE_SHIFT)) + self.tr_attrib = attr(VMSA.ATTR_P_MASK | + (11 << VMSA.ATTR_TYPE_SHIFT)) + self.cs_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_CS_MASK | + VMSA.ATTR_R_MASK | + VMSA.ATTR_A_MASK) + self.ds_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_W_MASK | + VMSA.ATTR_A_MASK) + self.es_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_W_MASK | + VMSA.ATTR_A_MASK) + self.ss_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_W_MASK | + VMSA.ATTR_A_MASK) + self.fs_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_W_MASK | + VMSA.ATTR_A_MASK) + self.gs_attrib = attr(VMSA.ATTR_P_MASK | + VMSA.ATTR_S_MASK | + VMSA.ATTR_W_MASK | + VMSA.ATTR_A_MASK) + + self.g_pat = 0x0007040600070406 + + def cpu_sku(self, family, model, stepping): + stepping &= 0xf + model &= 0xff + family &= 0xfff + + self.rdx.value = stepping + + if family > 0xf: + self.rdx.value |= 0xf00 | ((family - 0x0f) << 20) + else: + self.rdx.value |= family << 8 + + self.rdx.value |= ((model & 0xf) << 4) | ((model >> 4) << 16) + + def reset_addr(self, reset_addr): + reset_cs = reset_addr & 0xffff0000 + reset_ip = reset_addr & 0x0000ffff + + self.rip.value = reset_ip + self.cs_base.value = reset_cs + + +class SevInfoBlock(Struct): + + def __init__(self): + super().__init__(size=4) + self.register_field("reset_addr", Field.U32) + + +class OVMF(object): + + OVMF_TABLE_FOOTER_GUID = UUID("96b582de-1fb2-45f7-baea-a366c55a082d") + SEV_INFO_BLOCK_GUID = UUID("00f771de-1a7e-4fcb-890e-68c77e2fb44e") + + def __init__(self): + self.entries = {} + + def load(self, content): + expect = OVMF.OVMF_TABLE_FOOTER_GUID.bytes_le + actual = content[-48:-32] + if expect != actual: + raise Exception("OVMF footer GUID not found") + + tablelen = int.from_bytes(content[-50:-48], byteorder='little') + + if tablelen == 0: + raise Exception("OVMF tables zero length") + + table = content[-(50 + tablelen):-50] + + self.parse_table(table) + + def parse_table(self, data): + while len(data) > 0: + entryuuid = UUID(bytes_le=data[-16:]) + entrylen = int.from_bytes(data[-18:-16], byteorder='little') + entrydata = data[-entrylen:-18] + + self.entries[str(entryuuid)] = entrydata + + data = data[0:-(18 + entrylen)] + + def reset_addr(self): + if str(OVMF.SEV_INFO_BLOCK_GUID) not in self.entries: + raise Exception("SEV info block GUID not found") + + info = SevInfoBlock() + info.unpack(self.entries[str(OVMF.SEV_INFO_BLOCK_GUID)]) + + return info.reset_addr.value + + class GUIDTable(abc.ABC): GUID_LEN = 16 @@ -235,6 +665,26 @@ class ConfidentialVM(object): log.debug("VMSA CPU 1(sha256): %s" % sha256(self.vmsa_cpu1).hexdigest()) + def build_vmsas(self, family, model, stepping): + ovmf = OVMF() + ovmf.load(self.firmware) + + vmsa = VMSA() + vmsa.amd64_cpu_init() + vmsa.kvm_cpu_init() + vmsa.qemu_cpu_init() + + vmsa.cpu_sku(family, model, stepping) + + self.vmsa_cpu0 = vmsa.pack() + log.debug("VMSA CPU 0(sha256): %s" % + sha256(self.vmsa_cpu0).hexdigest()) + + vmsa.reset_addr(ovmf.reset_addr()) + self.vmsa_cpu1 = vmsa.pack() + log.debug("VMSA CPU 1(sha256): %s" % + sha256(self.vmsa_cpu1).hexdigest()) + def get_cpu_state(self): if self.num_cpus is None: raise UnsupportedUsageException( @@ -502,6 +952,12 @@ def parse_command_line(): help='VMSA state for the boot CPU') vmconfig.add_argument('--vmsa-cpu1', '-1', help='VMSA state for the additional CPUs') + vmconfig.add_argument('--cpu-family', type=int, + help='Hypervisor host CPU family number') + vmconfig.add_argument('--cpu-model', type=int, + help='Hypervisor host CPU model number') + vmconfig.add_argument('--cpu-stepping', type=int, + help='Hypervisor host CPU stepping number') vmconfig.add_argument('--tik', help='TIK file for domain') vmconfig.add_argument('--tek', @@ -560,6 +1016,20 @@ def check_usage(args): raise UnsupportedUsageException( "Either --firmware or --domain is required") + sku = [args.cpu_family, args.cpu_model, args.cpu_stepping] + if sku.count(None) == len(sku): + if args.vmsa_cpu1 is not None and args.vmsa_cpu0 is None: + raise UnsupportedUsageException( + "VMSA for additional CPU also requires VMSA for boot CPU") + else: + if args.vmsa_cpu0 is not None or args.vmsa_cpu1 is not None: + raise UnsupportedUsageException( + "VMSA files are mutually exclusive with CPU SKU") + + if sku.count(None) != 0: + raise UnsupportedUsageException( + "CPU SKU needs family, model and stepping for SEV-ES domain") + def attest(args): if args.domain is None: @@ -600,6 +1070,11 @@ def attest(args): if args.vmsa_cpu1 is not None: cvm.load_vmsa_cpu1(args.vmsa_cpu1) + if args.cpu_family is not None: + cvm.build_vmsas(args.cpu_family, + args.cpu_model, + args.cpu_stepping) + if args.domain is not None: cvm.load_domain(args.connect, args.domain, -- 2.37.3

On Sun, Oct 16, 2022 at 03:00:25PM -0400, Cole Robinson wrote: snip Ah yes, well spotted. Sigh. It *was* implemented fully. I tested every possible scenario. Then right before sending this, I deleted the unpack code as I thought it wasn't used. /facepalm. Yes, that is quiet simple, given we only need 1 int value. 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 :|

On 10/7/22 7:43 AM, Daniel P. Berrangé wrote: If this is missing, I'd make it fatal, libvirtd isn't new enough. Could happen if talking to a remote machine (or testing the script while f36 fedora libvirtd is running, which I did :) ) . It's going to fail later anyways. - Cole

It is possible to build OVMF for SEV with an embedded Grub that can fetch LUKS disk secrets. This adds support for injecting secrets in the required format. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- docs/manpages/virt-qemu-sev-validate.rst | 66 ++++++++++ tools/virt-qemu-sev-validate.py | 155 +++++++++++++++++++++-- 2 files changed, 212 insertions(+), 9 deletions(-) diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst index fcc13d68c8..7542bea9aa 100644 --- a/docs/manpages/virt-qemu-sev-validate.rst +++ b/docs/manpages/virt-qemu-sev-validate.rst @@ -187,6 +187,29 @@ understand any configuration mistakes that have been made. If the will be skipped. The result is that the validation will likely be reported as failed. +Secret injection options +------------------------ + +These options provide a way to inject a secret if validation of the +launch measurement passes. + +``--disk-password PATH`` + +Path to a file containing the password to use to unlock the LUKS container +for the guest disk. + +``--secret-header PATH`` + +Path to a file in which the injected secret header will be written in base64 +format and later injected into the domain. This is required if there is no +connection to libvirt, otherwise the secret will be directly injected. + +``--secret-payload PATH`` + +Path to a file in which the injected secret payload will be written in base64 +format and later injected into the domain. This is required if there is no +connection to libvirt, otherwise the secret will be directly injected. + EXAMPLES ======== @@ -261,6 +284,26 @@ automatically constructed VMSA: --build-id 13 \ --policy 7 +Validate the measurement of a SEV guest booting from disk and +inject a disk password on success: + +:: + + # virt-dom-sev-validate \ + --loader OVMF.sev.fd \ + --tk this-guest-tk.bin \ + --measurement Zs2pf19ubFSafpZ2WKkwquXvACx9Wt/BV+eJwQ/taO8jhyIj/F8swFrybR1fZ2ID \ + --api-major 0 \ + --api-minor 24 \ + --build-id 13 \ + --policy 3 \ + --disk-password passwd.txt \ + --secret-header secret-header.b64 \ + --secret-payload secret-payload.b64 + +The ``secret-header.b64`` and ``secret-payload.b64`` files can now be sent to +the virtualization host for injection. + Fetch from remote libvirt ------------------------- @@ -321,6 +364,18 @@ automatically constructed VMSA: --tk this-guest-tk.bin \ --domain fedora34x86_64 +Validate the measurement of a SEV guest booting from disk and +inject a disk password on success: + +:: + + # virt-dom-sev-validate \ + --connect qemu+ssh://root@some.remote.host/system \ + --loader OVMF.sev.fd \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 \ + --disk-password passwd.txt + Fetch from local libvirt ------------------------ @@ -371,6 +426,17 @@ automatically constructed VMSA: --tk this-guest-tk.bin \ --domain fedora34x86_64 +Validate the measurement of a SEV guest booting from disk and +inject a disk password on success: + +:: + + # virt-dom-sev-validate \ + --insecure \ + --tk this-guest-tk.bin \ + --domain fedora34x86_64 \ + --disk-password passwd.txt + EXIT STATUS =========== diff --git a/tools/virt-qemu-sev-validate.py b/tools/virt-qemu-sev-validate.py index 5da1353e60..2c5ad9083d 100755 --- a/tools/virt-qemu-sev-validate.py +++ b/tools/virt-qemu-sev-validate.py @@ -36,18 +36,21 @@ import abc import argparse -from base64 import b64decode +from base64 import b64decode, b64encode from hashlib import sha256 import hmac import libvirt import logging from lxml import etree +import os import re import socket from struct import pack import sys import traceback from uuid import UUID +from cryptography.hazmat.primitives.ciphers import Cipher, algorithms, modes + log = logging.getLogger() @@ -580,7 +583,26 @@ class KernelTable(GUIDTable): return entries -class ConfidentialVM(object): +class SecretsTable(GUIDTable): + + TABLE_GUID = UUID('{1e74f542-71dd-4d66-963e-ef4287ff173b}').bytes_le + DISK_PW_GUID = UUID('{736869e5-84f0-4973-92ec-06879ce3da0b}').bytes_le + + def __init__(self): + super().__init__(guid=self.TABLE_GUID, + lenlen=4) + self.disk_password = None + + def load_disk_password(self, path): + with open(path, 'rb') as fh: + self.disk_password = fh.read() + + def entries(self): + return self.build_entry(self.DISK_PW_GUID, + self.disk_password + bytes([0]), 4) + + +class ConfidentialVM(abc.ABC): POLICY_BIT_SEV_ES = 2 POLICY_VAL_SEV_ES = (1 << POLICY_BIT_SEV_ES) @@ -606,6 +628,7 @@ class ConfidentialVM(object): self.vmsa_cpu1 = None self.kernel_table = KernelTable() + self.secrets_table = SecretsTable() def is_sev_es(self): return self.policy & self.POLICY_VAL_SEV_ES @@ -758,6 +781,82 @@ class ConfidentialVM(object): raise AttestationFailedException( "Measurement does not match, VM is not trustworthy") + def build_secrets(self): + measurement, _ = self.get_measurements() + + iv = os.urandom(16) + + secret_table = self.secrets_table.build() + + cipher = Cipher(algorithms.AES(self.tek), modes.CTR(iv)) + enc = cipher.encryptor() + secret_table_ciphertext = (enc.update(secret_table) + + enc.finalize()) + + flags = 0 + + ## + # Table 55. LAUNCH_SECRET Packet Header Buffer + ## + header = ( + flags.to_bytes(4, byteorder='little') + + iv + ) + + # AMD Secure Encrypted Virtualization API , section 6.6 + # + # hdrmac = HMAC(0x01 || FLAGS || IV || GUEST_LENGTH || + # TRANS_LENGTH || DATA || + # MEASURE; GCTX.TIK) + # + msg = ( + bytes([0x01]) + + flags.to_bytes(4, byteorder='little') + + iv + + len(secret_table).to_bytes(4, byteorder='little') + + len(secret_table).to_bytes(4, byteorder='little') + + secret_table_ciphertext + + measurement + ) + + h = hmac.new(self.tik, msg, 'sha256') + header = ( + flags.to_bytes(4, byteorder='little') + + iv + + h.digest() + ) + + header64 = b64encode(header).decode('utf8') + secret64 = b64encode(secret_table_ciphertext).decode('utf8') + log.debug("Header: %s (%d bytes)" % (header64, len(header))) + log.debug("Secret: %s (%d bytes)" % ( + secret64, len(secret_table_ciphertext))) + + return header64, secret64 + + @abc.abstractmethod + def inject_secrets(self): + pass + + +class OfflineConfidentialVM(ConfidentialVM): + def __init__(self, + secret_header=None, + secret_payload=None, + **kwargs): + super().__init__(**kwargs) + + self.secret_header = secret_header + self.secret_payload = secret_payload + + def inject_secrets(self): + header64, secret64 = self.build_secrets() + + with open(self.secret_header, "wb") as fh: + fh.write(header64.encode('utf8')) + with open(self.secret_payload, "wb") as fh: + fh.write(secret64.encode('utf8')) + class LibvirtConfidentialVM(ConfidentialVM): def __init__(self, **kwargs): @@ -937,6 +1036,14 @@ class LibvirtConfidentialVM(ConfidentialVM): cpu_stepping = int(sig[0].get("stepping")) self.build_vmsas(cpu_family, cpu_model, cpu_stepping) + def inject_secrets(self): + header64, secret64 = self.build_secrets() + + params = {"sev-secret": secret64, + "sev-secret-header": header64} + self.dom.setLaunchSecurityState(params, 0) + self.dom.resume() + def parse_command_line(): parser = argparse.ArgumentParser( @@ -999,6 +1106,14 @@ def parse_command_line(): vmconn.add_argument('--ignore-config', '-g', action='store_true', help='Do not attempt to sanity check the guest config') + # Arguments related to secret injection + parser.add_argument('--disk-password', '-s', + help='Path to LUKS disk password to inject') + parser.add_argument('--secret-payload', + help='Path to file to write secret data payload to') + parser.add_argument('--secret-header', + help='Path to file to write secret data header to') + return parser.parse_args() @@ -1039,6 +1154,15 @@ def check_usage(args): raise UnsupportedUsageException( "Either --firmware or --domain is required") + if args.disk_password is not None: + if args.secret_header is None: + raise UnsupportedUsageException( + "Either --secret-header or --domain is required") + + if args.secret_payload is None: + raise UnsupportedUsageException( + "Either --secret-payload or --domain is required") + sku = [args.cpu_family, args.cpu_model, args.cpu_stepping] if sku.count(None) == len(sku): if args.vmsa_cpu1 is not None and args.vmsa_cpu0 is None: @@ -1053,15 +1177,22 @@ def check_usage(args): raise UnsupportedUsageException( "CPU SKU needs family, model and stepping for SEV-ES domain") + secret = [args.secret_payload, args.secret_header] + if secret.count(None) > 0 and secret.count(None) != len(secret): + raise UnsupportedUsageException( + "Both --secret-payload and --secret-header are required") + def attest(args): if args.domain is None: - cvm = ConfidentialVM(measurement=args.measurement, - api_major=args.api_major, - api_minor=args.api_minor, - build_id=args.build_id, - policy=args.policy, - num_cpus=args.num_cpus) + cvm = OfflineConfidentialVM(measurement=args.measurement, + api_major=args.api_major, + api_minor=args.api_minor, + build_id=args.build_id, + policy=args.policy, + num_cpus=args.num_cpus, + secret_header=args.secret_header, + secret_payload=args.secret_payload) else: cvm = LibvirtConfidentialVM(measurement=args.measurement, api_major=args.api_major, @@ -1105,10 +1236,16 @@ def attest(args): args.ignore_config) cvm.attest() - if not args.quiet: print("OK: Looks good to me") + if args.disk_password: + cvm.secrets_table.load_disk_password(args.disk_password) + + cvm.inject_secrets() + if not args.quiet: + print("OK: Injected password") + if __name__ == "__main__": args = parse_command_line() -- 2.37.3

On Fri, Oct 07, 2022 at 12:43:05PM +0100, Daniel P. Berrangé wrote: The doc reads well; and is a strict improvement. I only have a couple of small nits inline. [...] nit: "attest to" --> "attest the" nit: "DH certificate" --> "Diffie-Hellman (DH) certificate"? Might want to mention that the ``sevtool`` needs to be installed separately (or link to its GitHub repo?). I see it packaged at least in Fedora. Might want to expand the "PDH" on first use: PDH (Platform Diffie-Hellman key). It would also be nice to define what this key actually is (there are at least 11 keys, looking at AMD's SEV API doc). Consider rephrasing the above to something like: "First of all the Platform Diffie-Hellman key (PDH) for the hypervisor host needs to be obtained. The PDH is used to negotiate a master secret between the SEV firmware and external entities." [...] Nice! I'm going to give this a try on an SEV-ES box. Regardless, the rest of the doc reads well. Thank you for documenting! Feel free to choose what nits to address from the above. With or without the nits addressed: Reviewed-by: Kashyap Chamarthy <kchamart(a)redhat.com&gt; -- /kashyap

Despite efforts to make the virt-qemu-sev-validate tool friendly, it is a certainty that almost everyone who tries it will hit false negative results, getting a failure despite the VM being trustworthy. Diagnosing these problems is no easy matter, especially for those not familiar with SEV/SEV-ES in general. This extra docs text attempts to set out a checklist of items to look at to identify what went wrong. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- docs/manpages/virt-qemu-sev-validate.rst | 112 +++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/docs/manpages/virt-qemu-sev-validate.rst b/docs/manpages/virt-qemu-sev-validate.rst index 7542bea9aa..e0c18f2d20 100644 --- a/docs/manpages/virt-qemu-sev-validate.rst +++ b/docs/manpages/virt-qemu-sev-validate.rst @@ -437,6 +437,118 @@ inject a disk password on success: --domain fedora34x86_64 \ --disk-password passwd.txt +COMMON MISTAKES CHECKLIST +========================= + +The complexity of configuring a guest and validating its boot measurement +means it is very likely to see the failure:: + + ERROR: Measurement does not match, VM is not trustworthy + +This error message assumes the worst, but in most cases will failure will be +a result of either mis-configuring the guest, or passing the wrong information +when trying to validate it. The following information is a guide for what +items to check in order to stand the best chance of diagnosing the problem + +* Check the VM configuration for the DH certificate and session + blob in the libvirt guest XML. + + The content for these fields should be in base64 format, which is + what ``sevctl session`` generates. Other tools may generate the files + in binary format, so ensure it has been correctly converted to base64. + +* Check the VM configuration policy value matches the session blob + + The ``<policy>`` value in libvirt guest XML has to match the value + passed to the ``sevctl session`` command. + +* Check the correct TIK/TEK keypair are passed + + The TIK/TEK keypair are uniquely tied to each DH cert and session + blob. Make sure that the TIK/TEK keypair passed to this program + the ones matched to the DH cert and session blob configured for + the libvirt guest XML. This is one of the most common mistakes. + Further ensure that the TIK and TEK files are not swapped. + +* Check the firmware binary matches the one used to boot + + The firmware binary content is part of the data covered by the + launch measurement. Ensure that the firmware binary passed to + this program matches the one used to launch the guest. The + hypervisor host will periodically get software updates which + introduce a new firmware binary version. + +* Check the kernel, initrd and cmdline match the one used to boot + + If the guest is configured to use direct kernel boot, check that + the kernel, initrd and cmdline passed to this program match the + ones used to boot the guest. In the kernel cmdline whitespace + must be preserved exactly, including any leading or trailing + spaces. + +* Check whether the kernel hash measurement is enabled + + The ``kernelHashes`` property in the libvirt guest XML controls + whether hashes of the kernel, initrd and cmdline content are + covered by the boot measurement. If enabled, then the matching + content must be passed to this program. UIf disabled, then + the content must **NOT** be passed. + +* Check that the correct measurement hash is passed + + The measurement hash includes a nonce, so it will be different + on every boot attempt. Thus when validating the measuremnt it + is important ensure the most recent measurement is used. + +* Check the correct VMSA blobs / CPU SKU values for the host are used + + The VMSA blobs provide the initial register state for the + boot CPU and any additional CPUs. One of the registers + encodes the CPU SKU (family, model, stepping) of the physical + host CPU. Make sure that the VMSA blob used for validation + is one that matches the SKU of the host the guest is booted + on. Passing the CPU SKU values directly to the tool can + reduce the likelihood of using the wrong ones. + +* Check the CPU count is correct + + When passing VMSA blobs for SEV-ES guests, the number of CPUs + present will influence the measurement result. Ensure that the + correct vCPU count is used corresponding to the guest boot + attempt. + + +Best practice is to run this tool in completely offline mode and pass +all information as explicit command line parameters. When debugging +failures, however, it can be useful to tell it to connect to libvirt +and fetch information. If connecting to a remote libvirt instance, +it will fetch any information that can be trusted, which is the basic +VM launch state data. It will also sanity check the XML configuration +to identify some common mistakes. If the ``--insecure`` flag is passed +it can extract some configuration information and use that for the +attestation process. + +If the mistake still can't be identified, then this tool can be run +on the virtualization host. In that scenario the only three command +line parameters required are for the TIK, TEK and libvirt domain +name. It should be able to automatically determine all the other +information required. If it still reports a failure, this points +very strongly to the TIK/TEK pair not maching the configured +DH certificate and session blob. + +The ``--debug`` flag will display hashes and/or hex dumps for various +pieces of information used in the attestation process. Comparing the +``--debug`` output from running on the hypervisor host, against that +obtained when running in offline mode can give further guidance to +which parameter is inconsistent. + +As mentioned earlier in this document, bear in mind that in general +any attestation answers obtained from running on the hypervisor host +should not be trusted. So if a configuration mistake is identified +it is strongly recommended to re-run the attestation in offline mode +on a trusted machine. + + EXIT STATUS =========== -- 2.37.3

On Sun, Oct 16, 2022 at 03:27:39PM -0400, Cole Robinson wrote: Oh, I had forgotten that Hmm, yes, I'd stayed away from reset/rotate to avoid trouble :-) 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 :|