[PATCH V3 0/4] Add virDomainSetLaunchSecurityState API

V3 of https://listman.redhat.com/archives/libvir-list/2021-December/msg00366.html Like V1 and V2, this series is compile-tested only. I plan to work on functional testing soon. Unlike previous versions, this one got little self-scrutiny since I wanted to get it posted to the list for feedback on questions regarding 4/4. Changes since V2: - Add test case in qemumonitorjsontest - Add virsh implementation Jim Fehlig (4): libvirt: Introduce virDomainSetLaunchSecurityState public API remote: Add RPC support for the virDomainSetLaunchSecurityState API qemu: Implement the virDomainSetLaunchSecurityState API tools: Add domsetlaunchsecstate virsh command docs/manpages/virsh.rst | 25 ++++++++ include/libvirt/libvirt-domain.h | 36 +++++++++++ src/driver-hypervisor.h | 7 ++ src/libvirt-domain.c | 62 ++++++++++++++++++ src/libvirt_public.syms | 5 ++ src/qemu/qemu_driver.c | 88 +++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 14 ++++ src/qemu/qemu_monitor.h | 7 ++ src/qemu/qemu_monitor_json.c | 45 +++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++ src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++- src/remote_protocol-structs | 9 +++ tests/qemumonitorjsontest.c | 3 + tools/virsh-domain.c | 107 +++++++++++++++++++++++++++++++ 15 files changed, 431 insertions(+), 1 deletion(-) -- 2.34.1

This API allows setting a launch secret within a guests's memory. The launch secret is created by the guest owner after retrieving and verifying the launch measurement with virDomainGetLaunchSecurityInfo. The API uses virTypedParameter for input, allowing it to be expanded to support other confidential computing technologies. In the case of SEV, a basic guest launch workflow is described in the SEV API spec in section "1.3.1 Launch" https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf Signed-off-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- include/libvirt/libvirt-domain.h | 36 +++++++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 62 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ 4 files changed, 110 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 5d3e15766e..5f0a9b7572 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5102,6 +5102,7 @@ int virDomainSetLifecycleAction(virDomainPtr domain, # define VIR_DOMAIN_LAUNCH_SECURITY_SEV_MEASUREMENT "sev-measurement" /** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_API_MAJOR: * * Macro represents the API major version of the SEV host, @@ -5133,11 +5134,46 @@ int virDomainSetLifecycleAction(virDomainPtr domain, */ # define VIR_DOMAIN_LAUNCH_SECURITY_SEV_POLICY "sev-policy" +/** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER: + * + * A macro used to represent the SEV launch secret header. The secret header + * is a base64-encoded VIR_TYPED_PARAM_STRING containing artifacts needed by + * the SEV firmware to recover the plain text of the launch secret. See + * section "6.6 LAUNCH_SECRET" in the SEV API specification for a detailed + * description of the secret header. + */ +# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER "sev-secret-header" + +/** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET: + * + * A macro used to represent the SEV launch secret. The secret is a + * base64-encoded VIR_TYPED_PARAM_STRING containing an encrypted launch + * secret. The secret is created by the domain owner after the SEV launch + * measurement is retrieved and verified. + */ +# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET "sev-secret" + +/** + * VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS: + * + * A macro used to represent the physical address within the guest's memory + * where the secret will be set, as VIR_TYPED_PARAM_ULLONG. If not specified, + * the address will be determined by the hypervisor. + */ +# define VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS "sev-secret-set-address" + int virDomainGetLaunchSecurityInfo(virDomainPtr domain, virTypedParameterPtr *params, int *nparams, unsigned int flags); +int virDomainSetLaunchSecurityState(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + typedef enum { VIR_DOMAIN_GUEST_INFO_USERS = (1 << 0), /* return active users */ VIR_DOMAIN_GUEST_INFO_OS = (1 << 1), /* return OS information */ diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index d642af8a37..c83fb648a2 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1333,6 +1333,12 @@ typedef int int *nparams, unsigned int flags); +typedef int +(*virDrvDomainSetLaunchSecurityState)(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + typedef virDomainCheckpointPtr (*virDrvDomainCheckpointCreateXML)(virDomainPtr domain, const char *xmlDesc, @@ -1661,6 +1667,7 @@ struct _virHypervisorDriver { virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU; virDrvNodeGetSEVInfo nodeGetSEVInfo; virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo; + virDrvDomainSetLaunchSecurityState domainSetLaunchSecurityState; virDrvDomainCheckpointCreateXML domainCheckpointCreateXML; virDrvDomainCheckpointGetXMLDesc domainCheckpointGetXMLDesc; virDrvDomainListAllCheckpoints domainListAllCheckpoints; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 5708ff839b..82b0822d12 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12844,6 +12844,68 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, } +/** + * virDomainSetLaunchSecurityState: + * @domain: a domain object + * @params: pointer to launch security parameter objects + * @nparams: number of launch security parameters + * @flags: currently used, set to 0. + * + * Set a launch security secret in the guest's memory. The guest must be + * in a paused state, e.g. in state VIR_DOMIAN_PAUSED as reported by + * virDomainGetState. On success, the guest can be transitioned to a + * running state. On failure, the guest should be destroyed. + * + * A basic guest attestation process can be achieved by: + * - Start a secure guest in the paused state by passing VIR_DOMAIN_START_PAUSED + * to one of the virDomainCreate APIs + * - Retrieve the guest launch measurement with virDomainGetLaunchSecurityInfo + * - Verify launch measurement and generate a secret for the guest + * - Set the secret in the guest's memory with virDomainSetLaunchSecurityState + * - Start running the guest with virDomainResume + * + * See VIR_DOMAIN_LAUNCH_SECURITY_* for a detailed description of accepted + * launch security parameters. + * + * Returns -1 in case of failure, 0 in case of success. + */ +int virDomainSetLaunchSecurityState(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virConnectPtr conn = domain->conn; + + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d flags=0x%x", + params, nparams, flags); + VIR_TYPED_PARAMS_DEBUG(params, nparams); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgGoto(params, error); + virCheckPositiveArgGoto(nparams, error); + virCheckReadOnlyGoto(domain->conn->flags, error); + + if (virTypedParameterValidateSet(conn, params, nparams) < 0) + goto error; + + if (conn->driver->domainSetLaunchSecurityState) { + int ret; + ret = conn->driver->domainSetLaunchSecurityState(domain, params, + nparams, flags); + if (ret < 0) + goto error; + return ret; + } + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + /** * virDomainAgentSetResponseTimeout: * @domain: a domain object diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 788a967df7..f93692c427 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -911,4 +911,9 @@ LIBVIRT_7.8.0 { virNetworkCreateXMLFlags; } LIBVIRT_7.7.0; +LIBVIRT_8.0.0 { + global: + virDomainSetLaunchSecurityState; +} LIBVIRT_7.8.0; + # .... define new API here using predicted next version number .... -- 2.34.1

Signed-off-by: Jim Fehlig <jfehlig@suse.com> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 17 ++++++++++++++++- src/remote_protocol-structs | 9 +++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 5b179a927d..5b7ccfaebd 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8600,6 +8600,7 @@ static virHypervisorDriver hypervisor_driver = { .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = remoteDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = remoteDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 60010778ca..4f13cef662 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -272,6 +272,9 @@ const REMOTE_NODE_SEV_INFO_MAX = 64; /* Upper limit on number of launch security information entries */ const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64; +/* Upper limit on number of launch security state entries */ +const REMOTE_DOMAIN_LAUNCH_SECURITY_STATE_PARAMS_MAX = 64; + /* Upper limit on number of parameters describing a guest */ const REMOTE_DOMAIN_GUEST_INFO_PARAMS_MAX = 2048; @@ -3642,6 +3645,12 @@ struct remote_domain_get_launch_security_info_ret { remote_typed_param params<REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX>; }; +struct remote_domain_set_launch_security_state_args { + remote_nonnull_domain dom; + remote_typed_param params<REMOTE_DOMAIN_LAUNCH_SECURITY_STATE_PARAMS_MAX>; + unsigned int flags; +}; + /* nwfilter binding */ struct remote_nwfilter_binding_lookup_by_port_dev_args { @@ -6905,5 +6914,11 @@ enum remote_procedure { * @generate: both * @acl: none */ - REMOTE_PROC_DOMAIN_EVENT_MEMORY_DEVICE_SIZE_CHANGE = 438 + REMOTE_PROC_DOMAIN_EVENT_MEMORY_DEVICE_SIZE_CHANGE = 438, + + /** + * @generate: both + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_SET_LAUNCH_SECURITY_STATE = 439 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index dbef4ace79..d88176781d 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3005,6 +3005,14 @@ struct remote_domain_get_launch_security_info_ret { remote_typed_param * params_val; } params; }; +struct remote_domain_set_launch_security_state_args { + remote_nonnull_domain dom; + struct { + u_int params_len; + remote_typed_param * params_val; + } params; + u_int flags; +}; struct remote_nwfilter_binding_lookup_by_port_dev_args { remote_nonnull_string name; }; @@ -3680,4 +3688,5 @@ enum remote_procedure { REMOTE_PROC_NODE_DEVICE_IS_ACTIVE = 436, REMOTE_PROC_NETWORK_CREATE_XML_FLAGS = 437, REMOTE_PROC_DOMAIN_EVENT_MEMORY_DEVICE_SIZE_CHANGE = 438, + REMOTE_PROC_DOMAIN_SET_LAUNCH_SECURITY_STATE = 439, }; -- 2.34.1

Set a launch secret in guest memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabled guests in a paused state. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 14 ++++++ src/qemu/qemu_monitor.h | 7 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 3 ++ 6 files changed, 163 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aae622ea5d..889892a097 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20070,6 +20070,93 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; } + +static int +qemuDomainSetLaunchSecurityState(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + int rc; + size_t i; + g_autofree char *secrethdr = NULL; + g_autofree char *secret = NULL; + unsigned long long setaddr = 0; + bool hasSetaddr = false; + int state; + + virCheckFlags(0, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainSetLaunchSecurityStateEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; + + /* Currently only SEV is supported */ + if (!vm->def->sec || + vm->def->sec->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("setting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + } + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER)) { + secrethdr = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET)) { + secret = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS)) { + setaddr = param->value.ul; + hasSetaddr = true; + } + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto endjob; + + state = virDomainObjGetState(vm, NULL); + if (state != VIR_DOMAIN_PAUSED) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain must be in a paused state")); + goto endjob; + } + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorSetLaunchSecurityState(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, setaddr, hasSetaddr); + qemuDomainObjExitMonitor(driver, vm); + if (rc < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static const unsigned int qemuDomainGetGuestInfoSupportedTypes = VIR_DOMAIN_GUEST_INFO_USERS | VIR_DOMAIN_GUEST_INFO_OS | @@ -20943,6 +21030,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dda6ae9796..5272d49c2e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4379,6 +4379,20 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, } +int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetLaunchSecurityState(mon, secrethdr, secret, + setaddr, hasSetaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 29746f0b8e..87826e6268 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1454,6 +1454,13 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); +int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + typedef struct _qemuMonitorPRManagerInfo qemuMonitorPRManagerInfo; struct _qemuMonitorPRManagerInfo { bool connected; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a3d6eca569..37ee859a33 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8262,6 +8262,51 @@ qemuMonitorJSONGetSEVInfo(qemuMonitor *mon, } +/** + * Set a launch secret in guest memory + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + * + * The guest physical address (gpa) parameter is optional + */ +int +qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (hasSetaddr) { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", setaddr, + NULL); + } else { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + NULL); + } + if (cmd == NULL) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} + + /* * Example return data * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e88dfc9d50..64d9ebdaa3 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -476,6 +476,12 @@ qemuMonitorJSONGetVersion(qemuMonitor *mon, char **package) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + int qemuMonitorJSONGetMachines(qemuMonitor *mon, qemuMonitorMachineInfo ***machines) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1b0bd0870d..48e2a457ab 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1196,6 +1196,8 @@ GEN_TEST_FUNC(qemuMonitorJSONSetAction, QEMU_MONITOR_ACTION_REBOOT_RESET, QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN, QEMU_MONITOR_ACTION_PANIC_SHUTDOWN) +GEN_TEST_FUNC(qemuMonitorJSONSetLaunchSecurityState, "sev_secret_header", + "sev_secret", 0, true) static int testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque) @@ -3067,6 +3069,7 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONJobComplete); DO_TEST_GEN(qemuMonitorJSONBlockJobCancel); DO_TEST_GEN(qemuMonitorJSONSetAction); + DO_TEST_GEN(qemuMonitorJSONSetLaunchSecurityState); DO_TEST(qemuMonitorJSONGetBalloonInfo); DO_TEST(qemuMonitorJSONGetBlockInfo); DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo); -- 2.34.1

On Tue, Dec 14, 2021 at 09:46:05PM -0700, Jim Fehlig wrote:
Set a launch secret in guest memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabled guests in a paused state.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 14 ++++++ src/qemu/qemu_monitor.h | 7 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 3 ++ 6 files changed, 163 insertions(+)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 12/14/21 23:46, Jim Fehlig wrote:
Set a launch secret in guest memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabled guests in a paused state.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 14 ++++++ src/qemu/qemu_monitor.h | 7 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 3 ++ 6 files changed, 163 insertions(+)
Just checking - not active day to day, but this technology is what some of my team works on now... I only realized this because Dan pointed it out and I was trying to help someone new on my team see how commands are created in libvirt. He's more familiar with sevctl processing, but will need to contribute to libvirt/qemu in the coming months.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aae622ea5d..889892a097 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20070,6 +20070,93 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; }
+ +static int +qemuDomainSetLaunchSecurityState(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + int rc; + size_t i; + g_autofree char *secrethdr = NULL; + g_autofree char *secret = NULL; + unsigned long long setaddr = 0; + bool hasSetaddr = false; + int state; + + virCheckFlags(0, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainSetLaunchSecurityStateEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; + + /* Currently only SEV is supported */ + if (!vm->def->sec || + vm->def->sec->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("setting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + }
Would we need a specific capability check here before calling sev-inject-launch-secret since it's only available since qemu-6.0? Seems to me VIR_DOMAIN_LAUNCH_SECURITY_SEV would be keyed off the presence of the sev-guest object only. That would seem to be a qemu 2.12 type check. It's been a while since I looked at sources, so I'm rusty, but I think qemu_validate.c/qemuValidateDomainDef would check for QEMU_CAPS_SEV_GUEST only. This is unlike the query-sev-capabilities and query-sev-launch-measure which are present in 2.12. John
+ + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER)) { + secrethdr = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET)) { + secret = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS)) { + setaddr = param->value.ul; + hasSetaddr = true; + } + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto endjob; + + state = virDomainObjGetState(vm, NULL); + if (state != VIR_DOMAIN_PAUSED) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain must be in a paused state")); + goto endjob; + } + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorSetLaunchSecurityState(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, setaddr, hasSetaddr); + qemuDomainObjExitMonitor(driver, vm); + if (rc < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static const unsigned int qemuDomainGetGuestInfoSupportedTypes = VIR_DOMAIN_GUEST_INFO_USERS | VIR_DOMAIN_GUEST_INFO_OS | @@ -20943,6 +21030,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ };
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dda6ae9796..5272d49c2e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4379,6 +4379,20 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, }
+int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetLaunchSecurityState(mon, secrethdr, secret, + setaddr, hasSetaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 29746f0b8e..87826e6268 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1454,6 +1454,13 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
+int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + typedef struct _qemuMonitorPRManagerInfo qemuMonitorPRManagerInfo; struct _qemuMonitorPRManagerInfo { bool connected; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a3d6eca569..37ee859a33 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8262,6 +8262,51 @@ qemuMonitorJSONGetSEVInfo(qemuMonitor *mon, }
+/** + * Set a launch secret in guest memory + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + * + * The guest physical address (gpa) parameter is optional + */ +int +qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (hasSetaddr) { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", setaddr, + NULL); + } else { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + NULL); + } + if (cmd == NULL) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} + + /* * Example return data * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e88dfc9d50..64d9ebdaa3 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -476,6 +476,12 @@ qemuMonitorJSONGetVersion(qemuMonitor *mon, char **package) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4);
+int qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + int qemuMonitorJSONGetMachines(qemuMonitor *mon, qemuMonitorMachineInfo ***machines) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1b0bd0870d..48e2a457ab 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1196,6 +1196,8 @@ GEN_TEST_FUNC(qemuMonitorJSONSetAction, QEMU_MONITOR_ACTION_REBOOT_RESET, QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN, QEMU_MONITOR_ACTION_PANIC_SHUTDOWN) +GEN_TEST_FUNC(qemuMonitorJSONSetLaunchSecurityState, "sev_secret_header", + "sev_secret", 0, true)
static int testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque) @@ -3067,6 +3069,7 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONJobComplete); DO_TEST_GEN(qemuMonitorJSONBlockJobCancel); DO_TEST_GEN(qemuMonitorJSONSetAction); + DO_TEST_GEN(qemuMonitorJSONSetLaunchSecurityState); DO_TEST(qemuMonitorJSONGetBalloonInfo); DO_TEST(qemuMonitorJSONGetBlockInfo); DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo);

Hi John, Thanks for taking a look! On 12/17/21 12:48, John Ferlan wrote:
On 12/14/21 23:46, Jim Fehlig wrote:
Set a launch secret in guest memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabled guests in a paused state.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 14 ++++++ src/qemu/qemu_monitor.h | 7 +++ src/qemu/qemu_monitor_json.c | 45 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ tests/qemumonitorjsontest.c | 3 ++ 6 files changed, 163 insertions(+)
Just checking - not active day to day, but this technology is what some of my team works on now... I only realized this because Dan pointed it out and I was trying to help someone new on my team see how commands are created in libvirt. He's more familiar with sevctl processing, but will need to contribute to libvirt/qemu in the coming months.
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index aae622ea5d..889892a097 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20070,6 +20070,93 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; } + +static int +qemuDomainSetLaunchSecurityState(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + int rc; + size_t i; + g_autofree char *secrethdr = NULL; + g_autofree char *secret = NULL; + unsigned long long setaddr = 0; + bool hasSetaddr = false; + int state; + + virCheckFlags(0, -1); + if (virTypedParamsValidate(params, nparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET, + VIR_TYPED_PARAM_STRING, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS, + VIR_TYPED_PARAM_ULLONG, + NULL) < 0) + return -1; + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainSetLaunchSecurityStateEnsureACL(domain->conn, vm->def) < 0) + goto cleanup; + + /* Currently only SEV is supported */ + if (!vm->def->sec || + vm->def->sec->sectype != VIR_DOMAIN_LAUNCH_SECURITY_SEV) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("setting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + }
Would we need a specific capability check here before calling sev-inject-launch-secret since it's only available since qemu-6.0?
Good point. I made an attempt at including a caps check in V4 https://listman.redhat.com/archives/libvir-list/2021-December/msg00800.html Regards, Jim
Seems to me VIR_DOMAIN_LAUNCH_SECURITY_SEV would be keyed off the presence of the sev-guest object only. That would seem to be a qemu 2.12 type check. It's been a while since I looked at sources, so I'm rusty, but I think qemu_validate.c/qemuValidateDomainDef would check for QEMU_CAPS_SEV_GUEST only.
This is unlike the query-sev-capabilities and query-sev-launch-measure which are present in 2.12.
John
+ + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER)) { + secrethdr = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET)) { + secret = g_strdup(param->value.s); + } else if (STREQ(param->field, VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS)) { + setaddr = param->value.ul; + hasSetaddr = true; + } + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjCheckActive(vm) < 0) + goto endjob; + + state = virDomainObjGetState(vm, NULL); + if (state != VIR_DOMAIN_PAUSED) { + virReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("domain must be in a paused state")); + goto endjob; + } + + qemuDomainObjEnterMonitor(driver, vm); + rc = qemuMonitorSetLaunchSecurityState(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, setaddr, hasSetaddr); + qemuDomainObjExitMonitor(driver, vm); + if (rc < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + static const unsigned int qemuDomainGetGuestInfoSupportedTypes = VIR_DOMAIN_GUEST_INFO_USERS | VIR_DOMAIN_GUEST_INFO_OS | @@ -20943,6 +21030,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainSetLaunchSecurityState = qemuDomainSetLaunchSecurityState, /* 8.0.0 */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index dda6ae9796..5272d49c2e 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4379,6 +4379,20 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, } +int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONSetLaunchSecurityState(mon, secrethdr, secret, + setaddr, hasSetaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 29746f0b8e..87826e6268 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1454,6 +1454,13 @@ qemuMonitorGetSEVInfo(qemuMonitor *mon, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5); +int +qemuMonitorSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + typedef struct _qemuMonitorPRManagerInfo qemuMonitorPRManagerInfo; struct _qemuMonitorPRManagerInfo { bool connected; diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index a3d6eca569..37ee859a33 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8262,6 +8262,51 @@ qemuMonitorJSONGetSEVInfo(qemuMonitor *mon, } +/** + * Set a launch secret in guest memory + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + * + * The guest physical address (gpa) parameter is optional + */ +int +qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (hasSetaddr) { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", setaddr, + NULL); + } else { + cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + NULL); + } + if (cmd == NULL) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +} + + /* * Example return data * diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index e88dfc9d50..64d9ebdaa3 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -476,6 +476,12 @@ qemuMonitorJSONGetVersion(qemuMonitor *mon, char **package) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuMonitorJSONSetLaunchSecurityState(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long setaddr, + bool hasSetaddr); + int qemuMonitorJSONGetMachines(qemuMonitor *mon, qemuMonitorMachineInfo ***machines) diff --git a/tests/qemumonitorjsontest.c b/tests/qemumonitorjsontest.c index 1b0bd0870d..48e2a457ab 100644 --- a/tests/qemumonitorjsontest.c +++ b/tests/qemumonitorjsontest.c @@ -1196,6 +1196,8 @@ GEN_TEST_FUNC(qemuMonitorJSONSetAction, QEMU_MONITOR_ACTION_REBOOT_RESET, QEMU_MONITOR_ACTION_WATCHDOG_SHUTDOWN, QEMU_MONITOR_ACTION_PANIC_SHUTDOWN) +GEN_TEST_FUNC(qemuMonitorJSONSetLaunchSecurityState, "sev_secret_header", + "sev_secret", 0, true) static int testQemuMonitorJSONqemuMonitorJSONNBDServerStart(const void *opaque) @@ -3067,6 +3069,7 @@ mymain(void) DO_TEST_GEN(qemuMonitorJSONJobComplete); DO_TEST_GEN(qemuMonitorJSONBlockJobCancel); DO_TEST_GEN(qemuMonitorJSONSetAction); + DO_TEST_GEN(qemuMonitorJSONSetLaunchSecurityState); DO_TEST(qemuMonitorJSONGetBalloonInfo); DO_TEST(qemuMonitorJSONGetBlockInfo); DO_TEST(qemuMonitorJSONGetAllBlockStatsInfo);

After attesting a domain with the help of domlaunchsecinfo, domsetlaunchsecstate can be used to set a secret in the guest domain's memory prior to running the vcpus. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- Some questions and RFC regarding this patch: I'm not really fond of the command and function names and would appreciate suggestions :-). Also, is reading the secret header and secret from a file sufficient? The sev-tool 'package_secret' command writes the secret to a file. Lastly, I'm not sure what sizes to expect for secret and secret header. I may have overlooked it, but didn't find anything related to the size in the docs. I've temporarily set it to VSH_MAX_XML_FILE until we know a reasonable value. docs/manpages/virsh.rst | 25 ++++++++++ tools/virsh-domain.c | 107 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+) diff --git a/docs/manpages/virsh.rst b/docs/manpages/virsh.rst index e828f7ef68..0700b6e5df 100644 --- a/docs/manpages/virsh.rst +++ b/docs/manpages/virsh.rst @@ -2088,6 +2088,31 @@ launch security protection is active. If none is active, no parameters will be reported. +domsetlaunchsecstate +-------------------- + +**Syntax:** + +:: + + domsetlaunchsecstate domain --secrethdr hdr-filename + --secret secret-filename [--set-address address] + +Set a launch security secret in the guest's memory. The guest must have a +launchSecurity type enabled in its configuration and be in a paused state. +On success, the guest can be transitioned to a running state. On failure, +the guest should be destroyed. + +*--secrethdr* specifies a filename containing the base64-encoded secret header. +The header includes artifacts needed by the hypervisor firmware to recover the +plain text of the launch secret. *--secret* specifies the filename containing +the base64-encoded encrypted launch secret. + +The *--set-address* option can be used to specify a physical address within +the guest's memory to set the secret. If not specified, the address will be +determined by the hypervisor. + + dommemstat ---------- diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index c748fe2ba9..57978efef9 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -9570,6 +9570,107 @@ cmdDomLaunchSecInfo(vshControl * ctl, const vshCmd * cmd) return ret; } +/* + * "domsetlaunchsecstate" command + */ +static const vshCmdInfo info_domsetlaunchsecstate[] = { + {.name = "help", + .data = N_("Set domain launch security state") + }, + {.name = "desc", + .data = N_("Set a secret in the guest domain's memory") + }, + {.name = NULL} +}; + +static const vshCmdOptDef opts_domsetlaunchsecstate[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "secrethdr", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("path to file containing the secret header"), + }, + {.name = "secret", + .type = VSH_OT_STRING, + .flags = VSH_OFLAG_REQ_OPT, + .help = N_("path to file containing the secret"), + }, + {.name = "set-address", + .type = VSH_OT_INT, + .help = N_("physical address within the guest domain's memory to set the secret"), + }, + {.name = NULL} +}; + +static bool +cmdDomSetLaunchSecState(vshControl * ctl, const vshCmd * cmd) +{ + g_autoptr(virshDomain) dom = NULL; + const char *sechdrfile = NULL; + const char *secfile = NULL; + g_autofree char *sechdr = NULL; + g_autofree char *sec = NULL; + unsigned long long setaddr; + virTypedParameterPtr params = NULL; + int nparams = 0; + int maxparams = 0; + int rv; + bool ret = false; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "secrethdr", &sechdrfile) < 0) + return false; + + if (vshCommandOptStringReq(ctl, cmd, "secret", &secfile) < 0) + return false; + + if (sechdrfile == NULL || secfile == NULL) + return false; + + if (virFileReadAll(sechdrfile, VSH_MAX_XML_FILE, &sechdr) < 0) { + vshSaveLibvirtError(); + return false; + } + + if (virFileReadAll(secfile, VSH_MAX_XML_FILE, &sec) < 0) { + vshSaveLibvirtError(); + return false; + } + + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_HEADER, + sechdr) < 0) + return false; + + if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET, + sec) < 0) + return false; + + + if ((rv = vshCommandOptULongLong(ctl, cmd, "set-address", &setaddr)) < 0) { + return false; + } else if (rv > 0) { + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, + VIR_DOMAIN_LAUNCH_SECURITY_SEV_SECRET_SET_ADDRESS, + setaddr) < 0) + return false; + } + + if (virDomainSetLaunchSecurityState(dom, params, nparams, 0) != 0) { + vshError(ctl, "%s", _("Unable to set launch security state")); + goto cleanup; + } + + ret = true; + + cleanup: + virTypedParamsFree(params, nparams); + return ret; +} + /* * "qemu-monitor-command" command */ @@ -14595,6 +14696,12 @@ const vshCmdDef domManagementCmds[] = { .info = info_domlaunchsecinfo, .flags = 0 }, + {.name = "domsetlaunchsecstate", + .handler = cmdDomSetLaunchSecState, + .opts = opts_domsetlaunchsecstate, + .info = info_domsetlaunchsecstate, + .flags = 0 + }, {.name = "domname", .handler = cmdDomname, .opts = opts_domname, -- 2.34.1

On Tue, Dec 14, 2021 at 09:46:06PM -0700, Jim Fehlig wrote:
After attesting a domain with the help of domlaunchsecinfo, domsetlaunchsecstate can be used to set a secret in the guest domain's memory prior to running the vcpus.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
Some questions and RFC regarding this patch:
I'm not really fond of the command and function names and would appreciate suggestions :-).
I'm honestly not too fussed about the naming. THis command is really just about feature complete API coverage. I doubt many people will actually use virsh for this, instead they'll want a program that queries the measurement, verifies it and injects secret all in one go.
Also, is reading the secret header and secret from a file sufficient? The sev-tool 'package_secret' command writes the secret to a file.
Fine IMHO.
Lastly, I'm not sure what sizes to expect for secret and secret header. I may have overlooked it, but didn't find anything related to the size in the docs. I've temporarily set it to VSH_MAX_XML_FILE until we know a reasonable value.
I'm not too sure either, but as a general point, IMHO, almost all our use of virFileReadAll has no functional or security need for us to supply a limit at all. We really ought to just make it accept '-1' as a limit and treat that as unlimited. Meanwhile I'd suggest just letting it be 1 MB which is way bigger than i expect this data will be. You can only DoS yourself with virsh, and you'll be limited by the RPC protocol wire limit.
docs/manpages/virsh.rst | 25 ++++++++++ tools/virsh-domain.c | 107 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+)
With a 1 MB or similar size limit Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

On 12/16/21 08:02, Daniel P. Berrangé wrote:
On Tue, Dec 14, 2021 at 09:46:06PM -0700, Jim Fehlig wrote:
After attesting a domain with the help of domlaunchsecinfo, domsetlaunchsecstate can be used to set a secret in the guest domain's memory prior to running the vcpus.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
Some questions and RFC regarding this patch:
I'm not really fond of the command and function names and would appreciate suggestions :-).
I'm honestly not too fussed about the naming. THis command is really just about feature complete API coverage. I doubt many people will actually use virsh for this, instead they'll want a program that queries the measurement, verifies it and injects secret all in one go.
Also, is reading the secret header and secret from a file sufficient? The sev-tool 'package_secret' command writes the secret to a file.
Fine IMHO.
Lastly, I'm not sure what sizes to expect for secret and secret header. I may have overlooked it, but didn't find anything related to the size in the docs. I've temporarily set it to VSH_MAX_XML_FILE until we know a reasonable value.
I took another look at the API spec and section "6.6 LAUNCH_SECRET", subsection "6.6.2 Parameters" describes the parameters to the LUANCH_SECRET command and the layout the the secret header. IIUC, GUEST_PADDR and GUEST_LENGTH tell where to put the secret. GUEST_LENGTH cannot be greater than 16KB. And the secret header looks to be 104 bytes. https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
I'm not too sure either, but as a general point, IMHO, almost all our use of virFileReadAll has no functional or security need for us to supply a limit at all.
We really ought to just make it accept '-1' as a limit and treat that as unlimited. Meanwhile I'd suggest just letting it be 1 MB which is way bigger than i expect this data will be.
Given the above, 1MB is overkill. But still go with it as a safer, more future-proof value? Regards, Jim
You can only DoS yourself with virsh, and you'll be limited by the RPC protocol wire limit.
docs/manpages/virsh.rst | 25 ++++++++++ tools/virsh-domain.c | 107 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 132 insertions(+)
With a 1 MB or similar size limit
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Regards, Daniel

On Thu, Dec 16, 2021 at 12:06:22PM -0700, Jim Fehlig wrote:
On 12/16/21 08:02, Daniel P. Berrangé wrote:
On Tue, Dec 14, 2021 at 09:46:06PM -0700, Jim Fehlig wrote:
After attesting a domain with the help of domlaunchsecinfo, domsetlaunchsecstate can be used to set a secret in the guest domain's memory prior to running the vcpus.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
Some questions and RFC regarding this patch:
I'm not really fond of the command and function names and would appreciate suggestions :-).
I'm honestly not too fussed about the naming. THis command is really just about feature complete API coverage. I doubt many people will actually use virsh for this, instead they'll want a program that queries the measurement, verifies it and injects secret all in one go.
Also, is reading the secret header and secret from a file sufficient? The sev-tool 'package_secret' command writes the secret to a file.
Fine IMHO.
Lastly, I'm not sure what sizes to expect for secret and secret header. I may have overlooked it, but didn't find anything related to the size in the docs. I've temporarily set it to VSH_MAX_XML_FILE until we know a reasonable value.
I took another look at the API spec and section "6.6 LAUNCH_SECRET", subsection "6.6.2 Parameters" describes the parameters to the LUANCH_SECRET command and the layout the the secret header. IIUC, GUEST_PADDR and GUEST_LENGTH tell where to put the secret. GUEST_LENGTH cannot be greater than 16KB. And the secret header looks to be 104 bytes.
https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
I'm not too sure either, but as a general point, IMHO, almost all our use of virFileReadAll has no functional or security need for us to supply a limit at all.
We really ought to just make it accept '-1' as a limit and treat that as unlimited. Meanwhile I'd suggest just letting it be 1 MB which is way bigger than i expect this data will be.
Given the above, 1MB is overkill. But still go with it as a safer, more future-proof value?
Given the docs I'm happy if we drop it down to 64kb, as that still gives lots of headroom. 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 :|
participants (3)
-
Daniel P. Berrangé
-
Jim Fehlig
-
John Ferlan