Re: [PATCH v2 11/14] x86/sev: Extend the config-fs attestation support for an SVSM
From: Tom Lendacky
Date: Mon Mar 11 2024 - 12:16:21 EST
On 3/10/24 00:06, Kuppuswamy, Sathyanarayanan wrote:
On 3/8/24 10:35 AM, Tom Lendacky wrote:
When an SVSM is present, the guest can also request attestation reports
from the SVSM. These SVSM attestation reports can be used to attest the
SVSM and any services running within the SVSM.
Extend the config-fs attestation support to allow for an SVSM attestation
report. This involves creating four (4) new config-fs attributes:
- 'svsm' (input)
This attribute is used to determine whether the attestation request
should be sent to the SVSM or to the SEV firmware.
- 'service_guid' (input)
Used for requesting the attestation of a single service within the
SVSM. A null GUID implies that the SVSM_ATTEST_SERVICES call should
be used to request the attestation report. A non-null GUID implies
that the SVSM_ATTEST_SINGLE_SERVICE call should be used.
- 'service_manifest_version' (input)
Used with the SVSM_ATTEST_SINGLE_SERVICE call, the service version
represents a specific service manifest version be used for the
attestation report.
- 'manifestblob' (output)
Used to return the service manifest associated with the attestation
report.
Signed-off-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
---
Documentation/ABI/testing/configfs-tsm | 59 ++++++++++
arch/x86/include/asm/sev.h | 31 ++++-
arch/x86/kernel/sev.c | 50 ++++++++
drivers/virt/coco/sev-guest/sev-guest.c | 147 ++++++++++++++++++++++++
drivers/virt/coco/tsm.c | 95 ++++++++++++++-
include/linux/tsm.h | 11 ++
6 files changed, 390 insertions(+), 3 deletions(-)
diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
index dd24202b5ba5..a4663610bf7c 100644
--- a/Documentation/ABI/testing/configfs-tsm
+++ b/Documentation/ABI/testing/configfs-tsm
+
+What: /sys/kernel/config/tsm/report/$name/svsm
+Date: January, 2024
+KernelVersion: v6.9
+Contact: linux-coco@xxxxxxxxxxxxxxx
+Description:
+ (WO) Attribute is visible if a TSM implementation provider
+ supports the concept of attestation reports for TVMs running
+ under an SVSM, like SEV-SNP. Specifying a 1 (or other boolean
Since service_guid can be used for non SVSM services as well, can we use
a generic term "service" here? And let user specify the service type
(like service=svsm)
I suppose that's possible. I think we would need a better term than just
service, though, since service_guid is specific to a service within the
service provider... so maybe service_provider.
+ equivalent, e.g. "Y") implies that the attestation report
+ should come from the SVSM.
+ Secure VM Service Module for SEV-SNP Guests v1.00 Section 7.
+ https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
+
+What: /sys/kernel/config/tsm/report/$name/service_guid
+Date: January, 2024
+KernelVersion: v6.9
+Contact: linux-coco@xxxxxxxxxxxxxxx
+Description:
+ (WO) Attribute is visible if a TSM implementation provider
+ supports the concept of attestation reports for TVMs running
+ under an SVSM, like SEV-SNP. Specifying a empty or null GUID
+ (00000000-0000-0000-0000-000000) requests all active services
+ within the SVSM be part of the attestation report. Specifying
+ a non-null GUID requests an attestation report of just the
+ specified service using the manifest form specified by the
+ service_manifest_version attribute.
+ Secure VM Service Module for SEV-SNP Guests v1.00 Section 7.
+ https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/58019.pdf
+
I think it will be useful to the user if there is a attribute to list the service GUIDs
supported. It can help prevent user using incorrect or unsupported GUIDs.
A list of supported GUIDs can be obtained from the manifest of a
all-services attestation request.
>> + if (guid_is_null(&desc->service_guid)) {
+ call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SERVICES);
+ } else {
+ export_guid(attest_call.service_guid, &desc->service_guid);
+ attest_call.service_manifest_version = desc->service_manifest_version;
+
+ call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SINGLE_SERVICE);
+ }
Above initialization will not change during retry, right? Why not move it above
retry?
True, will move it outside of the loop.
+
+ /* Obtain the GUID string length */
+ guid_len = (len && buf[len - 1] == '\n') ? len - 1 : len;
+ if (guid_len && guid_len != UUID_STRING_LEN)
+ return -EINVAL;
+
I don't think you need above checks. I think guid_parse will fail, if it is not
a valid GUID.
Yes and no. The guid_parse() function will succeed if the string is longer
than UUID_STRING_LEN as long as it is a valid UUID up to UUID_STRING_LEN.
In other words, guid_parse() of:
aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee
and
aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee-gg
both succeed.
I'm ok with eliminating the length calculation and check if everyone is in
favor of doing that given the above behavior.
+ if (guid_len == UUID_STRING_LEN) {
+ rc = guid_parse(buf, &report->desc.service_guid);
+ if (rc)
+ return rc;
+ } else {
+ report->desc.service_guid = guid_null;
I think the default value will be guid_null right, why reset it to NULL for every failed attempt?
Default, yes. But what if it is written once, then a second time with an
invalid GUID. Should the previously written GUID still be used?
Thanks,
Tom