Message-ID: <649b74724043c_11e6852945a@dwillia2-xfh.jf.intel.com.notmuch>
Date:   Tue, 27 Jun 2023 16:44:50 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Dan Williams <dan.j.williams@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
        Shuah Khan <shuah@...nel.org>, Jonathan Corbet <corbet@....net>
CC:     "H . Peter Anvin" <hpa@...or.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        "Wander Lairson Costa" <wander@...hat.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Dionna Amalie Glaze <dionnaglaze@...gle.com>,
        Chong Cai <chongc@...gle.com>, Qinkun Bao <qinkun@...che.org>,
        Guorui Yu <GuoRui.Yu@...ux.alibaba.com>,
        "Du Fan" <fan.du@...el.com>, <linux-kernel@...r.kernel.org>,
        <linux-kselftest@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <dhowells@...hat.com>, <brijesh.singh@....com>,
        <atishp@...osinc.com>, <gregkh@...uxfoundation.org>
Subject: Re: [PATCH v3 3/3] selftests/tdx: Test GetQuote TDX attestation
 feature

Sathyanarayanan Kuppuswamy wrote:
> Hi Dan,
> 
> On 6/23/23 3:27 PM, Dan Williams wrote:
> > Dan Williams wrote:
> >> [ add David, Brijesh, and Atish]
> >>
> >> Kuppuswamy Sathyanarayanan wrote:
> >>> In TDX guest, the second stage of the attestation process is Quote
> >>> generation. This process is required to convert the locally generated
> >>> TDREPORT into a remotely verifiable Quote. It involves sending the
> >>> TDREPORT data to a Quoting Enclave (QE) which will verify the
> >>> integrity of the TDREPORT and sign it with an attestation key.
> >>>
> >>> Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to
> >>> allow the user agent to get the TD Quote.
> >>>
> >>> Add a kernel selftest module to verify the Quote generation feature.
> >>>
> >>> TD Quote generation involves following steps:
> >>>
> >>> * Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL.
> >>> * Embed the TDREPORT data in quote buffer and request for quote
> >>>   generation via TDX_CMD_GET_QUOTE IOCTL request.
> >>> * Upon completion of the GetQuote request, check for non zero value
> >>>   in the status field of Quote header to make sure the generated
> >>>   quote is valid.
> >>
> >> What this cover letter does not say is that this is adding another
> >> instance of the similar pattern as SNP_GET_REPORT.
> >>
> >> Linux is best served when multiple vendors trying to do similar
> >> operations are brought together behind a common ABI. We see this in the
> >> history of wrangling SCSI vendors behind common interfaces. Now multiple
> >> confidential computing vendors trying to develop similar flows with
> >> differentiated formats where that differentiation need not leak over the
> >> ABI boundary.
> > [..]
> > 
> > Below is a rough mock up of this approach to demonstrate the direction.
> > Again, the goal is to define an ABI that can support any vendor's
> > arch-specific attestation method and key provisioning flows without
> > leaking vendor-specific details, or confidential material over the
> > user/kernel ABI.
> 
> Thanks for working on this mock code and helping out. It gives me the
> general idea about your proposal.
> 
> > 
> > The observation is that there are a sufficient number of attestation
> > flows available to review where Linux can define a superset ABI to
> > contain them all. The other observation is that the implementations have
> > features that may cross-polinate over time. For example the SEV
> > privelege level consideration ("vmpl"), and the TDX RTMR (think TPM
> > PCRs) mechanisms address generic Confidential Computing use cases.
> 
> 
> I agree with your point about VMPL and RTMR feature cases. This observation
> is valid for AMD SEV and TDX attestation flows. But I am not sure whether
> it will hold true for other vendor implementations. Our sample set is not
> good enough to make this conclusion. The reason for my concern is, if you
> check the ABI interface used in the S390 arch attestation driver
> (drivers/s390/char/uvdevice.c), you would notice that there is a significant
> difference between the ABI used in that driver and SEV/TDX drivers. The S390
> driver attestation request appears to accept two data blobs as input, as well
> as a variety of vendor-specific header configurations.

I would need more time to investigate. It's also the case that if both
major x86 vendors plus ARM and/or RISC-V can all get behind the same
frontend then that is already success in my mind.

> Maybe the s390 attestation model is a special case, but, I think we consider
> this issue. Since we don't have a common spec, there is chance that any
> superset ABI we define now may not meet future vendor requirements. One way to
> handle it to leave enough space in the generic ABI to handle future vendor
> requirements.

Perhaps, but the goal here is to clearly indicate "this is how Linux
conveys confidential computing attestation concepts". If there is future
vendor innovation in this space it needs to consider how it meets the
established needs of Linux, not the other way round.

> I think it would be better if other vendors (like ARM or RISC) can comment and
> confirm whether this proposal meets their demands.

The more participation the better. Open source definitely involves a
component speaking up when the definition of things are still malleable,
or catching issues before they go upstream.

> > Vendor specific ioctls for all of this feels like surrender when Linux
> > already has the keys subsystem which has plenty of degrees of freedom
> > for tracking blobs with signatures and using those blobs to instantiate
> > other blobs. It already serves as the ABI wrapping various TPM
> > implementations and marshaling keys for storage encryption and other use
> > cases that intersect Confidential Computing.
> > 
> > The benefit of deprecating vendor-specific abstraction layers in
> > userspace is secondary. The primary benefit is collaboration. It enables
> > kernel developers from various architectures to collaborate on common
> > infrastructure. If, referring back to my previous example, SEV adopts an
> > RTMR-like mechanism and TDX adopts a vmpl-like mechanism it would be
> > unfortunate if those efforts were siloed, duplicated, and needlessly
> > differentiated to userspace. So while there are arguably a manageable
> > number of basic arch attestation methods the planned expansion of those
> > to build incremental functionality is where I believe we, as a
> > community, will be glad that we invested in a "Linux format" for all of
> > this.
> > 
> > An example, to show what the strawman patch below enables: (req_key is
> > the sample program from "man 2 request_key")
> > 
> > # ./req_key guest_attest guest_attest:0:0-$desc $(cat user_data | base64)
> > Key ID is 10e2f3a7
> > # keyctl pipe 0x10e2f3a7 | hexdump -C
> > 00000000  54 44 58 20 47 65 6e 65  72 61 74 65 64 20 51 75  |TDX Generated Qu|
> > 00000010  6f 74 65 00 00 00 00 00  00 00 00 00 00 00 00 00  |ote.............|
> > 00000020  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
> > *
> > 00004000
> > 
> > This is the kernel instantiating a TDX Quote without the TDREPORT
> > implementation detail ever leaving the kernel. Now, this is only the
> 
> IIUC, the idea here is to cache the quote data and return it to the user whenever
> possible, right? If yes, I think such optimization may not be very useful for our
> case. AFAIK, the quote data will change whenever there is a change in the guest
> measurement data. Since the validity of the generated quote will not be long,
> and the frequency of quote generation requests is expected to be less, we may not
> get much benefit from caching the quote data. I think we can keep this logic simple
> by directly retrieving the quote data from the quoting enclave whenever there is a
> request from the user.

The Keys subsystem already supports the concept of keys that expire
immediately, so no need for special consideration here that I can see.

> > top-half of what is needed. The missing bottom half takes that material
> > and uses it to instantiate derived key material like the storage
> > decryption key internal to the kernel. See "The Process" in
> > Documentation/security/keys/request-key.rst for how the Keys subsystem
> > handles the "keys for keys" use case.
> 
> This is only useful for key-server use case, right? Attestation can also be
> used for use cases like pattern matching or uploading some secure data, etc.
> Since key-server is not the only use case, does it make sense to suppport
> this derived key feature?

The Keys subsystem is just a way for both userspace and kernel space to
request the instantiation of blobs that mediate access to another
resource be it another key or something else. So key-server is only one
example client.

The other reason for defining a common frontend is so the kernel can
understand and mediate resource access as a kernel is wont to do. The
ioctl() approach blinds the kernel and requires userspace to repeatedly
solve problems in vendor specific ways. They Keys proposal also has the
property of not requiring userspace round trips for things like
unlocking storage. The driver can just do request_keys() and kick off
the process on its own.

> 
> > 
> > ---
> > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> > index f79ab13a5c28..0f775847028e 100644
> > --- a/drivers/virt/Kconfig
> > +++ b/drivers/virt/Kconfig
> > @@ -54,4 +54,8 @@ source "drivers/virt/coco/sev-guest/Kconfig"
> >  
> >  source "drivers/virt/coco/tdx-guest/Kconfig"
> >  
> > +config GUEST_ATTEST
> > +	tristate
> > +	select KEYS
> > +
> >  endif
> > diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> > index e9aa6fc96fab..66f6b838f8f4 100644
> > --- a/drivers/virt/Makefile
> > +++ b/drivers/virt/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_ACRN_HSM)		+= acrn/
> >  obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
> >  obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
> >  obj-$(CONFIG_INTEL_TDX_GUEST)	+= coco/tdx-guest/
> > +obj-$(CONFIG_GUEST_ATTEST)	+= coco/guest-attest/
> > diff --git a/drivers/virt/coco/guest-attest/Makefile b/drivers/virt/coco/guest-attest/Makefile
> > new file mode 100644
> > index 000000000000..5581c5a27588
> > --- /dev/null
> > +++ b/drivers/virt/coco/guest-attest/Makefile
> > @@ -0,0 +1,2 @@
> > +obj-$(CONFIG_GUEST_ATTEST) += guest_attest.o
> > +guest_attest-y := key.o
> > diff --git a/drivers/virt/coco/guest-attest/key.c b/drivers/virt/coco/guest-attest/key.c
> > new file mode 100644
> > index 000000000000..2a494b6dd7a7
> > --- /dev/null
> > +++ b/drivers/virt/coco/guest-attest/key.c
> > @@ -0,0 +1,159 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +#include <linux/seq_file.h>
> > +#include <linux/key-type.h>
> > +#include <linux/module.h>
> > +#include <linux/base64.h>
> > +
> > +#include <keys/request_key_auth-type.h>
> > +#include <keys/user-type.h>
> > +
> > +#include "guest-attest.h"
> 
> Can you share you guest-attest.h?

Apologies, missed a 'git add':

/* SPDX-License-Identifier: GPL-2.0-only */
/* Copyright(c) 2023 Intel Corporation. */

#ifndef __GUEST_ATTEST_H__
#define __GUEST_ATTEST_H__
#include <linux/list.h>

/*
 * arch specific ops, only one is expected to be registered at a time
 * i.e. either SEV or TDX, but not both
 */
struct guest_attest_ops {
        const char *name;
        struct module *module;
        struct list_head list;
        int (*request_attest)(struct key *key, int level,
                              struct key *dest_keyring, void *payload,
                              int payload_len, struct key *authkey);
};

#define GUEST_ATTEST_DATALEN 64

int register_guest_attest_ops(struct guest_attest_ops *ops);
void unregister_guest_attest_ops(struct guest_attest_ops *ops);

#endif /*__GUEST_ATTEST_H__ */


> 
> > +
> > +static LIST_HEAD(guest_attest_list);
> > +static DECLARE_RWSEM(guest_attest_rwsem);
> > +
> > +static struct guest_attest_ops *fetch_ops(void)
> > +{
> > +	return list_first_entry_or_null(&guest_attest_list,
> > +					struct guest_attest_ops, list);
> > +}
> > +
> > +static struct guest_attest_ops *get_ops(void)
> > +{
> > +	down_read(&guest_attest_rwsem);
> > +	return fetch_ops();
> > +}
> > +
> > +static void put_ops(void)
> > +{
> > +	up_read(&guest_attest_rwsem);
> > +}
> > +
> > +int register_guest_attest_ops(struct guest_attest_ops *ops)
> > +{
> > +	struct guest_attest_ops *conflict;
> > +	int rc;
> > +
> > +	down_write(&guest_attest_rwsem);
> > +	conflict = fetch_ops();
> > +	if (conflict) {
> > +		pr_err("\"%s\" ops already registered\n", conflict->name);
> > +		rc = -EEXIST;
> > +		goto out;
> > +	}
> > +	list_add(&ops->list, &guest_attest_list);
> > +	try_module_get(ops->module);
> > +	rc = 0;
> > +out:
> > +	up_write(&guest_attest_rwsem);
> > +	return rc;
> > +}
> > +EXPORT_SYMBOL_GPL(register_guest_attest_ops);
> > +
> > +void unregister_guest_attest_ops(struct guest_attest_ops *ops)
> > +{
> > +	down_write(&guest_attest_rwsem);
> > +	list_del(&ops->list);
> > +	up_write(&guest_attest_rwsem);
> > +	module_put(ops->module);
> > +}
> > +EXPORT_SYMBOL_GPL(unregister_guest_attest_ops);
> > +
> > +static int __guest_attest_request_key(struct key *key, int level,
> > +				      struct key *dest_keyring,
> > +				      const char *callout_info, int callout_len,
> > +				      struct key *authkey)
> > +{
> > +	struct guest_attest_ops *ops;
> > +	void *payload = NULL;
> > +	int rc, payload_len;
> > +
> > +	ops = get_ops();
> > +	if (!ops)
> > +		return -ENOKEY;
> > +
> > +	payload = kzalloc(max(GUEST_ATTEST_DATALEN, callout_len), GFP_KERNEL);
> > +	if (!payload) {
> > +		rc = -ENOMEM;
> > +		goto out;
> > +	}
> 
> Is the idea to get the values like vmpl part of the payload?

No, to me vmpl likely needs to be conveyed in the key-description.
Payload is simply the 64-bytes that both SEV and TDX take as input for
the attestation request. The AMD specification seems to imply that
payload is itself a public key? If that is the expectation then it may
be more appropriate to make that a separate retrieval vs something
passed in directly from userspace.

> 
> > +
> > +	payload_len = base64_decode(callout_info, callout_len, payload);
> > +	if (payload_len < 0 || payload_len > GUEST_ATTEST_DATALEN) {
> > +		rc = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	rc = ops->request_attest(key, level, dest_keyring, payload, payload_len,
> > +				 authkey);
> > +out:
> > +	kfree(payload);
> > +	put_ops();
> > +	return rc;
> > +}
> > +
> > +static int guest_attest_request_key(struct key *authkey, void *data)
> > +{
> > +	struct request_key_auth *rka = get_request_key_auth(authkey);
> > +	struct key *key = rka->target_key;
> > +	unsigned long long id;
> > +	int rc, level;
> > +
> > +	pr_debug("desc: %s op: %s callout: %s\n", key->description, rka->op,
> > +		 rka->callout_info ? (char *)rka->callout_info : "\"none\"");
> > +
> > +	if (sscanf(key->description, "guest_attest:%d:%llu", &level, &id) != 2)
> > +		return -EINVAL;
> > +
> 
> Can you explain some details about the id and level? It is not very clear why
> we need it.

@level is my mockup of the vmpl concept, and @id is something free-form
for now that the requester of the attestation knows what it means. This
id-scheme would follow a Linux-defined format. More discussion is needed
here about what attestation data is used for and opportunities to define
a common naming scheme. For example, what is the common name of the
guest_attest-key that supports retrieving the storage decryption key
from the key-server.

Powered by blists - more mailing lists