Re: [PATCH v3 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Sathyanarayanan Kuppuswamy]

+Atish

Atish, any comments on this topic from RISC-v?

On 6/26/23 11:57 AM, Dionna Amalie Glaze wrote:
> On Sun, Jun 25, 2023 at 8:06 PM Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> 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.
>>
>> 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.
>>
>> I think it would be better if other vendors (like ARM or RISC) can comment and
>> confirm whether this proposal meets their demands.
>>
> 
> The VMPL-based separation that will house the supervisor module known
> as SVSM can have protocols that implement a TPM command interface, or
> an RTMR-extension interface, and will also need to have an
> SVSM-specific protocol attestation report format to keep the secure
> chain of custody apparent. We'd have different formats and protocols
> in the kernel, at least, to speak to each technology. I'm not sure
> it's worth the trouble of papering over all the... 3-4 technologies
> with similar but still weirdly different formats and ways of doing
> things with an abstracted attestation ABI, especially since the output
> all has to be interpreted in an architecture-specific way anyway.
> 
> ARM's Confidential Computing Realm Management Extensions (RME) seems
> to be going along the lines of a runtime measurement register model
> with their hardware enforced security. The number of registers isn't
> prescribed in the spec.
> 
> +Joey Gouly +linux-coco@xxxxxxxxxxxxxxx as far as RME is concerned, do
> you know who would be best to weigh in on this discussion of a unified
> attestation model?


> 
>>>
>>> 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.
>>
>>> 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?
>>
>>>
>>> ---
>>> 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?
>>
>>> +
>>> +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?
>>
>>> +
>>> +     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.
>>
>>> +     if (!rka->callout_info) {
>>> +             rc = -EINVAL;
>>> +             goto out;
>>> +     }
>>> +
>>> +     rc = __guest_attest_request_key(key, level, rka->dest_keyring,
>>> +                                     rka->callout_info, rka->callout_len,
>>> +                                     authkey);
>>> +out:
>>> +     complete_request_key(authkey, rc);
>>> +     return rc;
>>> +}
>>> +
>>> +static int guest_attest_vet_description(const char *desc)
>>> +{
>>> +     unsigned long long id;
>>> +     int level;
>>> +
>>> +     if (sscanf(desc, "guest_attest:%d:%llu", &level, &id) != 2)
>>> +             return -EINVAL;
>>> +     return 0;
>>> +}
>>> +
>>> +static struct key_type key_type_guest_attest = {
>>> +     .name = "guest_attest",
>>> +     .preparse = user_preparse,
>>> +     .free_preparse = user_free_preparse,
>>> +     .instantiate = generic_key_instantiate,
>>> +     .revoke = user_revoke,
>>> +     .destroy = user_destroy,
>>> +     .describe = user_describe,
>>> +     .read = user_read,
>>> +     .vet_description = guest_attest_vet_description,
>>> +     .request_key = guest_attest_request_key,
>>> +};
>>> +
>>> +static int __init guest_attest_init(void)
>>> +{
>>> +     return register_key_type(&key_type_guest_attest);
>>> +}
>>> +
>>> +static void __exit guest_attest_exit(void)
>>> +{
>>> +     unregister_key_type(&key_type_guest_attest);
>>> +}
>>> +
>>> +module_init(guest_attest_init);
>>> +module_exit(guest_attest_exit);
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
>>> index 14246fc2fb02..9a1ec85369fe 100644
>>> --- a/drivers/virt/coco/tdx-guest/Kconfig
>>> +++ b/drivers/virt/coco/tdx-guest/Kconfig
>>> @@ -1,6 +1,7 @@
>>>  config TDX_GUEST_DRIVER
>>>       tristate "TDX Guest driver"
>>>       depends on INTEL_TDX_GUEST
>>> +     select GUEST_ATTEST
>>>       help
>>>         The driver provides userspace interface to communicate with
>>>         the TDX module to request the TDX guest details like attestation
>>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>> index 388491fa63a1..65b5aab284d9 100644
>>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>>> @@ -13,11 +13,13 @@
>>>  #include <linux/string.h>
>>>  #include <linux/uaccess.h>
>>>  #include <linux/set_memory.h>
>>> +#include <linux/key-type.h>
>>>
>>>  #include <uapi/linux/tdx-guest.h>
>>>
>>>  #include <asm/cpu_device_id.h>
>>>  #include <asm/tdx.h>
>>> +#include "../guest-attest/guest-attest.h"
>>>
>>>  /*
>>>   * Intel's SGX QE implementation generally uses Quote size less
>>> @@ -229,6 +231,62 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
>>>  };
>>>  MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>>>
>>> +static int tdx_request_attest(struct key *key, int level,
>>> +                           struct key *dest_keyring, void *payload,
>>> +                           int payload_len, struct key *authkey)
>>> +{
>>> +     u8 *tdreport;
>>> +     long ret;
>>> +
>>> +     tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
>>> +     if (!tdreport)
>>> +             return -ENOMEM;
>>> +
>>> +     /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
>>> +     ret = tdx_mcall_get_report0(payload, tdreport);
>>> +     if (ret)
>>> +             goto out;
>>> +
>>> +     mutex_lock(&quote_lock);
>>> +
>>> +     memset(qentry->buf, 0, qentry->buf_len);
>>> +     reinit_completion(&qentry->compl);
>>> +     qentry->valid = true;
>>> +
>>> +     /* Submit GetQuote Request using GetQuote hyperetall */
>>> +     ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
>>> +     if (ret) {
>>> +             pr_err("GetQuote hyperetall failed, status:%lx\n", ret);
>>> +             ret = -EIO;
>>> +             goto quote_failed;
>>> +     }
>>> +
>>> +     /*
>>> +      * Although the GHCI specification does not state explicitly that
>>> +      * the VMM must not wait indefinitely for the Quote request to be
>>> +      * completed, a sane VMM should always notify the guest after a
>>> +      * certain time, regardless of whether the Quote generation is
>>> +      * successful or not.  For now just assume the VMM will do so.
>>> +      */
>>> +     wait_for_completion(&qentry->compl);
>>> +
>>> +     ret = key_instantiate_and_link(key, qentry->buf, qentry->buf_len,
>>> +                                    dest_keyring, authkey);
>>> +
>>> +quote_failed:
>>> +     qentry->valid = false;
>>> +     mutex_unlock(&quote_lock);
>>> +out:
>>> +     kfree(tdreport);
>>> +     return ret;
>>> +}
>>> +
>>> +static struct guest_attest_ops tdx_attest_ops = {
>>> +     .name = KBUILD_MODNAME,
>>> +     .module = THIS_MODULE,
>>> +     .request_attest = tdx_request_attest,
>>> +};
>>> +
>>>  static int __init tdx_guest_init(void)
>>>  {
>>>       int ret;
>>> @@ -251,8 +309,14 @@ static int __init tdx_guest_init(void)
>>>       if (ret)
>>>               goto free_quote;
>>>
>>> +     ret = register_guest_attest_ops(&tdx_attest_ops);
>>> +     if (ret)
>>> +             goto free_irq;
>>> +
>>>       return 0;
>>>
>>> +free_irq:
>>> +     tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
>>>  free_quote:
>>>       free_quote_entry(qentry);
>>>  free_misc:
>>> @@ -264,6 +328,7 @@ module_init(tdx_guest_init);
>>>
>>>  static void __exit tdx_guest_exit(void)
>>>  {
>>> +     unregister_guest_attest_ops(&tdx_attest_ops);
>>>       tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
>>>       free_quote_entry(qentry);
>>>       misc_deregister(&tdx_misc_dev);
>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
> 
> 
> 
> --
> -Dionna Glaze, PhD (she/her)

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer