Received-SPF: pass (google.com: domain of linux-kernel+bounces-147163-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <3732b0d2-0471-48ce-89b8-43f425040846@amd.com>
Date: Tue, 16 Apr 2024 10:53:06 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Content-Language: en-US
To: Dan Williams <dan.j.williams@intel.com>, linux-kernel@vger.kernel.org,
 x86@kernel.org, linux-coco@lists.linux.dev, svsm-devel@coconut-svsm.dev
Cc: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
 "H. Peter Anvin" <hpa@zytor.com>, Andy Lutomirski <luto@kernel.org>,
 Peter Zijlstra <peterz@infradead.org>, Michael Roth <michael.roth@amd.com>,
 Ashish Kalra <ashish.kalra@amd.com>
References: <cover.1711405593.git.thomas.lendacky@amd.com>
 <1ca853e149fe37be748c028a9b0d00237eb73938.1711405593.git.thomas.lendacky@amd.com>
 <661e0e9418862_4d5612941c@dwillia2-mobl3.amr.corp.intel.com.notmuch>
From: Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH v3 11/14] x86/sev: Extend the config-fs attestation
 support for an SVSM
In-Reply-To: <661e0e9418862_4d5612941c@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?c1ZsTEdvM1BSRUUxRENTK0ZPbzhkaEFPbVRpcVdLSnovYjJJV2pIR3dDeFZC?=
 =?utf-8?B?VG5nekRPNEJ4N3ZNeTRRdFVJUjVNSjExNmZTd3F4Z3NQYWZucGJjd2wweU1j?=
 =?utf-8?B?TW9NbWZ2Ly9zYWlQKy83NG56V0Q5U3paem9RNm83MTRIdk5mblBIWFhwWjZU?=
 =?utf-8?B?bFQ2WW5pY0FVN0NvRjB3NlVpSnpKdm5HM05DeVNRQ045MVN6NDFnTkplbkpa?=
 =?utf-8?B?dCtjN2FpdnY2azNmUmRuV1BFT3F0ZTJ1OVpIRi9xMEl3ZW40L1NxVElaeGdE?=
 =?utf-8?B?cmx2eS9xc3lhTmRVR1FheklQNHRwMXk1dHliTUg1ZU9XeVRRTjg0RTdVcXB6?=
 =?utf-8?B?aXpueFRwbUNhbnRBSHBqRXdqckg1MU0zVU8yTnorQWY3dm44dVdxcnR1RzY0?=
 =?utf-8?B?YTVWV0IreGJBcGZIZ05peVNtMy9KYlhtWS9KUHlPSVFtK1V1d3lXTHdiODRT?=
 =?utf-8?B?Rzg5dUJEQkdpdFliQm1wVWs5cWM4bTZSa1VBUXNHQUFiWUJjZ3IrWjJmcG1W?=
 =?utf-8?B?aXZJeDdybTFEMk9pTWk5cUx1ZHg5c1NVSXlBOGJZekEzNzdFa0ZKTXhCa3Zp?=
 =?utf-8?B?VkxDUUtNWDFNRHhYZ1NtZm9NT0hDQzc0eHlNMDl1cXJXZFkwVnFZbkpYTTZn?=
 =?utf-8?B?SHdISmpCNzFUcEFXeHJoU1lHaitlQnEySjA5THBZNkZDQmlGUytSdjNOWXJ5?=
 =?utf-8?B?dllKUTIzZ0dJWFphcm5ub1R5b0kyNXhRdjdidE0xMnIwYTZ1YUw3N3BmbEl0?=
 =?utf-8?B?bi9lTTFYNnNTbDBtMkJWelllQ3pkZ2swcjVtdEVtcmZEOVdxSGVjcDJ3TXlU?=
 =?utf-8?B?WHdQaXlRc1dORS81c1NwY3I2Y3B6UlNnODcyenJjWGhhdE14Q3UxcDBJYkpr?=
 =?utf-8?B?cWlUdldPU1NKRUJLc0ZBN3lIYjl2MVhhK1VGTk9OKzBJWndFaVVyenBnNkRw?=
 =?utf-8?B?N1Z6SU9DbEdxMDZkMFh3M0dxQ1pkeTJNQTAyWXNwaUUyeCtDS3lUZUZoSzBS?=
 =?utf-8?B?SkZDNWh1eHFEelpGdkdqcFEybmJDYmFKbG14b043WEUxd1lzQ1pCbC9xeUJ5?=
 =?utf-8?B?anlRRElmTDVnRG1JeUdDMktVeVFYdWJ3WllqTTJZWG8yUkdGbCtsY0I3cE03?=
 =?utf-8?B?aHpsT08yWmF0UU84U2ZLYmNNR3IrVjBpTm14NVB3UUErMFh0TS9OSFN3YTRN?=
 =?utf-8?B?cWRCSDNMZ05HRGwwaE0vekMzMWVPZmxUYnBQRlhuSk9TQjJka0cwN1dZU3Rs?=
 =?utf-8?B?VFptbEEreHVzaUViRnpkY2Q1eFZFL2lGTTZERCt2cnF0eWVJWWQ2NTBjWkZq?=
 =?utf-8?B?eWdEWFU1VDBMTG5QTTUwamx4ZEhYRWN4L3pIRTZxOVVUSjZvSUpydTZXMk5U?=
 =?utf-8?B?cngrUjhyVTIrRnpMU0pnbUI5akYwQndKaFlXdzVMYW1DL1lVcTNDcWVYUGNI?=
 =?utf-8?B?a2hCbEJWaVM0d2lxejlmT0lRUUNZWXJUaUVTNmZ4aDcwT1JXT3NWUFYvSHow?=
 =?utf-8?B?Um13eDlZRXIvMU1yQWEvY2J2bER5T0RxWHZlZHVmNWs4MGFnWEYvWDVuM012?=
 =?utf-8?B?eUhRc2xqK0tIYmtWZVN1VHE4R3FsbnB1Z3pvL3orV3ZlUjV3S0hXbVJlUW83?=
 =?utf-8?B?TWlSTjlEaFlkY2ZoYUs1Z2thMktIWHJnM0JkYVpjYjR2OXNKNWtpbjJRc2I1?=
 =?utf-8?B?NFFKOHZ1UkJRWjFVUVg0SHdLV0c2VlFDS2FwNFJrZmM2UjRpMExSbmx2NllM?=
 =?utf-8?B?UUpLYXFydWJ3cHpxaU5FV0VvY3VZdEd6ZEFnZ3hQKzB1RGRRSFRqZnhNTUdH?=
 =?utf-8?B?VVg4WUMxdnlRc0phL3BPQ09yUExUYnRBU1FoTWhDTkdRNUdBSTJqMFh2TlVz?=
 =?utf-8?B?WVZIVXlkajROWXVzc1RQeEFzcStoZWZnZlRhbHdLNWhuN0QvcUw5RFRCcWlh?=
 =?utf-8?B?TzcyQnp2Y1RQR2dpNUJFUG1xV0Y4M2hLMmUyTGdkWHlVTzEvVGVXcXZSam1N?=
 =?utf-8?B?QUFwV0RESTFiSmRYZXdlemhiczMzeEErQjJrSy9GUmNNN1hPbk9LS2lKSFRQ?=
 =?utf-8?B?MU92azZUN3ZOdVZDWDRmdlNoSGVMVCtFSEpLZHZrQTdRTDZPWXRCRjZuUU0w?=
 =?utf-8?Q?m/kZMpq7TVnP6wLlPOB1SZ7ed?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: df7c8bdb-99d9-49f4-4b44-08dc5e2d4fb6
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Apr 2024 15:53:08.5246
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: wnxVFyJIf6qlcip9PqNqOL7XXrLu+oSDDRI6GclglrGZfWJ9LjWMVoJ0Yb0d6xNGBwtj5mIWhMXWZJYzYIcZbg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR12MB8557

On 4/16/24 00:37, 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:
>>
>>    - 'service-provider' (input)
>>      This attribute is used to determine whether the attestation request
>>      should be sent to the specified service provider or to the SEV
>>      firmware. The SVSM service provider is represented by the value
>>      'svsm'.
>>
>>    - 'service_guid' (input)
>>      Used for requesting the attestation of a single service within the
>>      service provider. 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  |  69 +++++++++++
>>   arch/x86/include/asm/sev.h              |  31 ++++-
>>   arch/x86/kernel/sev.c                   |  50 ++++++++
>>   drivers/virt/coco/sev-guest/sev-guest.c | 151 ++++++++++++++++++++++++
>>   drivers/virt/coco/tsm.c                 |  93 ++++++++++++++-
>>   include/linux/tsm.h                     |  11 ++
>>   6 files changed, 402 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
>> index dd24202b5ba5..72a7acdb5258 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.10
>> +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 @service_provider is
>> +		"svsm" this file contains the service manifest used for the SVSM
>> +		attestation report from the 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
> 
> Should this be "See 'service_provider' for the format of this blob"? To
> date external "format specification" links are only referenced once in
> this file, and this one is now duplicated.

Yes, I can do that for this and the other ones you identified below.

> 
> 
>> +
>>   What:		/sys/kernel/config/tsm/report/$name/provider
>>   Date:		September, 2023
>>   KernelVersion:	v6.7
>> @@ -80,3 +95,57 @@ Contact:	linux-coco@lists.linux.dev
>>   Description:
>>   		(RO) Indicates the minimum permissible value that can be written
>>   		to @privlevel.
>> +
>> +What:		/sys/kernel/config/tsm/report/$name/service_provider
>> +Date:		January, 2024
>> +KernelVersion:	v6.10
>> +Contact:	linux-coco@lists.linux.dev
>> +Description:
>> +		(WO) Attribute is visible if a TSM implementation provider
>> +		supports the concept of attestation reports from a service
>> +		provider for TVMs, like SEV-SNP running under an SVSM.
>> +		Specifying the service provider via this attribute will create
>> +		an attestation report as specified by the service provider.
>> +		Currently supported service-providers are:
>> +			svsm
>> +
>> +		For the SVSM service provider, see the 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
> 
> Given "SVSM" is a cross vendor concept should this explicitly
> callout: "For provider.service_provider == sev_guest.svsm" as
> preparation for other implementations defining their "svsm" manifest
> format?

I'm not sure. Do we need to get that specific? If SVSM is cross vendor, 
will it be using / adding to the existing SVSM specification? If not, 
then each vendor is likely to have its own name for the SVSM concept 
that would be unique enough...

> 

>> +
>> +What:		/sys/kernel/config/tsm/report/$name/service_manifest_version
>> +Date:		January, 2024
>> +KernelVersion:	v6.10
>> +Contact:	linux-coco@lists.linux.dev
>> +Description:
>> +		(WO) Attribute is visible if a TSM implementation provider
>> +		supports the concept of attestation reports from a service
>> +		provider for TVMs, like SEV-SNP running under an SVSM.
>> +		Indicates the service manifest version requested for the
>> +		attestation report. If this field is not set by the user,
>> +		the default manifest version of the service (the service's
>> +		initial/first manifest version) is returned. The initial
>> +		manifest version is always available.
> 
> ...and that number is zero? Is there any expectation that the kernel

Yes, that number is zero.

> sanity checks this version, or how does the user figure out they need to
> roll this request back?

Right now there aren't any non-zero versions, so there is nothing for 
the user to figure out. However, the service will determine when it 
creates a new version and then the user will need to understand what the 
reason for that is and decide. I'm not sure I can give you a specific 
answer at this stage, but we need to allow for a change in the manifest 
without affecting existing users.

And since the spec has been approved already, I can't really go back and 
add a requirement that manifest format always be additive.

> 
>> +
>> +		For the SVSM service provider, see the 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
> 

>> +static ssize_t tsm_report_service_provider_store(struct config_item *cfg,
>> +						 const char *buf, size_t len)
>> +{
>> +	struct tsm_report *report = to_tsm_report(cfg);
>> +	size_t sp_len;
>> +	char *sp;
>> +	int rc;
>> +
>> +	guard(rwsem_write)(&tsm_rwsem);
>> +	rc = try_advance_write_generation(report);
>> +	if (rc)
>> +		return rc;
>> +
>> +	sp_len = (buf[len - 1] != '\n') ? len : len - 1;
> 
> This feels like it wants a sysfs_strdup().

If there start to be more string oriented operations in the file, then 
it might be worth it. For now I don't really see a reason.

> 
>> +
>> +	sp = kstrndup(buf, sp_len, GFP_KERNEL);
>> +	if (!sp)
>> +		return -ENOMEM;
>> +	kfree(report->desc.service_provider);
>> +
>> +	report->desc.service_provider = sp;
>> +
>> +	return len;
>> +}
>> +CONFIGFS_ATTR_WO(tsm_report_, service_provider);
>> +

>>   #define TSM_DEFAULT_ATTRS() \
>>   	&tsm_report_attr_generation, \
>>   	&tsm_report_attr_provider
>> @@ -265,6 +348,9 @@ static struct configfs_attribute *tsm_report_extra_attrs[] = {
>>   	TSM_DEFAULT_ATTRS(),
>>   	&tsm_report_attr_privlevel,
>>   	&tsm_report_attr_privlevel_floor,
>> +	&tsm_report_attr_service_provider,
>> +	&tsm_report_attr_service_guid,
>> +	&tsm_report_attr_service_manifest_version,
> 
> Shouldn't this patch come after the configfs dynamic visibility so there
> is no point in the history where vestigial attributes show up?

Sure, I can do that.

Thanks,
Tom