Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support for an SVSM

[Tom Lendacky <thomas.lendacky@amd.com>]

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@amd.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@lists.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.

I've been playing with this a bit.

For the configfs support I was thinking of doing a per attribute 
is_visible() callback field since the TSM support is currently all in one 
group. The callback field would be checked in fs/configfs/dir.c 
populate_attrs(). A simple bool return value should suffice, I'm not sure 
we need to get into umode changes. If the field is NULL, then the 
attribute is displayed. If non-NULL, it depends on the callback return value.

In order to keep tsm.c as vendor neutral as possible, a way to 
provide/override a default callback would be needed. The new SVSM related 
fields would have a callback assigned that always returns false by 
default, so that these attributes wouldn't be visible. A new tsm.c 
interface that takes the attribute name and a callback function can be 
used to override the requested attribute. For example, the SEV guest 
driver could register a callback for these attributes that returns true 
when running under an SVSM or false otherwise. Or it could leave the 
default in place and register a callback when running under an SVSM that 
always returns true.

Thoughts?

Thanks,
Tom

> 
> [..]
>>>> +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 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?
> 
> [..]
>>>> +
>>>> +What:		/sys/kernel/config/tsm/report/$name/service_version
>>>> +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. 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.
> 
> 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.