lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <805b863c-1631-477d-9faf-f7569a8d80e4@amd.com>
Date: Mon, 11 Mar 2024 11:16:08 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@...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>,
 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/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.

> 
>> +		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.

>  >> +	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

> 

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ