From: Dionna Amalie Glaze <dionnaglaze@google.com>
To: Dan Williams <dan.j.williams@intel.com>
Cc: linux-coco@lists.linux.dev, Borislav Petkov <bp@alien8.de>,
Tom Lendacky <thomas.lendacky@amd.com>,
Brijesh Singh <brijesh.singh@amd.com>,
Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>,
peterz@infradead.org,
sathyanarayanan.kuppuswamy@linux.intel.com,
dave.hansen@linux.intel.com
Subject: Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
Date: Wed, 11 Oct 2023 09:13:31 -0700 [thread overview]
Message-ID: <CAAH4kHZkWYT5i=5EVqywh7jk-00kcXu7h4vbz24fTwAm42frVg@mail.gmail.com> (raw)
In-Reply-To: <169700206636.779347.12625001287120171667.stgit@dwillia2-xfh.jf.intel.com>
On Tue, Oct 10, 2023 at 10:27 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> The sevguest driver was a first mover in the confidential computing
> space. As a first mover that afforded some leeway to build the driver
> without concern for common infrastructure.
>
> Now that sevguest is no longer a singleton [1] the common operation of
> building and transmitting attestation report blobs can / should be made
> common. In this model the so called "TSM-provider" implementations can
> share a common envelope ABI even if the contents of that envelope remain
> vendor-specific. When / if the industry agrees on an attestation record
> format, that definition can also fit in the same ABI. In the meantime
> the kernel's maintenance burden is reduced and collaboration on the
> commons is increased.
>
> Convert sevguest to use CONFIG_TSM_REPORTS to retrieve the data that
> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
> retrieving the report blob via the TSM interface utility,
> assuming no nonce and VMPL==2:
>
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> echo 2 > $report/privlevel
> dd if=/dev/urandom bs=64 count=1 > $report/inblob
> hexdump -C $report/outblob
> cat $report/certs
> rmdir $report
>
> Given that the platform implementation is free to return empty
> certificate data if none is available it lets configfs-tsm be simplified
> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
> SNP_GET_REPORT alone.
>
> The old ioctls can be lazily deprecated, the main motivation of this
> effort is to stop the proliferation of new ioctls, and to increase
> cross-vendor collaboration.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> drivers/virt/coco/sev-guest/Kconfig | 1
> drivers/virt/coco/sev-guest/sev-guest.c | 139 +++++++++++++++++++++++++++++++
> 2 files changed, 140 insertions(+)
>
> diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> index da2d7ca531f0..1cffc72c41cb 100644
> --- a/drivers/virt/coco/sev-guest/Kconfig
> +++ b/drivers/virt/coco/sev-guest/Kconfig
> @@ -5,6 +5,7 @@ config SEV_GUEST
> select CRYPTO
> select CRYPTO_AEAD2
> select CRYPTO_GCM
> + select TSM_REPORTS
> help
> SEV-SNP firmware provides the guest a mechanism to communicate with
> the PSP without risk from a malicious hypervisor who wishes to read,
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index e5f8f115f4af..7fdc5a774eab 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -16,10 +16,12 @@
> #include <linux/miscdevice.h>
> #include <linux/set_memory.h>
> #include <linux/fs.h>
> +#include <linux/tsm.h>
> #include <crypto/aead.h>
> #include <linux/scatterlist.h>
> #include <linux/psp-sev.h>
> #include <linux/sockptr.h>
> +#include <linux/cleanup.h>
> #include <uapi/linux/sev-guest.h>
> #include <uapi/linux/psp-sev.h>
>
> @@ -768,6 +770,135 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> return key;
> }
>
> +struct snp_msg_report_resp_hdr {
> + u32 status;
> + u32 report_size;
> + u8 rsvd[24];
> +};
> +#define SNP_REPORT_INVALID_PARAM 0x16
> +#define SNP_REPORT_INVALID_KEY_SEL 0x27
> +
> +struct snp_msg_cert_entry {
> + unsigned char guid[16];
> + u32 offset;
> + u32 length;
> +};
> +
> +static int sev_report_new(struct tsm_report *report, void *data)
> +{
> + static const struct snp_msg_cert_entry zero_ent = { 0 };
> + int certs_size = 0, cert_count, i, offset;
> + struct tsm_desc *desc = &report->desc;
> + struct snp_guest_dev *snp_dev = data;
> + struct snp_msg_report_resp_hdr hdr;
> + const int report_size = SZ_4K;
> + const int ext_size = SZ_16K;
> + int ret, size = report_size + ext_size;
> + u8 *certs_address;
> +
> + if (desc->inblob_len != 64)
> + return -EINVAL;
> +
> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + guard(mutex)(&snp_cmd_mutex);
> +
> + /* Check if the VMPCK is not empty */
> + if (is_vmpck_empty(snp_dev)) {
> + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> + mutex_unlock(&snp_cmd_mutex);
> + return -ENOTTY;
> + }
> +
> + certs_address = buf + report_size;
> + struct snp_ext_report_req ext_req = {
> + .data = { .vmpl = desc->privlevel },
> + .certs_address = (__u64)certs_address,
> + .certs_len = ext_size,
> + };
> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> +
> + struct snp_guest_request_ioctl input = {
> + .msg_version = 1,
> + .req_data = (__u64)&ext_req,
> + .resp_data = (__u64)buf,
> + .exitinfo2 = 0xff,
> + };
> + struct snp_req_resp io = {
> + .req_data = KERNEL_SOCKPTR(&ext_req),
> + .resp_data = KERNEL_SOCKPTR(buf),
> + };
> +
> + ret = get_ext_report(snp_dev, &input, &io);
> +
> + if (ret)
> + return ret;
> +
> + memcpy(&hdr, buf, sizeof(hdr));
> + if (hdr.status == SNP_REPORT_INVALID_PARAM)
> + return -EINVAL;
> + if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> + return -EINVAL;
> + if (hdr.status)
> + return -ENXIO;
> + if ((hdr.report_size + sizeof(hdr)) > report_size)
> + return -ENOMEM;
> +
> + void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> + if (!rbuf)
> + return -ENOMEM;
> +
> + memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> + report->outblob = no_free_ptr(rbuf);
> + report->outblob_len = hdr.report_size;
> +
> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
> + break;
> + certs_size += certs[i].length;
> + }
> + cert_count = i;
> +
> + /* No certs to report */
> + if (cert_count == 0)
> + return 0;
> +
> + /* sanity check that the entire certs table with metadata fits */
> + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
> + return -ENXIO;
> +
> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> + if (!cbuf)
> + return -ENOMEM;
> +
> + /* Concatenate returned certs */
> + for (i = 0, offset = 0; i < cert_count; i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> + offset += certs[i].length;
> + }
This concatenation isn't going to work since you lose the identity of
the certificates. The GUIDs of the certificate table matter, and the
concatenation isn't necessarily easily parsed out.
Consider our use case of providing not just the ARK, ASK, and VCEK
certificates, but also a non-x.509 document that is a signed golden
measurement of the firmware. We may also want to provide the VLEK so
the endorsement key selection attribute of the MSG_REPORT_REQ can be
utilized to swap between VCEK and VLEK. I don't believe your patch
here allows for that selection either.
By the GHCB specification, the host is allowed to document their own
GUIDs and provide their data to the guest in this data blob.
I think probably what's better is for the configfs to just create
GUID-named files with the contents of the entries, but I don't think
just inblob of length 64 is going to handle the VLEK/VCEK selection.
> +
> + report->certs = no_free_ptr(cbuf);
> + report->certs_len = certs_size;
> +
> + return 0;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> + .name = KBUILD_MODNAME,
> + .report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> + tsm_unregister(&sev_tsm_ops);
> +}
> +
> static int __init sev_guest_probe(struct platform_device *pdev)
> {
> struct snp_secrets_page_layout *layout;
> @@ -841,6 +972,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
> snp_dev->input.resp_gpa = __pa(snp_dev->response);
> snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>
> + ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
> + if (ret)
> + goto e_free_cert_data;
> +
> + ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> + if (ret)
> + goto e_free_cert_data;
> +
> ret = misc_register(misc);
> if (ret)
> goto e_free_cert_data;
>
--
-Dionna Glaze, PhD (she/her)
next prev parent reply other threads:[~2023-10-11 16:13 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
2023-10-11 5:27 ` [PATCH v5 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target Dan Williams
2023-10-11 5:27 ` [PATCH v5 2/7] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-10-11 5:27 ` [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-10-11 6:29 ` Kuppuswamy Sathyanarayanan
2023-10-11 5:27 ` [PATCH v5 4/7] virt: sevguest: Prep for kernel internal get_ext_report() Dan Williams
2023-10-11 5:27 ` [PATCH v5 5/7] mm/slab: Add __free() support for kvfree Dan Williams
2023-10-11 6:31 ` Kuppuswamy Sathyanarayanan
2023-10-11 5:27 ` [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
2023-10-11 16:13 ` Dionna Amalie Glaze [this message]
2023-10-11 20:41 ` Dan Williams
2023-10-11 21:06 ` Dionna Amalie Glaze
2023-10-11 19:24 ` Tom Lendacky
2023-10-11 21:30 ` Dan Williams
2023-10-11 22:21 ` Dionna Amalie Glaze
2023-10-11 22:24 ` Tom Lendacky
2023-10-12 0:38 ` Dan Williams
2023-10-11 5:27 ` [PATCH v5 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
2023-10-11 6:44 ` [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Kuppuswamy Sathyanarayanan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAH4kHZkWYT5i=5EVqywh7jk-00kcXu7h4vbz24fTwAm42frVg@mail.gmail.com' \
--to=dionnaglaze@google.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=dan.j.williams@intel.com \
--cc=dave.hansen@linux.intel.com \
--cc=jpiotrowski@linux.microsoft.com \
--cc=linux-coco@lists.linux.dev \
--cc=peterz@infradead.org \
--cc=sathyanarayanan.kuppuswamy@linux.intel.com \
--cc=thomas.lendacky@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.