From: Dan Williams <dan.j.williams@intel.com>
To: Tom Lendacky <thomas.lendacky@amd.com>,
Dan Williams <dan.j.williams@intel.com>,
<linux-coco@lists.linux.dev>
Cc: Borislav Petkov <bp@alien8.de>,
Dionna Glaze <dionnaglaze@google.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 17:38:58 -0700 [thread overview]
Message-ID: <6527402239952_780ef294cd@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <9b08e4f0-9538-3680-2465-1b200ff0d777@amd.com>
Tom Lendacky wrote:
> On 10/11/23 16:30, Dan Williams wrote:
> > Tom Lendacky wrote:
> >> On 10/11/23 00:27, Dan Williams 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);
> >>
> >> Is this needed given the guard above?
> >>
> >>> + 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;
> >>> + }
> >>
> >> I agree with Dionna here, you must keep the GUID<-->Cert relationship
> >> here. I think you can just copy the full returned cert buffer into the
> >> destination buffer. Then it would look just like what the ioctl() returns
> >> and make it easier for userspace programs to switch to the new mechanism.
> >
> > This reverses the feedback from Jeremi where he asked for a separate
> > "certs" file.
>
> I'm not sure I follow, you would still have a separate certs file. It
> would contain the GUID table followed by the certs data and would be
> separate from the attestation report. You would be replacing the
> "Concatenate returned certs" loop with a straight memcpy. If you want to
> truncate the size to the actual data size, then you could go to the last
> entry in the GUID table and use the offset and length to arrive at the
> final size needed to be copied.
The issue is common ABI across vendors. TDX Quote does not have the
same "cert_table" scheme to convey this data. So that's why I arrived at
@auxblob to convey this "on the side" + "vendor format" data. I was
hoping that "concatenated PEM" was a point of commonality, but as Dionna
is pointing out these things are not quite the same.
> The current ioctl() has two requests, GET_REPORT and GET_EXT_REPORT. In
> the case of the latter, a separate buffer is used to hold the returned
> certs data (see struct snp_ext_report_req) and this would allow the data
> from the certs file to be treated exactly the same as if returned via the
> ioctl().
Right, that's what I would route to @auxblob.
> > Hmm, perhaps this should be an optional @auxblob attribute where a
> > backend can publish supplemental data to the report. The issue from a
> > common ABI perspective is that the SNP report format is independent of
> > the conveyed certificates and the TDX quote format includes a reference
> > to the "certs" from the "reportblob". In the SNP case that certs
> > reference is conveyed in the ioctl envelope which does not exist in the
> > configfs-tsm case.
> >
> > So the proposal is @auxblob is documented as supplemental data to the
> > report, and then when @provider indicates "sev-guest" the format of
> > @auxblob is defined by 'struct cert_table' in GHCB 4.1.8.1
> > MSG_REPORT_REQ.
>
> This will work, too. It allows the userspace to read and parse the data
> just as if it had been returned via the ioctl(). And calling it auxblob
> gives you the freedom to indicate that it is provider defined vs. certs
> which might imply a certain format.
Ok, sounds good.
next prev parent reply other threads:[~2023-10-12 0:39 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
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 [this message]
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=6527402239952_780ef294cd@dwillia2-xfh.jf.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=bp@alien8.de \
--cc=brijesh.singh@amd.com \
--cc=dave.hansen@linux.intel.com \
--cc=dionnaglaze@google.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).