[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa7cee6f-954b-4acf-a438-17ae3d1e781a@linux.intel.com>
Date: Mon, 11 Mar 2024 22:57:22 -0700
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Tom Lendacky <thomas.lendacky@....com>, linux-kernel@...r.kernel.org,
x86@...nel.org
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
"H. Peter Anvin" <hpa@...or.com>, Andy Lutomirski <luto@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Dan Williams <dan.j.williams@...el.com>, Michael Roth
<michael.roth@....com>, Ashish Kalra <ashish.kalra@....com>
Subject: Re: [PATCH v2 11/14] x86/sev: Extend the config-fs attestation
support for an SVSM
On 3/11/24 9:16 AM, Tom Lendacky wrote:
> 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@....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@...ts.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.
I am ok with 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@...ts.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.
So they have to make a request twice? Once with a NULL GUID to get the
manifest with all service list, and another to make service-specific request?
There should be a fixed list of service GUIDs, right? Why not list them by
default?
>
>> >> + 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.
Got it. Existing callers of guid_parse() does not seem to care about it. But I am fine either way.
>
>>
>>> + 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?
>
If the user write fails, why update the state? IMO, we can leave it at the old value. But, lets see what others think.
> Thanks,
> Tom
>
>>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
Powered by blists - more mailing lists