Message-ID: <f9329bb2-9000-4988-a500-17042c0081c9@intel.com>
Date: Mon, 5 Feb 2024 15:29:43 -0800
From: "Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@...el.com>
To: Dan Williams <dan.j.williams@...el.com>, 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>, 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/1/24 11:10 PM, 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.
>
>> +
>>  What:		/sys/kernel/config/tsm/report/$name/provider
>>  Date:		September, 2023
>>  KernelVersion:	v6.7
>> @@ -80,3 +95,43 @@ Contact:	linux-coco@...ts.linux.dev
>>  Description:
>>  		(RO) Indicates the minimum permissible value that can be written
>>  		to @privlevel.
>> +
>> +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.
>
> I worry that "svsm" is not going to be the last name for a selected
> family of services that might convey something to outblob. I wonder if
> this should just be a generic "service" attribute where "svsm" is only
> supported value to write today. Another service family could be
> introduced later and reuse the service_guid concept, or kernel gets
> lucky and a de-facto standard heads off proliferation here.
>
>> +
>> +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_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
> Given the small number of service GUIDs so far perhaps save someone the
> URL fetch and list it here?

How will user know about the available GUIDs? Is there a way for user to
query this list?

>
>> +
>> +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?
>
> What prompts new versions and how does that discovered by guest software?

Why user care about it? If it is going to affect manifestblob output, I
recommend adding that info there.

>
>> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
>> index b126e50a1358..4cafa92d1d3e 100644
>> --- a/arch/x86/include/asm/sev.h
>> +++ b/arch/x86/include/asm/sev.h
>> @@ -194,6 +194,27 @@ struct svsm_pvalidate_call {
>>  	struct svsm_pvalidate_entry entry[];
>>  };
>>  
>> +/*
>> + * The SVSM Attestation related structures
>> + */
>> +struct svsm_location_entry {
>> +	u64 pa;
>> +	u32 len;
>> +	u8 rsvd[4];
>> +};
>> +
>> +struct svsm_attestation_call {
>> +	struct svsm_location_entry report_buffer;
>> +	struct svsm_location_entry nonce;
>> +	struct svsm_location_entry manifest_buffer;
>> +	struct svsm_location_entry certificates_buffer;
>> +
>> +	/* For attesting a single service */
>> +	u8 service_guid[16];
>> +	u32 service_version;
>> +	u8 rsvd[4];
>> +};
>> +
>>  /*
>>   * SVSM protocol structure
>>   */
>> @@ -217,6 +238,10 @@ struct svsm_call {
>>  #define SVSM_CORE_CREATE_VCPU		2
>>  #define SVSM_CORE_DELETE_VCPU		3
>>  
>> +#define SVSM_ATTEST_CALL(x)		((1ULL << 32) | (x))
>> +#define SVSM_ATTEST_SERVICES		0
>> +#define SVSM_ATTEST_SINGLE_SERVICE	1
>> +
>>  #ifdef CONFIG_AMD_MEM_ENCRYPT
>>  extern void __sev_es_ist_enter(struct pt_regs *regs);
>>  extern void __sev_es_ist_exit(void);
>> @@ -287,6 +312,7 @@ void snp_set_wakeup_secondary_cpu(void);
>>  bool snp_init(struct boot_params *bp);
>>  void __init __noreturn snp_abort(void);
>>  int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio);
>> +int snp_issue_svsm_attestation_request(u64 call_id, struct svsm_attestation_call *input);
>>  void snp_accept_memory(phys_addr_t start, phys_addr_t end);
>>  u64 snp_get_unsupported_features(u64 status);
>>  u64 sev_get_status(void);
>> @@ -316,7 +342,10 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in
>>  {
>>  	return -ENOTTY;
>>  }
>> -
>> +static inline int snp_issue_svsm_attestation_request(u64 call_id, struct svsm_attestation_call *input)
>> +{
>> +	return -ENOTTY;
>> +}
>>  static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
>>  static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
>>  static inline u64 sev_get_status(void) { return 0; }
>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>> index 849df3aae4e1..83bc5efa8fcf 100644
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -2378,6 +2378,56 @@ static int __init init_sev_config(char *str)
>>  }
>>  __setup("sev=", init_sev_config);
>>  
>> +static void update_attestation_input(struct svsm_call *call, struct svsm_attestation_call *input)
>> +{
>> +	/* If (new) lengths have been returned, propograte them up */
>> +	if (call->rcx_out != call->rcx)
>> +		input->manifest_buffer.len = call->rcx_out;
>> +
>> +	if (call->rdx_out != call->rdx)
>> +		input->certificates_buffer.len = call->rdx_out;
>> +
>> +	if (call->r8_out != call->r8)
>> +		input->report_buffer.len = call->r8_out;
>> +}
>> +
>> +int snp_issue_svsm_attestation_request(u64 call_id, struct svsm_attestation_call *input)
>> +{
>> +	struct svsm_attestation_call *attest_call;
>> +	struct svsm_call call = {};
>> +	unsigned long flags;
>> +	u64 attest_call_pa;
>> +	int ret;
>> +
>> +	if (!vmpl)
>> +		return -EINVAL;
>> +
>> +	local_irq_save(flags);
>> +
>> +	call.caa = __svsm_get_caa();
>> +
>> +	attest_call = (struct svsm_attestation_call *)call.caa->svsm_buffer;
>> +	attest_call_pa = __svsm_get_caa_pa() + offsetof(struct svsm_ca, svsm_buffer);
>> +
>> +	memcpy(attest_call, input, sizeof(*attest_call));
> *attest_call = *input? Just to save that little bit of reviewer
> brainpower wondering if these things are the same type and size?
>
>> +
>> +	/*
>> +	 * Set input registers for the request and set RDX and R8 to known
>> +	 * values in order to detect length values being returned in them.
>> +	 */
>> +	call.rax = call_id;
>> +	call.rcx = attest_call_pa;
>> +	call.rdx = -1;
>> +	call.r8 = -1;
>> +	ret = svsm_protocol(&call);
>> +	update_attestation_input(&call, input);
>> +
>> +	local_irq_restore(flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(snp_issue_svsm_attestation_request);
>> +
>>  int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct snp_guest_request_ioctl *rio)
>>  {
>>  	struct ghcb_state state;
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index 1ff897913bf4..3693373c4227 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
>> @@ -783,6 +783,140 @@ struct snp_msg_cert_entry {
>>  	u32 length;
>>  };
>>  
>> +static int sev_svsm_report_new(struct tsm_report *report, void *data)
>> +{
>> +	unsigned int report_len, manifest_len, certificates_len;
>> +	void *report_blob, *manifest_blob, *certificates_blob;
>> +	struct svsm_attestation_call attest_call = {};
>> +	struct tsm_desc *desc = &report->desc;
>> +	unsigned int size;
>> +	bool try_again;
>> +	void *buffer;
>> +	u64 call_id;
>> +	int ret;
>> +
>> +	/*
>> +	 * Allocate pages for the request:
>> +	 * - Report blob (4K)
>> +	 * - Manifest blob (4K)
>> +	 * - Certificate blob (16K)
>> +	 *
>> +	 * Above addresses must be 4K aligned
>> +	 */
>> +	report_len = SZ_4K;
>> +	manifest_len = SZ_4K;
>> +	certificates_len = SEV_FW_BLOB_MAX_SIZE;
>> +
>> +retry:
>> +	size = report_len + manifest_len + certificates_len;
>> +	buffer = alloc_pages_exact(size, __GFP_ZERO);
>> +	if (!buffer)
>> +		return -ENOMEM;
>> +
>> +	report_blob = buffer;
>> +	attest_call.report_buffer.pa = __pa(report_blob);
>> +	attest_call.report_buffer.len = report_len;
>> +
>> +	manifest_blob = report_blob + report_len;
>> +	attest_call.manifest_buffer.pa = __pa(manifest_blob);
>> +	attest_call.manifest_buffer.len = manifest_len;
>> +
>> +	certificates_blob = manifest_blob + manifest_len;
>> +	attest_call.certificates_buffer.pa = __pa(certificates_blob);
>> +	attest_call.certificates_buffer.len = certificates_len;
>> +
>> +	attest_call.nonce.pa = __pa(desc->inblob);
>> +	attest_call.nonce.len = desc->inblob_len;
>> +
>> +	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_version = desc->service_version;
>> +
>> +		call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SINGLE_SERVICE);
>> +	}
>> +
>> +	ret = snp_issue_svsm_attestation_request(call_id, &attest_call);
>> +	switch (ret) {
>> +	case SVSM_SUCCESS:
>> +		break;
>> +	case SVSM_ERR_INVALID_PARAMETER:
>> +		try_again = false;
>> +
>> +		if (attest_call.report_buffer.len > report_len) {
>> +			report_len = PAGE_ALIGN(attest_call.report_buffer.len);
>> +			try_again = true;
>> +		}
>> +
>> +		if (attest_call.manifest_buffer.len > manifest_len) {
>> +			manifest_len = PAGE_ALIGN(attest_call.manifest_buffer.len);
>> +			try_again = true;
>> +		}
>> +
>> +		if (attest_call.certificates_buffer.len > certificates_len) {
>> +			certificates_len = PAGE_ALIGN(attest_call.certificates_buffer.len);
>> +			try_again = true;
>> +		}
>> +
>> +		/* If one of the buffers wasn't large enough, retry the request */
>> +		if (try_again) {
>> +			free_pages_exact(buffer, size);
>> +			goto retry;
> Is this expected to ever go past 1 retry? Fail after that? It would seem
> suspicious if the untrusted host kept asking the guest to allocate more
> and more memory. Is there a reasonable max that can be applied to those
> buffers?
>
>> +		}
>> +
>> +		ret = -EINVAL;
>> +		goto error;
>> +	case SVSM_ERR_BUSY:
>> +		ret = -EAGAIN;
>> +		goto error;
>> +	default:
>> +		pr_err_ratelimited("SVSM attestation request failed (%#x)\n", ret);
>> +		ret = -EINVAL;
>> +		goto error;
>> +	}
>> +
>> +	ret = -ENOMEM;
>> +
>> +	report_len = attest_call.report_buffer.len;
>> +	void *rbuf __free(kvfree) = kvzalloc(report_len, GFP_KERNEL);
>> +	if (!rbuf)
>> +		goto error;
>> +
>> +	memcpy(rbuf, report_blob, report_len);
>> +	report->outblob = no_free_ptr(rbuf);
>> +	report->outblob_len = report_len;
>> +
>> +	manifest_len = attest_call.manifest_buffer.len;
>> +	void *mbuf __free(kvfree) = kvzalloc(manifest_len, GFP_KERNEL);
>> +	if (!mbuf)
>> +		goto error;
>> +
>> +	memcpy(mbuf, manifest_blob, manifest_len);
>> +	report->manifestblob = no_free_ptr(mbuf);
>> +	report->manifestblob_len = manifest_len;
>> +
>> +	certificates_len = attest_call.certificates_buffer.len;
>> +	if (!certificates_len)
>> +		goto success;
>> +
>> +	void *cbuf __free(kvfree) = kvzalloc(certificates_len, GFP_KERNEL);
>> +	if (!cbuf)
>> +		goto error;
>> +
>> +	memcpy(cbuf, certificates_blob, certificates_len);
>> +	report->auxblob = no_free_ptr(cbuf);
>> +	report->auxblob_len = certificates_len;
>> +
>> +success:
>> +	ret = 0;
>> +
>> +error:
>> +	free_pages_exact(buffer, size);
> I was going to comment that mixing goto and cleanup.h helpers can be a
> recipe for confusion, but this looks clean to me.
>
>> +
>> +	return ret;
>> +}
>> +
>>  static int sev_report_new(struct tsm_report *report, void *data)
>>  {
>>  	struct snp_msg_cert_entry *cert_table;
>> @@ -797,6 +931,9 @@ static int sev_report_new(struct tsm_report *report, void *data)
>>  	if (desc->inblob_len != SNP_REPORT_USER_DATA_SIZE)
>>  		return -EINVAL;
>>  
>> +	if (desc->svsm)
>> +		return sev_svsm_report_new(report, data);
>> +
>>  	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
>>  	if (!buf)
>>  		return -ENOMEM;
>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>> index d1c2db83a8ca..33fa26406bc6 100644
>> --- a/drivers/virt/coco/tsm.c
>> +++ b/drivers/virt/coco/tsm.c
>> @@ -35,7 +35,7 @@ static DECLARE_RWSEM(tsm_rwsem);
>>   * The attestation report format is TSM provider specific, when / if a standard
>>   * materializes that can be published instead of the vendor layout. Until then
>>   * the 'provider' attribute indicates the format of 'outblob', and optionally
>> - * 'auxblob'.
>> + * 'auxblob' and 'manifestblob'.
>>   */
>>  
>>  struct tsm_report_state {
>> @@ -48,6 +48,7 @@ struct tsm_report_state {
>>  enum tsm_data_select {
>>  	TSM_REPORT,
>>  	TSM_CERTS,
>> +	TSM_MANIFEST,
>>  };
>>  
>>  static struct tsm_report *to_tsm_report(struct config_item *cfg)
>> @@ -119,6 +120,77 @@ static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
>>  }
>>  CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
>>  
>> +static ssize_t tsm_report_svsm_store(struct config_item *cfg,
>> +				     const char *buf, size_t len)
>> +{
>> +	struct tsm_report *report = to_tsm_report(cfg);
>> +	unsigned int val;
>> +	int rc;
>> +
>> +	rc = kstrtouint(buf, 0, &val);
>> +	if (rc)
>> +		return rc;
>> +
>> +	guard(rwsem_write)(&tsm_rwsem);
>> +	rc = try_advance_write_generation(report);
>> +	if (rc)
>> +		return rc;
>> +	report->desc.svsm = !!val;
>> +
>> +	return len;
>> +}
>> +CONFIGFS_ATTR_WO(tsm_report_, svsm);
> Modulo whether this should become "service" per above the rest of the
> configfs interface changes look good to me.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Powered by blists - more mailing lists