2023-07-28 20:57:58

by Dan Williams

[permalink] [raw]
Subject: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

The bulk of the justification for this patch kit is in "[PATCH 1/4]
keys: Introduce tsm keys". The short summary is that the current
approach of adding new char devs and new ioctls, for what amounts to the
same functionality with minor formatting differences across vendors, is
untenable. Common concepts and the community benefit from common
infrastructure.

Use Keys to build common infrastructure for confidential computing
attestation report blobs, convert sevguest to use it (leaving the
deprecation question alone for now), and pave the way for tdx-guest and
the eventual risc-v equivalent to use it in lieu of new ioctls.

The sevguest conversion is only compile-tested.

This submission is To:David since he needs to sign-off on the idea of a
new Keys type, the rest is up to the confidential-computing driver
maintainers to adopt.

Changes from / credit for internal review:
- highlight copy_{to,from}_sockptr() as a common way to mix
copy_user() and memcpy() paths (Andy)
- add MODULE_DESCRIPTION() (Andy)
- clarify how the user-defined portion blob might be used (Elena)
- clarify the key instantiation options (Sathya)
- drop usage of a list for registering providers (Sathya)
- drop list.h include from tsm.h (Andy)
- add a comment for how TSM_DATA_MAX was derived (Andy)
- stop open coding kmemdup_nul() (Andy)
- add types.h to tsm.h (Andy)
- fix punctuation in comment (Andy)
- reorder security/keys/Makefile (Andy)
- add some missing includes to tsm.c (Andy)
- undo an 81 column clang-format line break (Andy)
- manually reflow tsm_token indentation (Andy)
- move allocations after input validation in tsm_instantiate() (Andy)
- switch to bin2hex() in tsm_read() (Andy)
- move init/exit declarations next to their functions (Andy)


---

Dan Williams (4):
keys: Introduce tsm keys
virt: sevguest: Prep for kernel internal {get,get_ext}_report()
mm/slab: Add __free() support for kvfree
virt: sevguest: Add TSM key support for SNP_{GET,GET_EXT}_REPORT


drivers/virt/coco/sev-guest/Kconfig | 2
drivers/virt/coco/sev-guest/sev-guest.c | 135 ++++++++++++++-
include/keys/tsm.h | 71 ++++++++
include/linux/slab.h | 2
security/keys/Kconfig | 12 +
security/keys/Makefile | 1
security/keys/tsm.c | 282 +++++++++++++++++++++++++++++++
7 files changed, 494 insertions(+), 11 deletions(-)
create mode 100644 include/keys/tsm.h
create mode 100644 security/keys/tsm.c

base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5


2023-07-28 21:25:52

by Dan Williams

[permalink] [raw]
Subject: [PATCH 4/4] virt: sevguest: Add TSM key support for SNP_{GET, GET_EXT}_REPORT

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.

Note, only compile-tested.

Link: http://lore.kernel.org/r/[email protected] [1]
Cc: Borislav Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Dionna Glaze <[email protected]>
Cc: Brijesh Singh <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
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;
+
+ 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);
+
+ 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";
+ }
+
+ return 0;
+}
+
+static const struct tsm_key_ops sev_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .module = THIS_MODULE,
+ .auth_new = sev_auth_new,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+ unregister_tsm_provider(&sev_tsm_ops);
+}
+
static int __init sev_guest_probe(struct platform_device *pdev)
{
struct snp_secrets_page_layout *layout;
@@ -842,6 +921,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 = register_tsm_provider(&sev_tsm_ops, snp_dev);
+ 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;


2023-07-28 21:25:56

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Fri Jul 28, 2023 at 7:30 PM UTC, Dan Williams wrote:
> The bulk of the justification for this patch kit is in "[PATCH 1/4]

/patch kit/patch set/

> keys: Introduce tsm keys". The short summary is that the current
> approach of adding new char devs and new ioctls, for what amounts to the
> same functionality with minor formatting differences across vendors, is
> untenable. Common concepts and the community benefit from common
> infrastructure.
>
> Use Keys to build common infrastructure for confidential computing

/Keys/Linux keyring/

> attestation report blobs, convert sevguest to use it (leaving the
> deprecation question alone for now), and pave the way for tdx-guest and
> the eventual risc-v equivalent to use it in lieu of new ioctls.
>
> The sevguest conversion is only compile-tested.
>
> This submission is To:David since he needs to sign-off on the idea of a
> new Keys type, the rest is up to the confidential-computing driver
> maintainers to adopt.
>
> Changes from / credit for internal review:
> - highlight copy_{to,from}_sockptr() as a common way to mix
> copy_user() and memcpy() paths (Andy)
> - add MODULE_DESCRIPTION() (Andy)
> - clarify how the user-defined portion blob might be used (Elena)
> - clarify the key instantiation options (Sathya)
> - drop usage of a list for registering providers (Sathya)
> - drop list.h include from tsm.h (Andy)
> - add a comment for how TSM_DATA_MAX was derived (Andy)
> - stop open coding kmemdup_nul() (Andy)
> - add types.h to tsm.h (Andy)
> - fix punctuation in comment (Andy)
> - reorder security/keys/Makefile (Andy)
> - add some missing includes to tsm.c (Andy)
> - undo an 81 column clang-format line break (Andy)
> - manually reflow tsm_token indentation (Andy)
> - move allocations after input validation in tsm_instantiate() (Andy)
> - switch to bin2hex() in tsm_read() (Andy)
> - move init/exit declarations next to their functions (Andy)
>
>
> ---
>
> Dan Williams (4):
> keys: Introduce tsm keys
> virt: sevguest: Prep for kernel internal {get,get_ext}_report()
> mm/slab: Add __free() support for kvfree
> virt: sevguest: Add TSM key support for SNP_{GET,GET_EXT}_REPORT
>
>
> drivers/virt/coco/sev-guest/Kconfig | 2
> drivers/virt/coco/sev-guest/sev-guest.c | 135 ++++++++++++++-
> include/keys/tsm.h | 71 ++++++++
> include/linux/slab.h | 2
> security/keys/Kconfig | 12 +
> security/keys/Makefile | 1
> security/keys/tsm.c | 282 +++++++++++++++++++++++++++++++
> 7 files changed, 494 insertions(+), 11 deletions(-)
> create mode 100644 include/keys/tsm.h
> create mode 100644 security/keys/tsm.c
>
> base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5

So how does this scale? Does it scale to TDX, SGX, TPM's or even TEE's
(ARM SM, RISC-V Keystone etc.). I'm not sure about the scope but we want
of course something that adapts to multiple use cases, right?

BR, Jarkko

2023-07-28 21:45:23

by Dan Williams

[permalink] [raw]
Subject: [PATCH 3/4] mm/slab: Add __free() support for kvfree

Allow for the declaration of variables that trigger kvfree() when they
go out of scope.

Cc: Andrew Morton <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
include/linux/slab.h | 2 ++
1 file changed, 2 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 848c7c82ad5a..241025367943 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -746,6 +746,8 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla
extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
__realloc_size(3);
extern void kvfree(const void *addr);
+DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
+
extern void kvfree_sensitive(const void *addr, size_t len);

unsigned int kmem_cache_size(struct kmem_cache *s);


2023-07-29 20:39:06

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> The bulk of the justification for this patch kit is in "[PATCH 1/4]
> keys: Introduce tsm keys". The short summary is that the current
> approach of adding new char devs and new ioctls, for what amounts to
> the same functionality with minor formatting differences across
> vendors, is untenable. Common concepts and the community benefit from
> common infrastructure.

I agree with this, but ...

> Use Keys to build common infrastructure for confidential computing
> attestation report blobs, convert sevguest to use it (leaving the
> deprecation question alone for now), and pave the way for tdx-guest
> and the eventual risc-v equivalent to use it in lieu of new ioctls.
>
> The sevguest conversion is only compile-tested.
>
> This submission is To:David since he needs to sign-off on the idea of
> a new Keys type, the rest is up to the confidential-computing driver
> maintainers to adopt.

So why is this a keys subsystem thing? The keys in question cannot be
used to do any key operations. It looks like a transport layer for
attestation reports rather than anything key like.

To give an analogy with the TPM: We do have a TPM interface to keys
because it can be used for things like sealing (TPM stores a symmetric
key) and even asymmetric operations (although TPM key support for that
in 1.2 was just removed). However, in direct analogy with confidential
computing: the TPM does have an attestation interface: TPM2_Quote and
TPM2_Certify (among others) which is deliberately *not* wired in to the
keys subsystem because the outputs are intended for external verifiers.

If the goal is to unify the interface for transporting attestation
reports, why not pull the attestation ioctls out of sevguest into
something common?

I also don't see in your interface where the nonce goes? Most
attestation reports combine the report output with a user supplied
nonce which gets added to the report signature to defend against
replay.

Finally, I can see the logic in using this to do key release, because
the external relying entity usually wishes to transport secrets into
the enclave, but the currently developing use case for that seems to be
to use a confidential guest vTPM because then we can use the existing
TPM disk key interfaces. Inventing something completely new isn't
going to fly because all consumers have to be updated to use it (even
though keys is a common interface, using key payloads isn't ... plus
the systemd TPM disk encryption key doesn't even use kernel keys, it
unwraps in userspace).

James


2023-07-30 05:21:34

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
> On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > The bulk of the justification for this patch kit is in "[PATCH 1/4]
> > keys: Introduce tsm keys". The short summary is that the current
> > approach of adding new char devs and new ioctls, for what amounts to
> > the same functionality with minor formatting differences across
> > vendors, is untenable. Common concepts and the community benefit from
> > common infrastructure.
>
> I agree with this, but ...
>
> > Use Keys to build common infrastructure for confidential computing
> > attestation report blobs, convert sevguest to use it (leaving the
> > deprecation question alone for now), and pave the way for tdx-guest
> > and the eventual risc-v equivalent to use it in lieu of new ioctls.
> >
> > The sevguest conversion is only compile-tested.
> >
> > This submission is To:David since he needs to sign-off on the idea of
> > a new Keys type, the rest is up to the confidential-computing driver
> > maintainers to adopt.
>
> So why is this a keys subsystem thing? The keys in question cannot be
> used to do any key operations. It looks like a transport layer for
> attestation reports rather than anything key like.

Yes, it has ended up as just a transport layer.

> To give an analogy with the TPM: We do have a TPM interface to keys
> because it can be used for things like sealing (TPM stores a symmetric
> key) and even asymmetric operations (although TPM key support for that
> in 1.2 was just removed). However, in direct analogy with confidential
> computing: the TPM does have an attestation interface: TPM2_Quote and
> TPM2_Certify (among others) which is deliberately *not* wired in to the
> keys subsystem because the outputs are intended for external verifiers.
>
> If the goal is to unify the interface for transporting attestation
> reports, why not pull the attestation ioctls out of sevguest into
> something common?

That's fair. I originally started out with a draft trusted-keys
implementation, but abandoned it because that really wants a vTPM
backend. There is no kernel consumer for attestation reports like other
key blobs, so that leaves either a key-type that is just a transport
layer or a new ABI.

I have a personal distaste for ioctls and the presence of user-defined
blobs in the Keyring subsystem made me think "why not just have a
key-type to convey the per-TSM attestation reports". Is that a fair
observation?

An ioctl interface would make sense for a common report format, but the
presence of per-TSM options and per-TSM format modifiers (like SEV
privilege level and "extended" attestation reports) attracted me to the
ability to just have "options" specified at report instantiation time.
I.e. like the options specified to trusted-key instantiation.

> I also don't see in your interface where the nonce goes? Most
> attestation reports combine the report output with a user supplied
> nonce which gets added to the report signature to defend against
> replay.

The user supplied data is another argument to instantiate the report
blob. The instantiation format is:

auth <ascii hex blob user data> [options]

...for example:

# dd if=/dev/urandom of=pubkey bs=1 count=64
# keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u

> Finally, I can see the logic in using this to do key release, because
> the external relying entity usually wishes to transport secrets into
> the enclave, but the currently developing use case for that seems to be
> to use a confidential guest vTPM because then we can use the existing
> TPM disk key interfaces. Inventing something completely new isn't
> going to fly because all consumers have to be updated to use it (even
> though keys is a common interface, using key payloads isn't ... plus
> the systemd TPM disk encryption key doesn't even use kernel keys, it
> unwraps in userspace).

I do think the eventual vTPM enabling is separate from this and I
mention that in the changelogs. That functionality like
SNP_GET_DERIVED_KEY is amenable to a trusted-keys frontend and being
unified with existing TPM paths.

This report interface on the other hand just needs a single ABI to
retrieve all these vendor formats (until industry standardization steps
in) and it needs to be flexible (within reason) for all the TSM-specific
options to be conveyed. I do not trust my ioctl ABI minefield avoidance
skills to get that right. Key blob instantiation feels up to the task.

2023-07-30 13:23:13

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Sat, 2023-07-29 at 21:56 -0700, Dan Williams wrote:
> James Bottomley wrote:
> > On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > > The bulk of the justification for this patch kit is in "[PATCH
> > > 1/4] keys: Introduce tsm keys". The short summary is that the
> > > current approach of adding new char devs and new ioctls, for what
> > > amounts to the same functionality with minor formatting
> > > differences across vendors, is untenable. Common concepts and the
> > > community benefit from common infrastructure.
> >
> > I agree with this, but ...
> >
> > > Use Keys to build common infrastructure for confidential
> > > computing attestation report blobs, convert sevguest to use it
> > > (leaving the deprecation question alone for now), and pave the
> > > way for tdx-guest and the eventual risc-v equivalent to use it in
> > > lieu of new ioctls.
> > >
> > > The sevguest conversion is only compile-tested.
> > >
> > > This submission is To:David since he needs to sign-off on the
> > > idea of a new Keys type, the rest is up to the confidential-
> > > computing driver maintainers to adopt.
> >
> > So why is this a keys subsystem thing?  The keys in question cannot
> > be used to do any key operations.  It looks like a transport layer
> > for attestation reports rather than anything key like.
>
> Yes, it has ended up as just a transport layer.
>
> > To give an analogy with the TPM: We do have a TPM interface to keys
> > because it can be used for things like sealing (TPM stores a
> > symmetric key) and even asymmetric operations (although TPM key
> > support for that in 1.2 was just removed).  However, in direct
> > analogy with confidential computing: the TPM does have an
> > attestation interface: TPM2_Quote and TPM2_Certify (among others)
> > which is deliberately *not* wired in to the keys subsystem because
> > the outputs are intended for external verifiers.
> >
> > If the goal is to unify the interface for transporting attestation
> > reports, why not pull the attestation ioctls out of sevguest into
> > something common?
>
> That's fair. I originally started out with a draft trusted-keys
> implementation, but abandoned it because that really wants a vTPM
> backend. There is no kernel consumer for attestation reports like
> other key blobs, so that leaves either a key-type that is just a
> transport layer or a new ABI.
>
> I have a personal distaste for ioctls and the presence of user-
> defined blobs in the Keyring subsystem made me think "why not just
> have a key-type to convey the per-TSM attestation reports". Is that a
> fair observation?

The trouble with this argument is that it's an argument for every new
ioctl becoming a key type. We have a ton of interfaces for
transporting information across the kernel to user boundary: sysfs,
filesystem, configfs, debugfs, etc ... although to be fair the
fashionably acceptable one does seem to change each year. Since
there's nothing really transactional about this, what about a simple
sysfs one? You echo in the nonce to a binary attribute and cat the
report. Any additional stuff, like the cert chain, can appear as
additional attributes?


> An ioctl interface would make sense for a common report format, but
> the presence of per-TSM options and per-TSM format modifiers (like
> SEV privilege level and "extended" attestation reports) attracted me
> to the ability to just have "options" specified at report
> instantiation time.

The "extended" report is nothing but a way of getting the signing key
cert chain. It's really just a glorified caching mechanism to relieve
the relying party from the job of doing the lookup themselves.

> I.e. like the options specified to trusted-key instantiation.
>
> > I also don't see in your interface where the nonce goes?  Most
> > attestation reports combine the report output with a user supplied
> > nonce which gets added to the report signature to defend against
> > replay.
>
> The user supplied data is another argument to instantiate the report
> blob. The instantiation format is:
>
>     auth <ascii hex blob user data> [options]
>
> ...for example:
>
>     # dd if=/dev/urandom of=pubkey bs=1 count=64
>     # keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey)
> privlevel=2" @u
>
> > Finally, I can see the logic in using this to do key release,
> > because the external relying entity usually wishes to transport
> > secrets into the enclave, but the currently developing use case for
> > that seems to be to use a confidential guest vTPM because then we
> > can use the existing TPM disk key interfaces.  Inventing something
> > completely new isn't going to fly because all consumers have to be
> > updated to use it (even though keys is a common interface, using
> > key payloads isn't ... plus the systemd TPM disk encryption key
> > doesn't even use kernel keys, it unwraps in userspace).
>
> I do think the eventual vTPM enabling is separate from this and I
> mention that in the changelogs.

vTPM requires no enabling: it will just work with the existing trusted
key interface.

> That functionality like SNP_GET_DERIVED_KEY is amenable to a
> trusted-keys frontend and being unified with existing TPM paths.

To get a bit off topic, I'm not sure derived keys are much use. The
problem is in SNP that by the time the PSP does the derivation, the key
is both tied to the physical system and derived from a measurement too
general to differentiate between VM images (so one VM could read
another VMs stored secrets).

>
> This report interface on the other hand just needs a single ABI to
> retrieve all these vendor formats (until industry standardization
> steps in) and it needs to be flexible (within reason) for all the
> TSM-specific options to be conveyed. I do not trust my ioctl ABI
> minefield avoidance skills to get that right. Key blob instantiation
> feels up to the task.

To repeat: there's nothing keylike about it.

If you think that the keyctl mechanism for transporting information
across the kernel boundary should be generalised and presented as an
alternative to our fashion of the year interface for this, then that's
what you should do (and, I'm afraid to add, cc all the other
opinionated people who've also produced the flavour of the year
interfaces). Sneaking it in as a one-off is the wrong way to proceed
on something like this.

James



2023-07-31 19:18:43

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
> On Sat, 2023-07-29 at 21:56 -0700, Dan Williams wrote:
> > James Bottomley wrote:
> > > On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > > > The bulk of the justification for this patch kit is in "[PATCH
> > > > 1/4] keys: Introduce tsm keys". The short summary is that the
> > > > current approach of adding new char devs and new ioctls, for what
> > > > amounts to the same functionality with minor formatting
> > > > differences across vendors, is untenable. Common concepts and the
> > > > community benefit from common infrastructure.
> > >
> > > I agree with this, but ...
> > >
> > > > Use Keys to build common infrastructure for confidential
> > > > computing attestation report blobs, convert sevguest to use it
> > > > (leaving the deprecation question alone for now), and pave the
> > > > way for tdx-guest and the eventual risc-v equivalent to use it in
> > > > lieu of new ioctls.
> > > >
> > > > The sevguest conversion is only compile-tested.
> > > >
> > > > This submission is To:David since he needs to sign-off on the
> > > > idea of a new Keys type, the rest is up to the confidential-
> > > > computing driver maintainers to adopt.
> > >
> > > So why is this a keys subsystem thing?? The keys in question cannot
> > > be used to do any key operations.? It looks like a transport layer
> > > for attestation reports rather than anything key like.
> >
> > Yes, it has ended up as just a transport layer.
> >
> > > To give an analogy with the TPM: We do have a TPM interface to keys
> > > because it can be used for things like sealing (TPM stores a
> > > symmetric key) and even asymmetric operations (although TPM key
> > > support for that in 1.2 was just removed).? However, in direct
> > > analogy with confidential computing: the TPM does have an
> > > attestation interface: TPM2_Quote and TPM2_Certify (among others)
> > > which is deliberately *not* wired in to the keys subsystem because
> > > the outputs are intended for external verifiers.
> > >
> > > If the goal is to unify the interface for transporting attestation
> > > reports, why not pull the attestation ioctls out of sevguest into
> > > something common?
> >
> > That's fair. I originally started out with a draft trusted-keys
> > implementation, but abandoned it because that really wants a vTPM
> > backend. There is no kernel consumer for attestation reports like
> > other key blobs, so that leaves either a key-type that is just a
> > transport layer or a new ABI.
> >
> > I have a personal distaste for ioctls and the presence of user-
> > defined blobs in the Keyring subsystem made me think "why not just
> > have a key-type to convey the per-TSM attestation reports". Is that a
> > fair observation?
>
> The trouble with this argument is that it's an argument for every new
> ioctl becoming a key type.

Yeah, that's a danger, I don't want Linux keyring to become the blob
transporter subsystem.

While this usage is "security" adjacent the precedent is not a great
one.

> We have a ton of interfaces for transporting information across the
> kernel to user boundary: sysfs, filesystem, configfs, debugfs, etc ...
> although to be fair the fashionably acceptable one does seem to change
> each year. Since there's nothing really transactional about this,
> what about a simple sysfs one? You echo in the nonce to a binary
> attribute and cat the report. Any additional stuff, like the cert
> chain, can appear as additional attributes?

That should be straightforward to mock up and it keeps the property I
like of common ABI with optional per-TSM modifiers.

> > An ioctl interface would make sense for a common report format, but
> > the presence of per-TSM options and per-TSM format modifiers (like
> > SEV privilege level and "extended" attestation reports) attracted me
> > to the ability to just have "options" specified at report
> > instantiation time.
>
> The "extended" report is nothing but a way of getting the signing key
> cert chain. It's really just a glorified caching mechanism to relieve
> the relying party from the job of doing the lookup themselves.
>
> > I.e. like the options specified to trusted-key instantiation.
> >
> > > I also don't see in your interface where the nonce goes?? Most
> > > attestation reports combine the report output with a user supplied
> > > nonce which gets added to the report signature to defend against
> > > replay.
> >
> > The user supplied data is another argument to instantiate the report
> > blob. The instantiation format is:
> >
> > ??? auth <ascii hex blob user data> [options]
> >
> > ...for example:
> >
> > ??? # dd if=/dev/urandom of=pubkey bs=1 count=64
> > ??? # keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey)
> > privlevel=2" @u
> >
> > > Finally, I can see the logic in using this to do key release,
> > > because the external relying entity usually wishes to transport
> > > secrets into the enclave, but the currently developing use case for
> > > that seems to be to use a confidential guest vTPM because then we
> > > can use the existing TPM disk key interfaces.? Inventing something
> > > completely new isn't going to fly because all consumers have to be
> > > updated to use it (even though keys is a common interface, using
> > > key payloads isn't ... plus the systemd TPM disk encryption key
> > > doesn't even use kernel keys, it unwraps in userspace).
> >
> > I do think the eventual vTPM enabling is separate from this and I
> > mention that in the changelogs.
>
> vTPM requires no enabling: it will just work with the existing trusted
> key interface.

Oh, I had not seen a TSM implemenetation that presented an TPM
API-interface so I had been thining one had to be built around
facilities like derived keys. I agree the best vTPM is just a TPM.

> > That functionality like SNP_GET_DERIVED_KEY is amenable to a
> > trusted-keys frontend and being unified with existing TPM paths.
>
> To get a bit off topic, I'm not sure derived keys are much use. The
> problem is in SNP that by the time the PSP does the derivation, the key
> is both tied to the physical system and derived from a measurement too
> general to differentiate between VM images (so one VM could read
> another VMs stored secrets).
>
> >
> > This report interface on the other hand just needs a single ABI to
> > retrieve all these vendor formats (until industry standardization
> > steps in) and it needs to be flexible (within reason) for all the
> > TSM-specific options to be conveyed. I do not trust my ioctl ABI
> > minefield avoidance skills to get that right. Key blob instantiation
> > feels up to the task.
>
> To repeat: there's nothing keylike about it.
>
> If you think that the keyctl mechanism for transporting information
> across the kernel boundary should be generalised and presented as an
> alternative to our fashion of the year interface for this, then that's
> what you should do (and, I'm afraid to add, cc all the other
> opinionated people who've also produced the flavour of the year
> interfaces). Sneaking it in as a one-off is the wrong way to proceed
> on something like this.

Fair enough, I'll take a look at the sysfs conversion and we can go from
there.

2023-07-31 19:31:08

by Dan Williams

[permalink] [raw]
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 <[email protected]> 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/[email protected] [1]
> > Cc: Borislav Petkov <[email protected]>
> > Cc: Tom Lendacky <[email protected]>
> > Cc: Dionna Glaze <[email protected]>
> > Cc: Brijesh Singh <[email protected]>
> > Signed-off-by: Dan Williams <[email protected]>
> > ---
> > 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.

2023-07-31 19:53:57

by Peter Gonda

[permalink] [raw]
Subject: Re: [PATCH 4/4] virt: sevguest: Add TSM key support for SNP_{GET, GET_EXT}_REPORT

> > >
> > > +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.

Great, we can then name this const.

Yes that's why typically the public key, any context, and nonce would
be hashed. Then we can include the digest into the report.

>
> >
> > > +
> > > + 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.

The way this is currently setup suggests that a user should add a
pubkey with the 'keyctl add tsm ...'. But if a user does this as
described here it won't allow them to set up a secure protocol with a
remote entity.

I think a user could abuse the naming of this system to do the correct
thing by using 'keyctl add tsm ..' over data which is not a public key
and is instead a digest of some public key with additional protocol
data.

2023-07-31 20:04:41

by Peter Gonda

[permalink] [raw]
Subject: Re: [PATCH 4/4] virt: sevguest: Add TSM key support for SNP_{GET, GET_EXT}_REPORT

On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <[email protected]> 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

>
> Note, only compile-tested.
>
> Link: http://lore.kernel.org/r/[email protected] [1]
> Cc: Borislav Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Dionna Glaze <[email protected]>
> Cc: Brijesh Singh <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> 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?

> +
> + 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?

> +
> + 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?

2023-08-01 12:51:26

by Huang, Kai

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Sun, 2023-07-30 at 08:59 -0400, James Bottomley wrote:
> On Sat, 2023-07-29 at 21:56 -0700, Dan Williams wrote:
> > James Bottomley wrote:
> > > On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > > > The bulk of the justification for this patch kit is in "[PATCH
> > > > 1/4] keys: Introduce tsm keys". The short summary is that the
> > > > current approach of adding new char devs and new ioctls, for what
> > > > amounts to the same functionality with minor formatting
> > > > differences across vendors, is untenable. Common concepts and the
> > > > community benefit from common infrastructure.
> > >
> > > I agree with this, but ...
> > >
> > > > Use Keys to build common infrastructure for confidential
> > > > computing attestation report blobs, convert sevguest to use it
> > > > (leaving the deprecation question alone for now), and pave the
> > > > way for tdx-guest and the eventual risc-v equivalent to use it in
> > > > lieu of new ioctls.
> > > >
> > > > The sevguest conversion is only compile-tested.
> > > >
> > > > This submission is To:David since he needs to sign-off on the
> > > > idea of a new Keys type, the rest is up to the confidential-
> > > > computing driver maintainers to adopt.
> > >
> > > So why is this a keys subsystem thing?  The keys in question cannot
> > > be used to do any key operations.  It looks like a transport layer
> > > for attestation reports rather than anything key like.
> >
> > Yes, it has ended up as just a transport layer.
> >
> > > To give an analogy with the TPM: We do have a TPM interface to keys
> > > because it can be used for things like sealing (TPM stores a
> > > symmetric key) and even asymmetric operations (although TPM key
> > > support for that in 1.2 was just removed).  However, in direct
> > > analogy with confidential computing: the TPM does have an
> > > attestation interface: TPM2_Quote and TPM2_Certify (among others)
> > > which is deliberately *not* wired in to the keys subsystem because
> > > the outputs are intended for external verifiers.
> > >
> > > If the goal is to unify the interface for transporting attestation
> > > reports, why not pull the attestation ioctls out of sevguest into
> > > something common?
> >
> > That's fair. I originally started out with a draft trusted-keys
> > implementation, but abandoned it because that really wants a vTPM
> > backend. There is no kernel consumer for attestation reports like
> > other key blobs, so that leaves either a key-type that is just a
> > transport layer or a new ABI.
> >
> > I have a personal distaste for ioctls and the presence of user-
> > defined blobs in the Keyring subsystem made me think "why not just
> > have a key-type to convey the per-TSM attestation reports". Is that a
> > fair observation?
>
> The trouble with this argument is that it's an argument for every new
> ioctl becoming a key type. We have a ton of interfaces for
> transporting information across the kernel to user boundary: sysfs,
> filesystem, configfs, debugfs, etc ... although to be fair the
> fashionably acceptable one does seem to change each year. Since
> there's nothing really transactional about this, what about a simple
> sysfs one? You echo in the nonce to a binary attribute and cat the
> report. Any additional stuff, like the cert chain, can appear as
> additional attributes?
>

Sorry perhaps a dumb question to ask:

As it has been adequately put, the remote verifiable report normally contains a
nonce. For instance, it can be a per-session or per-request nonce from the
remote verification service to the confidential VM.  

IIUC, exposing attestation report via /sysfs means many processes (in the
confidential VM) can potentially see the report and the nonce.  My question is
whether such nonce should be considered as a secret thus should be only visible
to the process which is responsible for talking to the remote verification
service? Using IOCTL seems can avoid such exposure.

Probably exposing nonce is fine, but I don't know.

In fact, I raised whether we should use /sysfs to get TDX's TDREPORT (which can
only be verified on local machine, thus needs to be singed as a Quote by the SGX
Quoting Enclave) when we were upstreaming (the first part of) TDX attestation:

https://lore.kernel.org/lkml/20220501183500.2242828-1-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m18fd5167dfa32c4702dd6b4bd472ad9e8f579ad8

Quote the relevant part here:

>
> Implement a basic attestation driver to allow TD userspace to get the
> TDREPORT, which is sent to QE by the attestation software to generate
> a Quote for remote verification.
>
> Also note that explicit access permissions are not enforced in this
> driver because the quote and measurements are not a secret. However
> the access permissions of the device node can be used to set any
> desired access policy. The udev default is usually root access
> only.

The IOCTL vs /sysfs isn't discussed.

For instance, after rough thinking, why is the IOCTL better than below approach
using /sysfs?

echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
cat /sys/kernel/coco/tdx/attest/tdreport

Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.

The benefit of using IOCTL I can think of now is it is perhaps more secure, as
with IOCTL the REPORTDATA and the TDREPORT is visible to the process which calls
the IOCTL, while using the /sysfs they are potentially visible to any process.
Especially the REPORTDATA, i.e. it can come from attestation service after the
TD attestation agent sets up a secure connection with it.




2023-08-01 13:56:55

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
[...]
>
> Sorry perhaps a dumb question to ask:
>
> As it has been adequately put, the remote verifiable report normally
> contains a nonce.  For instance, it can be a per-session or per-
> request nonce from the remote verification service to the
> confidential VM.  
>
> IIUC, exposing attestation report via /sysfs means many processes (in
> the confidential VM) can potentially see the report and the nonce. 
> My question is whether such nonce should be considered as a secret
> thus should be only visible to the process which is responsible for
> talking to the remote verification service?  Using IOCTL seems can
> avoid such exposure.

OK, so the nonce seems to be a considerably misunderstood piece of this
(and not just by you), so I'll try to go over carefully what it is and
why. The problem we have in pretty much any signature based
attestation evidence scheme is when I, the attesting party, present the
signed evidence to you, the relying part, how do you know I got it
today from the system in question not five days ago when I happen to
have engineered the correct conditions? The solution to this currency
problem is to incorporate a challenge supplied by the relying party
(called a nonce) into the signature. The nonce must be unpredictable
enough that the attesting party can't guess it beforehand and it must
be unique so that the attesting party can't go through its records and
find an attestation signature with the same nonce and supply that
instead.

This property of unpredictability and uniqueness is usually satisfied
simply by sending a random number. However, as you can also see, since
the nonce is supplied by the relying party to the attesting party, it
eventually gets known to both, so can't be a secret to one or the
other. Because of the unpredictability requirement, it's generally
frowned on to have nonces based on anything other than random numbers,
because that might lead to predictability.

James


2023-08-01 13:57:49

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-01 at 08:03 -0400, James Bottomley wrote:
> On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
> [...]
> >
> > Sorry perhaps a dumb question to ask:
> >
> > As it has been adequately put, the remote verifiable report
> > normally contains a nonce.  For instance, it can be a per-session
> > or per-request nonce from the remote verification service to the
> > confidential VM.  
> >
> > IIUC, exposing attestation report via /sysfs means many processes
> > (in the confidential VM) can potentially see the report and the
> > nonce. My question is whether such nonce should be considered as a
> > secret thus should be only visible to the process which is
> > responsible for talking to the remote verification service?  Using
> > IOCTL seems can avoid such exposure.
>
> OK, so the nonce seems to be a considerably misunderstood piece of
> this (and not just by you), so I'll try to go over carefully what it
> is and why.  The problem we have in pretty much any signature based
> attestation evidence scheme is when I, the attesting party, present
> the signed evidence to you, the relying part, how do you know I got
> it today from the system in question not five days ago when I happen
> to have engineered the correct conditions?  The solution to this
> currency problem is to incorporate a challenge supplied by the
> relying party (called a nonce) into the signature.  The nonce must be
> unpredictable enough that the attesting party can't guess it
> beforehand and it must be unique so that the attesting party can't go
> through its records and find an attestation signature with the same
> nonce and supply that instead.
>
> This property of unpredictability and uniqueness is usually satisfied
> simply by sending a random number.  However, as you can also see,
> since the nonce is supplied by the relying party to the attesting
> party, it eventually gets known to both, so can't be a secret to one
> or the other.  Because of the unpredictability requirement, it's
> generally frowned on to have nonces based on anything other than
> random numbers, because that might lead to predictability.

I suppose there is a situation where you use the nonce to bind other
details of the attesting party. For instance, in confidential
computing, the parties often exchange secrets after successful
attestation. To do this, the attesting party generates an ephemeral
public key. It then communicates the key binding by constructing a new
nonce as

<new nonce> = hash( <relying party nonce> || <public key> )

and using that new nonce in the attestation report signature.

So the relying party can also reconstruct the new nonce (if it knows
the key) and verify that it has a current attestation report *and* that
the attesting party wants secrets encrypted to <public key>. This
scheme does rely on the fact that the thing generating the attestation
signature must only send reports to the owner of the enclave, so that
untrusted third parties, like the host owner, can't generate a report
with their own nonce and thus fake out the key exchange.

James


2023-08-02 01:16:12

by Huang, Kai

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-01 at 08:30 -0400, James Bottomley wrote:
> On Tue, 2023-08-01 at 08:03 -0400, James Bottomley wrote:
> > On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
> > [...]
> > >
> > > Sorry perhaps a dumb question to ask:
> > >
> > > As it has been adequately put, the remote verifiable report
> > > normally contains a nonce.  For instance, it can be a per-session
> > > or per-request nonce from the remote verification service to the
> > > confidential VM.  
> > >
> > > IIUC, exposing attestation report via /sysfs means many processes
> > > (in the confidential VM) can potentially see the report and the
> > > nonce. My question is whether such nonce should be considered as a
> > > secret thus should be only visible to the process which is
> > > responsible for talking to the remote verification service?  Using
> > > IOCTL seems can avoid such exposure.
> >
> > OK, so the nonce seems to be a considerably misunderstood piece of
> > this (and not just by you), so I'll try to go over carefully what it
> > is and why.  The problem we have in pretty much any signature based
> > attestation evidence scheme is when I, the attesting party, present
> > the signed evidence to you, the relying part, how do you know I got
> > it today from the system in question not five days ago when I happen
> > to have engineered the correct conditions?  The solution to this
> > currency problem is to incorporate a challenge supplied by the
> > relying party (called a nonce) into the signature.  The nonce must be
> > unpredictable enough that the attesting party can't guess it
> > beforehand and it must be unique so that the attesting party can't go
> > through its records and find an attestation signature with the same
> > nonce and supply that instead.
> >
> > This property of unpredictability and uniqueness is usually satisfied
> > simply by sending a random number.  However, as you can also see,
> > since the nonce is supplied by the relying party to the attesting
> > party, it eventually gets known to both, so can't be a secret to one
> > or the other.  Because of the unpredictability requirement, it's
> > generally frowned on to have nonces based on anything other than
> > random numbers, because that might lead to predictability.

Thanks for explaining!

So in other words, in general nonce shouldn't be a secret due to it's
unpredictability, thus using /sysfs to expose attestation report should be OK?

>
> I suppose there is a situation where you use the nonce to bind other
> details of the attesting party. For instance, in confidential
> computing, the parties often exchange secrets after successful
> attestation. To do this, the attesting party generates an ephemeral
> public key. It then communicates the key binding by constructing a new
> nonce as
>
> <new nonce> = hash( <relying party nonce> || <public key> )
>
> and using that new nonce in the attestation report signature.

This looks like taking advantage of the attestation flow to carry additional
info that can be communicated _after_ attestation is done. Not sure the
benefit? For instance, shouldn't we normally use symmetric key for exchanging
secrets after attestation?

>
> So the relying party can also reconstruct the new nonce (if it knows
> the key) and verify that it has a current attestation report *and* that
> the attesting party wants secrets encrypted to <public key>. This
> scheme does rely on the fact that the thing generating the attestation
> signature must only send reports to the owner of the enclave, so that
> untrusted third parties, like the host owner, can't generate a report
> with their own nonce and thus fake out the key exchange.

Sorry I am not sure I am following this. For TDX only the confidential VM can
put the nonce to the report (because the specific instruction to get the local-
verifiable report out from firmware can only be made from the confidential VM).
Not sure other vendors' implementations though.

2023-08-02 13:26:36

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Wed, 2023-08-02 at 00:10 +0000, Huang, Kai wrote:
> On Tue, 2023-08-01 at 08:30 -0400, James Bottomley wrote:
> > On Tue, 2023-08-01 at 08:03 -0400, James Bottomley wrote:
> > > On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
> > > [...]
> > > >
> > > > Sorry perhaps a dumb question to ask:
> > > >
> > > > As it has been adequately put, the remote verifiable report
> > > > normally contains a nonce.  For instance, it can be a per-
> > > > session or per-request nonce from the remote verification
> > > > service to the confidential VM.  
> > > >
> > > > IIUC, exposing attestation report via /sysfs means many
> > > > processes (in the confidential VM) can potentially see the
> > > > report and the nonce. My question is whether such nonce should
> > > > be considered as a secret thus should be only visible to the
> > > > process which is responsible for talking to the remote
> > > > verification service? 
> > > > Using IOCTL seems can avoid such exposure.
> > >
> > > OK, so the nonce seems to be a considerably misunderstood piece
> > > of this (and not just by you), so I'll try to go over carefully
> > > what it is and why.  The problem we have in pretty much any
> > > signature based attestation evidence scheme is when I, the
> > > attesting party, present the signed evidence to you, the relying
> > > part, how do you know I got it today from the system in question
> > > not five days ago when I happen to have engineered the correct
> > > conditions?  The solution to this currency problem is to
> > > incorporate a challenge supplied by the relying party (called a
> > > nonce) into the signature.  The nonce must be unpredictable
> > > enough that the attesting party can't guess it beforehand and it
> > > must be unique so that the attesting party can't go through its
> > > records and find an attestation signature with the same
> > > nonce and supply that instead.
> > >
> > > This property of unpredictability and uniqueness is usually
> > > satisfied simply by sending a random number.  However, as you can
> > > also see, since the nonce is supplied by the relying party to the
> > > attesting party, it eventually gets known to both, so can't be a
> > > secret to one or the other.  Because of the unpredictability
> > > requirement, it's generally frowned on to have nonces based on
> > > anything other than random numbers, because that might lead to
> > > predictability.
>
> Thanks for explaining!
>
> So in other words, in general nonce shouldn't be a secret due to it's
> unpredictability, thus using /sysfs to expose attestation report
> should be OK?

There's no reason I can think of it should be secret (well, except
security through obscurity in case someone is monitoring for a replay).

> > I suppose there is a situation where you use the nonce to bind
> > other details of the attesting party.  For instance, in
> > confidential computing, the parties often exchange secrets after
> > successful attestation.  To do this, the attesting party generates
> > an ephemeral public key.  It then communicates the key binding by
> > constructing a new nonce as
> >
> > <new nonce> = hash( <relying party nonce> || <public key> )
> >
> > and using that new nonce in the attestation report signature.
>
> This looks like taking advantage of the attestation flow to carry
> additional info that can be communicated _after_ attestation is done.

Well, no, the <new nonce> must be part of the attestation report.

>   Not sure the benefit?  For instance, shouldn't we normally use
> symmetric key for exchanging secrets after attestation?

Yes, but how do you get the symmetric key? A pre-chosen symmetric key
would have to be in the enclave as an existing secret, which can't be
done if you have to provision secrets. The way around this is to use a
key agreement to generate a symmetric key on the fly. The problem,
when you are doing things like Diffie Hellman Ephemeral (DHE) to give
you this transport encryption key is that of endpoint verification.
You can provision a public certificate in the enclave to verify the
remote (so a malicious remote can't inject false secrets), but the
remote needs some assurance that it has established communication with
the correct local (otherwise it would give up its secrets to anyone).
A binding of the local public DHE key to the attestation report can do
this.

> > So the relying party can also reconstruct the new nonce (if it
> > knows the key) and verify that it has a current attestation report
> > *and* that the attesting party wants secrets encrypted to <public
> > key>.  This scheme does rely on the fact that the thing generating
> > the attestation signature must only send reports to the owner of
> > the enclave, so that untrusted third parties, like the host owner,
> > can't generate a report with their own nonce and thus fake out the
> > key exchange.
>
> Sorry I am not sure I am following this.

If you use an attestation report for binding, you have to be sure no
third party could generate the report and give a false binding.

For instance, this isn't true of a TPM2_Quote because anyone who can
get into the tss group can generate one.

James


>   For TDX only the confidential VM can put the nonce to the report
> (because the specific instruction to get the local-verifiable report
> out from firmware can only be made from the confidential VM).
> Not sure other vendors' implementations though.
>


2023-08-03 00:10:20

by Huang, Kai

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Wed, 2023-08-02 at 08:41 -0400, James Bottomley wrote:
> On Wed, 2023-08-02 at 00:10 +0000, Huang, Kai wrote:
> > On Tue, 2023-08-01 at 08:30 -0400, James Bottomley wrote:
> > > On Tue, 2023-08-01 at 08:03 -0400, James Bottomley wrote:
> > > > On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
> > > > [...]
> > > > >
> > > > > Sorry perhaps a dumb question to ask:
> > > > >
> > > > > As it has been adequately put, the remote verifiable report
> > > > > normally contains a nonce.  For instance, it can be a per-
> > > > > session or per-request nonce from the remote verification
> > > > > service to the confidential VM.  
> > > > >
> > > > > IIUC, exposing attestation report via /sysfs means many
> > > > > processes (in the confidential VM) can potentially see the
> > > > > report and the nonce. My question is whether such nonce should
> > > > > be considered as a secret thus should be only visible to the
> > > > > process which is responsible for talking to the remote
> > > > > verification service? 
> > > > > Using IOCTL seems can avoid such exposure.
> > > >
> > > > OK, so the nonce seems to be a considerably misunderstood piece
> > > > of this (and not just by you), so I'll try to go over carefully
> > > > what it is and why.  The problem we have in pretty much any
> > > > signature based attestation evidence scheme is when I, the
> > > > attesting party, present the signed evidence to you, the relying
> > > > part, how do you know I got it today from the system in question
> > > > not five days ago when I happen to have engineered the correct
> > > > conditions?  The solution to this currency problem is to
> > > > incorporate a challenge supplied by the relying party (called a
> > > > nonce) into the signature.  The nonce must be unpredictable
> > > > enough that the attesting party can't guess it beforehand and it
> > > > must be unique so that the attesting party can't go through its
> > > > records and find an attestation signature with the same
> > > > nonce and supply that instead.
> > > >
> > > > This property of unpredictability and uniqueness is usually
> > > > satisfied simply by sending a random number.  However, as you can
> > > > also see, since the nonce is supplied by the relying party to the
> > > > attesting party, it eventually gets known to both, so can't be a
> > > > secret to one or the other.  Because of the unpredictability
> > > > requirement, it's generally frowned on to have nonces based on
> > > > anything other than random numbers, because that might lead to
> > > > predictability.
> >
> > Thanks for explaining!
> >
> > So in other words, in general nonce shouldn't be a secret due to it's
> > unpredictability, thus using /sysfs to expose attestation report
> > should be OK?
>
> There's no reason I can think of it should be secret (well, except
> security through obscurity in case someone is monitoring for a replay).

Thanks.

>
> > > I suppose there is a situation where you use the nonce to bind
> > > other details of the attesting party.  For instance, in
> > > confidential computing, the parties often exchange secrets after
> > > successful attestation.  To do this, the attesting party generates
> > > an ephemeral public key.  It then communicates the key binding by
> > > constructing a new nonce as
> > >
> > > <new nonce> = hash( <relying party nonce> || <public key> )
> > >
> > > and using that new nonce in the attestation report signature.
> >
> > This looks like taking advantage of the attestation flow to carry
> > additional info that can be communicated _after_ attestation is done.
>
> Well, no, the <new nonce> must be part of the attestation report.
>
> >   Not sure the benefit?  For instance, shouldn't we normally use
> > symmetric key for exchanging secrets after attestation?
>
> Yes, but how do you get the symmetric key? A pre-chosen symmetric key
> would have to be in the enclave as an existing secret, which can't be
> done if you have to provision secrets. The way around this is to use a
> key agreement to generate a symmetric key on the fly. The problem,
> when you are doing things like Diffie Hellman Ephemeral (DHE) to give
> you this transport encryption key is that of endpoint verification.
> You can provision a public certificate in the enclave to verify the
> remote (so a malicious remote can't inject false secrets), but the
> remote needs some assurance that it has established communication with
> the correct local (otherwise it would give up its secrets to anyone).
> A binding of the local public DHE key to the attestation report can do
> this.
>

Based on my limit cryptography knowledge I guess you mean using attestation flow
for mutual authentication? I was thinking we already have a TLS connection
established and attestation is to make sure the attesting party is truly the one
but not someone who is compromised. Anyway thanks a lot for explaining!

> >

2023-08-04 03:57:28

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

Huang, Kai wrote:
> On Sun, 2023-07-30 at 08:59 -0400, James Bottomley wrote:
> > On Sat, 2023-07-29 at 21:56 -0700, Dan Williams wrote:
> > > James Bottomley wrote:
> > > > On Fri, 2023-07-28 at 12:30 -0700, Dan Williams wrote:
> > > > > The bulk of the justification for this patch kit is in "[PATCH
> > > > > 1/4] keys: Introduce tsm keys". The short summary is that the
> > > > > current approach of adding new char devs and new ioctls, for what
> > > > > amounts to the same functionality with minor formatting
> > > > > differences across vendors, is untenable. Common concepts and the
> > > > > community benefit from common infrastructure.
> > > >
> > > > I agree with this, but ...
> > > >
> > > > > Use Keys to build common infrastructure for confidential
> > > > > computing attestation report blobs, convert sevguest to use it
> > > > > (leaving the deprecation question alone for now), and pave the
> > > > > way for tdx-guest and the eventual risc-v equivalent to use it in
> > > > > lieu of new ioctls.
> > > > >
> > > > > The sevguest conversion is only compile-tested.
> > > > >
> > > > > This submission is To:David since he needs to sign-off on the
> > > > > idea of a new Keys type, the rest is up to the confidential-
> > > > > computing driver maintainers to adopt.
> > > >
> > > > So why is this a keys subsystem thing?? The keys in question cannot
> > > > be used to do any key operations.? It looks like a transport layer
> > > > for attestation reports rather than anything key like.
> > >
> > > Yes, it has ended up as just a transport layer.
> > >
> > > > To give an analogy with the TPM: We do have a TPM interface to keys
> > > > because it can be used for things like sealing (TPM stores a
> > > > symmetric key) and even asymmetric operations (although TPM key
> > > > support for that in 1.2 was just removed).? However, in direct
> > > > analogy with confidential computing: the TPM does have an
> > > > attestation interface: TPM2_Quote and TPM2_Certify (among others)
> > > > which is deliberately *not* wired in to the keys subsystem because
> > > > the outputs are intended for external verifiers.
> > > >
> > > > If the goal is to unify the interface for transporting attestation
> > > > reports, why not pull the attestation ioctls out of sevguest into
> > > > something common?
> > >
> > > That's fair. I originally started out with a draft trusted-keys
> > > implementation, but abandoned it because that really wants a vTPM
> > > backend. There is no kernel consumer for attestation reports like
> > > other key blobs, so that leaves either a key-type that is just a
> > > transport layer or a new ABI.
> > >
> > > I have a personal distaste for ioctls and the presence of user-
> > > defined blobs in the Keyring subsystem made me think "why not just
> > > have a key-type to convey the per-TSM attestation reports". Is that a
> > > fair observation?
> >
> > The trouble with this argument is that it's an argument for every new
> > ioctl becoming a key type. We have a ton of interfaces for
> > transporting information across the kernel to user boundary: sysfs,
> > filesystem, configfs, debugfs, etc ... although to be fair the
> > fashionably acceptable one does seem to change each year. Since
> > there's nothing really transactional about this, what about a simple
> > sysfs one? You echo in the nonce to a binary attribute and cat the
> > report. Any additional stuff, like the cert chain, can appear as
> > additional attributes?
> >
>
> Sorry perhaps a dumb question to ask:
>
> As it has been adequately put, the remote verifiable report normally contains a
> nonce. For instance, it can be a per-session or per-request nonce from the
> remote verification service to the confidential VM. ?
>
> IIUC, exposing attestation report via /sysfs means many processes (in the
> confidential VM) can potentially see the report and the nonce.? My question is
> whether such nonce should be considered as a secret thus should be only visible
> to the process which is responsible for talking to the remote verification
> service? Using IOCTL seems can avoid such exposure.
>
> Probably exposing nonce is fine, but I don't know.
>
> In fact, I raised whether we should use /sysfs to get TDX's TDREPORT (which can
> only be verified on local machine, thus needs to be singed as a Quote by the SGX
> Quoting Enclave) when we were upstreaming (the first part of) TDX attestation:
>
> https://lore.kernel.org/lkml/20220501183500.2242828-1-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m18fd5167dfa32c4702dd6b4bd472ad9e8f579ad8
>
> Quote the relevant part here:
>
> >
> > Implement a basic attestation driver to allow TD userspace to get the
> > TDREPORT, which is sent to QE by the attestation software to generate
> > a Quote for remote verification.
> >
> > Also note that explicit access permissions are not enforced in this
> > driver because the quote and measurements are not a secret. However
> > the access permissions of the device node can be used to set any
> > desired access policy. The udev default is usually root access
> > only.
>
> The IOCTL vs /sysfs isn't discussed.
>
> For instance, after rough thinking, why is the IOCTL better than below approach
> using /sysfs?
>
> echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
> cat /sys/kernel/coco/tdx/attest/tdreport
>
> Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
> TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.
>
> The benefit of using IOCTL I can think of now is it is perhaps more secure, as
> with IOCTL the REPORTDATA and the TDREPORT is visible to the process which calls
> the IOCTL, while using the /sysfs they are potentially visible to any process.
> Especially the REPORTDATA, i.e. it can come from attestation service after the
> TD attestation agent sets up a secure connection with it.

James and Dionna answered the nonce question. The kernel could enforce
"nonce || pubkey" where only pubkey is user provided. It's a
contract that the kernel need not enforce, but maybe it should.

As for sysfs and multiple requesters it is indeed awkward especially
with the suggestion that this is not a configure once and done after
establishing a channel with the attestation agent. That said the kernel
gets to pick which use cases it wants to maintain. Lets compare Keys and
sysfs side-by-side with actual code.

2023-08-04 06:13:23

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
> On Tue, 2023-08-01 at 11:45 +0000, Huang, Kai wrote:
> [...]
> >
> > Sorry perhaps a dumb question to ask:
> >
> > As it has been adequately put, the remote verifiable report normally
> > contains a nonce.? For instance, it can be a per-session or per-
> > request nonce from the remote verification service to the
> > confidential VM. ?
> >
> > IIUC, exposing attestation report via /sysfs means many processes (in
> > the confidential VM) can potentially see the report and the nonce.?
> > My question is whether such nonce should be considered as a secret
> > thus should be only visible to the process which is responsible for
> > talking to the remote verification service?? Using IOCTL seems can
> > avoid such exposure.
>
> OK, so the nonce seems to be a considerably misunderstood piece of this
> (and not just by you), so I'll try to go over carefully what it is and
> why. The problem we have in pretty much any signature based
> attestation evidence scheme is when I, the attesting party, present the
> signed evidence to you, the relying part, how do you know I got it
> today from the system in question not five days ago when I happen to
> have engineered the correct conditions? The solution to this currency
> problem is to incorporate a challenge supplied by the relying party
> (called a nonce) into the signature. The nonce must be unpredictable
> enough that the attesting party can't guess it beforehand and it must
> be unique so that the attesting party can't go through its records and
> find an attestation signature with the same nonce and supply that
> instead.
>
> This property of unpredictability and uniqueness is usually satisfied
> simply by sending a random number. However, as you can also see, since
> the nonce is supplied by the relying party to the attesting party, it
> eventually gets known to both, so can't be a secret to one or the
> other. Because of the unpredictability requirement, it's generally
> frowned on to have nonces based on anything other than random numbers,
> because that might lead to predictability.

The kernel could enforce that a nonce be provided by some convention,
perhaps a user-type key of the same name as the tsm-type key.

That enforces that the payload is always combined with a nonce to
discourage insecure practice building a system that just conveys a raw
pub-key.

2023-08-04 17:24:52

by Daniel P. Berrangé

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, Aug 01, 2023 at 11:45:12AM +0000, Huang, Kai wrote:
> The IOCTL vs /sysfs isn't discussed.
>
> For instance, after rough thinking, why is the IOCTL better than below approach
> using /sysfs?
>
> echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
> cat /sys/kernel/coco/tdx/attest/tdreport
>
> Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
> TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.

What would you suggest as behaviour with multiple processes writing
into 'reportdata' and trying to read from 'tdreport' in parallel ?
Splitting input and output across separate files removes any
transactional relationship between input and output. This approach
feels like it could easily result in buggy behaviour from concurrent
application usage, which would not be an issue with ioctl()

Also note, there needs to be scope for more than 1 input and 1 output
data items. For SNP guests, the VMPL is a input, and if fetching a
VMPL 0 report from under SVSM [1], an optional service GUID is needed.
With SVSM, there are three distinct output data blobs - attestation
report, services manifest and certificate data.

With regards,
Daniel

[1] https://www.amd.com/system/files/TechDocs/58019_1.00.pdf
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|


2023-08-04 22:25:53

by Huang, Kai

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Fri, 2023-08-04 at 17:19 +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 01, 2023 at 11:45:12AM +0000, Huang, Kai wrote:
> > The IOCTL vs /sysfs isn't discussed.
> >
> > For instance, after rough thinking, why is the IOCTL better than below approach
> > using /sysfs?
> >
> > echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
> > cat /sys/kernel/coco/tdx/attest/tdreport
> >
> > Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the driver to call
> > TDCALL to get the TDREPORT, which is available at '/sys/.../tdreport'.
>
> What would you suggest as behaviour with multiple processes writing
> into 'reportdata' and trying to read from 'tdreport' in parallel ?
> Splitting input and output across separate files removes any
> transactional relationship between input and output. This approach
> feels like it could easily result in buggy behaviour from concurrent
> application usage, which would not be an issue with ioctl()

At that time we believed attestation is a relatively low frequent operation thus
it's acceptable to not support concurrent report generation. While kernel is
processing one report the other requests need to wait. This shouldn't be a
problem because the TDREPORT mentioned above is directly from TDX firmware and
won't block for long time.

And in that context we were splitting getting TDREPORT and Quote, meaning after
getting TDREPORT we could have another mechanism (e.g., using IOCTL()) to get
Quote, which could be made to support concurrent requests.

Now if we want to use /sysfs to get the Quote, I am still expecting attestation
should be a low frequent operation, thus to me not supporting concurrent
requests is still acceptable. But since getting Quote involves asking VMM to
get a signed Quote from another userspace process (SGX QE) or even from another
VM (where QE runs) depending on the deployment, the latency of being blocked
from reading /sysfs may be a concern. But we can support return -EINTR if
needed so not a blocking issue I suppose.

Do you have any use case that supporting concurrent attestation requests is
important?

Btw I am not against using IOCTL() :-)

>
> Also note, there needs to be scope for more than 1 input and 1 output
> data items. For SNP guests, the VMPL is a input, and if fetching a
> VMPL 0 report from under SVSM [1], an optional service GUID is needed.
> With SVSM, there are three distinct output data blobs - attestation
> report, services manifest and certificate data.

Yeah we need to find someway to unify.

2023-08-05 03:08:06

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
[..]
> > This report interface on the other hand just needs a single ABI to
> > retrieve all these vendor formats (until industry standardization
> > steps in) and it needs to be flexible (within reason) for all the
> > TSM-specific options to be conveyed. I do not trust my ioctl ABI
> > minefield avoidance skills to get that right. Key blob instantiation
> > feels up to the task.
>
> To repeat: there's nothing keylike about it.

From that perspective there's nothing keylike about user-keys either.
Those are just blobs that userspace gets to define how they are used and
the keyring is just a transport. I also think that this interface *is*
key-like in that it is used in the flow of requesting other key
material. The ability to set policy on who can request and instantiate
these pre-requisite reports can be controlled by request-key policy.

If there was vendor standardization I would be open to /dev/tsmX
interface, but I do not think this deserves brand new ABI from scratch.

> If you think that the keyctl mechanism for transporting information
> across the kernel boundary should be generalised and presented as an
> alternative to our fashion of the year interface for this, then that's
> what you should do (and, I'm afraid to add, cc all the other
> opinionated people who've also produced the flavour of the year
> interfaces).

So I am coming back to this after seeing the thrash that the sysfs
proposal is already causing [1]. sysfs is simply not the right interface
for a transactional interface. My assumption that this interface would
be something that happens once is contraindicated by Peter and Dionna.
So sysfs would require a userspace agent to arbitrate multiple
requesters reconfiguring this all the time.

[1]: http://lore.kernel.org/r/[email protected]

> Sneaking it in as a one-off is the wrong way to proceed
> on something like this.

Where is the sneaking in cc'ing all the relevant maintainers of the
keyring subsystem and their mailing list? Yes, please add others to the
cc.

The question for me at this point is whether a new:

/dev/tsmX

...ABI is worth inventing, or if a key-type is sufficient. To Peter's
concern, this key-type imposes no restrictions over what sevguest
already allows. New options are easy to add to the key instantiation
interface and I expect different vendors are likely to develop workalike
functionality to keep option proliferation to a minimum. Unlike ioctl()
there does not need to be as careful planning about the binary format of
the input payload for per vendor options. Just add more tokens to the
instantiation command-line.

The keyring is also sysfs-like in that it is amenable to manipulation
with command line tools that all systems have available, or by
libkeyctl.

To the concern about where do we draw the line for other use cases that
want to use this as a precedent is to point out that this usage is
demonstrably part of a key material provisioning flow.

2023-08-05 13:34:18

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Fri, 2023-08-04 at 17:19 +0100, Daniel P. Berrangé wrote:
> On Tue, Aug 01, 2023 at 11:45:12AM +0000, Huang, Kai wrote:
> > The IOCTL vs /sysfs isn't discussed.
> >
> > For instance, after rough thinking, why is the IOCTL better than
> > below approach
> > using /sysfs?
> >
> > echo <REPORTDATA> > /sys/kernel/coco/tdx/attest/reportdata
> > cat /sys/kernel/coco/tdx/attest/tdreport
> >
> > Each "echo <REPORTDATA>" to '/sys/.../reportdata' triggers the
> > driver to call
> > TDCALL to get the TDREPORT, which is available at
> > '/sys/.../tdreport'.
>
> What would you suggest as behaviour with multiple processes writing
> into 'reportdata' and trying to read from 'tdreport' in parallel ?
> Splitting input and output across separate files removes any
> transactional relationship between input and output. This approach
> feels like it could easily result in buggy behaviour from concurrent
> application usage, which would not be an issue with ioctl()

What's the use case where there are multiple outstanding reports? The
only use case I've currently seen is single external relying party
requesting a report with a challenge.

> Also note, there needs to be scope for more than 1 input and 1 output
> data items. For SNP guests, the VMPL is a input, and if fetching a
> VMPL 0 report from under SVSM [1], an optional service GUID is
> needed. With SVSM, there are three distinct output data blobs -
> attestation report, services manifest and certificate data.

That's quite simple isn't it? All the possible additional input
parameters appear as files. If you don't echo anything into them, they
take the default values. There's usually a single parameter that
causes the transaction to start (usually the nonce) and the transaction
takes the current values from all the files.

I'm not saying sysfs can substitute for all the transactionality of
ioctl, but in this case where everything is low volume and single
threaded it seems a reasonable choice. For a more volume based
transactional approach, something more configfs like would work better,
so is there a use case for that?

James


2023-08-05 15:02:21

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> James Bottomley wrote:
> [..]
> > > This report interface on the other hand just needs a single ABI
> > > to retrieve all these vendor formats (until industry
> > > standardization steps in) and it needs to be flexible (within
> > > reason) for all the TSM-specific options to be conveyed. I do not
> > > trust my ioctl ABI minefield avoidance skills to get that right.
> > > Key blob instantiation feels up to the task.
> >
> > To repeat: there's nothing keylike about it.
>
> From that perspective there's nothing keylike about user-keys either.

Whataboutism may be popular in politics at the moment, but it shouldn't
be a justification for API abuse: Just because you might be able to
argue something else is an abuse of an API doesn't give you the right
to abuse it further.

> Those are just blobs that userspace gets to define how they are used
> and the keyring is just a transport. I also think that this interface
> *is* key-like in that it is used in the flow of requesting other key
> material. The ability to set policy on who can request and
> instantiate these pre-requisite reports can be controlled by request-
> key policy.

I thought we agreed back here:

https://lore.kernel.org/linux-coco/[email protected]/

That it ended up as "just a transport interface". Has something
changed that?

[...]
> > Sneaking it in as a one-off is the wrong way to proceed
> > on something like this.
>
> Where is the sneaking in cc'ing all the relevant maintainers of the
> keyring subsystem and their mailing list? Yes, please add others to
> the cc.

I was thinking more using the term pubkey in the text about something
that is more like a nonce:

https://lore.kernel.org/linux-coco/169057265801.180586.10867293237672839356.stgit@dwillia2-xfh.jf.intel.com/

That looked to me designed to convince the casual observer that keys
were involved.

> The question for me at this point is whether a new:
>
>         /dev/tsmX
>
> ...ABI is worth inventing, or if a key-type is sufficient. To Peter's
> concern, this key-type imposes no restrictions over what sevguest
> already allows. New options are easy to add to the key instantiation
> interface and I expect different vendors are likely to develop
> workalike functionality to keep option proliferation to a minimum.
> Unlike ioctl() there does not need to be as careful planning about
> the binary format of the input payload for per vendor options. Just
> add more tokens to the instantiation command-line.

I still think this is pretty much an arbitrary transport interface.
The question of how frequently it is used and how transactional it has
to be depend on the use cases (which I think would bear further
examination). What you mostly want to do is create a transaction by
adding parameters individually, kick it off and then read a set of
results back. Because the format of the inputs and outputs is highly
specific to the architecture, the kernel shouldn't really be doing any
inspection or modification. For low volume single threaded use, this
can easily be done by sysfs. For high volume multi-threaded use,
something like configfs or a generic keyctl like object transport
interface would be more appropriate. However, if you think the latter,
it should still be proposed as a new generic kernel to userspace
transactional transport mechanism.


James


2023-08-08 01:31:26

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
> On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> > James Bottomley wrote:
> > [..]
> > > > This report interface on the other hand just needs a single ABI
> > > > to retrieve all these vendor formats (until industry
> > > > standardization steps in) and it needs to be flexible (within
> > > > reason) for all the TSM-specific options to be conveyed. I do not
> > > > trust my ioctl ABI minefield avoidance skills to get that right.
> > > > Key blob instantiation feels up to the task.
> > >
> > > To repeat: there's nothing keylike about it.
> >
> > From that perspective there's nothing keylike about user-keys either.
>
> Whataboutism may be popular in politics at the moment, but it shouldn't
> be a justification for API abuse: Just because you might be able to
> argue something else is an abuse of an API doesn't give you the right
> to abuse it further.

That appears to be the disagreement, that the "user" key type is an
abuse of the keyctl subsystem. Is that the general consensus that it was
added as a mistake that is not be repeated?

Otherwise there is significant amount of thought that has gone into
keyctl including quotas, permissions, and instantiation flows.


> > Those are just blobs that userspace gets to define how they are used
> > and the keyring is just a transport. I also think that this interface
> > *is* key-like in that it is used in the flow of requesting other key
> > material. The ability to set policy on who can request and
> > instantiate these pre-requisite reports can be controlled by request-
> > key policy.
>
> I thought we agreed back here:
>
> https://lore.kernel.org/linux-coco/[email protected]/
>
> That it ended up as "just a transport interface". Has something
> changed that?

This feedback cast doubt on the assumption that attestation reports are
infrequently generated:

http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com

Now, the kernel is within its rights to weigh in on that question with
an ABI that is awkward for that use case, or it can decide up front that
sysfs is not built for transactions.

> [...]
> > > Sneaking it in as a one-off is the wrong way to proceed
> > > on something like this.
> >
> > Where is the sneaking in cc'ing all the relevant maintainers of the
> > keyring subsystem and their mailing list? Yes, please add others to
> > the cc.
>
> I was thinking more using the term pubkey in the text about something
> that is more like a nonce:
>
> https://lore.kernel.org/linux-coco/169057265801.180586.10867293237672839356.stgit@dwillia2-xfh.jf.intel.com/
>
> That looked to me designed to convince the casual observer that keys
> were involved.

Ok, I see where you were going, at the same time I was trusting
keyrings@ community to ask about that detail and was unaware of any
advocacy against new key types.

> > The question for me at this point is whether a new:
> >
> > ????????/dev/tsmX
> >
> > ...ABI is worth inventing, or if a key-type is sufficient. To Peter's
> > concern, this key-type imposes no restrictions over what sevguest
> > already allows. New options are easy to add to the key instantiation
> > interface and I expect different vendors are likely to develop
> > workalike functionality to keep option proliferation to a minimum.
> > Unlike ioctl() there does not need to be as careful planning about
> > the binary format of the input payload for per vendor options. Just
> > add more tokens to the instantiation command-line.
>
> I still think this is pretty much an arbitrary transport interface.
> The question of how frequently it is used and how transactional it has
> to be depend on the use cases (which I think would bear further
> examination). What you mostly want to do is create a transaction by
> adding parameters individually, kick it off and then read a set of
> results back. Because the format of the inputs and outputs is highly
> specific to the architecture, the kernel shouldn't really be doing any
> inspection or modification. For low volume single threaded use, this
> can easily be done by sysfs. For high volume multi-threaded use,
> something like configfs or a generic keyctl like object transport
> interface would be more appropriate. However, if you think the latter,
> it should still be proposed as a new generic kernel to userspace
> transactional transport mechanism.

Perhaps we can get more detail about the proposed high-volume use case:
Dionna, Peter?

I think the minimum bar for ABI success here is that options are not
added without touching a common file that everyone can agree what the
option is, no more drivers/virt/coco/$vendor ABI isolation. If concepts
like VMPL and RTMR are going to have cross-vendor workalike
functionality one day then the kernel community picks one name for
shared concepts. The other criteria for success is that the frontend
needs no change when standardization arrives, assuming all vendors get
their optionality into that spec definition.

keyring lessened my workload with how it can accept ascii token options
whereas ioctl() needs more upfront thought.

2023-08-08 17:43:50

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

James Bottomley wrote:
[..]
> > This feedback cast doubt on the assumption that attestation reports
> > are infrequently generated:
> >
> > http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com
>
> Well, I just read attestation would be called more than once at boot.
> That doesn't necessarily require a concurrent interface.

Ok, I have not seen vigorous defense of the high frequency use case, and
that problem is solvable, it just needs a userspace daemon to front the
interface.

2023-08-08 17:44:03

by Dionna Amalie Glaze

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

>
> At least that was not the level of concurrency I was worried about. The
> sysfs approach makes it so that concurrency problem of option-writing vs
> report-reading is pushed to userspace.
>

The reason I would advocate against making attestation report
collection single-threaded in user space at a machine level is that
there are new schemes of attested connections that may become the
basis of server handshakes. I think folks are mainly looking at this
from the use case of

1. workload will do large amounts of work on behalf of the VM owner,
provided it gets a sealing key released by the VM owner once on boot
after proving its code identity

however I'm thinking of the case of a more user-centric use case that
enables service users to challenge for proof of workload identity

2. workload is a server that accepts incoming connections that include
a hardware attestation challenge. It generates an attestation report
that includes the challenge as part of the connection handshake

This posits the existence of such an advanced user, but high security
applications also have users with high expectations. I want the option
to be open to empower more users to have access to provable workload
provenance, not just the VM owners that are unlocking resources.

> For example, take the following script:
>
> $ cat -n get_report
> 1 #!/bin/bash
> 2 tsm=/sys/class/tsm/tsm0
> 3 echo $1 > ${tsm}/privlevel
> 4 echo $2 > ${tsm}/format
> 5 echo "hex encoded attestation report for: $(cat ${tsm}/provider)"
> 6 xxd -p -c 0 -r ${tsm}/report
>
> The kernel handles the concurrency of line 6 where it synchronizes
> against new writes to the options for the duration of emitting a
> coherent report. However, if you do:
>
> $ get_report 2 extended > reportA & get_report 0 default > reportB
>
> ...there is race between those invocations to set the options and read
> the report.
>
> So to solve that concurrency problem userspace needs to be well behaved
> and only have one thread at a time configuring the options and reading
> out the report before the next agent is allowed to proceed. There are
> several ways to do that, but the kernel only guarantees that the state
> of the options is snapshotted for the duration of 6.



--
-Dionna Glaze, PhD (she/her)

2023-08-08 17:46:19

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

Sathyanarayanan Kuppuswamy wrote:
>
>
> On 8/8/23 7:19 AM, James Bottomley wrote:
> > On Mon, 2023-08-07 at 16:33 -0700, Dan Williams wrote:
> >> James Bottomley wrote:
> >>> On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> >>>> James Bottomley wrote:
> >>>> [..]
> >>>>>> This report interface on the other hand just needs a single
> >>>>>> ABI to retrieve all these vendor formats (until industry
> >>>>>> standardization steps in) and it needs to be flexible (within
> >>>>>> reason) for all the TSM-specific options to be conveyed. I do
> >>>>>> not trust my ioctl ABI minefield avoidance skills to get that
> >>>>>> right. Key blob instantiation feels up to the task.
> >>>>>
> >>>>> To repeat: there's nothing keylike about it.
> >>>>
> >>>> From that perspective there's nothing keylike about user-keys
> >>>> either.
> >>>
> >>> Whataboutism may be popular in politics at the moment, but it
> >>> shouldn't be a justification for API abuse: Just because you might
> >>> be able to argue something else is an abuse of an API doesn't give
> >>> you the right to abuse it further.
> >>
> >> That appears to be the disagreement, that the "user" key type is an
> >> abuse of the keyctl subsystem. Is that the general consensus that it
> >> was added as a mistake that is not be repeated?
> >
> > I didn't say anything about your assertion, just that you seemed to be
> > trying to argue it. However, if you look at the properties of keys:
> >
> > https://www.kernel.org/doc/html/v5.0/security/keys/core.html
> >
> > You'll see that none of them really applies to the case you're trying
> > to add.
> >
> >> Otherwise there is significant amount of thought that has gone into
> >> keyctl including quotas, permissions, and instantiation flows.
> >>
> >>
> >>>> Those are just blobs that userspace gets to define how they are
> >>>> used and the keyring is just a transport. I also think that this
> >>>> interface *is* key-like in that it is used in the flow of
> >>>> requesting other key material. The ability to set policy on who
> >>>> can request and instantiate these pre-requisite reports can be
> >>>> controlled by request-key policy.
> >>>
> >>> I thought we agreed back here:
> >>>
> >>> https://lore.kernel.org/linux-coco/[email protected]/
> >>>
> >>> That it ended up as "just a transport interface".? Has something
> >>> changed that?
> >>
> >> This feedback cast doubt on the assumption that attestation reports
> >> are infrequently generated:
> >>
> >> http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com
> >
> > Well, I just read attestation would be called more than once at boot.
> > That doesn't necessarily require a concurrent interface.
> >
>
> Agree. Currently, both sev-guest and tdx-guest (Quote generation part) IOCTL
> drivers use a mutex to serialize the cmd requests. By design, TDX GET_QUOTE
> hypercall also does not support concurrent requests. Since the attestation
> request is expected to be less frequent and not time-critical, I don't see a
> need to support concurrent interfaces.

At least that was not the level of concurrency I was worried about. The
sysfs approach makes it so that concurrency problem of option-writing vs
report-reading is pushed to userspace.

For example, take the following script:

$ cat -n get_report
1 #!/bin/bash
2 tsm=/sys/class/tsm/tsm0
3 echo $1 > ${tsm}/privlevel
4 echo $2 > ${tsm}/format
5 echo "hex encoded attestation report for: $(cat ${tsm}/provider)"
6 xxd -p -c 0 -r ${tsm}/report

The kernel handles the concurrency of line 6 where it synchronizes
against new writes to the options for the duration of emitting a
coherent report. However, if you do:

$ get_report 2 extended > reportA & get_report 0 default > reportB

...there is race between those invocations to set the options and read
the report.

So to solve that concurrency problem userspace needs to be well behaved
and only have one thread at a time configuring the options and reading
out the report before the next agent is allowed to proceed. There are
several ways to do that, but the kernel only guarantees that the state
of the options is snapshotted for the duration of 6.

2023-08-08 17:47:00

by Peter Gonda

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, Aug 8, 2023 at 8:19 AM James Bottomley
<[email protected]> wrote:
>
> On Mon, 2023-08-07 at 16:33 -0700, Dan Williams wrote:
> > James Bottomley wrote:
> > > On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> > > > James Bottomley wrote:
> > > > [..]
> > > > > > This report interface on the other hand just needs a single
> > > > > > ABI to retrieve all these vendor formats (until industry
> > > > > > standardization steps in) and it needs to be flexible (within
> > > > > > reason) for all the TSM-specific options to be conveyed. I do
> > > > > > not trust my ioctl ABI minefield avoidance skills to get that
> > > > > > right. Key blob instantiation feels up to the task.
> > > > >
> > > > > To repeat: there's nothing keylike about it.
> > > >
> > > > From that perspective there's nothing keylike about user-keys
> > > > either.
> > >
> > > Whataboutism may be popular in politics at the moment, but it
> > > shouldn't be a justification for API abuse: Just because you might
> > > be able to argue something else is an abuse of an API doesn't give
> > > you the right to abuse it further.
> >
> > That appears to be the disagreement, that the "user" key type is an
> > abuse of the keyctl subsystem. Is that the general consensus that it
> > was added as a mistake that is not be repeated?
>
> I didn't say anything about your assertion, just that you seemed to be
> trying to argue it. However, if you look at the properties of keys:
>
> https://www.kernel.org/doc/html/v5.0/security/keys/core.html
>
> You'll see that none of them really applies to the case you're trying
> to add.
>
> > Otherwise there is significant amount of thought that has gone into
> > keyctl including quotas, permissions, and instantiation flows.
> >
> >
> > > > Those are just blobs that userspace gets to define how they are
> > > > used and the keyring is just a transport. I also think that this
> > > > interface *is* key-like in that it is used in the flow of
> > > > requesting other key material. The ability to set policy on who
> > > > can request and instantiate these pre-requisite reports can be
> > > > controlled by request-key policy.
> > >
> > > I thought we agreed back here:
> > >
> > > https://lore.kernel.org/linux-coco/[email protected]/
> > >
> > > That it ended up as "just a transport interface". Has something
> > > changed that?
> >
> > This feedback cast doubt on the assumption that attestation reports
> > are infrequently generated:
> >
> > http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com
>
> Well, I just read attestation would be called more than once at boot.
> That doesn't necessarily require a concurrent interface.
>
> > Now, the kernel is within its rights to weigh in on that question
> > with an ABI that is awkward for that use case, or it can decide up
> > front that sysfs is not built for transactions.
>
> I thought pretty much everyone agreed sysfs isn't really transactional.
> However, if the frequency of use of this is low enough, CC attestation
> doesn't need to be transactional either. All you need is the ability
> to look at the inputs and outputs and to specify new ones if required.
> Sysfs works for this provided two entities don't want to supply inputs
> at the same time.
>
> > > [...]
> > > > > Sneaking it in as a one-off is the wrong way to proceed
> > > > > on something like this.
> > > >
> > > > Where is the sneaking in cc'ing all the relevant maintainers of
> > > > the keyring subsystem and their mailing list? Yes, please add
> > > > others to the cc.
> > >
> > > I was thinking more using the term pubkey in the text about
> > > something that is more like a nonce:
> > >
> > > https://lore.kernel.org/linux-coco/169057265801.180586.10867293237672839356.stgit@dwillia2-xfh.jf.intel.com/
> > >
> > > That looked to me designed to convince the casual observer that
> > > keys were involved.
> >
> > Ok, I see where you were going, at the same time I was trusting
> > keyrings@ community to ask about that detail and was unaware of any
> > advocacy against new key types.
>
> I'm not advocating against new key types. I'm saying what you're
> proposing is simply a data transport layer and, as such, has no
> properties that really make it a key type.
>
> > > > The question for me at this point is whether a new:
> > > >
> > > > /dev/tsmX
> > > >
> > > > ...ABI is worth inventing, or if a key-type is sufficient. To
> > > > Peter's concern, this key-type imposes no restrictions over what
> > > > sevguest already allows. New options are easy to add to the key
> > > > instantiation interface and I expect different vendors are likely
> > > > to develop workalike functionality to keep option proliferation
> > > > to a minimum. Unlike ioctl() there does not need to be as careful
> > > > planning about the binary format of the input payload for per
> > > > vendor options. Just add more tokens to the instantiation
> > > > command-line.

But given that on the other end of an attestation quote is an
attestation verifier. I would actually much prefer the ability to
carefully plan the binary format. Since that attestation verifier will
need to do so in any case.

> > >
> > > I still think this is pretty much an arbitrary transport interface.
> > > The question of how frequently it is used and how transactional it
> > > has to be depend on the use cases (which I think would bear further
> > > examination). What you mostly want to do is create a transaction
> > > by adding parameters individually, kick it off and then read a set
> > > of results back. Because the format of the inputs and outputs is
> > > highly specific to the architecture, the kernel shouldn't really be
> > > doing any inspection or modification. For low volume single
> > > threaded use, this can easily be done by sysfs. For high volume
> > > multi-threaded use, something like configfs or a generic keyctl
> > > like object transport interface would be more appropriate.
> > > However, if you think the latter, it should still be proposed as a
> > > new generic kernel to userspace transactional transport mechanism.
> >
> > Perhaps we can get more detail about the proposed high-volume use
> > case: Dionna, Peter?
>
> Well, that's why I asked for use cases. I have one which is very low
> volume and single threaded. I'm not sure what use case you have since
> you never outlined it and I see hints from Red Hat that they worry
> about concurrency. So it's interface design 101: collect the use cases
> first.

I don't have a usecase in mind. I am just concerned with decisions
made here affecting the ability for CoCo users to come up with their
own use cases that might need high quote volume.

>
> > I think the minimum bar for ABI success here is that options are not
> > added without touching a common file that everyone can agree what the
> > option is, no more drivers/virt/coco/$vendor ABI isolation. If
> > concepts like VMPL and RTMR are going to have cross-vendor workalike
> > functionality one day then the kernel community picks one name for
> > shared concepts. The other criteria for success is that the frontend
> > needs no change when standardization arrives, assuming all vendors
> > get their optionality into that spec definition.

Since verifiers will need to understand each vendor's ABI to correctly
verify the quotes I am still not sure why having isolated drivers is a
bad thing.

>
> I don't think RTMR would ever be cross vendor. It's sort of a cut down
> TPM with a limited number of PCRs. Even Intel seems to be admitting
> this when they justified putting a vTPM into TDX at the OC3 Q and A
> session (no tools currently work with RTMRs and the TPM ecosystem is
> fairly solid, so using a vTPM instead of RTMRs gives us an industry
> standard workflow).

I'm not so sure about this statement. ARM's CCA already has Realm
Extendable Measurements (REMs) which seem to be exactly RTMRs in all
but name. Maybe we need a vendor agnostic term for these 'Measurement
Registers'? Since we now have 3 different vendors for them: CCA's REM,
TDX's RMTRs, TPM's PCRs.

2023-08-08 18:17:52

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Mon, 2023-08-07 at 16:33 -0700, Dan Williams wrote:
> James Bottomley wrote:
> > On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> > > James Bottomley wrote:
> > > [..]
> > > > > This report interface on the other hand just needs a single
> > > > > ABI to retrieve all these vendor formats (until industry
> > > > > standardization steps in) and it needs to be flexible (within
> > > > > reason) for all the TSM-specific options to be conveyed. I do
> > > > > not trust my ioctl ABI minefield avoidance skills to get that
> > > > > right. Key blob instantiation feels up to the task.
> > > >
> > > > To repeat: there's nothing keylike about it.
> > >
> > > From that perspective there's nothing keylike about user-keys
> > > either.
> >
> > Whataboutism may be popular in politics at the moment, but it
> > shouldn't be a justification for API abuse: Just because you might
> > be able to argue something else is an abuse of an API doesn't give
> > you the right to abuse it further.
>
> That appears to be the disagreement, that the "user" key type is an
> abuse of the keyctl subsystem. Is that the general consensus that it
> was added as a mistake that is not be repeated?

I didn't say anything about your assertion, just that you seemed to be
trying to argue it. However, if you look at the properties of keys:

https://www.kernel.org/doc/html/v5.0/security/keys/core.html

You'll see that none of them really applies to the case you're trying
to add.

> Otherwise there is significant amount of thought that has gone into
> keyctl including quotas, permissions, and instantiation flows.
>
>
> > > Those are just blobs that userspace gets to define how they are
> > > used and the keyring is just a transport. I also think that this
> > > interface *is* key-like in that it is used in the flow of
> > > requesting other key material. The ability to set policy on who
> > > can request and instantiate these pre-requisite reports can be
> > > controlled by request-key policy.
> >
> > I thought we agreed back here:
> >
> > https://lore.kernel.org/linux-coco/[email protected]/
> >
> > That it ended up as "just a transport interface".  Has something
> > changed that?
>
> This feedback cast doubt on the assumption that attestation reports
> are infrequently generated:
>
> http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com

Well, I just read attestation would be called more than once at boot.
That doesn't necessarily require a concurrent interface.

> Now, the kernel is within its rights to weigh in on that question
> with an ABI that is awkward for that use case, or it can decide up
> front that sysfs is not built for transactions.

I thought pretty much everyone agreed sysfs isn't really transactional.
However, if the frequency of use of this is low enough, CC attestation
doesn't need to be transactional either. All you need is the ability
to look at the inputs and outputs and to specify new ones if required.
Sysfs works for this provided two entities don't want to supply inputs
at the same time.

> > [...]
> > > > Sneaking it in as a one-off is the wrong way to proceed
> > > > on something like this.
> > >
> > > Where is the sneaking in cc'ing all the relevant maintainers of
> > > the keyring subsystem and their mailing list? Yes, please add
> > > others to the cc.
> >
> > I was thinking more using the term pubkey in the text about
> > something that is more like a nonce:
> >
> > https://lore.kernel.org/linux-coco/169057265801.180586.10867293237672839356.stgit@dwillia2-xfh.jf.intel.com/
> >
> > That looked to me designed to convince the casual observer that
> > keys were involved.
>
> Ok, I see where you were going, at the same time I was trusting
> keyrings@ community to ask about that detail and was unaware of any
> advocacy against new key types.

I'm not advocating against new key types. I'm saying what you're
proposing is simply a data transport layer and, as such, has no
properties that really make it a key type.

> > > The question for me at this point is whether a new:
> > >
> > >         /dev/tsmX
> > >
> > > ...ABI is worth inventing, or if a key-type is sufficient. To
> > > Peter's concern, this key-type imposes no restrictions over what
> > > sevguest already allows. New options are easy to add to the key
> > > instantiation interface and I expect different vendors are likely
> > > to develop workalike functionality to keep option proliferation
> > > to a minimum. Unlike ioctl() there does not need to be as careful
> > > planning about the binary format of the input payload for per
> > > vendor options. Just add more tokens to the instantiation
> > > command-line.
> >
> > I still think this is pretty much an arbitrary transport interface.
> > The question of how frequently it is used and how transactional it
> > has to be depend on the use cases (which I think would bear further
> > examination).  What you mostly want to do is create a transaction
> > by adding parameters individually, kick it off and then read a set
> > of results back.  Because the format of the inputs and outputs is
> > highly specific to the architecture, the kernel shouldn't really be
> > doing any inspection or modification.  For low volume single
> > threaded use, this can easily be done by sysfs.  For high volume
> > multi-threaded use, something like configfs or a generic keyctl
> > like object transport interface would be more appropriate. 
> > However, if you think the latter, it should still be proposed as a
> > new generic kernel to userspace transactional transport mechanism.
>
> Perhaps we can get more detail about the proposed high-volume use
> case: Dionna, Peter?

Well, that's why I asked for use cases. I have one which is very low
volume and single threaded. I'm not sure what use case you have since
you never outlined it and I see hints from Red Hat that they worry
about concurrency. So it's interface design 101: collect the use cases
first.

> I think the minimum bar for ABI success here is that options are not
> added without touching a common file that everyone can agree what the
> option is, no more drivers/virt/coco/$vendor ABI isolation. If
> concepts like VMPL and RTMR are going to have cross-vendor workalike
> functionality one day then the kernel community picks one name for
> shared concepts. The other criteria for success is that the frontend
> needs no change when standardization arrives, assuming all vendors
> get their optionality into that spec definition.

I don't think RTMR would ever be cross vendor. It's sort of a cut down
TPM with a limited number of PCRs. Even Intel seems to be admitting
this when they justified putting a vTPM into TDX at the OC3 Q and A
session (no tools currently work with RTMRs and the TPM ecosystem is
fairly solid, so using a vTPM instead of RTMRs gives us an industry
standard workflow).

James


> keyring lessened my workload with how it can accept ascii token
> options whereas ioctl() needs more upfront thought.


2023-08-08 18:55:27

by Dionna Amalie Glaze

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

>
> I do not see sysfs precluding a use case like that. If the kernel can
> call out to userspace for TLS connection setup [1], then advanced user
> can call out to a daemon for workload provenance setup. Recall that TDX
> will round trip through the quoting enclave for these reports and,
> without measuring, that seems to have the potential to dominate the
> setup time vs the communication to ask a daemon to convey a report.
>

It's rather hard to get new daemons approved for container
distributions since they end up as resource hogs.
I really don't think it's appropriate to delegate to a daemon to
single-thread use of a kernel interface when the interface could
provide functional semantics to begin with.

> [1]: https://lore.kernel.org/all/168174169259.9520.1911007910797225963.stgit@91.116.238.104.host.secureserver.net/



--
-Dionna Glaze, PhD (she/her)

Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports



On 8/8/23 7:19 AM, James Bottomley wrote:
> On Mon, 2023-08-07 at 16:33 -0700, Dan Williams wrote:
>> James Bottomley wrote:
>>> On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
>>>> James Bottomley wrote:
>>>> [..]
>>>>>> This report interface on the other hand just needs a single
>>>>>> ABI to retrieve all these vendor formats (until industry
>>>>>> standardization steps in) and it needs to be flexible (within
>>>>>> reason) for all the TSM-specific options to be conveyed. I do
>>>>>> not trust my ioctl ABI minefield avoidance skills to get that
>>>>>> right. Key blob instantiation feels up to the task.
>>>>>
>>>>> To repeat: there's nothing keylike about it.
>>>>
>>>> From that perspective there's nothing keylike about user-keys
>>>> either.
>>>
>>> Whataboutism may be popular in politics at the moment, but it
>>> shouldn't be a justification for API abuse: Just because you might
>>> be able to argue something else is an abuse of an API doesn't give
>>> you the right to abuse it further.
>>
>> That appears to be the disagreement, that the "user" key type is an
>> abuse of the keyctl subsystem. Is that the general consensus that it
>> was added as a mistake that is not be repeated?
>
> I didn't say anything about your assertion, just that you seemed to be
> trying to argue it. However, if you look at the properties of keys:
>
> https://www.kernel.org/doc/html/v5.0/security/keys/core.html
>
> You'll see that none of them really applies to the case you're trying
> to add.
>
>> Otherwise there is significant amount of thought that has gone into
>> keyctl including quotas, permissions, and instantiation flows.
>>
>>
>>>> Those are just blobs that userspace gets to define how they are
>>>> used and the keyring is just a transport. I also think that this
>>>> interface *is* key-like in that it is used in the flow of
>>>> requesting other key material. The ability to set policy on who
>>>> can request and instantiate these pre-requisite reports can be
>>>> controlled by request-key policy.
>>>
>>> I thought we agreed back here:
>>>
>>> https://lore.kernel.org/linux-coco/[email protected]/
>>>
>>> That it ended up as "just a transport interface".  Has something
>>> changed that?
>>
>> This feedback cast doubt on the assumption that attestation reports
>> are infrequently generated:
>>
>> http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com
>
> Well, I just read attestation would be called more than once at boot.
> That doesn't necessarily require a concurrent interface.
>

Agree. Currently, both sev-guest and tdx-guest (Quote generation part) IOCTL
drivers use a mutex to serialize the cmd requests. By design, TDX GET_QUOTE
hypercall also does not support concurrent requests. Since the attestation
request is expected to be less frequent and not time-critical, I don't see a
need to support concurrent interfaces.

>> Now, the kernel is within its rights to weigh in on that question
>> with an ABI that is awkward for that use case, or it can decide up
>> front that sysfs is not built for transactions.
>
> I thought pretty much everyone agreed sysfs isn't really transactional.
> However, if the frequency of use of this is low enough, CC attestation
> doesn't need to be transactional either. All you need is the ability
> to look at the inputs and outputs and to specify new ones if required.
> Sysfs works for this provided two entities don't want to supply inputs
> at the same time.
>
>>> [...]
>>>>> Sneaking it in as a one-off is the wrong way to proceed
>>>>> on something like this.
>>>>
>>>> Where is the sneaking in cc'ing all the relevant maintainers of
>>>> the keyring subsystem and their mailing list? Yes, please add
>>>> others to the cc.
>>>
>>> I was thinking more using the term pubkey in the text about
>>> something that is more like a nonce:
>>>
>>> https://lore.kernel.org/linux-coco/169057265801.180586.10867293237672839356.stgit@dwillia2-xfh.jf.intel.com/
>>>
>>> That looked to me designed to convince the casual observer that
>>> keys were involved.
>>
>> Ok, I see where you were going, at the same time I was trusting
>> keyrings@ community to ask about that detail and was unaware of any
>> advocacy against new key types.
>
> I'm not advocating against new key types. I'm saying what you're
> proposing is simply a data transport layer and, as such, has no
> properties that really make it a key type.
>
>>>> The question for me at this point is whether a new:
>>>>
>>>>         /dev/tsmX
>>>>
>>>> ...ABI is worth inventing, or if a key-type is sufficient. To
>>>> Peter's concern, this key-type imposes no restrictions over what
>>>> sevguest already allows. New options are easy to add to the key
>>>> instantiation interface and I expect different vendors are likely
>>>> to develop workalike functionality to keep option proliferation
>>>> to a minimum. Unlike ioctl() there does not need to be as careful
>>>> planning about the binary format of the input payload for per
>>>> vendor options. Just add more tokens to the instantiation
>>>> command-line.
>>>
>>> I still think this is pretty much an arbitrary transport interface.
>>> The question of how frequently it is used and how transactional it
>>> has to be depend on the use cases (which I think would bear further
>>> examination).  What you mostly want to do is create a transaction
>>> by adding parameters individually, kick it off and then read a set
>>> of results back.  Because the format of the inputs and outputs is
>>> highly specific to the architecture, the kernel shouldn't really be
>>> doing any inspection or modification.  For low volume single
>>> threaded use, this can easily be done by sysfs.  For high volume
>>> multi-threaded use, something like configfs or a generic keyctl
>>> like object transport interface would be more appropriate. 
>>> However, if you think the latter, it should still be proposed as a
>>> new generic kernel to userspace transactional transport mechanism.
>>
>> Perhaps we can get more detail about the proposed high-volume use
>> case: Dionna, Peter?
>
> Well, that's why I asked for use cases. I have one which is very low
> volume and single threaded. I'm not sure what use case you have since
> you never outlined it and I see hints from Red Hat that they worry
> about concurrency. So it's interface design 101: collect the use cases
> first.
>
>> I think the minimum bar for ABI success here is that options are not
>> added without touching a common file that everyone can agree what the
>> option is, no more drivers/virt/coco/$vendor ABI isolation. If
>> concepts like VMPL and RTMR are going to have cross-vendor workalike
>> functionality one day then the kernel community picks one name for
>> shared concepts. The other criteria for success is that the frontend
>> needs no change when standardization arrives, assuming all vendors
>> get their optionality into that spec definition.
>
> I don't think RTMR would ever be cross vendor. It's sort of a cut down
> TPM with a limited number of PCRs. Even Intel seems to be admitting
> this when they justified putting a vTPM into TDX at the OC3 Q and A
> session (no tools currently work with RTMRs and the TPM ecosystem is
> fairly solid, so using a vTPM instead of RTMRs gives us an industry
> standard workflow).
>
> James
>
>
>> keyring lessened my workload with how it can accept ascii token
>> options whereas ioctl() needs more upfront thought.
>
>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

2023-08-08 19:47:12

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

Dionna Amalie Glaze wrote:
> >
> > At least that was not the level of concurrency I was worried about. The
> > sysfs approach makes it so that concurrency problem of option-writing vs
> > report-reading is pushed to userspace.
> >
>
> The reason I would advocate against making attestation report
> collection single-threaded in user space at a machine level is that
> there are new schemes of attested connections that may become the
> basis of server handshakes. I think folks are mainly looking at this
> from the use case of
>
> 1. workload will do large amounts of work on behalf of the VM owner,
> provided it gets a sealing key released by the VM owner once on boot
> after proving its code identity
>
> however I'm thinking of the case of a more user-centric use case that
> enables service users to challenge for proof of workload identity
>
> 2. workload is a server that accepts incoming connections that include
> a hardware attestation challenge. It generates an attestation report
> that includes the challenge as part of the connection handshake
>
> This posits the existence of such an advanced user, but high security
> applications also have users with high expectations. I want the option
> to be open to empower more users to have access to provable workload
> provenance, not just the VM owners that are unlocking resources.

I do not see sysfs precluding a use case like that. If the kernel can
call out to userspace for TLS connection setup [1], then advanced user
can call out to a daemon for workload provenance setup. Recall that TDX
will round trip through the quoting enclave for these reports and,
without measuring, that seems to have the potential to dominate the
setup time vs the communication to ask a daemon to convey a report.

[1]: https://lore.kernel.org/all/168174169259.9520.1911007910797225963.stgit@91.116.238.104.host.secureserver.net/

2023-08-08 20:06:20

by Dionna Amalie Glaze

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

> Isn't this more runtime attestation? In which case you wouldn't use
> the boot report. I assume someone somewhere is hacking the TPM-TLS
> protocol to also do RTMRs, but it strikes me we could just use a vTPM
> and the existing protocols.
>
> Even if you don't do anything as complex as TPM-TLS (and continuing
> runtime attestation), you can still make TLS conditioned on a private
> key released after a successful boot time attestation. Since the boot
> evidence never changes, there's not much point doing it on each
> connection, so relying on a private key conditioned on boot evidence is
> just as good.
>
> James
>

The TPM quote will need to be bound to the VM instance, so there will
still be a hardware attestation in there that incorporates the user's
challenge.
Anything less than that is subject to replay attacks, no? Am I missing
a clever trick?

--
-Dionna Glaze, PhD (she/her)

2023-08-08 20:37:13

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

Dionna Amalie Glaze wrote:
> >
> > I do not see sysfs precluding a use case like that. If the kernel can
> > call out to userspace for TLS connection setup [1], then advanced user
> > can call out to a daemon for workload provenance setup. Recall that TDX
> > will round trip through the quoting enclave for these reports and,
> > without measuring, that seems to have the potential to dominate the
> > setup time vs the communication to ask a daemon to convey a report.
> >
>
> It's rather hard to get new daemons approved for container
> distributions since they end up as resource hogs.
> I really don't think it's appropriate to delegate to a daemon to
> single-thread use of a kernel interface when the interface could
> provide functional semantics to begin with.

That's fair, it's also not without precedence for the kernel to await a
strong motivation of a use case before taking on a higher maintenance
burden. Unifying kernel interfaces is important for maintainability and
difficult / needs care. sysfs simplifies maintainability (but exports
complexity to userspace), keyring simplifies that (but there is a valid
argument that this is not a key), ioctl complicates that (it is not as
amenable to transport unification as the above options).

2023-08-08 21:48:24

by Dionna Amalie Glaze

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

> Trusting the vTPM is a one time thing. Once trust in the TPM is
> established, you don't need to be worried about replay and you can just
> use standard TPM primitives for everything onward, even when doing
> point in time runtime attestation.
>

It's a one time thing for who? It seems like you're still only looking
at the 1. use case and not the 2. use case. Every different person
establishing a connection with the service will need to independently
establish trust in the TPM.


--
-Dionna Glaze, PhD (she/her)

2023-08-08 22:15:15

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-08 at 09:07 -0700, Dionna Amalie Glaze wrote:
> >
> > At least that was not the level of concurrency I was worried about.
> > The sysfs approach makes it so that concurrency problem of
> > option-writing vs report-reading is pushed to userspace.
> >
>
> The reason I would advocate against making attestation report
> collection single-threaded in user space at a machine level is that
> there are new schemes of attested connections that may become the
> basis of server handshakes. I think folks are mainly looking at this
> from the use case of
>
> 1. workload will do large amounts of work on behalf of the VM owner,
> provided it gets a sealing key released by the VM owner once on boot
> after proving its code identity

Right, that's the case for boot time attestation.

> however I'm thinking of the case of a more user-centric use case that
> enables service users to challenge for proof of workload identity
>
> 2. workload is a server that accepts incoming connections that
> include a hardware attestation challenge. It generates an attestation
> report that includes the challenge as part of the connection
> handshake

Isn't this more runtime attestation? In which case you wouldn't use
the boot report. I assume someone somewhere is hacking the TPM-TLS
protocol to also do RTMRs, but it strikes me we could just use a vTPM
and the existing protocols.

Even if you don't do anything as complex as TPM-TLS (and continuing
runtime attestation), you can still make TLS conditioned on a private
key released after a successful boot time attestation. Since the boot
evidence never changes, there's not much point doing it on each
connection, so relying on a private key conditioned on boot evidence is
just as good.

James


2023-08-08 22:49:42

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-08 at 11:48 -0700, Dionna Amalie Glaze wrote:
> > Isn't this more runtime attestation?  In which case you wouldn't
> > use the boot report.  I assume someone somewhere is hacking the
> > TPM-TLS protocol to also do RTMRs, but it strikes me we could just
> > use a vTPM and the existing protocols.
> >
> > Even if you don't do anything as complex as TPM-TLS (and continuing
> > runtime attestation), you can still make TLS conditioned on a
> > private key released after a successful boot time attestation. 
> > Since the boot evidence never changes, there's not much point doing
> > it on each connection, so relying on a private key conditioned on
> > boot evidence is just as good.
> >
> > James
> >
>
> The TPM quote will need to be bound to the VM instance, so there will
> still be a hardware attestation in there that incorporates the user's
> challenge.

Well, it's all in the protocol: A TLS-TPM system using a physical TPM
has to do an EK/AK makecredential/activatecredential to verify it's
talking to a real TPM. In the CC vTPM case that step is substituted by
doing a vTPM attestation. However, the point is in each case the
verification step is only done once before you trust the TPM. After
that, it's the TPM key you trust because the proof, in either case, was
that the key is TPM generated and the TPM should be tamper proof
(enforced by the casing for a physical TPM and the situation in the
VMPL or other software protection for the vTPM).

> Anything less than that is subject to replay attacks, no? Am I
> missing a clever trick?

Trusting the vTPM is a one time thing. Once trust in the TPM is
established, you don't need to be worried about replay and you can just
use standard TPM primitives for everything onward, even when doing
point in time runtime attestation.

James


2023-08-08 22:54:31

by James Bottomley

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-08 at 13:04 -0700, Dionna Amalie Glaze wrote:
> > Trusting the vTPM is a one time thing.  Once trust in the TPM is
> > established, you don't need to be worried about replay and you can
> > just use standard TPM primitives for everything onward, even when
> > doing point in time runtime attestation.
> >
>
> It's a one time thing for who?

Well, in TLS-TPM it tends to be a one time thing per endpoint
regardless of number of connections.

> It seems like you're still only looking at the 1. use case and not
> the 2. use case. Every different person establishing a connection
> with the service will need to independently establish trust in the
> TPM.

For an ephemeral TPM, the EK should be guaranteed to be random and
therefore non repeating, so there's not much need for the nonce to add
non-repeatability. So, in theory, the vTPM/EK binding can be published
once and relied on even for multiple different tenant endpoints, sort
of like the EK cert for a physical TPM.

James


2023-08-08 23:32:29

by Dionna Amalie Glaze

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

> For an ephemeral TPM, the EK should be guaranteed to be random and
> therefore non repeating, so there's not much need for the nonce to add
> non-repeatability. So, in theory, the vTPM/EK binding can be published
> once and relied on even for multiple different tenant endpoints, sort
> of like the EK cert for a physical TPM.
>

Okay that sounds reasonable.

Regarding my other comment about daemons, we might already be in that
state for containers even without the sysfs proposal, given that the
sev-guest device requires root.
We'd need a daemon to provide protected access to the attestation
report (e.g., https://github.com/confidential-containers/attestation-agent)
so that's a bit of a sad situation.

--
-Dionna Glaze, PhD (she/her)

2023-08-09 01:36:50

by Huang, Kai

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, 2023-08-08 at 11:17 -0700, Dan Williams wrote:
> Dionna Amalie Glaze wrote:
> > >
> > > I do not see sysfs precluding a use case like that. If the kernel can
> > > call out to userspace for TLS connection setup [1], then advanced user
> > > can call out to a daemon for workload provenance setup. Recall that TDX
> > > will round trip through the quoting enclave for these reports and,
> > > without measuring, that seems to have the potential to dominate the
> > > setup time vs the communication to ask a daemon to convey a report.
> > >
> >
> > It's rather hard to get new daemons approved for container
> > distributions since they end up as resource hogs.
> > I really don't think it's appropriate to delegate to a daemon to
> > single-thread use of a kernel interface when the interface could
> > provide functional semantics to begin with.
>
> That's fair, it's also not without precedence for the kernel to await a
> strong motivation of a use case before taking on a higher maintenance
> burden. Unifying kernel interfaces is important for maintainability and
> difficult / needs care. sysfs simplifies maintainability (but exports
> complexity to userspace), keyring simplifies that (but there is a valid
> argument that this is not a key), ioctl complicates that (it is not as
> amenable to transport unification as the above options).
>

I don't quite follow why ioctl() is not amenable to transport unification as the
/sysfs? IIUC both are new ABI(s) to the userspace thus userspace needs to adopt
anyway.  

On the other hand, ioctl() seems to be able to handle concurrent requests better
than /sysfs, if we want to support the case that integrating attestation to the
handshake protocols.

2023-08-09 03:58:50

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

Huang, Kai wrote:
> On Tue, 2023-08-08 at 11:17 -0700, Dan Williams wrote:
> > Dionna Amalie Glaze wrote:
> > > >
> > > > I do not see sysfs precluding a use case like that. If the kernel can
> > > > call out to userspace for TLS connection setup [1], then advanced user
> > > > can call out to a daemon for workload provenance setup. Recall that TDX
> > > > will round trip through the quoting enclave for these reports and,
> > > > without measuring, that seems to have the potential to dominate the
> > > > setup time vs the communication to ask a daemon to convey a report.
> > > >
> > >
> > > It's rather hard to get new daemons approved for container
> > > distributions since they end up as resource hogs.
> > > I really don't think it's appropriate to delegate to a daemon to
> > > single-thread use of a kernel interface when the interface could
> > > provide functional semantics to begin with.
> >
> > That's fair, it's also not without precedence for the kernel to await a
> > strong motivation of a use case before taking on a higher maintenance
> > burden. Unifying kernel interfaces is important for maintainability and
> > difficult / needs care. sysfs simplifies maintainability (but exports
> > complexity to userspace), keyring simplifies that (but there is a valid
> > argument that this is not a key), ioctl complicates that (it is not as
> > amenable to transport unification as the above options).
> >
>
> I don't quite follow why ioctl() is not amenable to transport unification as the
> /sysfs? IIUC both are new ABI(s) to the userspace thus userspace needs to adopt
> anyway. ?

Recall that the concern here is kernel maintainability, the kernel can
decide to export complexity to userspace. In that light, ioctl() code is
grotty sysfs is not. sysfs attributes (tsm blob options) are easy to
reason about and audit, ioctl() is not. sysfs is easy to extend with
local attributes to augment the core, ioctl() forces all the optionality
to be planned up front.

Basically, if you hand me a choice between maintaining a cross vendor
ioctl() ABI vs a sysfs ABI, I am picking sysfs every time.

> On the other hand, ioctl() seems to be able to handle concurrent requests better
> than /sysfs, if we want to support the case that integrating attestation to the
> handshake protocols.

There is not an exceedingly strong case for high frequency concurrent
requests vs boot time attestation and deriving further secrets from
that.

2023-08-09 16:36:27

by Peter Gonda

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue, Aug 8, 2023 at 9:28 PM Dan Williams <[email protected]> wrote:
>
> Huang, Kai wrote:
> > On Tue, 2023-08-08 at 11:17 -0700, Dan Williams wrote:
> > > Dionna Amalie Glaze wrote:
> > > > >
> > > > > I do not see sysfs precluding a use case like that. If the kernel can
> > > > > call out to userspace for TLS connection setup [1], then advanced user
> > > > > can call out to a daemon for workload provenance setup. Recall that TDX
> > > > > will round trip through the quoting enclave for these reports and,
> > > > > without measuring, that seems to have the potential to dominate the
> > > > > setup time vs the communication to ask a daemon to convey a report.
> > > > >
> > > >
> > > > It's rather hard to get new daemons approved for container
> > > > distributions since they end up as resource hogs.
> > > > I really don't think it's appropriate to delegate to a daemon to
> > > > single-thread use of a kernel interface when the interface could
> > > > provide functional semantics to begin with.
> > >
> > > That's fair, it's also not without precedence for the kernel to await a
> > > strong motivation of a use case before taking on a higher maintenance
> > > burden. Unifying kernel interfaces is important for maintainability and
> > > difficult / needs care. sysfs simplifies maintainability (but exports
> > > complexity to userspace), keyring simplifies that (but there is a valid
> > > argument that this is not a key), ioctl complicates that (it is not as
> > > amenable to transport unification as the above options).
> > >
> >
> > I don't quite follow why ioctl() is not amenable to transport unification as the
> > /sysfs? IIUC both are new ABI(s) to the userspace thus userspace needs to adopt
> > anyway.
>
> Recall that the concern here is kernel maintainability, the kernel can
> decide to export complexity to userspace. In that light, ioctl() code is
> grotty sysfs is not. sysfs attributes (tsm blob options) are easy to
> reason about and audit, ioctl() is not. sysfs is easy to extend with
> local attributes to augment the core, ioctl() forces all the optionality
> to be planned up front.
>
> Basically, if you hand me a choice between maintaining a cross vendor
> ioctl() ABI vs a sysfs ABI, I am picking sysfs every time.

Thanks for the explanation. My pushback isn't because I really want an
IOCTL, rather I want the user to have the ability to get the exact
attestation report they want. This interface shown here was too
restrictive. If this can be accomplished with another ABI that sounds
fine to me.

2023-08-10 15:56:00

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

On Tue Aug 8, 2023 at 2:33 AM EEST, Dan Williams wrote:
> James Bottomley wrote:
> > On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> > > James Bottomley wrote:
> > > [..]
> > > > > This report interface on the other hand just needs a single ABI
> > > > > to retrieve all these vendor formats (until industry
> > > > > standardization steps in) and it needs to be flexible (within
> > > > > reason) for all the TSM-specific options to be conveyed. I do not
> > > > > trust my ioctl ABI minefield avoidance skills to get that right.
> > > > > Key blob instantiation feels up to the task.
> > > >
> > > > To repeat: there's nothing keylike about it.
> > >
> > > From that perspective there's nothing keylike about user-keys either.
> >
> > Whataboutism may be popular in politics at the moment, but it shouldn't
> > be a justification for API abuse: Just because you might be able to
> > argue something else is an abuse of an API doesn't give you the right
> > to abuse it further.
>
> That appears to be the disagreement, that the "user" key type is an
> abuse of the keyctl subsystem. Is that the general consensus that it was
> added as a mistake that is not be repeated?
>
> Otherwise there is significant amount of thought that has gone into
> keyctl including quotas, permissions, and instantiation flows.

I would focus on just fixing known obvious issues in the patch set and
improve the description what it does.

This looks like a discussion where the patch set is not advertised in
a way that it is understandable, not necessarily that it is all wrong.

E.g. why not name the key type as attestation key or something more
intuitive rather than three letter acronym?

I don't think this will converge to anything with argumentation in the
current state of where we are right now.

BR, Jarkko