Message-ID: <ed3d8b93-2631-45fb-dd9d-69a97a9338a7@amd.com>
Date: Fri, 16 Feb 2024 13:07:34 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Dan Williams <dan.j.williams@...el.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>, Michael Roth <michael.roth@....com>,
 Ashish Kalra <ashish.kalra@....com>
Subject: Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support
 for an SVSM

On 2/12/24 20:34, Dan Williams wrote:
> Tom Lendacky wrote:
>> On 2/2/24 01:10, Dan Williams wrote:
>>> 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_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  |  55 ++++++++++
>>>>    arch/x86/include/asm/sev.h              |  31 +++++-
>>>>    arch/x86/kernel/sev.c                   |  50 +++++++++
>>>>    drivers/virt/coco/sev-guest/sev-guest.c | 137 ++++++++++++++++++++++++
>>>>    drivers/virt/coco/tsm.c                 |  95 +++++++++++++++-
>>>>    include/linux/tsm.h                     |  11 ++
>>>>    6 files changed, 376 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
>>>> index dd24202b5ba5..c5423987d323 100644
>>>> --- a/Documentation/ABI/testing/configfs-tsm
>>>> +++ b/Documentation/ABI/testing/configfs-tsm
>>>> @@ -31,6 +31,21 @@ Description:
>>>>    		Standardization v2.03 Section 4.1.8.1 MSG_REPORT_REQ.
>>>>    		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf
>>>>    
>>>> +What:		/sys/kernel/config/tsm/report/$name/manifestblob
>>>> +Date:		January, 2024
>>>> +KernelVersion:	v6.9
>>>> +Contact:	linux-coco@...ts.linux.dev
>>>> +Description:
>>>> +		(RO) Optional supplemental data that a TSM may emit, visibility
>>>> +		of this attribute depends on TSM, and may be empty if no
>>>> +		manifest data is available.
>>>> +
>>>> +		When @provider is "sev_guest" and the "svsm" attribute is set
>>>> +		this file contains the service manifest used for the SVSM
>>>> +		attestation report from 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 wish configfs had better dynamic visibility so that this could be
>>> hidden when not active... oh well.
>>
>> So do I. I played with the idea of having two different structs and
>> registering one or the other based on whether an SVSM was present. Thoughts?
> 
> That's the status quo for the differences between TDX vs SEV
> (tsm_report_default_type vs tsm_report_extra_type). I think a
> "tsm_report_service_type " can be a new superset of
> tsm_report_extra_type. That that just starts to get messy if
> implementations start to pick and choose on a finer granularity what
> they support. For example, what if TDX supports these new service
> attributes, but not privlevel.
> 
> It seems straightforward to add an is_visible() callback to
> 'struct configfs_item_operations'. Then a common superset of all the
> attributes could be specified, but with an is_visible() implementation
> that consults with data registered with tsm_register() rather than the
> @type argument directly.

Let me look into that.

> 
> [..]
>>>> +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 any non-zero value
>>>
>>> Just use kstrtobool just to have a bit more form to it, and who knows
>>> maybe keeping some non-zero numbers reserved turns out useful someday.
>>>
>>> ...or see below, maybe this shouldn't be an "svsm" flag.
>>>
>>>> +		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
>>>
>>> So this affects the output format of outblob? I think @outblob should
>>> probably document the fact that it depends on @provider, @privlevel, and
>>> now @svsm. Probably all of the format links should live under @outblob
>>> not @provider.
>>
>> It doesn't change the output format, it is still a standard SNP
>> attestation report. What changes is that a SHA-512 hash of the nonce and
>> the manifest are taken and passed as report data as opposed to just the
>> nonce value.
> 
> If it is the same format, and the input is user controlled then I am
> confused what the new ABI is selecting? Could it not be inferred by
> privlevel?

The new ABI selects whether to go through the SVSM to get the attestation
report, which will additionally return a manifest that, along with the
nonce, has become part of the report through hashing.

But, yes, I mentioned in a previous reply [1] that we could use privlevel
to determine whether to invoke attestation through an SVSM request or
through the standard method of issuing an extended guest request.

[1] https://lore.kernel.org/lkml/3fca61f2-6fe0-4431-818e-9c7b96c6a391@amd.com/

> 
> [..]
>>>> +
>>>> +What:		/sys/kernel/config/tsm/report/$name/service_version
>>>> +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. Indicates the service manifest
>>>> +		version requested for the attestation report.
>>>> +		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
>>>
>>> Perhaps document that version 1 is assumed and is always compatible?
>>
>> Can do.
>>
>>>
>>> What prompts new versions and how does that discovered by guest software?
>>
>> New versions will depend on the service that is running. Changes or
>> updates to that service would document the new versions manifest output.
>> There isn't currently a discoverable way other than calling with the
>> requested version and seeing if an error is returned.
>>
>> A possible extension to the SVSM attestation protocol could create a
>> version query call.
> 
> Can the version be made to not matter, or be inferred by the presence of
> a new enumerated service capability? For example, make the same compat
> guarantees that ACPI methods do between versions where extensions are
> optional and software can always use v1 without breaking? Otherwise, I
> am not grokking what software should do with this version.

Software can always use v1. The idea is that if a service wanted to
provide additional information or change the information provided in the
service manifest, then it would have to do that via a new version of its
manifest so as not to break existing users. By default, zero would be used
for the service manifest version and have to be updated by the user if
they wanted a different one.

> 
> Separately, is this a version for the service protocol or a version of
> the manifest format? The description makes it sound like the latter, but
> the "service_version" name makes it sound like the former.

Correct, it is for the manifest version. I can rename it to
service_manifest or service_manifest_version. I'd rather not rename it to
manifest_version since it is specific to an individual service.

Thanks,
Tom

Powered by blists - more mailing lists