| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <64c7f7ddd777c_51ad029436@dwillia2-xfh.jf.intel.com.notmuch>
Date: Mon, 31 Jul 2023 11:05:17 -0700
From: Dan Williams <dan.j.williams@...el.com>
To: Peter Gonda <pgonda@...gle.com>,
Dan Williams <dan.j.williams@...el.com>
CC: <dhowells@...hat.com>, Borislav Petkov <bp@...en8.de>,
Tom Lendacky <thomas.lendacky@....com>,
Dionna Glaze <dionnaglaze@...gle.com>,
"Brijesh Singh" <brijesh.singh@....com>, <peterz@...radead.org>,
<linux-coco@...ts.linux.dev>, <keyrings@...r.kernel.org>,
<x86@...nel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] virt: sevguest: Add TSM key support for SNP_{GET,
GET_EXT}_REPORT
Peter Gonda wrote:
> On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@...el.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 TSM keys to retrieve the blobs that the
> > SNP_{GET,GET_EXT}_REPORT ioctls produce. The flow for retrieving the
> > SNP_GET_REPORT blob via the keyctl utility would be:
> >
> > dd if=/dev/urandom of=pubkey bs=1 count=64
> > keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u
> > keyctl print $key_id | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> >
> > ...while the SNP_GET_EXT_REPORT flow adds the "format=extended" option
> > to the request flow:
> >
> > keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2 format=extended" @u
> >
> > The output format from 'keyctl print' is:
> >
> > <pubkey blob> <auth blob desc[:format]> <auth blob>
> >
> > ...where the blobs are hex encoded and the descriptor string is either
> > "sev" or "sev:extended" in this case.
> >
> > Note, the Keys subsystem frontend for the functionality that
> > SNP_GET_DERIVED_KEY represents is saved for follow-on work that likely
> > needs to become a new trusted-keys type. 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 colloboration.
>
> collaboration
got it.
>
> >
> > Note, only compile-tested.
> >
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@...en8.de>
> > Cc: Tom Lendacky <thomas.lendacky@....com>
> > Cc: Dionna Glaze <dionnaglaze@...gle.com>
> > Cc: Brijesh Singh <brijesh.singh@....com>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> > ---
> > drivers/virt/coco/sev-guest/Kconfig | 2 +
> > drivers/virt/coco/sev-guest/sev-guest.c | 87 +++++++++++++++++++++++++++++++
> > 2 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> > index da2d7ca531f0..bce43d4639ce 100644
> > --- a/drivers/virt/coco/sev-guest/Kconfig
> > +++ b/drivers/virt/coco/sev-guest/Kconfig
> > @@ -2,9 +2,11 @@ config SEV_GUEST
> > tristate "AMD SEV Guest driver"
> > default m
> > depends on AMD_MEM_ENCRYPT
> > + depends on KEYS
> > select CRYPTO
> > select CRYPTO_AEAD2
> > select CRYPTO_GCM
> > + select TSM_KEYS
> > 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 f48c4764a7a2..2bdca268272d 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -21,6 +21,7 @@
> > #include <linux/psp-sev.h>
> > #include <uapi/linux/sev-guest.h>
> > #include <uapi/linux/psp-sev.h>
> > +#include <keys/tsm.h>
> >
> > #include <asm/svm.h>
> > #include <asm/sev.h>
> > @@ -769,6 +770,84 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> > return key;
> > }
> >
> > +static int sev_auth_new(struct tsm_key_payload *t, void *provider_data)
> > +{
> > + struct snp_guest_dev *snp_dev = provider_data;
> > + const int report_size = SZ_16K;
> > + const int ext_size =
> > + PAGE_ALIGN_DOWN(TSM_DATA_MAX - report_size - sizeof(*t));
> > + int ret;
> > +
> > + if (t->pubkey_len != 64)
> > + return -EINVAL;
>
> Magic number?
>
> We only support asymmetric keys with public keys exactly equal to 64
> bytes? Is that only p256? SNP uses p384 can we atleast allow that
> curve too? But why not let userspace what key type they want to use?
The kernel has no control here. See Table 20 MSG_REPORT_REQ Message
Structure (https://www.amd.com/system/files/TechDocs/56860.pdf)
...only 64-byte payloads are accepted. I assume one could specify less
than 64-bytes and zero-fill the rest, but that's a contract between the
requester and the attester.
>
> > +
> > + if (t->auth_blob_format[0] &&
> > + strcmp(t->auth_blob_format, "extended") != 0)
> > + return -EINVAL;
> > +
> > + if (t->auth_blob_format[0]) {
> > + u8 *buf __free(kvfree) =
> > + kvzalloc(report_size + ext_size, GFP_KERNEL);
> > +
> > + struct snp_ext_report_req req = {
> > + .data = { .vmpl = t->privlevel },
> > + .certs_address = (__u64)buf + report_size,
> > + .certs_len = ext_size,
> > + };
> > + memcpy(&req.data.user_data, t->pubkey, 64);
>
> Again without any freshness from the remote party, of what use is this
> attestation report?
This interface is just marshaling the same data that could be retrieved
via SNP_GET_REPORT ioctl on the sevguest chardev today. So I would point
you back to vendor documentation for how this report is used. See "VM
Launch and Attestation" here:
https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf
I am just here to stanch the proliferation of new chardevs and new
ioctls for this TSM-common operation. This effort was started when TDX
patches showed up to take a 64-byte input payload and wrap it in a
attestation report with its own chardev and ioctls.
>
> > +
> > + struct snp_guest_request_ioctl input = {
> > + .msg_version = 1,
> > + .req_data = (__u64) &req,
> > + .resp_data = (__u64) buf,
> > + };
> > +
> > + ret = get_ext_report(snp_dev, &input, SNP_KARG);
> > + if (ret)
> > + return ret;
> > +
> > + no_free_ptr(buf);
> > + t->auth_blob = buf;
> > + t->auth_blob_len = report_size + ext_size;
> > + t->auth_blob_desc = "sev";
> > + } else {
> > + u8 *buf __free(kvfree) = kvzalloc(report_size, GFP_KERNEL);
> > +
> > + struct snp_report_req req = {
> > + .vmpl = t->privlevel,
> > + };
> > + memcpy(&req.user_data, t->pubkey, 64);
> > +
> > + struct snp_guest_request_ioctl input = {
> > + .msg_version = 1,
> > + .req_data = (__u64) &req,
> > + .resp_data = (__u64) buf,
> > + };
> > +
> > + ret = get_report(snp_dev, &input, SNP_KARG);
> > + if (ret)
> > + return ret;
> > +
> > + no_free_ptr(buf);
> > + t->auth_blob = buf;
> > + t->auth_blob_len = report_size;
> > + t->auth_blob_desc = "sev";
> > + }
>
> Can we reduce the code duplication between the branches here?
I'll take a look.
Powered by blists - more mailing lists