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

  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(-)
  

On Fri, Jan 26, 2024 at 2:19 PM Tom Lendacky <thomas.lendacky@amd.com> 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.

This is where I'm torn. If there's an SVSM, it's there to provide
paravirtualization for unenlightened guests /or/ it's there to protect
runtime measurement registers. I don't see there being any particular
value in bifurcating the attestation report space by adding this
option. If there's an SVSM present, the configfs-tsm report should
return the full SVSM attestation only.

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

I know that this is specified for the SVSM, but I still don't know
what the intended use case is such that we wouldn't simply always
return the full service manifest.
I can see an argument for an evidence requester not being ready for a
new manifest version and wanting the older version until they can
bridge the gap. I don't see that as needing configuration from the
user space. We can either require new service GUIDs for new versions,
require manifest ABIs to be internally versioned to be additive-only
to not break verifiers that understand up to manifest byte X, or we
allow breaking version changes through control plane configuration
that's passed directly to the SVSM.

New versions get new GUIDs allows for gradual deprecation at the
expense of size. I think that is a reasonable trade-off to keep from
making tsm/report vendor-specific.

>
>   - 'manifestblob' (output)
>     Used to return the service manifest associated with the attestation
>     report.

Given the above, I think we can just append the manifest to the report
since the report size is known a priori.

>
> 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
> +
>  What:          /sys/kernel/config/tsm/report/$name/provider
>  Date:          September, 2023
>  KernelVersion: v6.7
> @@ -80,3 +95,43 @@ 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/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
> +               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@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 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
> +
> +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
> 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));
> +
> +       /*
> +        * 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;
> +               }
> +
> +               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);
> +
> +       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);
> +
> +static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
> +                                            const char *buf, size_t len)
> +{
> +       struct tsm_report *report = to_tsm_report(cfg);
> +       size_t guid_len;
> +       int rc;
> +
> +       guard(rwsem_write)(&tsm_rwsem);
> +       rc = try_advance_write_generation(report);
> +       if (rc)
> +               return rc;
> +
> +       /* 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;
> +
> +       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;
> +       }
> +
> +       return len;
> +}
> +CONFIGFS_ATTR_WO(tsm_report_, service_guid);
> +
> +static ssize_t tsm_report_service_version_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.service_version = val;
> +
> +       return len;
> +}
> +CONFIGFS_ATTR_WO(tsm_report_, service_version);
> +
>  static ssize_t tsm_report_inblob_write(struct config_item *cfg,
>                                        const void *buf, size_t count)
>  {
> @@ -163,6 +235,9 @@ static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
>         if (select == TSM_REPORT) {
>                 out = report->outblob;
>                 len = report->outblob_len;
> +       } else if (select == TSM_MANIFEST) {
> +               out = report->manifestblob;
> +               len = report->manifestblob_len;
>         } else {
>                 out = report->auxblob;
>                 len = report->auxblob_len;
> @@ -188,7 +263,7 @@ static ssize_t read_cached_report(struct tsm_report *report, void *buf,
>
>         /*
>          * A given TSM backend always fills in ->outblob regardless of
> -        * whether the report includes an auxblob or not.
> +        * whether the report includes an auxblob/manifestblob or not.
>          */
>         if (!report->outblob ||
>             state->read_generation != state->write_generation)
> @@ -224,8 +299,10 @@ static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
>
>         kvfree(report->outblob);
>         kvfree(report->auxblob);
> +       kvfree(report->manifestblob);
>         report->outblob = NULL;
>         report->auxblob = NULL;
> +       report->manifestblob = NULL;
>         rc = ops->report_new(report, provider.data);
>         if (rc < 0)
>                 return rc;
> @@ -252,6 +329,15 @@ static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
>  }
>  CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
>
> +static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
> +                                           size_t count)
> +{
> +       struct tsm_report *report = to_tsm_report(cfg);
> +
> +       return tsm_report_read(report, buf, count, TSM_MANIFEST);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
> +
>  #define TSM_DEFAULT_ATTRS() \
>         &tsm_report_attr_generation, \
>         &tsm_report_attr_provider
> @@ -265,6 +351,9 @@ static struct configfs_attribute *tsm_report_extra_attrs[] = {
>         TSM_DEFAULT_ATTRS(),
>         &tsm_report_attr_privlevel,
>         &tsm_report_attr_privlevel_floor,
> +       &tsm_report_attr_svsm,
> +       &tsm_report_attr_service_guid,
> +       &tsm_report_attr_service_version,
>         NULL,
>  };
>
> @@ -280,6 +369,7 @@ static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
>  static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
>         TSM_DEFAULT_BIN_ATTRS(),
>         &tsm_report_attr_auxblob,
> +       &tsm_report_attr_manifestblob,
>         NULL,
>  };
>
> @@ -288,6 +378,7 @@ static void tsm_report_item_release(struct config_item *cfg)
>         struct tsm_report *report = to_tsm_report(cfg);
>         struct tsm_report_state *state = to_state(report);
>
> +       kvfree(report->manifestblob);
>         kvfree(report->auxblob);
>         kvfree(report->outblob);
>         kfree(state);
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> index de8324a2223c..7c36b8448b4f 100644
> --- a/include/linux/tsm.h
> +++ b/include/linux/tsm.h
> @@ -4,6 +4,7 @@
>
>  #include <linux/sizes.h>
>  #include <linux/types.h>
> +#include <linux/uuid.h>
>
>  #define TSM_INBLOB_MAX 64
>  #define TSM_OUTBLOB_MAX SZ_32K
> @@ -19,11 +20,17 @@
>   * @privlevel: optional privilege level to associate with @outblob
>   * @inblob_len: sizeof @inblob
>   * @inblob: arbitrary input data
> + * @svsm: optional indicator of where to obtain the tsm report blob
> + * @service_guid: optional SVSM service guid to attest
> + * @service_version: optional SVSM service manifest version requested
>   */
>  struct tsm_desc {
>         unsigned int privlevel;
>         size_t inblob_len;
>         u8 inblob[TSM_INBLOB_MAX];
> +       bool svsm;
> +       guid_t service_guid;
> +       unsigned int service_version;
>  };
>
>  /**
> @@ -33,6 +40,8 @@ struct tsm_desc {
>   * @outblob: generated evidence to provider to the attestation agent
>   * @auxblob_len: sizeof(@auxblob)
>   * @auxblob: (optional) auxiliary data to the report (e.g. certificate data)
> + * @manifestblob_len: sizeof(@manifestblob)
> + * @manifestblob: (optional) manifest data associated with the report
>   */
>  struct tsm_report {
>         struct tsm_desc desc;
> @@ -40,6 +49,8 @@ struct tsm_report {
>         u8 *outblob;
>         size_t auxblob_len;
>         u8 *auxblob;
> +       size_t manifestblob_len;
> +       u8 *manifestblob;
>  };
>
>  /**
> --
> 2.42.0
>
>
  

On 1/26/24 19:27, Dionna Amalie Glaze wrote:
> On Fri, Jan 26, 2024 at 2:19 PM Tom Lendacky <thomas.lendacky@amd.com> 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.
> 
> This is where I'm torn. If there's an SVSM, it's there to provide
> paravirtualization for unenlightened guests /or/ it's there to protect

An SVSM is for enlightened guests. A para-visor would be for unenlightened
guests.

> runtime measurement registers. I don't see there being any particular
> value in bifurcating the attestation report space by adding this
> option. If there's an SVSM present, the configfs-tsm report should
> return the full SVSM attestation only.

I don't necessarily agree with that. The guest should still be able to
request a traditional attestation report.

Maybe I can remove the SVSM attribute and direct the call based on
requested VMPL level. If VMPL0 is requested, it goes through the SVSM.
If VMPL1+ is requested, it goes to the ASP.

That would mean that the privlevel_floor would need to stay at zero.

> 
>>
>>    - '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.
> 
> I know that this is specified for the SVSM, but I still don't know
> what the intended use case is such that we wouldn't simply always
> return the full service manifest.
> I can see an argument for an evidence requester not being ready for a
> new manifest version and wanting the older version until they can
> bridge the gap. I don't see that as needing configuration from the
> user space. We can either require new service GUIDs for new versions,
> require manifest ABIs to be internally versioned to be additive-only
> to not break verifiers that understand up to manifest byte X, or we
> allow breaking version changes through control plane configuration
> that's passed directly to the SVSM.
> 
> New versions get new GUIDs allows for gradual deprecation at the
> expense of size. I think that is a reasonable trade-off to keep from
> making tsm/report vendor-specific.

This was requested and discussed during the SVSM spec review and there
were no objections raised. See the this thread where this was discussed:

https://lore.kernel.org/linux-coco/09819cb3-1938-fe86-b948-28aaffbe584e@amd.com/

The changes you're requesting would require a new version of the spec
and updates to the protocol.

> 
>>
>>    - 'manifestblob' (output)
>>      Used to return the service manifest associated with the attestation
>>      report.
> 
> Given the above, I think we can just append the manifest to the report
> since the report size is known a priori.

We could have theoretically done the same thing with the auxblob (certs
data), but that is separate. I prefer the idea of having an individual
entry per piece of data being returned.

Thanks,
Tom

> 
>>
>> 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
>> +
>>   What:          /sys/kernel/config/tsm/report/$name/provider
>>   Date:          September, 2023
>>   KernelVersion: v6.7
>> @@ -80,3 +95,43 @@ 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/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
>> +               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@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 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
>> +
>> +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
>> 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));
>> +
>> +       /*
>> +        * 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;
>> +               }
>> +
>> +               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);
>> +
>> +       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);
>> +
>> +static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
>> +                                            const char *buf, size_t len)
>> +{
>> +       struct tsm_report *report = to_tsm_report(cfg);
>> +       size_t guid_len;
>> +       int rc;
>> +
>> +       guard(rwsem_write)(&tsm_rwsem);
>> +       rc = try_advance_write_generation(report);
>> +       if (rc)
>> +               return rc;
>> +
>> +       /* 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;
>> +
>> +       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;
>> +       }
>> +
>> +       return len;
>> +}
>> +CONFIGFS_ATTR_WO(tsm_report_, service_guid);
>> +
>> +static ssize_t tsm_report_service_version_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.service_version = val;
>> +
>> +       return len;
>> +}
>> +CONFIGFS_ATTR_WO(tsm_report_, service_version);
>> +
>>   static ssize_t tsm_report_inblob_write(struct config_item *cfg,
>>                                         const void *buf, size_t count)
>>   {
>> @@ -163,6 +235,9 @@ static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
>>          if (select == TSM_REPORT) {
>>                  out = report->outblob;
>>                  len = report->outblob_len;
>> +       } else if (select == TSM_MANIFEST) {
>> +               out = report->manifestblob;
>> +               len = report->manifestblob_len;
>>          } else {
>>                  out = report->auxblob;
>>                  len = report->auxblob_len;
>> @@ -188,7 +263,7 @@ static ssize_t read_cached_report(struct tsm_report *report, void *buf,
>>
>>          /*
>>           * A given TSM backend always fills in ->outblob regardless of
>> -        * whether the report includes an auxblob or not.
>> +        * whether the report includes an auxblob/manifestblob or not.
>>           */
>>          if (!report->outblob ||
>>              state->read_generation != state->write_generation)
>> @@ -224,8 +299,10 @@ static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
>>
>>          kvfree(report->outblob);
>>          kvfree(report->auxblob);
>> +       kvfree(report->manifestblob);
>>          report->outblob = NULL;
>>          report->auxblob = NULL;
>> +       report->manifestblob = NULL;
>>          rc = ops->report_new(report, provider.data);
>>          if (rc < 0)
>>                  return rc;
>> @@ -252,6 +329,15 @@ static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
>>   }
>>   CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
>>
>> +static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
>> +                                           size_t count)
>> +{
>> +       struct tsm_report *report = to_tsm_report(cfg);
>> +
>> +       return tsm_report_read(report, buf, count, TSM_MANIFEST);
>> +}
>> +CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
>> +
>>   #define TSM_DEFAULT_ATTRS() \
>>          &tsm_report_attr_generation, \
>>          &tsm_report_attr_provider
>> @@ -265,6 +351,9 @@ static struct configfs_attribute *tsm_report_extra_attrs[] = {
>>          TSM_DEFAULT_ATTRS(),
>>          &tsm_report_attr_privlevel,
>>          &tsm_report_attr_privlevel_floor,
>> +       &tsm_report_attr_svsm,
>> +       &tsm_report_attr_service_guid,
>> +       &tsm_report_attr_service_version,
>>          NULL,
>>   };
>>
>> @@ -280,6 +369,7 @@ static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
>>   static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
>>          TSM_DEFAULT_BIN_ATTRS(),
>>          &tsm_report_attr_auxblob,
>> +       &tsm_report_attr_manifestblob,
>>          NULL,
>>   };
>>
>> @@ -288,6 +378,7 @@ static void tsm_report_item_release(struct config_item *cfg)
>>          struct tsm_report *report = to_tsm_report(cfg);
>>          struct tsm_report_state *state = to_state(report);
>>
>> +       kvfree(report->manifestblob);
>>          kvfree(report->auxblob);
>>          kvfree(report->outblob);
>>          kfree(state);
>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>> index de8324a2223c..7c36b8448b4f 100644
>> --- a/include/linux/tsm.h
>> +++ b/include/linux/tsm.h
>> @@ -4,6 +4,7 @@
>>
>>   #include <linux/sizes.h>
>>   #include <linux/types.h>
>> +#include <linux/uuid.h>
>>
>>   #define TSM_INBLOB_MAX 64
>>   #define TSM_OUTBLOB_MAX SZ_32K
>> @@ -19,11 +20,17 @@
>>    * @privlevel: optional privilege level to associate with @outblob
>>    * @inblob_len: sizeof @inblob
>>    * @inblob: arbitrary input data
>> + * @svsm: optional indicator of where to obtain the tsm report blob
>> + * @service_guid: optional SVSM service guid to attest
>> + * @service_version: optional SVSM service manifest version requested
>>    */
>>   struct tsm_desc {
>>          unsigned int privlevel;
>>          size_t inblob_len;
>>          u8 inblob[TSM_INBLOB_MAX];
>> +       bool svsm;
>> +       guid_t service_guid;
>> +       unsigned int service_version;
>>   };
>>
>>   /**
>> @@ -33,6 +40,8 @@ struct tsm_desc {
>>    * @outblob: generated evidence to provider to the attestation agent
>>    * @auxblob_len: sizeof(@auxblob)
>>    * @auxblob: (optional) auxiliary data to the report (e.g. certificate data)
>> + * @manifestblob_len: sizeof(@manifestblob)
>> + * @manifestblob: (optional) manifest data associated with the report
>>    */
>>   struct tsm_report {
>>          struct tsm_desc desc;
>> @@ -40,6 +49,8 @@ struct tsm_report {
>>          u8 *outblob;
>>          size_t auxblob_len;
>>          u8 *auxblob;
>> +       size_t manifestblob_len;
>> +       u8 *manifestblob;
>>   };
>>
>>   /**
>> --
>> 2.42.0
>>
>>
> 
>
  

On Mon, Jan 29, 2024 at 7:02 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 1/26/24 19:27, Dionna Amalie Glaze wrote:
> > On Fri, Jan 26, 2024 at 2:19 PM Tom Lendacky <thomas.lendacky@amd.com> 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.
> >
> > This is where I'm torn. If there's an SVSM, it's there to provide
> > paravirtualization for unenlightened guests /or/ it's there to protect
>
> An SVSM is for enlightened guests. A para-visor would be for unenlightened
> guests.
>
> > runtime measurement registers. I don't see there being any particular
> > value in bifurcating the attestation report space by adding this
> > option. If there's an SVSM present, the configfs-tsm report should
> > return the full SVSM attestation only.
>
> I don't necessarily agree with that. The guest should still be able to
> request a traditional attestation report.
>
> Maybe I can remove the SVSM attribute and direct the call based on
> requested VMPL level. If VMPL0 is requested, it goes through the SVSM.
> If VMPL1+ is requested, it goes to the ASP.
>
> That would mean that the privlevel_floor would need to stay at zero.
>
> >
> >>
> >>    - '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.
> >
> > I know that this is specified for the SVSM, but I still don't know
> > what the intended use case is such that we wouldn't simply always
> > return the full service manifest.
> > I can see an argument for an evidence requester not being ready for a
> > new manifest version and wanting the older version until they can
> > bridge the gap. I don't see that as needing configuration from the
> > user space. We can either require new service GUIDs for new versions,
> > require manifest ABIs to be internally versioned to be additive-only
> > to not break verifiers that understand up to manifest byte X, or we
> > allow breaking version changes through control plane configuration
> > that's passed directly to the SVSM.
> >
> > New versions get new GUIDs allows for gradual deprecation at the
> > expense of size. I think that is a reasonable trade-off to keep from
> > making tsm/report vendor-specific.
>
> This was requested and discussed during the SVSM spec review and there
> were no objections raised. See the this thread where this was discussed:
>
> https://lore.kernel.org/linux-coco/09819cb3-1938-fe86-b948-28aaffbe584e@amd.com/
>

We also hadn't had the configfs-tsm unification point, so I think it's
worth folding in that discussion.
In terms of querying specific services, would you help me with a
concrete example of where the evidence collector ought to query a
specific version instead of the service enumeration?

> The changes you're requesting would require a new version of the spec
> and updates to the protocol.
>

I think the changes I'm requesting are to just limit the exposure of
the protocol to linux. What specifically about what I wrote requires a
change to the spec? Is it the lack of plural handling of 'its GUID
value' in "Each service will document its GUID value and the format of
its manifest content."?

> >
> >>
> >>    - 'manifestblob' (output)
> >>      Used to return the service manifest associated with the attestation
> >>      report.
> >
> > Given the above, I think we can just append the manifest to the report
> > since the report size is known a priori.
>
> We could have theoretically done the same thing with the auxblob (certs
> data), but that is separate. I prefer the idea of having an individual
> entry per piece of data being returned.

Fair enough, another RO blob seems okay enough.

>
> Thanks,
> Tom
>
> >
> >>
> >> 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
> >> +
> >>   What:          /sys/kernel/config/tsm/report/$name/provider
> >>   Date:          September, 2023
> >>   KernelVersion: v6.7
> >> @@ -80,3 +95,43 @@ 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/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
> >> +               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@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 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
> >> +
> >> +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
> >> 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));
> >> +
> >> +       /*
> >> +        * 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;
> >> +               }
> >> +
> >> +               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);
> >> +
> >> +       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);
> >> +
> >> +static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
> >> +                                            const char *buf, size_t len)
> >> +{
> >> +       struct tsm_report *report = to_tsm_report(cfg);
> >> +       size_t guid_len;
> >> +       int rc;
> >> +
> >> +       guard(rwsem_write)(&tsm_rwsem);
> >> +       rc = try_advance_write_generation(report);
> >> +       if (rc)
> >> +               return rc;
> >> +
> >> +       /* 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;
> >> +
> >> +       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;
> >> +       }
> >> +
> >> +       return len;
> >> +}
> >> +CONFIGFS_ATTR_WO(tsm_report_, service_guid);
> >> +
> >> +static ssize_t tsm_report_service_version_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.service_version = val;
> >> +
> >> +       return len;
> >> +}
> >> +CONFIGFS_ATTR_WO(tsm_report_, service_version);
> >> +
> >>   static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> >>                                         const void *buf, size_t count)
> >>   {
> >> @@ -163,6 +235,9 @@ static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> >>          if (select == TSM_REPORT) {
> >>                  out = report->outblob;
> >>                  len = report->outblob_len;
> >> +       } else if (select == TSM_MANIFEST) {
> >> +               out = report->manifestblob;
> >> +               len = report->manifestblob_len;
> >>          } else {
> >>                  out = report->auxblob;
> >>                  len = report->auxblob_len;
> >> @@ -188,7 +263,7 @@ static ssize_t read_cached_report(struct tsm_report *report, void *buf,
> >>
> >>          /*
> >>           * A given TSM backend always fills in ->outblob regardless of
> >> -        * whether the report includes an auxblob or not.
> >> +        * whether the report includes an auxblob/manifestblob or not.
> >>           */
> >>          if (!report->outblob ||
> >>              state->read_generation != state->write_generation)
> >> @@ -224,8 +299,10 @@ static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> >>
> >>          kvfree(report->outblob);
> >>          kvfree(report->auxblob);
> >> +       kvfree(report->manifestblob);
> >>          report->outblob = NULL;
> >>          report->auxblob = NULL;
> >> +       report->manifestblob = NULL;
> >>          rc = ops->report_new(report, provider.data);
> >>          if (rc < 0)
> >>                  return rc;
> >> @@ -252,6 +329,15 @@ static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
> >>   }
> >>   CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
> >>
> >> +static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
> >> +                                           size_t count)
> >> +{
> >> +       struct tsm_report *report = to_tsm_report(cfg);
> >> +
> >> +       return tsm_report_read(report, buf, count, TSM_MANIFEST);
> >> +}
> >> +CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
> >> +
> >>   #define TSM_DEFAULT_ATTRS() \
> >>          &tsm_report_attr_generation, \
> >>          &tsm_report_attr_provider
> >> @@ -265,6 +351,9 @@ static struct configfs_attribute *tsm_report_extra_attrs[] = {
> >>          TSM_DEFAULT_ATTRS(),
> >>          &tsm_report_attr_privlevel,
> >>          &tsm_report_attr_privlevel_floor,
> >> +       &tsm_report_attr_svsm,
> >> +       &tsm_report_attr_service_guid,
> >> +       &tsm_report_attr_service_version,
> >>          NULL,
> >>   };
> >>
> >> @@ -280,6 +369,7 @@ static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
> >>   static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
> >>          TSM_DEFAULT_BIN_ATTRS(),
> >>          &tsm_report_attr_auxblob,
> >> +       &tsm_report_attr_manifestblob,
> >>          NULL,
> >>   };
> >>
> >> @@ -288,6 +378,7 @@ static void tsm_report_item_release(struct config_item *cfg)
> >>          struct tsm_report *report = to_tsm_report(cfg);
> >>          struct tsm_report_state *state = to_state(report);
> >>
> >> +       kvfree(report->manifestblob);
> >>          kvfree(report->auxblob);
> >>          kvfree(report->outblob);
> >>          kfree(state);
> >> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> >> index de8324a2223c..7c36b8448b4f 100644
> >> --- a/include/linux/tsm.h
> >> +++ b/include/linux/tsm.h
> >> @@ -4,6 +4,7 @@
> >>
> >>   #include <linux/sizes.h>
> >>   #include <linux/types.h>
> >> +#include <linux/uuid.h>
> >>
> >>   #define TSM_INBLOB_MAX 64
> >>   #define TSM_OUTBLOB_MAX SZ_32K
> >> @@ -19,11 +20,17 @@
> >>    * @privlevel: optional privilege level to associate with @outblob
> >>    * @inblob_len: sizeof @inblob
> >>    * @inblob: arbitrary input data
> >> + * @svsm: optional indicator of where to obtain the tsm report blob
> >> + * @service_guid: optional SVSM service guid to attest
> >> + * @service_version: optional SVSM service manifest version requested
> >>    */
> >>   struct tsm_desc {
> >>          unsigned int privlevel;
> >>          size_t inblob_len;
> >>          u8 inblob[TSM_INBLOB_MAX];
> >> +       bool svsm;
> >> +       guid_t service_guid;
> >> +       unsigned int service_version;
> >>   };
> >>
> >>   /**
> >> @@ -33,6 +40,8 @@ struct tsm_desc {
> >>    * @outblob: generated evidence to provider to the attestation agent
> >>    * @auxblob_len: sizeof(@auxblob)
> >>    * @auxblob: (optional) auxiliary data to the report (e.g. certificate data)
> >> + * @manifestblob_len: sizeof(@manifestblob)
> >> + * @manifestblob: (optional) manifest data associated with the report
> >>    */
> >>   struct tsm_report {
> >>          struct tsm_desc desc;
> >> @@ -40,6 +49,8 @@ struct tsm_report {
> >>          u8 *outblob;
> >>          size_t auxblob_len;
> >>          u8 *auxblob;
> >> +       size_t manifestblob_len;
> >> +       u8 *manifestblob;
> >>   };
> >>
> >>   /**
> >> --
> >> 2.42.0
> >>
> >>
> >
> >
  

On 1/29/24 14:04, Dionna Amalie Glaze wrote:
> On Mon, Jan 29, 2024 at 7:02 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> On 1/26/24 19:27, Dionna Amalie Glaze wrote:
>>> On Fri, Jan 26, 2024 at 2:19 PM Tom Lendacky <thomas.lendacky@amd.com> 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.
>>>
>>> This is where I'm torn. If there's an SVSM, it's there to provide
>>> paravirtualization for unenlightened guests /or/ it's there to protect
>>
>> An SVSM is for enlightened guests. A para-visor would be for unenlightened
>> guests.
>>
>>> runtime measurement registers. I don't see there being any particular
>>> value in bifurcating the attestation report space by adding this
>>> option. If there's an SVSM present, the configfs-tsm report should
>>> return the full SVSM attestation only.
>>
>> I don't necessarily agree with that. The guest should still be able to
>> request a traditional attestation report.
>>
>> Maybe I can remove the SVSM attribute and direct the call based on
>> requested VMPL level. If VMPL0 is requested, it goes through the SVSM.
>> If VMPL1+ is requested, it goes to the ASP.
>>
>> That would mean that the privlevel_floor would need to stay at zero.
>>
>>>
>>>>
>>>>     - '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.
>>>
>>> I know that this is specified for the SVSM, but I still don't know
>>> what the intended use case is such that we wouldn't simply always
>>> return the full service manifest.
>>> I can see an argument for an evidence requester not being ready for a
>>> new manifest version and wanting the older version until they can
>>> bridge the gap. I don't see that as needing configuration from the
>>> user space. We can either require new service GUIDs for new versions,
>>> require manifest ABIs to be internally versioned to be additive-only
>>> to not break verifiers that understand up to manifest byte X, or we
>>> allow breaking version changes through control plane configuration
>>> that's passed directly to the SVSM.
>>>
>>> New versions get new GUIDs allows for gradual deprecation at the
>>> expense of size. I think that is a reasonable trade-off to keep from
>>> making tsm/report vendor-specific.
>>
>> This was requested and discussed during the SVSM spec review and there
>> were no objections raised. See the this thread where this was discussed:
>>
>> https://lore.kernel.org/linux-coco/09819cb3-1938-fe86-b948-28aaffbe584e@amd.com/
>>
> 
> We also hadn't had the configfs-tsm unification point, so I think it's
> worth folding in that discussion.
> In terms of querying specific services, would you help me with a
> concrete example of where the evidence collector ought to query a
> specific version instead of the service enumeration?

Here is where the request was initially brought up:

https://lore.kernel.org/linux-coco/fbc84da05c5343c5228c5adb697d4b66f1ea6308.camel@HansenPartnership.com/

> 
>> The changes you're requesting would require a new version of the spec
>> and updates to the protocol.
>>
> 
> I think the changes I'm requesting are to just limit the exposure of
> the protocol to linux. What specifically about what I wrote requires a
> change to the spec? Is it the lack of plural handling of 'its GUID
> value' in "Each service will document its GUID value and the format of
> its manifest content."?

The spec is currently written so that a service has a single GUID. If I
understand correctly, you are asking that each version of the manifest
for a service gets a unique GUID. That would require a change to the
specification to document such a behavior and possibly a protocol
modification to somehow indicate to ignore the version field when
requesting a single service attestation or a new protocol that does not
take/use a version.

Thanks,
Tom

> 
>>>
>>>>
>>>>     - 'manifestblob' (output)
>>>>       Used to return the service manifest associated with the attestation
>>>>       report.
>>>
>>> Given the above, I think we can just append the manifest to the report
>>> since the report size is known a priori.
>>
>> We could have theoretically done the same thing with the auxblob (certs
>> data), but that is separate. I prefer the idea of having an individual
>> entry per piece of data being returned.
> 
> Fair enough, another RO blob seems okay enough.
> 
>>
>> Thanks,
>> Tom
>>
>>>
  

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.

> +
>  What:		/sys/kernel/config/tsm/report/$name/provider
>  Date:		September, 2023
>  KernelVersion:	v6.7
> @@ -80,3 +95,43 @@ 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/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.

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@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 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@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?

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.
  

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@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.
>
>> +
>>  What:		/sys/kernel/config/tsm/report/$name/provider
>>  Date:		September, 2023
>>  KernelVersion:	v6.7
>> @@ -80,3 +95,43 @@ 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/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.
>
> 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@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 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@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?
>
> 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.
  

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?

> 
>> +
>>   What:		/sys/kernel/config/tsm/report/$name/provider
>>   Date:		September, 2023
>>   KernelVersion:	v6.7
>> @@ -80,3 +95,43 @@ 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/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.

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

I can work something up along that line and see what it looks like.

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

I can do that. I'll put the currently defined one(s) along with the link.

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

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

Ok.

> 
>> +
>> +	/*
>> +	 * 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?

The report buffer and manifest buffer should be fixed after boot, but the 
certs buffer could be dynamic. But I wouldn't expect that the provider is 
updating the certs that often.

I can put a maximum retry counter in place here, with 3 attempts seeming 
reasonable.

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

Let me re-work it for the next version and see how it all looks.

Thanks,
Tom
  

On 2/5/24 17:29, Kuppuswamy, Sathyanarayanan wrote:
> 
> 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@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.
>>
>>> +
>>>   What:		/sys/kernel/config/tsm/report/$name/provider
>>>   Date:		September, 2023
>>>   KernelVersion:	v6.7
>>> @@ -80,3 +95,43 @@ 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/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.
>>
>> 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@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 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?

In a sense, yes. You can request an all services attestation which will 
return a manifest containing all the active services GUIDs.

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

Will do.

Thanks,
Tom

> 
>>
  

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.

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

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.

Let me look into that.

> 
> [..]
>>>> +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?

The new ABI selects whether to go through the SVSM to get the attestation
report, which will additionally return a manifest that, along with the
nonce, has become part of the report through hashing.

But, yes, I mentioned in a previous reply [1] that we could use privlevel
to determine whether to invoke attestation through an SVSM request or
through the standard method of issuing an extended guest request.

[1] https://lore.kernel.org/lkml/3fca61f2-6fe0-4431-818e-9c7b96c6a391@amd.com/

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

Software can always use v1. The idea is that if a service wanted to
provide additional information or change the information provided in the
service manifest, then it would have to do that via a new version of its
manifest so as not to break existing users. By default, zero would be used
for the service manifest version and have to be updated by the user if
they wanted a different one.

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

Correct, it is for the manifest version. I can rename it to
service_manifest or service_manifest_version. I'd rather not rename it to
manifest_version since it is specific to an individual service.

Thanks,
Tom
  

Tom Lendacky wrote:
[..]
> > 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?
> 
> The new ABI selects whether to go through the SVSM to get the attestation
> report, which will additionally return a manifest that, along with the
> nonce, has become part of the report through hashing.

Ah yeah, that's a lot to overlead to the meaning of privlevel.

> 
> But, yes, I mentioned in a previous reply [1] that we could use privlevel
> to determine whether to invoke attestation through an SVSM request or
> through the standard method of issuing an extended guest request.
> 
> [1] https://lore.kernel.org/lkml/3fca61f2-6fe0-4431-818e-9c7b96c6a391@amd.com/

Missed that, thanks. Lets keep explicit as you have it and not overload
privlevel.

[..]
> > 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.
> 
> Software can always use v1. The idea is that if a service wanted to
> provide additional information or change the information provided in the
> service manifest, then it would have to do that via a new version of its
> manifest so as not to break existing users. By default, zero would be used
> for the service manifest version and have to be updated by the user if
> they wanted a different one.

Can it just be the case the manifest can only grow but old fields never
change? Then software can determine the "version" based on manifest size
and no software gets built with an explicit version check, and is
instead built to understand a certain point in the evolution of the
manifest.

To be clear this is my standard response to any specification that
transmits a payload that "may change in the future", if it is an
awkward fit in this case it would at least be good to clarify why.

> > 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.
> 
> Correct, it is for the manifest version. I can rename it to
> service_manifest or service_manifest_version. I'd rather not rename it to
> manifest_version since it is specific to an individual service.

FWIW, service_manifest_version makes it crystal clear to me, but maybe
even better would be that the output size already conveys that, this
attribute is not needed, and userspace reads as much of the manifest as
it knows about.
  

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.
  

Tom Lendacky wrote:
> 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?

Sounds like a patch I want to see, yes. So the idea is the low-level
driver registers the is_visible() callback to the core and that gets to
filter attributes?

Hmm, as long as it ends up looking similar to sysfs is_visible()
prototype.

It could even just be a bitmask of attributes that gets passed in by the
provider, something like:

static struct configfs_attribute *tsm_report_attrs[] = {
        [TSM_REPORT_ATTR_GENERATION] = &tsm_report_attr_generation,
        [TSM_REPORT_ATTR_PROVIDER] = &tsm_report_attr_provider
        [TSM_REPORT_ATTR_PRIVLEVEL] = &tsm_report_attr_privlevel,
        [TSM_REPORT_ATTR_FLOOR] = &tsm_report_attr_privlevel_floor,
        NULL,
};

bool tsm_report_is_visible(struct config_item *cfg, struct configfs_attribute *attr, int n)
{
	return test_bit(n, &provider.attr_mask);
}

..and in is_bin_visible() for the binary attributes?
  

On 2/23/24 18:02, Dan Williams wrote:
> Tom Lendacky wrote:
>> 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?
> 
> Sounds like a patch I want to see, yes. So the idea is the low-level
> driver registers the is_visible() callback to the core and that gets to
> filter attributes?
> 
> Hmm, as long as it ends up looking similar to sysfs is_visible()
> prototype.
> 
> It could even just be a bitmask of attributes that gets passed in by the
> provider, something like:
> 
> static struct configfs_attribute *tsm_report_attrs[] = {
>          [TSM_REPORT_ATTR_GENERATION] = &tsm_report_attr_generation,
>          [TSM_REPORT_ATTR_PROVIDER] = &tsm_report_attr_provider
>          [TSM_REPORT_ATTR_PRIVLEVEL] = &tsm_report_attr_privlevel,
>          [TSM_REPORT_ATTR_FLOOR] = &tsm_report_attr_privlevel_floor,
>          NULL,
> };
> 
> bool tsm_report_is_visible(struct config_item *cfg, struct configfs_attribute *attr, int n)
> {
> 	return test_bit(n, &provider.attr_mask);
> }
> 
> ..and in is_bin_visible() for the binary attributes?

I was thinking something along the lines of the following for the configfs
support:

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 18677cd4e62f..c224060c1b6b 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -589,12 +589,20 @@ static int populate_attrs(struct config_item *item)
  		return -EINVAL;
  	if (t->ct_attrs) {
  		for (i = 0; (attr = t->ct_attrs[i]) != NULL; i++) {
+			if (attr->ca_is_visible && !attr->ca_is_visible(attr))
+				continue;
+
  			if ((error = configfs_create_file(item, attr)))
  				break;
  		}
  	}
-	if (t->ct_bin_attrs) {
+	if (t->ct_bin_attrs && !error) {
  		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
+			attr = &bin_attr->cb_attr;
+
+			if (attr->ca_is_visible && !attr->ca_is_visible(attr))
+				continue;
+
  			error = configfs_create_bin_file(item, bin_attr);
  			if (error)
  				break;
diff --git a/include/linux/configfs.h b/include/linux/configfs.h
index 2606711adb18..93d346e2afc1 100644
--- a/include/linux/configfs.h
+++ b/include/linux/configfs.h
@@ -112,39 +112,63 @@ static inline void configfs_add_default_group(struct config_group *new_group,
  	list_add_tail(&new_group->group_entry, &group->default_groups);
  }
  
+typedef bool (*configfs_is_visible_t)(const struct configfs_attribute *attr);
+
  struct configfs_attribute {
  	const char		*ca_name;
  	struct module 		*ca_owner;
  	umode_t			ca_mode;
+	configfs_is_visible_t	ca_is_visible;
  	ssize_t (*show)(struct config_item *, char *);
  	ssize_t (*store)(struct config_item *, const char *, size_t);
  };
  
-#define CONFIGFS_ATTR(_pfx, _name)			\
+#define __CONFIGFS_ATTR(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IRUGO | S_IWUSR,		\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.show		= _pfx##_name##_show,		\
  	.store		= _pfx##_name##_store,		\
  }
  
-#define CONFIGFS_ATTR_RO(_pfx, _name)			\
+#define __CONFIGFS_ATTR_RO(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IRUGO,			\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.show		= _pfx##_name##_show,		\
  }
  
-#define CONFIGFS_ATTR_WO(_pfx, _name)			\
+#define __CONFIGFS_ATTR_WO(_pfx, _name, _vis)		\
  static struct configfs_attribute _pfx##attr_##_name = {	\
  	.ca_name	= __stringify(_name),		\
  	.ca_mode	= S_IWUSR,			\
  	.ca_owner	= THIS_MODULE,			\
+	.ca_is_visible	= _vis,				\
  	.store		= _pfx##_name##_store,		\
  }
  
+#define CONFIGFS_ATTR(_pfx, _name)			\
+	__CONFIGFS_ATTR(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_RO(_pfx, _name)			\
+	__CONFIGFS_ATTR_RO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_WO(_pfx, _name)			\
+	__CONFIGFS_ATTR_WO(_pfx, _name, NULL)
+
+#define CONFIGFS_ATTR_VISIBLE(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_RO(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR_RO(_pfx, _name, _vis)
+
+#define CONFIGFS_ATTR_VISIBLE_WO(_pfx, _name, _vis)	\
+	__CONFIGFS_ATTR_WO(_pfx, _name, _vis)
+
  struct file;
  struct vm_area_struct;
  
@@ -156,43 +180,64 @@ struct configfs_bin_attribute {
  	ssize_t (*write)(struct config_item *, const void *, size_t);
  };
  
-#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz)		\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IRUGO | S_IWUSR,		\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.read		= _pfx##_name##_read,			\
-	.write		= _pfx##_name##_write,			\
+#define __CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)		\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IRUGO | S_IWUSR,			\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.read		= _pfx##_name##_read,				\
+	.write		= _pfx##_name##_write,				\
  }
  
-#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz)	\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IRUGO,			\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.read		= _pfx##_name##_read,			\
+#define __CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)	\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IRUGO,				\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.read		= _pfx##_name##_read,				\
  }
  
-#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz)	\
-static struct configfs_bin_attribute _pfx##attr_##_name = {	\
-	.cb_attr = {						\
-		.ca_name	= __stringify(_name),		\
-		.ca_mode	= S_IWUSR,			\
-		.ca_owner	= THIS_MODULE,			\
-	},							\
-	.cb_private	= _priv,				\
-	.cb_max_size	= _maxsz,				\
-	.write		= _pfx##_name##_write,			\
+#define __CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)	\
+static struct configfs_bin_attribute _pfx##attr_##_name = {		\
+	.cb_attr = {							\
+		.ca_name	= __stringify(_name),			\
+		.ca_mode	= S_IWUSR,				\
+		.ca_owner	= THIS_MODULE,				\
+		.ca_is_visible	= _vis,					\
+	},								\
+	.cb_private	= _priv,					\
+	.cb_max_size	= _maxsz,					\
+	.write		= _pfx##_name##_write,				\
  }
  
+#define CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz)			\
+	__CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz)		\
+	__CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz)		\
+	__CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, NULL)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE(_pfx, _name, _priv, _maxs, _vis)	\
+	__CONFIGFS_BIN_ATTR(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_RO(_pfx, _name, _priv, _maxsz, _vis)	\
+	__CONFIGFS_BIN_ATTR_RO(_pfx, _name, _priv, _maxsz, _vis)
+
+#define CONFIGFS_BIN_ATTR_VISIBLE_WO(_pfx, _name, _priv, _maxsz, _vis)	\
+	__CONFIGFS_BIN_ATTR_WO(_pfx, _name, _priv, _maxsz, _vis)
+
  /*
   * If allow_link() exists, the item can symlink(2) out to other
   * items.  If the item is a group, it may support mkdir(2).


And then the following for implementing it for tsm, which allows for as
simple or complicated an is_visible() function as required:

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 4ac62c896670..f29310a4a357 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -1022,6 +1022,11 @@ static int sev_report_new(struct tsm_report *report, void *data)
  	return 0;
  }
  
+static bool svsm_make_visible(const struct configfs_attribute *attr)
+{
+	return true;
+}
+
  static struct tsm_ops sev_tsm_ops = {
  	.name = KBUILD_MODNAME,
  	.report_new = sev_report_new,
@@ -1116,6 +1121,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
  	if (ret)
  		goto e_free_cert_data;
  
+	if (snp_get_vmpl()) {
+		/* Make the SVSM-related attributes visible */
+		tsm_set_visibility("svsm", svsm_make_visible);
+		tsm_set_visibility("service_guid", svsm_make_visible);
+		tsm_set_visibility("service_manifest_version", svsm_make_visible);
+		tsm_set_visibility("manifestblob", svsm_make_visible);
+	}
+
  	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
  	if (ret)
  		goto e_free_cert_data;
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
index 51e02001bb4d..ebb3c642f548 100644
--- a/drivers/virt/coco/tsm.c
+++ b/drivers/virt/coco/tsm.c
@@ -64,6 +64,11 @@ static struct tsm_report_state *to_state(struct tsm_report *report)
  	return container_of(report, struct tsm_report_state, report);
  }
  
+static bool not_visible(const struct configfs_attribute *attr)
+{
+	return false;
+}
+
  static int try_advance_write_generation(struct tsm_report *report)
  {
  	struct tsm_report_state *state = to_state(report);
@@ -139,7 +144,7 @@ static ssize_t tsm_report_svsm_store(struct config_item *cfg,
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, svsm);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, svsm, not_visible);
  
  static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
  					     const char *buf, size_t len)
@@ -168,7 +173,7 @@ static ssize_t tsm_report_service_guid_store(struct config_item *cfg,
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, service_guid);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_guid, not_visible);
  
  static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg,
  							 const char *buf, size_t len)
@@ -189,7 +194,7 @@ static ssize_t tsm_report_service_manifest_version_store(struct config_item *cfg
  
  	return len;
  }
-CONFIGFS_ATTR_WO(tsm_report_, service_manifest_version);
+CONFIGFS_ATTR_VISIBLE_WO(tsm_report_, service_manifest_version, not_visible);
  
  static ssize_t tsm_report_inblob_write(struct config_item *cfg,
  				       const void *buf, size_t count)
@@ -336,7 +341,7 @@ static ssize_t tsm_report_manifestblob_read(struct config_item *cfg, void *buf,
  
  	return tsm_report_read(report, buf, count, TSM_MANIFEST);
  }
-CONFIGFS_BIN_ATTR_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX);
+CONFIGFS_BIN_ATTR_VISIBLE_RO(tsm_report_, manifestblob, NULL, TSM_OUTBLOB_MAX, not_visible);
  
  #define TSM_DEFAULT_ATTRS() \
  	&tsm_report_attr_generation, \
@@ -444,6 +449,43 @@ static struct configfs_subsystem tsm_configfs = {
  	.su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
  };
  
+int tsm_set_visibility(const char *name, configfs_is_visible_t func)
+{
+	struct configfs_bin_attribute *bin_attr;
+	struct configfs_attribute *attr;
+	const struct config_item_type *t;
+	unsigned int i;
+
+	guard(rwsem_write)(&tsm_rwsem);
+
+	t = provider.type;
+
+	if (t->ct_attrs) {
+		for (i = 0; (attr = t->ct_attrs[i]) != NULL; i++) {
+			if (strcmp(attr->ca_name, name))
+				continue;
+
+			attr->ca_is_visible = func;
+			return 0;
+		}
+	}
+
+	if (t->ct_bin_attrs) {
+		for (i = 0; (bin_attr = t->ct_bin_attrs[i]) != NULL; i++) {
+			attr = &bin_attr->cb_attr;
+
+			if (strcmp(attr->ca_name, name))
+				continue;
+
+			attr->ca_is_visible = func;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(tsm_set_visibility);
+
  int tsm_register(const struct tsm_ops *ops, void *priv,
  		 const struct config_item_type *type)
  {
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
index c4aed3059500..01b075a4debc 100644
--- a/include/linux/tsm.h
+++ b/include/linux/tsm.h
@@ -5,6 +5,7 @@
  #include <linux/sizes.h>
  #include <linux/types.h>
  #include <linux/uuid.h>
+#include <linux/configfs.h>
  
  #define TSM_INBLOB_MAX 64
  #define TSM_OUTBLOB_MAX SZ_32K
@@ -77,4 +78,7 @@ extern const struct config_item_type tsm_report_extra_type;
  int tsm_register(const struct tsm_ops *ops, void *priv,
  		 const struct config_item_type *type);
  int tsm_unregister(const struct tsm_ops *ops);
+
+int tsm_set_visibility(const char *name, configfs_is_visible_t func);
+
  #endif /* __TSM_H */

Thanks,
Tom

>   
>