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

From: Dan Williams
Date: Fri Feb 02 2024 - 02:11:10 EST

Next message: Yu Kuai: "Re: [PATCH v4] md/raid5: fix atomicity violation in raid5_cache_count"
Previous message: Arnd Bergmann: "Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt"
In reply to: Tom Lendacky: "Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support for an SVSM"
Next in thread: Kuppuswamy, Sathyanarayanan: "Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support for an SVSM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
> +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@xxxxxxxxxxxxxxx
> +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?

> +
> +What: /sys/kernel/config/tsm/report/$name/service_version
> +Date: January, 2024
> +KernelVersion: v6.9
> +Contact: linux-coco@xxxxxxxxxxxxxxx
> +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?

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

Next message: Yu Kuai: "Re: [PATCH v4] md/raid5: fix atomicity violation in raid5_cache_count"
Previous message: Arnd Bergmann: "Re: [PATCH 0/4] apply page shift to PFN instead of VA in pfn_to_virt"
In reply to: Tom Lendacky: "Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support for an SVSM"
Next in thread: Kuppuswamy, Sathyanarayanan: "Re: [PATCH 10/11] x86/sev: Extend the config-fs attestation support for an SVSM"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]