Messages in this thread | | | Date | Mon, 11 Mar 2024 11:16:08 -0500 | Subject | Re: [PATCH v2 11/14] x86/sev: Extend the config-fs attestation support for an SVSM | From | Tom Lendacky <> |
| |
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@amd.com> >> --- >> 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@lists.linux.dev >> +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@lists.linux.dev >> +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
>
| |