[RFC PATCH 0/3] Add virDomainInjectLaunchSecret API
Hi All! This series is an RFC impl for the missing API needed for SEV attestation as discussed here https://listman.redhat.com/archives/libvir-list/2021-May/msg00196.html I pinged about the status a few weeks back, found it had stalled, and agreed to work on an impl after returning from vacation https://listman.redhat.com/archives/libvir-list/2021-October/msg01052.html Although the series is only compile tested, I wanted to share it early in case others are considering the task. While discussing the missing API, Daniel suggested virDomainSetLaunchSecurityInfo https://listman.redhat.com/archives/libvir-list/2021-October/msg01074.html but noted the asymmetry with virDomainGetLaunchSecurityInfo. I decided to go with virDomainInjectLaunchSecret, which better describes the function IMO. I also decided to go with an explicit set of parameters, following in the footsteps of virDrvDomainAuthorizedSSHKeys*. It wasn't until patch 3 that I realized virTypedParameter is definitely a better approach for an API that may need future support for other types of secrets. I'll make that change in a V1 after collecting feedback on this RFC. Regards, Jim Jim Fehlig (3): libvirt: Introduce virDomainInjectLaunchSecret public API remote: Implement domain inject launch secret API qemu: Implement the virDomainInjectLaunchSecret API include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 +++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ src/qemu/qemu_driver.c | 53 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 12 +++++++ src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 34 ++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 +++ src/remote/remote_daemon_dispatch.c | 27 +++++++++++++++ src/remote/remote_driver.c | 32 +++++++++++++++++ src/remote/remote_protocol.x | 16 ++++++++- src/remote_protocol-structs | 8 +++++ 13 files changed, 261 insertions(+), 1 deletion(-) -- 2.33.0
An API inject a launch secret into the domain's memory. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 69 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 2f017c5b68..418ee4bd2d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5091,6 +5091,12 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, int *nparams, unsigned int flags); +int virDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + 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..a308754d5b 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -1333,6 +1333,13 @@ typedef int int *nparams, unsigned int flags); +typedef int +(*virDrvDomainInjectLaunchSecret)(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags); + typedef virDomainCheckpointPtr (*virDrvDomainCheckpointCreateXML)(virDomainPtr domain, const char *xmlDesc, @@ -1661,6 +1668,7 @@ struct _virHypervisorDriver { virDrvConnectBaselineHypervisorCPU connectBaselineHypervisorCPU; virDrvNodeGetSEVInfo nodeGetSEVInfo; virDrvDomainGetLaunchSecurityInfo domainGetLaunchSecurityInfo; + virDrvDomainInjectLaunchSecret domainInjectLaunchSecret; virDrvDomainCheckpointCreateXML domainCheckpointCreateXML; virDrvDomainCheckpointGetXMLDesc domainCheckpointGetXMLDesc; virDrvDomainListAllCheckpoints domainListAllCheckpoints; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ce7cafde36..877c65c04f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12818,6 +12818,56 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, } +/** + * virDomainInjectLaunchSecret: + * @domain: a domain object + * @secrethdr: Base64 encoded secret header + * @secret: Base64 encoded secret + * @injectaddr: Domain memory address where the secret will be injected + * @flags: currently used, set to 0. + * + * Inject a launch secret in the domain's memory. secrethdr and secret are + * passed to the underlying hypervisor as is. injectaddr can be used to + * specify an address in the domain memory where the secret will be injected. + * It can be set to 0 for the hypervisor default. + * + * Returns -1 in case of failure, 0 in case of success. + */ +int virDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags) +{ + virConnectPtr conn = domain->conn; + + VIR_DOMAIN_DEBUG(domain, "secrethdr=%p, secret=%p injectaddr=%llu flags=0x%x", + secrethdr, secret, injectaddr, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgGoto(secrethdr, error); + virCheckNonNullArgGoto(secret, error); + virCheckPositiveArgGoto(injectaddr, error); + virCheckReadOnlyGoto(conn->flags, error); + + if (conn->driver->domainInjectLaunchSecret) { + int ret; + ret = conn->driver->domainInjectLaunchSecret(domain, secrethdr, + secret, injectaddr, 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..c5e708d475 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_7.10.0 { + global: + virDomainInjectLaunchSecret; +} LIBVIRT_7.8.0; + # .... define new API here using predicted next version number .... -- 2.33.0
On Tue, Nov 16, 2021 at 19:23:52 -0700, Jim Fehlig wrote:
An API inject a launch secret into the domain's memory.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 69 insertions(+)
Note that I don't know enough about SEV to do a full review. These are merely observations based on the interface of qemu and the documentation for them.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ce7cafde36..877c65c04f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12818,6 +12818,56 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, }
+/** + * virDomainInjectLaunchSecret: + * @domain: a domain object + * @secrethdr: Base64 encoded secret header + * @secret: Base64 encoded secret + * @injectaddr: Domain memory address where the secret will be injected + * @flags: currently used, set to 0. + * + * Inject a launch secret in the domain's memory. secrethdr and secret are + * passed to the underlying hypervisor as is.
While this might be true for qemu IMO it doesn't make much sense to mention this in the documentation because it's not exactly important to the user that it's not modified.
injectaddr can be used to + * specify an address in the domain memory where the secret will be injected. + * It can be set to 0 for the hypervisor default.
This makes an assumption that address 0 will never be used to inject the secret. Note that qemu actually supports that as the 'gpa' attribute of the command is optional, so you can populate it with a 0 to inject into address 0. In general the API feels too-much tied to the implementation of sev used here and IMO isn't future proof, but I don't have enough examples to prove my point.
On 11/17/21 03:33, Peter Krempa wrote:
On Tue, Nov 16, 2021 at 19:23:52 -0700, Jim Fehlig wrote:
An API inject a launch secret into the domain's memory.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 69 insertions(+)
Note that I don't know enough about SEV to do a full review. These are merely observations based on the interface of qemu and the documentation for them.
diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ce7cafde36..877c65c04f 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -12818,6 +12818,56 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, }
+/** + * virDomainInjectLaunchSecret: + * @domain: a domain object + * @secrethdr: Base64 encoded secret header + * @secret: Base64 encoded secret + * @injectaddr: Domain memory address where the secret will be injected + * @flags: currently used, set to 0. + * + * Inject a launch secret in the domain's memory. secrethdr and secret are + * passed to the underlying hypervisor as is.
While this might be true for qemu IMO it doesn't make much sense to mention this in the documentation because it's not exactly important to the user that it's not modified.
Ok. I need to improve the description in a V1.
injectaddr can be used to + * specify an address in the domain memory where the secret will be injected. + * It can be set to 0 for the hypervisor default.
This makes an assumption that address 0 will never be used to inject the secret. Note that qemu actually supports that as the 'gpa' attribute of the command is optional, so you can populate it with a 0 to inject into address 0.
Good point, thanks!
In general the API feels too-much tied to the implementation of sev used here and IMO isn't future proof, but I don't have enough examples to prove my point.
Using virTypedParams for input will help with future-proofness. I'm still not fond of the API name. I see Daniel has suggested a better name. I'll respond to his post with some other ideas I had. Regards, Jim
On Tue, Nov 16, 2021 at 07:23:52PM -0700, Jim Fehlig wrote:
An API inject a launch secret into the domain's memory.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 69 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 2f017c5b68..418ee4bd2d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5091,6 +5091,12 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, int *nparams, unsigned int flags);
+int virDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags);
I thought of a better name at last, that shows its relation to virDomainGetLaunchSecurityInfo without implying that they are the direct inverse of each other: virDomainSetLaunchSecurityState(...) Also, we whould bear in mind that the set of state parameters may be differnt for vendors other than AMD, and even later generations of AMD SEV might want more parameters. So lets use a 'virTypedParameter' array for this methodeg virDomainSetLaunchSecurityState(virDomainPtr dom, virTypedParameterPtr params, int nparams, unsigned int flags); 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 11/23/21 10:28, Daniel P. Berrangé wrote:
On Tue, Nov 16, 2021 at 07:23:52PM -0700, Jim Fehlig wrote:
An API inject a launch secret into the domain's memory.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- include/libvirt/libvirt-domain.h | 6 ++++ src/driver-hypervisor.h | 8 +++++ src/libvirt-domain.c | 50 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 ++++ 4 files changed, 69 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 2f017c5b68..418ee4bd2d 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -5091,6 +5091,12 @@ int virDomainGetLaunchSecurityInfo(virDomainPtr domain, int *nparams, unsigned int flags);
+int virDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags);
I thought of a better name at last, that shows its relation to virDomainGetLaunchSecurityInfo without implying that they are the direct inverse of each other:
virDomainSetLaunchSecurityState(...)
I need to get over my distaste for 'launch' in the API name. virDomainGetLaunchSecurityInfo already exists, so no changing that. And not including 'launch' in the Set API would be a source of confusion. If we were creating the names anew, I'd prefer something like virDomain{Get,Set}PrestartSecret.
Also, we whould bear in mind that the set of state parameters may be differnt for vendors other than AMD, and even later generations of AMD SEV might want more parameters.
Nod.
So lets use a 'virTypedParameter' array for this methodeg
Right. I mentioned that in the cover letter. While hacking on patch3 I realized explicit params was a no-go :-).
virDomainSetLaunchSecurityState(virDomainPtr dom, virTypedParameterPtr params, int nparams, unsigned int flags);
Thanks! I'll include this in a V1. Regards, Jim
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/remote/remote_daemon_dispatch.c | 27 ++++++++++++++++++++++++ src/remote/remote_driver.c | 32 +++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 16 ++++++++++++++- src/remote_protocol-structs | 8 ++++++++ 4 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 689001889e..f2f7b35f53 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3451,6 +3451,33 @@ remoteDispatchDomainGetLaunchSecurityInfo(virNetServer *server G_GNUC_UNUSED, return rv; } +static int +remoteDispatchDomainInjectLaunchSecret(virNetServer *server G_GNUC_UNUSED, + virNetServerClient *client, + virNetMessage *msg G_GNUC_UNUSED, + struct virNetMessageError *rerr, + remote_domain_inject_launch_secret_args *args) +{ + int rv = -1; + virConnectPtr conn = remoteGetHypervisorConn(client); + virDomainPtr dom = NULL; + + if (!conn) + goto cleanup; + + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + + rv = virDomainInjectLaunchSecret(dom, args->secrethdr, args->secret, + args->injectaddr, args->flags); + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virObjectUnref(dom); + return rv; +} + static int remoteDispatchDomainGetPerfEvents(virNetServer *server G_GNUC_UNUSED, virNetServerClient *client, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 235c406a5a..4fbb3c5bad 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1893,6 +1893,37 @@ remoteDomainGetLaunchSecurityInfo(virDomainPtr domain, return rv; } +static int +remoteDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags) +{ + int rv = -1; + struct private_data *priv = domain->conn->privateData; + remote_domain_inject_launch_secret_args args; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.secrethdr = (char *) secrethdr; + args.secret = (char *) secret; + args.injectaddr = injectaddr; + args.flags = flags; + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_INJECT_LAUNCH_SECRET, + (xdrproc_t) xdr_remote_domain_inject_launch_secret_args, (char *) &args, + (xdrproc_t) xdr_void, (char *) NULL) == -1) + goto done; + + rv = 0; + + done: + remoteDriverUnlock(priv); + return rv; +} + static int remoteDomainGetPerfEvents(virDomainPtr domain, virTypedParameterPtr *params, @@ -8574,6 +8605,7 @@ static virHypervisorDriver hypervisor_driver = { .domainAuthorizedSSHKeysSet = remoteDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = remoteDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = remoteDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainInjectLaunchSecret = remoteDomainInjectLaunchSecret, /* 7.10.0 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 60010778ca..fb0da81e9a 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -3900,6 +3900,14 @@ struct remote_domain_event_memory_device_size_change_msg { unsigned hyper size; }; +struct remote_domain_inject_launch_secret_args { + remote_nonnull_domain dom; + remote_nonnull_string secrethdr; + remote_nonnull_string secret; + unsigned hyper injectaddr; + unsigned int flags; +}; + /*----- Protocol. -----*/ /* Define the program number, protocol version and procedure numbers here. */ @@ -6905,5 +6913,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: none + * @acl: domain:write + */ + REMOTE_PROC_DOMAIN_INJECT_LAUNCH_SECRET = 439 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index dbef4ace79..c9e26b0ce1 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -3241,6 +3241,13 @@ struct remote_domain_event_memory_device_size_change_msg { remote_nonnull_string alias; uint64_t size; }; +struct remote_domain_inject_launch_secret_args { + remote_nonnull_domain dom; + remote_nonnull_string secrethdr; + remote_nonnull_string secret; + uint64_t injectaddr; + u_int flags; +}; enum remote_procedure { REMOTE_PROC_CONNECT_OPEN = 1, REMOTE_PROC_CONNECT_CLOSE = 2, @@ -3680,4 +3687,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_INJECT_LAUNCH_SECRET = 439, }; -- 2.33.0
Inject a launch secret in domain memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabed domains. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 12 ++++++++ src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 34 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 5 files changed, 110 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d954635dde..58e3f08afe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20104,6 +20104,58 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; } + +static int +qemuDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainInjectLaunchSecretEnsureACL(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", + _("injecting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + } + + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob; + + if (qemuMonitorInjectLaunchSecret(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, injectaddr) < 0) + goto endjob; + + if (qemuDomainObjExitMonitor(driver, vm) < 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 | @@ -20981,6 +21033,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainAuthorizedSSHKeysSet = qemuDomainAuthorizedSSHKeysSet, /* 6.10.0 */ .domainGetMessages = qemuDomainGetMessages, /* 7.1.0 */ .domainStartDirtyRateCalc = qemuDomainStartDirtyRateCalc, /* 7.2.0 */ + .domainInjectLaunchSecret = qemuDomainInjectLaunchSecret, /* 7.10.0 */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 810dac209d..c64469a03b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4383,6 +4383,18 @@ qemuMonitorGetSEVMeasurement(qemuMonitor *mon) } +int +qemuMonitorInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONInjectLaunchSecret(mon, secrethdr, secret, injectaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0dd7b1c4e2..2dec2b57bb 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1445,6 +1445,12 @@ int qemuMonitorBlockdevMediumInsert(qemuMonitor *mon, char * qemuMonitorGetSEVMeasurement(qemuMonitor *mon); +int +qemuMonitorInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr); + 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 4669b9135d..69aef078ec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8124,6 +8124,40 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon) } +/** + * The function is used to inject a launch secret in an SEV guest. + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + */ +int +qemuMonitorJSONInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", injectaddr, + 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 f7fb13f56c..95758cdc6e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -368,6 +368,11 @@ int qemuMonitorJSONSystemWakeup(qemuMonitor *mon); char *qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon); +int qemuMonitorJSONInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr); + int qemuMonitorJSONGetVersion(qemuMonitor *mon, int *major, int *minor, -- 2.33.0
On Tue, Nov 16, 2021 at 19:23:54 -0700, Jim Fehlig wrote:
Inject a launch secret in domain memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabed domains.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 12 ++++++++ src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 34 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 5 files changed, 110 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d954635dde..58e3f08afe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20104,6 +20104,58 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; }
+ +static int +qemuDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainInjectLaunchSecretEnsureACL(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", + _("injecting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + }
Missing check that the VM is running.
+ + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob;
No need for async monitor without an async job
+ + if (qemuMonitorInjectLaunchSecret(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, injectaddr) < 0) + goto endjob; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup:
[...]
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 810dac209d..c64469a03b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4383,6 +4383,18 @@ qemuMonitorGetSEVMeasurement(qemuMonitor *mon) }
+int +qemuMonitorInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + QEMU_CHECK_MONITOR(mon);
You cleverly avoid logging the secret here ...
+ + return qemuMonitorJSONInjectLaunchSecret(mon, secrethdr, secret, injectaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo)
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4669b9135d..69aef078ec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8124,6 +8124,40 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon) }
+/** + * The function is used to inject a launch secret in an SEV guest. + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + */ +int +qemuMonitorJSONInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", injectaddr, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
... but once this is sent to the monitor it can be logged if QMP traffic logging is enabled. Thus either the secret is not so secret, but the qemu documentation nor the documentation you've added seem to be lessening the secrecy of the passed string in any way, or the 'sev-inject-launch-secret' is broken by design as it doesn't support passing an encrypted object via qcrypto. In case when the secret must really be kept secret the QMP command must be fixed first, otherwise libvirt is not able to guarantee any secrecy.
+ return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +}
On 11/17/21 03:39, Peter Krempa wrote:
On Tue, Nov 16, 2021 at 19:23:54 -0700, Jim Fehlig wrote:
Inject a launch secret in domain memory using the sev-inject-launch-secret QMP API. Only supported for SEV-enabed domains.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/qemu/qemu_driver.c | 53 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 12 ++++++++ src/qemu/qemu_monitor.h | 6 ++++ src/qemu/qemu_monitor_json.c | 34 +++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 5 ++++ 5 files changed, 110 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index d954635dde..58e3f08afe 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -20104,6 +20104,58 @@ qemuDomainGetLaunchSecurityInfo(virDomainPtr domain, return ret; }
+ +static int +qemuDomainInjectLaunchSecret(virDomainPtr domain, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr, + unsigned int flags) +{ + virQEMUDriver *driver = domain->conn->privateData; + virDomainObj *vm; + int ret = -1; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomainObjFromDomain(domain))) + goto cleanup; + + if (virDomainInjectLaunchSecretEnsureACL(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", + _("injecting a launch secret is only supported in SEV-enabled domains")); + goto cleanup; + }
Missing check that the VM is running.
+ + if (qemuDomainObjBeginJob(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + if (qemuDomainObjEnterMonitorAsync(driver, vm, QEMU_ASYNC_JOB_NONE) < 0) + goto endjob;
No need for async monitor without an async job
+ + if (qemuMonitorInjectLaunchSecret(QEMU_DOMAIN_PRIVATE(vm)->mon, + secrethdr, secret, injectaddr) < 0) + goto endjob; + + if (qemuDomainObjExitMonitor(driver, vm) < 0) + goto endjob; + + ret = 0; + + endjob: + qemuDomainObjEndJob(driver, vm); + + cleanup:
[...]
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 810dac209d..c64469a03b 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4383,6 +4383,18 @@ qemuMonitorGetSEVMeasurement(qemuMonitor *mon) }
+int +qemuMonitorInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + QEMU_CHECK_MONITOR(mon);
You cleverly avoid logging the secret here ...
+ + return qemuMonitorJSONInjectLaunchSecret(mon, secrethdr, secret, injectaddr); +} + + int qemuMonitorGetPRManagerInfo(qemuMonitor *mon, GHashTable **retinfo)
[...]
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4669b9135d..69aef078ec 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -8124,6 +8124,40 @@ qemuMonitorJSONGetSEVMeasurement(qemuMonitor *mon) }
+/** + * The function is used to inject a launch secret in an SEV guest. + * + * Example JSON: + * + * { "execute" : "sev-inject-launch-secret", + * "data": { "packet-header": "str", "secret": "str", "gpa": "uint64" } } + */ +int +qemuMonitorJSONInjectLaunchSecret(qemuMonitor *mon, + const char *secrethdr, + const char *secret, + unsigned long long injectaddr) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) reply = NULL; + + if (!(cmd = qemuMonitorJSONMakeCommand("sev-inject-launch-secret", + "s:packet-header", secrethdr, + "s:secret", secret, + "U:gpa", injectaddr, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
... but once this is sent to the monitor it can be logged if QMP traffic logging is enabled.
Thus either the secret is not so secret, but the qemu documentation nor the documentation you've added seem to be lessening the secrecy of the passed string in any way, or the 'sev-inject-launch-secret' is broken by design as it doesn't support passing an encrypted object via qcrypto.
AFAICT, the secret is an encrypted object. See section "6.6 LAUNCH_SECRET" in the following doc https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf I'll need to do more snooping so I can improve the API documentation in 1/3. A clearer description of the API and its parameters would help with questions like this. Regards, Jim
In case when the secret must really be kept secret the QMP command must be fixed first, otherwise libvirt is not able to guarantee any secrecy.
+ return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + return 0; +}
participants (3)
-
Daniel P. Berrangé -
Jim Fehlig -
Peter Krempa