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

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

[Dan Williams]

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

[PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

One of the common operations of a TSM (Trusted Security Module) is to
provide a way for a TVM (confidential computing guest execution
environment) to take a measurement of its run state and use that with a
key-exchange protocol to establish a shared secret with a third-party /
remote attestation agent. The concept is common across TSMs, but the
implementations are unfortunately vendor specific. While the industry
grapples with a common definition of this attestation format [1], Linux
need not make this problem worse by defining a new ABI per TSM that
wants to perform a similar operation. The current momentum has been to
invent new ioctl-ABI per TSM per function which at best is an abdication
of the kernel's responsibility to make common infrastructure concepts
share common ABI.

The proposal, targeted to conceptually work with TDX, SEV, COVE if not
more, is to define a new key type that produces a TSM common blob format
and moves the vendor specificity inside that envelope. The common Linux
definition is:

    "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"

This approach later allows for the standardization of the attestation
blob format without needing to change the Linux ABI. TSM specific
options are encoded in the frontend request format where the options
like SEV:vmpl (privilege level) can be specified and TSMs that do not
support them can decide to ignore them or fail if they are specified.
For now, "privlevel=" and "format=" are the only implemented options.

Example of establishing a tsm key and dumping the provider-specific
report:

    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 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C

Now, this patch ends up being a fairly simple custom-key format because
most of the follow-on work that happens after publishing a TSM-wrapped
public-key is performed by userspace. The TSM key is just one step in
establishing a shared secret that can be used to unlock other keys. For
example a user-key could be populated with the resulting shared secret
and that could be used as a master-key for an encrypted-key
(security/keys/encrypted-keys/encrypted.c).

While the discussion that led to this proposal hinted at a new
trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
the TSM [2], more work is needed to fetch a secret from the TSM
directly. The trusted-key core expects a pre-established secure channel
to seal and unseal secrets locally. For that reason a "tsm" flavor
trusted-key is saved for follow on work. That will likely starting as a
wrapper around SNP_GET_DERIVED_KEY.

Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@mail.gmail.com [2]
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Samuel Ortiz <sameo@rivosinc.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/keys/tsm.h     |   71 ++++++++++++
 security/keys/Kconfig  |   12 ++
 security/keys/Makefile |    1 
 security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 366 insertions(+)
 create mode 100644 include/keys/tsm.h
 create mode 100644 security/keys/tsm.c

diff --git a/include/keys/tsm.h b/include/keys/tsm.h
new file mode 100644
index 000000000000..61a81017d8f5
--- /dev/null
+++ b/include/keys/tsm.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __TSM_H
+#define __TSM_H
+
+#include <linux/types.h>
+#include <linux/module.h>
+
+/*
+ * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
+ * report formats. This mirrors the trusted/encrypted key blob max size.
+ */
+#define TSM_DATA_MAX 32767
+#define TSM_PUBKEY_MAX 64
+#define TSM_FORMAT_MAX 16
+
+/**
+ * DOC: TSM Keys
+ *
+ * Trusted Security Module Keys are a common provider of blobs that
+ * facilitate key-exchange between a TVM (confidential computing guest)
+ * and an attestation service. A TSM key combines a user-defined blob
+ * (likely a public-key for a key-exchance protocol) with a signed
+ * attestation report. That combined blob is then used to obtain
+ * secrets provided by an agent that can validate the attestation
+ * report.
+ *
+ * A full implementation uses a tsm key to, for example, establish a
+ * shared secret and then use that communication channel to instantiate
+ * other keys. The expectation is that the requester of the tsm key
+ * knows a priori the key-exchange protocol associated with the
+ * 'pubkey'.
+ *
+ * The attestation report format is TSM provider specific, when / if a
+ * standard materializes it is only a change to the auth_blob_desc
+ * member of 'struct tsm_key_payload', to convey that common format.
+ */
+
+/**
+ * struct tsm_key_payload - generic payload for vendor TSM blobs
+ * @privlevel: optional privilege level to associate with @pubkey
+ * @pubkey_len: how much of @pubkey is valid
+ * @pubkey: the public key-exchange blob to include in the attestation report
+ * @auth_blob_desc: base ascii descriptor of @auth_blob
+ * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
+ * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
+ * @auth_blob: TSM specific attestation report blob
+ */
+struct tsm_key_payload {
+	int privlevel;
+	size_t pubkey_len;
+	u8 pubkey[TSM_PUBKEY_MAX];
+	const char *auth_blob_desc;
+	char auth_blob_format[TSM_FORMAT_MAX];
+	size_t auth_blob_len;
+	u8 *auth_blob;
+};
+
+/*
+ * arch specific ops, only one is expected to be registered at a time
+ * i.e. only one of SEV, TDX, COVE, etc.
+ */
+struct tsm_key_ops {
+	const char *name;
+	struct module *module;
+	int (*auth_new)(struct tsm_key_payload *t, void *provider_data);
+};
+
+int register_tsm_provider(const struct tsm_key_ops *ops, void *provider_data);
+void unregister_tsm_provider(const struct tsm_key_ops *ops);
+
+#endif /* __TSM_H */
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index abb03a1b2a5c..f530cbd876fc 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -133,3 +133,15 @@ config KEY_NOTIFICATIONS
 	  on keys and keyrings on which the caller has View permission.
 	  This makes use of pipes to handle the notification buffer and
 	  provides KEYCTL_WATCH_KEY to enable/disable watches.
+
+config TSM_KEYS
+	tristate "TSM-based (Confidential Computing) keys"
+	help
+	  TSM (Trusted Security Module) key support arranges to convey a blob to
+	  a confidentiality and integrity protected virtual machine / guest
+	  (TVM). The blob instantiation flow involves submitting the measurement
+	  report of a TVM, obtained from the platform TSM, to a remote
+	  attestation service that only provides the key if the report passes
+	  validation. The report format is vendor specific and the validation is
+	  user policy, so this feature requires a /etc/request-key.conf handler
+	  for communicating with the remote attestation provider.
diff --git a/security/keys/Makefile b/security/keys/Makefile
index 5f40807f05b3..96972a4815d5 100644
--- a/security/keys/Makefile
+++ b/security/keys/Makefile
@@ -28,5 +28,6 @@ obj-$(CONFIG_ASYMMETRIC_KEY_TYPE) += keyctl_pkey.o
 # Key types
 #
 obj-$(CONFIG_BIG_KEYS) += big_key.o
+obj-$(CONFIG_TSM_KEYS) += tsm.o
 obj-$(CONFIG_TRUSTED_KEYS) += trusted-keys/
 obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
diff --git a/security/keys/tsm.c b/security/keys/tsm.c
new file mode 100644
index 000000000000..d9532319f819
--- /dev/null
+++ b/security/keys/tsm.c
@@ -0,0 +1,282 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/err.h>
+#include <linux/uuid.h>
+#include <linux/slab.h>
+#include <linux/rwsem.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/parser.h>
+#include <linux/cleanup.h>
+#include <linux/key-type.h>
+
+#include <keys/tsm.h>
+#include <keys/user-type.h>
+
+static struct tsm_provider {
+	const struct tsm_key_ops *ops;
+	void *data;
+} provider;
+static DECLARE_RWSEM(tsm_key_rwsem);
+
+static const struct tsm_key_ops *get_ops(void)
+{
+	down_read(&tsm_key_rwsem);
+	return provider.ops;
+}
+
+static void *get_data(void)
+{
+	lockdep_assert_held(&tsm_key_rwsem);
+	return provider.data;
+}
+
+static void put_ops(void)
+{
+	up_read(&tsm_key_rwsem);
+}
+
+int register_tsm_provider(const struct tsm_key_ops *ops,
+			  void *provider_data)
+{
+	const struct tsm_key_ops *conflict;
+	int rc;
+
+	down_write(&tsm_key_rwsem);
+	conflict = provider.ops;
+	if (conflict) {
+		pr_err("\"%s\" ops already registered\n", conflict->name);
+		rc = -EEXIST;
+		goto out;
+	}
+	try_module_get(ops->module);
+	provider.ops = ops;
+	provider.data = provider_data;
+	rc = 0;
+out:
+	up_write(&tsm_key_rwsem);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(register_tsm_provider);
+
+void unregister_tsm_provider(const struct tsm_key_ops *ops)
+{
+	down_write(&tsm_key_rwsem);
+	provider.ops = NULL;
+	provider.data = NULL;
+	module_put(ops->module);
+	up_write(&tsm_key_rwsem);
+}
+EXPORT_SYMBOL_GPL(unregister_tsm_provider);
+
+enum {
+	Opt_err,
+	Opt_auth,
+	Opt_format,
+	Opt_privlevel,
+};
+
+static const match_table_t tsm_tokens = {
+	{ Opt_auth, "auth" },
+	{ Opt_privlevel, "privlevel=%s" },
+	{ Opt_format, "format=%s" },
+	{ Opt_err, NULL },
+};
+
+/*
+ * tsm_parse - parse the tsm request data
+ *
+ * input format: "auth <hex pubkey data> [options]"
+ *
+ * Checks for options and parses a hex blob of data to be wrapped by the
+ * TSM attestation format.
+ *
+ * options:
+ *     privlevel= integer for selecting the privelege level of the
+ *                request, if the platform TSM supports that concept. To
+ *                date only SEV accepts this option. Default 0.
+ *     format=    string modifier for the format, if the platform TSM
+ *                supports multiple formats. To date only SEV accepts an
+ *                "extended" argument. Default "".
+ *
+ * On success returns 0, otherwise -EINVAL.
+ */
+static int tsm_parse(char *input, struct tsm_key_payload *t)
+{
+	substring_t args[MAX_OPT_ARGS];
+	unsigned long optmask = 0;
+	unsigned int privlevel;
+	int token, rc;
+	char *p;
+
+	/* all tsm keys must start with "auth" as a placeholder command */
+	p = strsep(&input, " \t");
+	if (!p)
+		return -EINVAL;
+	token = match_token(p, tsm_tokens, args);
+	switch (token) {
+	case Opt_auth:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* next is the pubkey hex blob */
+	p = strsep(&input, " \t");
+	if (!p)
+		return -EINVAL;
+	t->pubkey_len = strlen(p) / 2;
+	if (t->pubkey_len > TSM_PUBKEY_MAX)
+		return -EINVAL;
+	rc = hex2bin(t->pubkey, p, t->pubkey_len);
+	if (rc < 0)
+		return -EINVAL;
+
+	/* last is zero or more options */
+	while ((p = strsep(&input, " \t"))) {
+		if (*p == '\0' || *p == ' ' || *p == '\t')
+			continue;
+		token = match_token(p, tsm_tokens, args);
+		/* each option can appear only once */
+		if (test_and_set_bit(token, &optmask))
+			return -EINVAL;
+		switch (token) {
+		case Opt_privlevel:
+			rc = kstrtouint(args[0].from, 16, &privlevel);
+			if (rc)
+				return rc;
+			t->privlevel = privlevel;
+			break;
+		case Opt_format:
+			if (strlen(args[0].from) >= TSM_FORMAT_MAX)
+				return -EINVAL;
+			strscpy(t->auth_blob_format, args[0].from,
+				TSM_FORMAT_MAX);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static int tsm_instantiate(struct key *key, struct key_preparsed_payload *prep)
+{
+	size_t datalen = prep->datalen;
+	const struct tsm_key_ops *ops;
+	int rc;
+
+	if (datalen <= 0 || datalen > TSM_DATA_MAX || !prep->data)
+		return -EINVAL;
+
+	char *datablob __free(kfree) =
+		kmemdup_nul(prep->data, datalen, GFP_KERNEL);
+	if (!datablob)
+		return -ENOMEM;
+
+	struct tsm_key_payload *t __free(kfree) =
+		kzalloc(sizeof(*t), GFP_KERNEL);
+	if (!t)
+		return -ENOMEM;
+
+	rc = tsm_parse(datablob, t);
+	if (rc < 0)
+		return rc;
+
+	ops = get_ops();
+	if (ops)
+		rc = ops->auth_new(t, get_data());
+	else
+		rc = -ENXIO;
+	put_ops();
+
+	if (rc)
+		return rc;
+
+	rc = key_payload_reserve(key, sizeof(*t) + t->auth_blob_len);
+	if (rc) {
+		kvfree(t->auth_blob);
+		return rc;
+	}
+
+	rcu_assign_keypointer(key, t);
+	no_free_ptr(t);
+	return 0;
+}
+
+/*
+ * tsm_read - format and copy out the tsm auth record
+ *
+ * The resulting datablob format is:
+ * <pubkey blob> <auth blob desc[:format]> <auth blob>
+ *
+ * On success, return to userspace the size of the formatted payload.
+ */
+static long tsm_read(const struct key *key, char *buffer, size_t buflen)
+{
+	size_t asciiblob_len, desc_len;
+	struct tsm_key_payload *t;
+	char *buf, *format = NULL;
+	const int nr_spaces = 2;
+
+	t = dereference_key_locked(key);
+
+	desc_len = strlen(t->auth_blob_desc);
+	if (t->auth_blob_format[0]) {
+		format = &t->auth_blob_format[0];
+		desc_len += strlen(format) + 1;
+	}
+
+	asciiblob_len =
+		t->pubkey_len * 2 + desc_len + t->auth_blob_len * 2 + nr_spaces;
+
+	if (!buffer || buflen < asciiblob_len)
+		return asciiblob_len;
+
+	buf = bin2hex(buffer, t->pubkey, t->pubkey_len);
+	if (format)
+		buf += sprintf(buf, " %s:%s ", t->auth_blob_desc, format);
+	else
+		buf += sprintf(buf, " %s ", t->auth_blob_desc);
+	buf = bin2hex(buf, t->auth_blob, t->auth_blob_len);
+
+	/* sanity check for future changes to this function */
+	WARN_ON_ONCE(buf - buffer != asciiblob_len);
+
+	return asciiblob_len;
+}
+
+static void tsm_destroy(struct key *key)
+{
+	struct tsm_key_payload *t = key->payload.data[0];
+
+	kvfree(t->auth_blob);
+	kfree(t);
+}
+
+static struct key_type key_type_tsm = {
+	.name = "tsm",
+	.instantiate = tsm_instantiate,
+	.destroy = tsm_destroy,
+	.describe = user_describe,
+	.read = tsm_read,
+};
+
+static int __init tsm_key_init(void)
+{
+	return register_key_type(&key_type_tsm);
+}
+module_init(tsm_key_init);
+
+static void __exit tsm_key_exit(void)
+{
+	unregister_key_type(&key_type_tsm);
+}
+module_exit(tsm_key_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports as Key payloads");

Re: [PATCH 1/4] keys: Introduce tsm keys

[Jarkko Sakkinen]

On Fri Jul 28, 2023 at 7:30 PM UTC, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its run state and use that with a
> key-exchange protocol to establish a shared secret with a third-party /
> remote attestation agent. The concept is common across TSMs, but the

This is obfuscated "white paper" alike language.

I have no idea what TSM's and TVM's are and I do not want to know. Even
confidential computing is useless buzzword in the context of providing
a key type for attestation.

I would replace "tsm" with "attestation".

BR, Jarkko

Re: [PATCH 1/4] keys: Introduce tsm keys

[Peter Gonda]

On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its run state and use that with a
> key-exchange protocol to establish a shared secret with a third-party /
> remote attestation agent. The concept is common across TSMs, but the
> implementations are unfortunately vendor specific. While the industry
> grapples with a common definition of this attestation format [1], Linux
> need not make this problem worse by defining a new ABI per TSM that
> wants to perform a similar operation. The current momentum has been to
> invent new ioctl-ABI per TSM per function which at best is an abdication
> of the kernel's responsibility to make common infrastructure concepts
> share common ABI.
>
> The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> more, is to define a new key type that produces a TSM common blob format
> and moves the vendor specificity inside that envelope. The common Linux
> definition is:
>
>     "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"
>
> This approach later allows for the standardization of the attestation
> blob format without needing to change the Linux ABI. TSM specific
> options are encoded in the frontend request format where the options
> like SEV:vmpl (privilege level) can be specified and TSMs that do not
> support them can decide to ignore them or fail if they are specified.
> For now, "privlevel=" and "format=" are the only implemented options.
>
> Example of establishing a tsm key and dumping the provider-specific
> report:
>
>     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 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C

What is the purpose of this report? What does it prove to whom? I'm a
bit confused because it doesn't seem like there is an ability for a
remote party to participate in a challenge and response to introduce
any freshness into this protocol.

Also shouldn't the report have a little more context into the key we
are signing? For instance what type of public key is this? And what is
its purpose? In your example this isn't even a valid public key.

>
> Now, this patch ends up being a fairly simple custom-key format because
> most of the follow-on work that happens after publishing a TSM-wrapped
> public-key is performed by userspace. The TSM key is just one step in
> establishing a shared secret that can be used to unlock other keys. For
> example a user-key could be populated with the resulting shared secret
> and that could be used as a master-key for an encrypted-key
> (security/keys/encrypted-keys/encrypted.c).
>
> While the discussion that led to this proposal hinted at a new
> trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
> the TSM [2], more work is needed to fetch a secret from the TSM
> directly. The trusted-key core expects a pre-established secure channel
> to seal and unseal secrets locally. For that reason a "tsm" flavor
> trusted-key is saved for follow on work. That will likely starting as a
> wrapper around SNP_GET_DERIVED_KEY.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@mail.gmail.com [2]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  include/keys/tsm.h     |   71 ++++++++++++
>  security/keys/Kconfig  |   12 ++
>  security/keys/Makefile |    1
>  security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 366 insertions(+)
>  create mode 100644 include/keys/tsm.h
>  create mode 100644 security/keys/tsm.c
>
> diff --git a/include/keys/tsm.h b/include/keys/tsm.h
> new file mode 100644
> index 000000000000..61a81017d8f5
> --- /dev/null
> +++ b/include/keys/tsm.h
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_H
> +#define __TSM_H
> +
> +#include <linux/types.h>
> +#include <linux/module.h>
> +
> +/*
> + * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
> + * report formats. This mirrors the trusted/encrypted key blob max size.
> + */
> +#define TSM_DATA_MAX 32767
> +#define TSM_PUBKEY_MAX 64
> +#define TSM_FORMAT_MAX 16
> +
> +/**
> + * DOC: TSM Keys
> + *
> + * Trusted Security Module Keys are a common provider of blobs that
> + * facilitate key-exchange between a TVM (confidential computing guest)
> + * and an attestation service. A TSM key combines a user-defined blob

Are we limited to only doing key-exchanges between guests and
attestation services? What if some user would like to handle the
attestation verification without a service?

> + * (likely a public-key for a key-exchance protocol) with a signed

key-exchange

> + * attestation report. That combined blob is then used to obtain
> + * secrets provided by an agent that can validate the attestation
> + * report.
> + *
> + * A full implementation uses a tsm key to, for example, establish a

Should 'TSM' be capitalized everywhere? Or does it not matter?

> + * shared secret and then use that communication channel to instantiate
> + * other keys. The expectation is that the requester of the tsm key
> + * knows a priori the key-exchange protocol associated with the
> + * 'pubkey'.

Can we instead be very specific about what protocols and cryptography
are being used?

> + *
> + * The attestation report format is TSM provider specific, when / if a

I'm confused about the TSM terminology and what a TSM provider is. Is
TSM the confidential compute framework of the vendor? So for Intel
this is TDX, and the TSM provider is the SEAM module?

> + * standard materializes it is only a change to the auth_blob_desc
> + * member of 'struct tsm_key_payload', to convey that common format.
> + */
> +
> +/**
> + * struct tsm_key_payload - generic payload for vendor TSM blobs
> + * @privlevel: optional privilege level to associate with @pubkey
> + * @pubkey_len: how much of @pubkey is valid
> + * @pubkey: the public key-exchange blob to include in the attestation report
> + * @auth_blob_desc: base ascii descriptor of @auth_blob
> + * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
> + * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
> + * @auth_blob: TSM specific attestation report blob
> + */
> +struct tsm_key_payload {
> +       int privlevel;
> +       size_t pubkey_len;
> +       u8 pubkey[TSM_PUBKEY_MAX];
> +       const char *auth_blob_desc;
> +       char auth_blob_format[TSM_FORMAT_MAX];
> +       size_t auth_blob_len;
> +       u8 *auth_blob;
> +};

How is freshness incorporated into the key exchange protocol? Wouldn't
we need to do a challenge response between each remote party that we
need to attest the provenance of @pubkey too?

> +
> +/*
> + * tsm_parse - parse the tsm request data
> + *
> + * input format: "auth <hex pubkey data> [options]"
> + *
> + * Checks for options and parses a hex blob of data to be wrapped by the
> + * TSM attestation format.
> + *
> + * options:
> + *     privlevel= integer for selecting the privelege level of the

privilege

> + *                request, if the platform TSM supports that concept. To
> + *                date only SEV accepts this option. Default 0.

SEV-SNP or just SNP? Plain SEV or SEV-ES doesn't actually support this
interface at all.


> + *     format=    string modifier for the format, if the platform TSM
> + *                supports multiple formats. To date only SEV accepts an
> + *                "extended" argument. Default "".
> + *
> + * On success returns 0, otherwise -EINVAL.
> + */

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Peter Gonda wrote:
> On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > One of the common operations of a TSM (Trusted Security Module) is to
> > provide a way for a TVM (confidential computing guest execution
> > environment) to take a measurement of its run state and use that with a
> > key-exchange protocol to establish a shared secret with a third-party /
> > remote attestation agent. The concept is common across TSMs, but the
> > implementations are unfortunately vendor specific. While the industry
> > grapples with a common definition of this attestation format [1], Linux
> > need not make this problem worse by defining a new ABI per TSM that
> > wants to perform a similar operation. The current momentum has been to
> > invent new ioctl-ABI per TSM per function which at best is an abdication
> > of the kernel's responsibility to make common infrastructure concepts
> > share common ABI.
> >
> > The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> > more, is to define a new key type that produces a TSM common blob format
> > and moves the vendor specificity inside that envelope. The common Linux
> > definition is:
> >
> >     "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"
> >
> > This approach later allows for the standardization of the attestation
> > blob format without needing to change the Linux ABI. TSM specific
> > options are encoded in the frontend request format where the options
> > like SEV:vmpl (privilege level) can be specified and TSMs that do not
> > support them can decide to ignore them or fail if they are specified.
> > For now, "privlevel=" and "format=" are the only implemented options.
> >
> > Example of establishing a tsm key and dumping the provider-specific
> > report:
> >
> >     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 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> 
> What is the purpose of this report? What does it prove to whom? I'm a
> bit confused because it doesn't seem like there is an ability for a
> remote party to participate in a challenge and response to introduce
> any freshness into this protocol.
> 
> Also shouldn't the report have a little more context into the key we
> are signing? For instance what type of public key is this? And what is
> its purpose? In your example this isn't even a valid public key.
> 
> >
> > Now, this patch ends up being a fairly simple custom-key format because
> > most of the follow-on work that happens after publishing a TSM-wrapped
> > public-key is performed by userspace. The TSM key is just one step in
> > establishing a shared secret that can be used to unlock other keys. For
> > example a user-key could be populated with the resulting shared secret
> > and that could be used as a master-key for an encrypted-key
> > (security/keys/encrypted-keys/encrypted.c).
> >
> > While the discussion that led to this proposal hinted at a new
> > trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
> > the TSM [2], more work is needed to fetch a secret from the TSM
> > directly. The trusted-key core expects a pre-established secure channel
> > to seal and unseal secrets locally. For that reason a "tsm" flavor
> > trusted-key is saved for follow on work. That will likely starting as a
> > wrapper around SNP_GET_DERIVED_KEY.
> >
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@mail.gmail.com [2]
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: David Howells <dhowells@redhat.com>
> > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  include/keys/tsm.h     |   71 ++++++++++++
> >  security/keys/Kconfig  |   12 ++
> >  security/keys/Makefile |    1
> >  security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 366 insertions(+)
> >  create mode 100644 include/keys/tsm.h
> >  create mode 100644 security/keys/tsm.c
> >
> > diff --git a/include/keys/tsm.h b/include/keys/tsm.h
> > new file mode 100644
> > index 000000000000..61a81017d8f5
> > --- /dev/null
> > +++ b/include/keys/tsm.h
> > @@ -0,0 +1,71 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_H
> > +#define __TSM_H
> > +
> > +#include <linux/types.h>
> > +#include <linux/module.h>
> > +
> > +/*
> > + * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
> > + * report formats. This mirrors the trusted/encrypted key blob max size.
> > + */
> > +#define TSM_DATA_MAX 32767
> > +#define TSM_PUBKEY_MAX 64
> > +#define TSM_FORMAT_MAX 16
> > +
> > +/**
> > + * DOC: TSM Keys
> > + *
> > + * Trusted Security Module Keys are a common provider of blobs that
> > + * facilitate key-exchange between a TVM (confidential computing guest)
> > + * and an attestation service. A TSM key combines a user-defined blob
> 
> Are we limited to only doing key-exchanges between guests and
> attestation services? What if some user would like to handle the
> attestation verification without a service?

From the kernel perspective it does not matter, it is just marshalling
the quote data. I assume local attestation could be built around this,
but that's all user-space policy.

> 
> > + * (likely a public-key for a key-exchance protocol) with a signed
> 
> key-exchange

got it.

> 
> > + * attestation report. That combined blob is then used to obtain
> > + * secrets provided by an agent that can validate the attestation
> > + * report.
> > + *
> > + * A full implementation uses a tsm key to, for example, establish a
> 
> Should 'TSM' be capitalized everywhere? Or does it not matter?

Probably should be.

> > + * shared secret and then use that communication channel to instantiate
> > + * other keys. The expectation is that the requester of the tsm key
> > + * knows a priori the key-exchange protocol associated with the
> > + * 'pubkey'.
> 
> Can we instead be very specific about what protocols and cryptography
> are being used?

Again this is a contract to which the kernel is not a party. The
requester knows the significance of the user-data, and it knows where to
send the combined user-data plus quote to provision further secrets.

Not that I like that arrangement, but the kernel is not enabled by these
TSM implementations to know much more than "user-data in", "report out".

> 
> > + *
> > + * The attestation report format is TSM provider specific, when / if a
> 
> I'm confused about the TSM terminology and what a TSM provider is. Is
> TSM the confidential compute framework of the vendor? So for Intel
> this is TDX, and the TSM provider is the SEAM module?

Yes, I borrowed this term from the TDISP specification where the "Trusted
Security Module" is the overarching term for the confidential compute
infrastructure for the vendor. Yes, TDX + SEAM is a TSM and SEV-SNP +
PSP is a TSM.

> 
> > + * standard materializes it is only a change to the auth_blob_desc
> > + * member of 'struct tsm_key_payload', to convey that common format.
> > + */
> > +
> > +/**
> > + * struct tsm_key_payload - generic payload for vendor TSM blobs
> > + * @privlevel: optional privilege level to associate with @pubkey
> > + * @pubkey_len: how much of @pubkey is valid
> > + * @pubkey: the public key-exchange blob to include in the attestation report
> > + * @auth_blob_desc: base ascii descriptor of @auth_blob
> > + * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
> > + * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
> > + * @auth_blob: TSM specific attestation report blob
> > + */
> > +struct tsm_key_payload {
> > +       int privlevel;
> > +       size_t pubkey_len;
> > +       u8 pubkey[TSM_PUBKEY_MAX];
> > +       const char *auth_blob_desc;
> > +       char auth_blob_format[TSM_FORMAT_MAX];
> > +       size_t auth_blob_len;
> > +       u8 *auth_blob;
> > +};
> 
> How is freshness incorporated into the key exchange protocol? Wouldn't
> we need to do a challenge response between each remote party that we
> need to attest the provenance of @pubkey too?

That's left to userspace.

> 
> > +
> > +/*
> > + * tsm_parse - parse the tsm request data
> > + *
> > + * input format: "auth <hex pubkey data> [options]"
> > + *
> > + * Checks for options and parses a hex blob of data to be wrapped by the
> > + * TSM attestation format.
> > + *
> > + * options:
> > + *     privlevel= integer for selecting the privelege level of the
> 
> privilege

got it.

> 
> > + *                request, if the platform TSM supports that concept. To
> > + *                date only SEV accepts this option. Default 0.
> 
> SEV-SNP or just SNP? Plain SEV or SEV-ES doesn't actually support this
> interface at all.

Yeah, I was using "SEV" as a shorthand for the AMD "TSM". So maybe just
"SNP" is appropriate?

Re: [PATCH 1/4] keys: Introduce tsm keys

[Peter Gonda]

On Mon, Jul 31, 2023 at 11:48 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Peter Gonda wrote:
> > On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > One of the common operations of a TSM (Trusted Security Module) is to
> > > provide a way for a TVM (confidential computing guest execution
> > > environment) to take a measurement of its run state and use that with a
> > > key-exchange protocol to establish a shared secret with a third-party /
> > > remote attestation agent. The concept is common across TSMs, but the
> > > implementations are unfortunately vendor specific. While the industry
> > > grapples with a common definition of this attestation format [1], Linux
> > > need not make this problem worse by defining a new ABI per TSM that
> > > wants to perform a similar operation. The current momentum has been to
> > > invent new ioctl-ABI per TSM per function which at best is an abdication
> > > of the kernel's responsibility to make common infrastructure concepts
> > > share common ABI.
> > >
> > > The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> > > more, is to define a new key type that produces a TSM common blob format
> > > and moves the vendor specificity inside that envelope. The common Linux
> > > definition is:
> > >
> > >     "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"
> > >
> > > This approach later allows for the standardization of the attestation
> > > blob format without needing to change the Linux ABI. TSM specific
> > > options are encoded in the frontend request format where the options
> > > like SEV:vmpl (privilege level) can be specified and TSMs that do not
> > > support them can decide to ignore them or fail if they are specified.
> > > For now, "privlevel=" and "format=" are the only implemented options.
> > >
> > > Example of establishing a tsm key and dumping the provider-specific
> > > report:
> > >
> > >     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 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> >
> > What is the purpose of this report? What does it prove to whom? I'm a
> > bit confused because it doesn't seem like there is an ability for a
> > remote party to participate in a challenge and response to introduce
> > any freshness into this protocol.
> >
> > Also shouldn't the report have a little more context into the key we
> > are signing? For instance what type of public key is this? And what is
> > its purpose? In your example this isn't even a valid public key.
> >
> > >
> > > Now, this patch ends up being a fairly simple custom-key format because
> > > most of the follow-on work that happens after publishing a TSM-wrapped
> > > public-key is performed by userspace. The TSM key is just one step in
> > > establishing a shared secret that can be used to unlock other keys. For
> > > example a user-key could be populated with the resulting shared secret
> > > and that could be used as a master-key for an encrypted-key
> > > (security/keys/encrypted-keys/encrypted.c).
> > >
> > > While the discussion that led to this proposal hinted at a new
> > > trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
> > > the TSM [2], more work is needed to fetch a secret from the TSM
> > > directly. The trusted-key core expects a pre-established secure channel
> > > to seal and unseal secrets locally. For that reason a "tsm" flavor
> > > trusted-key is saved for follow on work. That will likely starting as a
> > > wrapper around SNP_GET_DERIVED_KEY.
> > >
> > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > > Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@mail.gmail.com [2]
> > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > Cc: David Howells <dhowells@redhat.com>
> > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > ---
> > >  include/keys/tsm.h     |   71 ++++++++++++
> > >  security/keys/Kconfig  |   12 ++
> > >  security/keys/Makefile |    1
> > >  security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  4 files changed, 366 insertions(+)
> > >  create mode 100644 include/keys/tsm.h
> > >  create mode 100644 security/keys/tsm.c
> > >
> > > diff --git a/include/keys/tsm.h b/include/keys/tsm.h
> > > new file mode 100644
> > > index 000000000000..61a81017d8f5
> > > --- /dev/null
> > > +++ b/include/keys/tsm.h
> > > @@ -0,0 +1,71 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __TSM_H
> > > +#define __TSM_H
> > > +
> > > +#include <linux/types.h>
> > > +#include <linux/module.h>
> > > +
> > > +/*
> > > + * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
> > > + * report formats. This mirrors the trusted/encrypted key blob max size.
> > > + */
> > > +#define TSM_DATA_MAX 32767
> > > +#define TSM_PUBKEY_MAX 64
> > > +#define TSM_FORMAT_MAX 16
> > > +
> > > +/**
> > > + * DOC: TSM Keys
> > > + *
> > > + * Trusted Security Module Keys are a common provider of blobs that
> > > + * facilitate key-exchange between a TVM (confidential computing guest)
> > > + * and an attestation service. A TSM key combines a user-defined blob
> >
> > Are we limited to only doing key-exchanges between guests and
> > attestation services? What if some user would like to handle the
> > attestation verification without a service?
>
> From the kernel perspective it does not matter, it is just marshalling
> the quote data. I assume local attestation could be built around this,
> but that's all user-space policy.

Makes sense. Can we tweak the language of these comments then?

>
> >
> > > + * (likely a public-key for a key-exchance protocol) with a signed
> >
> > key-exchange
>
> got it.
>
> >
> > > + * attestation report. That combined blob is then used to obtain
> > > + * secrets provided by an agent that can validate the attestation
> > > + * report.
> > > + *
> > > + * A full implementation uses a tsm key to, for example, establish a
> >
> > Should 'TSM' be capitalized everywhere? Or does it not matter?
>
> Probably should be.
>
> > > + * shared secret and then use that communication channel to instantiate
> > > + * other keys. The expectation is that the requester of the tsm key
> > > + * knows a priori the key-exchange protocol associated with the
> > > + * 'pubkey'.
> >
> > Can we instead be very specific about what protocols and cryptography
> > are being used?
>
> Again this is a contract to which the kernel is not a party. The
> requester knows the significance of the user-data, and it knows where to
> send the combined user-data plus quote to provision further secrets.
>
> Not that I like that arrangement, but the kernel is not enabled by these
> TSM implementations to know much more than "user-data in", "report out".

Can you explain why using this key API is better than the ioctl
version? Is there an overhead to adding keys? You could imagine some
userspace application that receives RPCs and does some attestation for
each one, would adding then deleting a huge number of keys present any
issues?

>
> >
> > > + *
> > > + * The attestation report format is TSM provider specific, when / if a
> >
> > I'm confused about the TSM terminology and what a TSM provider is. Is
> > TSM the confidential compute framework of the vendor? So for Intel
> > this is TDX, and the TSM provider is the SEAM module?
>
> Yes, I borrowed this term from the TDISP specification where the "Trusted
> Security Module" is the overarching term for the confidential compute
> infrastructure for the vendor. Yes, TDX + SEAM is a TSM and SEV-SNP +
> PSP is a TSM.

Thanks for the explanation.

>
> >
> > > + * standard materializes it is only a change to the auth_blob_desc
> > > + * member of 'struct tsm_key_payload', to convey that common format.
> > > + */
> > > +
> > > +/**
> > > + * struct tsm_key_payload - generic payload for vendor TSM blobs
> > > + * @privlevel: optional privilege level to associate with @pubkey
> > > + * @pubkey_len: how much of @pubkey is valid
> > > + * @pubkey: the public key-exchange blob to include in the attestation report
> > > + * @auth_blob_desc: base ascii descriptor of @auth_blob
> > > + * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
> > > + * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
> > > + * @auth_blob: TSM specific attestation report blob
> > > + */
> > > +struct tsm_key_payload {
> > > +       int privlevel;
> > > +       size_t pubkey_len;
> > > +       u8 pubkey[TSM_PUBKEY_MAX];
> > > +       const char *auth_blob_desc;
> > > +       char auth_blob_format[TSM_FORMAT_MAX];
> > > +       size_t auth_blob_len;
> > > +       u8 *auth_blob;
> > > +};
> >
> > How is freshness incorporated into the key exchange protocol? Wouldn't
> > we need to do a challenge response between each remote party that we
> > need to attest the provenance of @pubkey too?
>
> That's left to userspace.

But you haven't allowed userspace to add any data into the quote other
than just the raw public key.

The current sevguest ioctl allows users to pass arbitrary userdata.
This would allow for some nonce to be included.

At a highlevel I'm not sure why this is better than each vendor having
their own driver. It doesn't seem that difficult for userspace to deal
with these systems given userspace will need to be written carefully
for these PKI protocols anyways.


>
> >
> > > + *                request, if the platform TSM supports that concept. To
> > > + *                date only SEV accepts this option. Default 0.
> >
> > SEV-SNP or just SNP? Plain SEV or SEV-ES doesn't actually support this
> > interface at all.
>
> Yeah, I was using "SEV" as a shorthand for the AMD "TSM". So maybe just
> "SNP" is appropriate?

I'm not sure what AMD prefers. I'll let them chime in.

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Peter Gonda wrote:
> On Mon, Jul 31, 2023 at 11:48 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > Peter Gonda wrote:
> > > On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > > >
> > > > One of the common operations of a TSM (Trusted Security Module) is to
> > > > provide a way for a TVM (confidential computing guest execution
> > > > environment) to take a measurement of its run state and use that with a
> > > > key-exchange protocol to establish a shared secret with a third-party /
> > > > remote attestation agent. The concept is common across TSMs, but the
> > > > implementations are unfortunately vendor specific. While the industry
> > > > grapples with a common definition of this attestation format [1], Linux
> > > > need not make this problem worse by defining a new ABI per TSM that
> > > > wants to perform a similar operation. The current momentum has been to
> > > > invent new ioctl-ABI per TSM per function which at best is an abdication
> > > > of the kernel's responsibility to make common infrastructure concepts
> > > > share common ABI.
> > > >
> > > > The proposal, targeted to conceptually work with TDX, SEV, COVE if not
> > > > more, is to define a new key type that produces a TSM common blob format
> > > > and moves the vendor specificity inside that envelope. The common Linux
> > > > definition is:
> > > >
> > > >     "<hex encoded pubkey> <blob descriptor> <hex encoded attestation blob>"
> > > >
> > > > This approach later allows for the standardization of the attestation
> > > > blob format without needing to change the Linux ABI. TSM specific
> > > > options are encoded in the frontend request format where the options
> > > > like SEV:vmpl (privilege level) can be specified and TSMs that do not
> > > > support them can decide to ignore them or fail if they are specified.
> > > > For now, "privlevel=" and "format=" are the only implemented options.
> > > >
> > > > Example of establishing a tsm key and dumping the provider-specific
> > > > report:
> > > >
> > > >     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 280877394 | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> > >
> > > What is the purpose of this report? What does it prove to whom? I'm a
> > > bit confused because it doesn't seem like there is an ability for a
> > > remote party to participate in a challenge and response to introduce
> > > any freshness into this protocol.
> > >
> > > Also shouldn't the report have a little more context into the key we
> > > are signing? For instance what type of public key is this? And what is
> > > its purpose? In your example this isn't even a valid public key.
> > >
> > > >
> > > > Now, this patch ends up being a fairly simple custom-key format because
> > > > most of the follow-on work that happens after publishing a TSM-wrapped
> > > > public-key is performed by userspace. The TSM key is just one step in
> > > > establishing a shared secret that can be used to unlock other keys. For
> > > > example a user-key could be populated with the resulting shared secret
> > > > and that could be used as a master-key for an encrypted-key
> > > > (security/keys/encrypted-keys/encrypted.c).
> > > >
> > > > While the discussion that led to this proposal hinted at a new
> > > > trusted-key (security/keys/trusted-keys/trusted_core.c) type rooted in
> > > > the TSM [2], more work is needed to fetch a secret from the TSM
> > > > directly. The trusted-key core expects a pre-established secure channel
> > > > to seal and unseal secrets locally. For that reason a "tsm" flavor
> > > > trusted-key is saved for follow on work. That will likely starting as a
> > > > wrapper around SNP_GET_DERIVED_KEY.
> > > >
> > > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > > > Link: http://lore.kernel.org/r/CAAH4kHYLETfPk-sMD-QSJd0fJ7Qnt04FBwFuEkpnehB5U7D_yw@mail.gmail.com [2]
> > > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > Cc: David Howells <dhowells@redhat.com>
> > > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > > Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > >  include/keys/tsm.h     |   71 ++++++++++++
> > > >  security/keys/Kconfig  |   12 ++
> > > >  security/keys/Makefile |    1
> > > >  security/keys/tsm.c    |  282 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 366 insertions(+)
> > > >  create mode 100644 include/keys/tsm.h
> > > >  create mode 100644 security/keys/tsm.c
> > > >
> > > > diff --git a/include/keys/tsm.h b/include/keys/tsm.h
> > > > new file mode 100644
> > > > index 000000000000..61a81017d8f5
> > > > --- /dev/null
> > > > +++ b/include/keys/tsm.h
> > > > @@ -0,0 +1,71 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +#ifndef __TSM_H
> > > > +#define __TSM_H
> > > > +
> > > > +#include <linux/types.h>
> > > > +#include <linux/module.h>
> > > > +
> > > > +/*
> > > > + * @TSM_DATA_MAX: a reasonable max with enough space for known attestation
> > > > + * report formats. This mirrors the trusted/encrypted key blob max size.
> > > > + */
> > > > +#define TSM_DATA_MAX 32767
> > > > +#define TSM_PUBKEY_MAX 64
> > > > +#define TSM_FORMAT_MAX 16
> > > > +
> > > > +/**
> > > > + * DOC: TSM Keys
> > > > + *
> > > > + * Trusted Security Module Keys are a common provider of blobs that
> > > > + * facilitate key-exchange between a TVM (confidential computing guest)
> > > > + * and an attestation service. A TSM key combines a user-defined blob
> > >
> > > Are we limited to only doing key-exchanges between guests and
> > > attestation services? What if some user would like to handle the
> > > attestation verification without a service?
> >
> > From the kernel perspective it does not matter, it is just marshalling
> > the quote data. I assume local attestation could be built around this,
> > but that's all user-space policy.
> 
> Makes sense. Can we tweak the language of these comments then?

Will do.

> 
> >
> > >
> > > > + * (likely a public-key for a key-exchance protocol) with a signed
> > >
> > > key-exchange
> >
> > got it.
> >
> > >
> > > > + * attestation report. That combined blob is then used to obtain
> > > > + * secrets provided by an agent that can validate the attestation
> > > > + * report.
> > > > + *
> > > > + * A full implementation uses a tsm key to, for example, establish a
> > >
> > > Should 'TSM' be capitalized everywhere? Or does it not matter?
> >
> > Probably should be.
> >
> > > > + * shared secret and then use that communication channel to instantiate
> > > > + * other keys. The expectation is that the requester of the tsm key
> > > > + * knows a priori the key-exchange protocol associated with the
> > > > + * 'pubkey'.
> > >
> > > Can we instead be very specific about what protocols and cryptography
> > > are being used?
> >
> > Again this is a contract to which the kernel is not a party. The
> > requester knows the significance of the user-data, and it knows where to
> > send the combined user-data plus quote to provision further secrets.
> >
> > Not that I like that arrangement, but the kernel is not enabled by these
> > TSM implementations to know much more than "user-data in", "report out".
> 
> Can you explain why using this key API is better than the ioctl
> version? Is there an overhead to adding keys?

Setting aside that folks that have been involved in the Keyring
subsystem a lot longer than I are not keen on this usage [1], I expect
the overhead is negligible. Keys are already used in RPC scenarios and
can be destroyed immediately after being instantiated and read.

[1]: http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com

> You could imagine some userspace application that receives RPCs and
> does some attestation for each one, would adding then deleting a huge
> number of keys present any issues?

I can imagine a lot of scenarios, but reading the SEV-SNP whitepaper it
seems to imply that this is a launch-time one-off report that
establishes a channel to convey other secrets. So my expectation is that
this interface is used to bootstrap a guest and never again. Are you
aware of a high frequency use case for these reports?```

> > > > + * The attestation report format is TSM provider specific, when / if a
> > >
> > > I'm confused about the TSM terminology and what a TSM provider is. Is
> > > TSM the confidential compute framework of the vendor? So for Intel
> > > this is TDX, and the TSM provider is the SEAM module?
> >
> > Yes, I borrowed this term from the TDISP specification where the "Trusted
> > Security Module" is the overarching term for the confidential compute
> > infrastructure for the vendor. Yes, TDX + SEAM is a TSM and SEV-SNP +
> > PSP is a TSM.
> 
> Thanks for the explanation.
> 
> >
> > >
> > > > + * standard materializes it is only a change to the auth_blob_desc
> > > > + * member of 'struct tsm_key_payload', to convey that common format.
> > > > + */
> > > > +
> > > > +/**
> > > > + * struct tsm_key_payload - generic payload for vendor TSM blobs
> > > > + * @privlevel: optional privilege level to associate with @pubkey
> > > > + * @pubkey_len: how much of @pubkey is valid
> > > > + * @pubkey: the public key-exchange blob to include in the attestation report
> > > > + * @auth_blob_desc: base ascii descriptor of @auth_blob
> > > > + * @auth_blob_format: for TSMs with multiple formats, extend @auth_blob_desc
> > > > + * @auth_blob_len: TSM provider length of the array it publishes in @auth_blob
> > > > + * @auth_blob: TSM specific attestation report blob
> > > > + */
> > > > +struct tsm_key_payload {
> > > > +       int privlevel;
> > > > +       size_t pubkey_len;
> > > > +       u8 pubkey[TSM_PUBKEY_MAX];
> > > > +       const char *auth_blob_desc;
> > > > +       char auth_blob_format[TSM_FORMAT_MAX];
> > > > +       size_t auth_blob_len;
> > > > +       u8 *auth_blob;
> > > > +};
> > >
> > > How is freshness incorporated into the key exchange protocol? Wouldn't
> > > we need to do a challenge response between each remote party that we
> > > need to attest the provenance of @pubkey too?
> >
> > That's left to userspace.
> 
> But you haven't allowed userspace to add any data into the quote other
> than just the raw public key.

That is not allowed by the SNP firmware interface. The only input is the
64-byte user-buffer that the SNP whitepaper calls a public-key.

> The current sevguest ioctl allows users to pass arbitrary userdata.
> This would allow for some nonce to be included.

It's not arbitrary user-data, it is only meant to a pubkey per the "VM
Launch and Attestation" section of the SNP whitepaper.

> At a highlevel I'm not sure why this is better than each vendor having
> their own driver. It doesn't seem that difficult for userspace to deal
> with these systems given userspace will need to be written carefully
> for these PKI protocols anyways.

The common facilities can still be made common. Namely, the interface to
take in a pubkey / user-data and the location to pull the report need
not have any vendor specificity.

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dionna Amalie Glaze]

>
> > You could imagine some userspace application that receives RPCs and
> > does some attestation for each one, would adding then deleting a huge
> > number of keys present any issues?
>
> I can imagine a lot of scenarios, but reading the SEV-SNP whitepaper it
> seems to imply that this is a launch-time one-off report that
> establishes a channel to convey other secrets. So my expectation is that
> this interface is used to bootstrap a guest and never again. Are you
> aware of a high frequency use case for these reports?```
>

Attestations may be requested by RPCs themselves when a service
decides to allow a user to present their own challenge nonces that
should be included in the hardware attestation. The "once at boot"
workflow only works for a specific type of application.

> > > >
> > > > How is freshness incorporated into the key exchange protocol? Wouldn't
> > > > we need to do a challenge response between each remote party that we
> > > > need to attest the provenance of @pubkey too?
> > >
> > > That's left to userspace.
> >
> > But you haven't allowed userspace to add any data into the quote other
> > than just the raw public key.
>
> That is not allowed by the SNP firmware interface. The only input is the
> 64-byte user-buffer that the SNP whitepaper calls a public-key.
>

The whitepaper presents a hypothetical usage of the attestation
facility. It is not prescriptive. With only 64 bytes, you're most
likely to be providing a nonce or a hash, and not a full public key.
Indeed, you may be presenting sha512(nonce || pubkey).

> > The current sevguest ioctl allows users to pass arbitrary userdata.
> > This would allow for some nonce to be included.
>
> It's not arbitrary user-data, it is only meant to a pubkey per the "VM
> Launch and Attestation" section of the SNP whitepaper.
>

It really is arbitrary. We've also been discussing including hashed
vTPM quotes to tie them to the hardware attestation. That's not
necessarily how it'll be used going forward, but the interface needs
to allow for this flexibility.

> > At a highlevel I'm not sure why this is better than each vendor having
> > their own driver. It doesn't seem that difficult for userspace to deal
> > with these systems given userspace will need to be written carefully
> > for these PKI protocols anyways.
>
> The common facilities can still be made common. Namely, the interface to
> take in a pubkey / user-data and the location to pull the report need
> not have any vendor specificity.

I can understand the desire to abstract now that there are 2
datapoints instead of 1, but given that you've said folks aren't keen
on this usage of the key system and developers of these drivers are
also not keen, maybe we should let there be some vendor specifics
until we have a better idea how this will work with more technologies?
RISC-V and ARM have attestation facilities coming, and it might be
hard to shoehorn all their capabilities into this as well.

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

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Dionna Amalie Glaze wrote:
> >
> > > You could imagine some userspace application that receives RPCs and
> > > does some attestation for each one, would adding then deleting a huge
> > > number of keys present any issues?
> >
> > I can imagine a lot of scenarios, but reading the SEV-SNP whitepaper it
> > seems to imply that this is a launch-time one-off report that
> > establishes a channel to convey other secrets. So my expectation is that
> > this interface is used to bootstrap a guest and never again. Are you
> > aware of a high frequency use case for these reports?```
> >
> 
> Attestations may be requested by RPCs themselves when a service
> decides to allow a user to present their own challenge nonces that
> should be included in the hardware attestation. The "once at boot"
> workflow only works for a specific type of application.
> 
> > > > >
> > > > > How is freshness incorporated into the key exchange protocol? Wouldn't
> > > > > we need to do a challenge response between each remote party that we
> > > > > need to attest the provenance of @pubkey too?
> > > >
> > > > That's left to userspace.
> > >
> > > But you haven't allowed userspace to add any data into the quote other
> > > than just the raw public key.
> >
> > That is not allowed by the SNP firmware interface. The only input is the
> > 64-byte user-buffer that the SNP whitepaper calls a public-key.
> >
> 
> The whitepaper presents a hypothetical usage of the attestation
> facility. It is not prescriptive. With only 64 bytes, you're most
> likely to be providing a nonce or a hash, and not a full public key.
> Indeed, you may be presenting sha512(nonce || pubkey).
> 
> > > The current sevguest ioctl allows users to pass arbitrary userdata.
> > > This would allow for some nonce to be included.
> >
> > It's not arbitrary user-data, it is only meant to a pubkey per the "VM
> > Launch and Attestation" section of the SNP whitepaper.
> >
> 
> It really is arbitrary. We've also been discussing including hashed
> vTPM quotes to tie them to the hardware attestation. That's not
> necessarily how it'll be used going forward, but the interface needs
> to allow for this flexibility.

Yeah, my "it's not arbitrary" was too strong. What I meant is that Peter
seemed to be alluding to losing some ability to attach user-data to the
report in this new interface, and I was just pointing out that the same
ability to attach data is present in the proposal.

> > > At a highlevel I'm not sure why this is better than each vendor having
> > > their own driver. It doesn't seem that difficult for userspace to deal
> > > with these systems given userspace will need to be written carefully
> > > for these PKI protocols anyways.
> >
> > The common facilities can still be made common. Namely, the interface to
> > take in a pubkey / user-data and the location to pull the report need
> > not have any vendor specificity.
> 
> I can understand the desire to abstract now that there are 2
> datapoints instead of 1, but given that you've said folks aren't keen

s/folks/James/

Jarkko's concern I believe was more associated with my hand-waving about
trusted-keys.

> on this usage of the key system and developers of these drivers are
> also not keen, maybe we should let there be some vendor specifics
> until we have a better idea how this will work with more technologies?
> RISC-V and ARM have attestation facilities coming, and it might be
> hard to shoehorn all their capabilities into this as well.

No, the time to resolve that is now. SEV and TDX are the only ones I
have had the bandwidth to consider right now, but s390 has a thing and
the RISC-V thing seems to be at the point where they are willing to help
out here. This discomfort is the point. Linux has limited chances to
attack technical debt when it comes to ABI, and upstream acceptance is
that forcing function to collaborate.

I think the sysfs proposal is easy to stand up. At the same time I think
it will have challenges similar to the concern that James raised about
Keys becoming a blob transport layer. I.e. I am concerned Greg would
also have concerns with sysfs becoming a blob transport layer especially
if this not a "configure-once / configure-rarely" interface that would
be more amenable to sysfs. That's typical Linux, lets see the patches
and compare the approaches. I might still go the route of, as James
suggests, broaching the subject of Linux Keyring becoming a general
"blob-ring" facility.

Your response about how this might be used for high frequency RPC use
cases tends to bolster what I think Linux Keyring is better suited for
than sysfs. A common ioctl() ABI is a pain to deploy here without it
just unifying a bunch of vendor ioctls behind maybe one chardev which is
not much of a win / recipe for collaboration.

To me the discussion about how this is used and what is the role of Keys
is a useful community discussion. It is still too late to return to
per-vendor corners.

Re: [PATCH 1/4] keys: Introduce tsm keys

[Peter Gonda]

> > >
> > > > > + * shared secret and then use that communication channel to instantiate
> > > > > + * other keys. The expectation is that the requester of the tsm key
> > > > > + * knows a priori the key-exchange protocol associated with the
> > > > > + * 'pubkey'.
> > > >
> > > > Can we instead be very specific about what protocols and cryptography
> > > > are being used?
> > >
> > > Again this is a contract to which the kernel is not a party. The
> > > requester knows the significance of the user-data, and it knows where to
> > > send the combined user-data plus quote to provision further secrets.
> > >
> > > Not that I like that arrangement, but the kernel is not enabled by these
> > > TSM implementations to know much more than "user-data in", "report out".
> >
> > Can you explain why using this key API is better than the ioctl
> > version? Is there an overhead to adding keys?
>
> Setting aside that folks that have been involved in the Keyring
> subsystem a lot longer than I are not keen on this usage [1], I expect
> the overhead is negligible. Keys are already used in RPC scenarios and
> can be destroyed immediately after being instantiated and read.

OK the overhead is negligible. But why is this any better?

To me this seems strictly worse to me as a user since I have much less
input into the hardware attestation which is one of the primary
benefits of confidential compute. I don't want the kernel limiting
what cryptographic algorithm I use, or limiting attestation reports to
signing pubkeys.

I understand having a proliferation of similar drivers may not be
ideal but given the hardware lift required to make confidential
compute happen will we really see too many?

>
> [1]: http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Peter Gonda wrote:
> > > >
> > > > > > + * shared secret and then use that communication channel to instantiate
> > > > > > + * other keys. The expectation is that the requester of the tsm key
> > > > > > + * knows a priori the key-exchange protocol associated with the
> > > > > > + * 'pubkey'.
> > > > >
> > > > > Can we instead be very specific about what protocols and cryptography
> > > > > are being used?
> > > >
> > > > Again this is a contract to which the kernel is not a party. The
> > > > requester knows the significance of the user-data, and it knows where to
> > > > send the combined user-data plus quote to provision further secrets.
> > > >
> > > > Not that I like that arrangement, but the kernel is not enabled by these
> > > > TSM implementations to know much more than "user-data in", "report out".
> > >
> > > Can you explain why using this key API is better than the ioctl
> > > version? Is there an overhead to adding keys?
> >
> > Setting aside that folks that have been involved in the Keyring
> > subsystem a lot longer than I are not keen on this usage [1], I expect
> > the overhead is negligible. Keys are already used in RPC scenarios and
> > can be destroyed immediately after being instantiated and read.
> 
> OK the overhead is negligible. But why is this any better?
> 
> To me this seems strictly worse to me as a user since I have much less
> input into the hardware attestation which is one of the primary
> benefits of confidential compute. I don't want the kernel limiting
> what cryptographic algorithm I use, or limiting attestation reports to
> signing pubkeys.

The current proposal on the table is not to have the kernel enforce
anything with respect to the format of the "pubkey" payload. The only
feedback so far I have seen about improving the semantics here is
enforce a nonce which the ioctl() interface just has to trust userspace
is handling and the Keyring approach can enforce a callout for that
input.

> I understand having a proliferation of similar drivers may not be
> ideal but given the hardware lift required to make confidential
> compute happen will we really see too many?

From my perspective this discussion has already been worth it as some
people were unaware that security relevant development had started under
drivers/virt/coco/. The details of the kernel/user contract are coming
into focus.

In general, the point at which to have these types of discussions is at
the 1 -> 2 implementation transition, to my knowledge we are already up
to 5 of these things (AMD, Intel, RISC-V, ARM, S390).

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Peter Gonda wrote:
[..]
> OK the overhead is negligible. But why is this any better?

I need to answer this directly. This is better because vendorA is forced
to collaborate with vendorB about instantiation options. No more "stick
new feature that might be duplicative in a siloed driver in
drivers/virt/coco/$vendor". This is better for the kernel.

Once standardization materialized this is already ahead of the game
providing a front end to instantiate that common format. This interface
choice forces ongoing community collaboration on how options are
presented. It takes a position that all existing options that sevguest
already exports are in scope and provides a place to have a discussion
about how and when to add more options. This is better for
collaboration.

Re: [PATCH 1/4] keys: Introduce tsm keys

[Jarkko Sakkinen]

On Mon Jul 31, 2023 at 7:33 PM EEST, Peter Gonda wrote:
> What is the purpose of this report? What does it prove to whom? I'm a
> bit confused because it doesn't seem like there is an ability for a
> remote party to participate in a challenge and response to introduce
> any freshness into this protocol.
>
> Also shouldn't the report have a little more context into the key we
> are signing? For instance what type of public key is this? And what is
> its purpose? In your example this isn't even a valid public key.

Yeah, I agree.

It is pretty hard to even evaluate whether this should be in kernel or
could handled by the user space (perhaps with something less intrusive
added to the kernel).

With cover letter starting with not one but two three letter acronyms
that are not common vocabulary is already a red flag for me at least.

A lot more clarity is needed on what the heck this thing is anyway.

BR, Jarkko

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dan Williams]

Jarkko Sakkinen wrote:
> On Mon Jul 31, 2023 at 7:33 PM EEST, Peter Gonda wrote:
> > What is the purpose of this report? What does it prove to whom? I'm a
> > bit confused because it doesn't seem like there is an ability for a
> > remote party to participate in a challenge and response to introduce
> > any freshness into this protocol.
> >
> > Also shouldn't the report have a little more context into the key we
> > are signing? For instance what type of public key is this? And what is
> > its purpose? In your example this isn't even a valid public key.
> 
> Yeah, I agree.
> 
> It is pretty hard to even evaluate whether this should be in kernel or
> could handled by the user space (perhaps with something less intrusive
> added to the kernel).
> 
> With cover letter starting with not one but two three letter acronyms
> that are not common vocabulary is already a red flag for me at least.
> 
> A lot more clarity is needed on what the heck this thing is anyway.

Apologies Jarkko, the assumption of this code was that because
drivers/virt/coco/sevguest.c was already exporting an ABI that put no
definition on the payload that this new key type would not need to
either.

However, your questioning proves the point that stashing security
relevant ABI in drivers/virt/coco/ is not a recipe to get worthwhile
review from security minded kernel practitioners.

So I can see why my explanation of "just do what sevguest was getting
away with" is not a great answer. Lets try again.

As mentioned in the AMD whitepaper [1]. Confidential computing has a use
case for a guest to prove to an attestation agent that it is running in
a valid configuration before that agent deploys other keys and secrets
to that VM to run a workload. One way to do that is to wrap a report of
the launch state of the VM with a public-key and if that report passes
validation use that key to send encrypted payloads back to the guest.

In this model the kernel is only a conduit to get 64-bytes of data
signed by the report, and the kernel need not have any requirements on
that data. That said, it could. That's where I would look to
recommendations from Dionna and James and others about what contract the
kernel *could*  enforce to ensure that best security practices are being
deployed.  I expect that also helps this implementation cross the
threshold from "blob store" to "key" as the Keyring expects.

[1]: Section "VM Launch & Attestation" https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dionna Amalie Glaze]

> In this model the kernel is only a conduit to get 64-bytes of data
> signed by the report, and the kernel need not have any requirements on
> that data. That said, it could. That's where I would look to
> recommendations from Dionna and James and others about what contract the
> kernel *could*  enforce to ensure that best security practices are being
> deployed.  I expect that also helps this implementation cross the
> threshold from "blob store" to "key" as the Keyring expects.
>

I believe there is no security best practice here yet to enforce.

Attestation reports are a power feature, just like a TPM. A TPM
doesn't place any restriction on the format for its nonce, and like
James said, there's already a special entry point for TPM-wrapped keys
in the key API. Users have access to TPM quotes through /dev/tpm0 and
can pass whatever nonce. I don't think we need to limit access to the
interface this hardware gives us because we're trying to avoid another
char device by saying a report is a key.

The coming addition of the SVSM to further isolate the guest and
provide extra "security devices" is also something to be aware of.
There will be a vTPM protocol and a new type of attestation that's
rooted to VMPL0 while Linux is still in VMPL3. I don't think this will
make sev-guest an unnecessary device though, since it's still
undecided how the TPM hierarchy can bind itself to the hardware in a
non-adhoc manner: there's no "attested TPM" spec to have something
between the null hierarchy and the more persistent attestation key
hierarchy. And TCG isn't in the business of specifying how to
virtualize the TPM technology, so we might have to manually link the
two together by getting the tpm quote and then doing a further binding
operation with the sev-guest device.

So, can we give unfettered access to the hardware through not a Key
API but an Attestation API, and for historical reasons allow vTPM to
be its own thing with extra commands? The SVSM could allow users to
have access to more commands than getting an attestation report, like
a virtual HSM separate from the TPM. We wouldn't be able to access it
without a SEV-SNP-specific device. Does that mean it can't be
upstreamed? Do all confidential computing technologies specifically
not differentiate themselves in their capabilities to all use the same
kernel API for interaction?

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

Re: [PATCH 1/4] keys: Introduce tsm keys

[James Bottomley]

On Fri, 2023-08-04 at 09:37 -0700, Dionna Amalie Glaze wrote:
[...]
> 
> The coming addition of the SVSM to further isolate the guest and
> provide extra "security devices" is also something to be aware of.
> There will be a vTPM protocol and a new type of attestation that's
> rooted to VMPL0 while Linux is still in VMPL3. I don't think this
> will make sev-guest an unnecessary device though, since it's still
> undecided how the TPM hierarchy can bind itself to the hardware in a
> non-adhoc manner: there's no "attested TPM" spec to have something
> between the null hierarchy and the more persistent attestation key
> hierarchy. And TCG isn't in the business of specifying how to
> virtualize the TPM technology, so we might have to manually link the
> two together by getting the tpm quote and then doing a further
> binding operation with the sev-guest device.

Just on this one, it's already specified in the latest SVSM doc:

https://lore.kernel.org/linux-coco/a2f31400-9e1c-c12a-ad7f-ea0265a12068@amd.com/

The Service Attestation Data on page 36-37.  It says TPMT_PUBLIC of the
EK.  However, what it doesn't say is *which* EK.  I already sent in a
comment saying it should be the TCG template for the P-256 curve EK.

So asking the SVSM to give you the attestation report for the VTPM
service binds the EK of the vTPM.

James

Re: [PATCH 1/4] keys: Introduce tsm keys

[Dionna Amalie Glaze]

>
> Just on this one, it's already specified in the latest SVSM doc:
>
> https://lore.kernel.org/linux-coco/a2f31400-9e1c-c12a-ad7f-ea0265a12068@amd.com/
>
> The Service Attestation Data on page 36-37.  It says TPMT_PUBLIC of the
> EK.  However, what it doesn't say is *which* EK.  I already sent in a
> comment saying it should be the TCG template for the P-256 curve EK.
>
> So asking the SVSM to give you the attestation report for the VTPM
> service binds the EK of the vTPM.
>

Yes, thanks. It sounds like you have to ask the SVSM to certify the EK
separately from asking the TPM for a quote. We can't rely entirely on
the TPM API and avoid a sev-guest device for talking to the SVSM. Or
are you saying the SVSM attestation report will get encoded in the
x.509 EK certificate that the TPM API returns, such as the report is
in a cert extension? I'm less clear on how TPM software would
interpret the Issuer of that cert.

> James
>


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

Re: [PATCH 1/4] keys: Introduce tsm keys

[James Bottomley]

On Fri, 2023-08-04 at 10:07 -0700, Dionna Amalie Glaze wrote:
> > 
> > Just on this one, it's already specified in the latest SVSM doc:
> > 
> > https://lore.kernel.org/linux-coco/a2f31400-9e1c-c12a-ad7f-ea0265a12068@amd.com/
> > 
> > The Service Attestation Data on page 36-37.  It says TPMT_PUBLIC of
> > the
> > EK.  However, what it doesn't say is *which* EK.  I already sent in
> > a
> > comment saying it should be the TCG template for the P-256 curve
> > EK.
> > 
> > So asking the SVSM to give you the attestation report for the VTPM
> > service binds the EK of the vTPM.
> > 
> 
> Yes, thanks. It sounds like you have to ask the SVSM to certify the
> EK separately from asking the TPM for a quote.

That's right.

>  We can't rely entirely on the TPM API and avoid a sev-guest device
> for talking to the SVSM.

Yes, you have to make a SVSM service attestation call initially to
validate the vTPM.

>  Or are you saying the SVSM attestation report will get encoded in
> the x.509 EK certificate that the TPM API returns, such as the report
> is in a cert extension? I'm less clear on how TPM software would
> interpret the Issuer of that cert.

There is no certificate.  It's more like the Google Cloud: the SVSM
vTPM has no EK cert, so you have to ask something else for validation
of the EK, in this case a service attestation quote from the SVSM which
confirms the binding of the current SVSM to the EK and then you use
that EK pub to verify the vTPM.

There's already experimental work to get this supported in Keylime, but
doing it properly involves making the registrar EK validation mechanism
pluggable.

James

[PATCH 2/4] virt: sevguest: Prep for kernel internal {get, get_ext}_report()

[Dan Williams]

In preparation for using the TSM key facility to convey attestation blobs
to userspace, add an argument to flag whether @arg is a user buffer or a
kernel buffer.

While TSM keys is meant to replace existing confidenital computing
ioctl() implementations for attestation report retrieval the old ioctl()
path needs to stick around for a deprecation period.

No behavior change intended, just introduce the copy wrappers and @type
argument.

Note that these wrappers are similar to copy_{to,from}_sockptr(). If
this approach moves forward that concept is something that can be
generalized into a helper with a generic name.

Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c |   48 ++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 97dbe715e96a..f48c4764a7a2 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -470,7 +470,32 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 	return 0;
 }
 
-static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+enum snp_arg_type {
+	SNP_UARG,
+	SNP_KARG,
+};
+
+static unsigned long copy_from(void *to, unsigned long from, unsigned long n,
+			       enum snp_arg_type type)
+{
+	if (type == SNP_UARG)
+		return copy_from_user(to, (void __user *)from, n);
+	memcpy(to, (void *)from, n);
+	return 0;
+}
+
+static unsigned long copy_to(unsigned long to, const void *from,
+			     unsigned long n, enum snp_arg_type type)
+{
+	if (type == SNP_UARG)
+		return copy_to_user((void __user *)to, from, n);
+	memcpy((void *)to, from, n);
+	return 0;
+}
+
+static int get_report(struct snp_guest_dev *snp_dev,
+		      struct snp_guest_request_ioctl *arg,
+		      enum snp_arg_type type)
 {
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_report_resp *resp;
@@ -482,7 +507,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from(&req, arg->req_data, sizeof(req), type))
 		return -EFAULT;
 
 	/*
@@ -501,7 +526,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	if (rc)
 		goto e_free;
 
-	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
 		rc = -EFAULT;
 
 e_free:
@@ -550,7 +575,9 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 	return rc;
 }
 
-static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+static int get_ext_report(struct snp_guest_dev *snp_dev,
+			  struct snp_guest_request_ioctl *arg,
+			  enum snp_arg_type type)
 {
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_ext_report_req req;
@@ -562,7 +589,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from(&req, arg->req_data, sizeof(req), type))
 		return -EFAULT;
 
 	/* userspace does not want certificate data */
@@ -611,14 +638,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (ret)
 		goto e_free;
 
-	if (npages &&
-	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
-			 req.certs_len)) {
+	if (npages && copy_to(req.certs_address, snp_dev->certs_data,
+			      req.certs_len, type)) {
 		ret = -EFAULT;
 		goto e_free;
 	}
 
-	if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+	if (copy_to(arg->resp_data, resp, sizeof(*resp), type))
 		ret = -EFAULT;
 
 e_free:
@@ -653,13 +679,13 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 
 	switch (ioctl) {
 	case SNP_GET_REPORT:
-		ret = get_report(snp_dev, &input);
+		ret = get_report(snp_dev, &input, SNP_UARG);
 		break;
 	case SNP_GET_DERIVED_KEY:
 		ret = get_derived_key(snp_dev, &input);
 		break;
 	case SNP_GET_EXT_REPORT:
-		ret = get_ext_report(snp_dev, &input);
+		ret = get_ext_report(snp_dev, &input, SNP_UARG);
 		break;
 	default:
 		break;

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

[Dan Williams]

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

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 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);

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

[Dan Williams]

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/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/coco/sev-guest/Kconfig     |    2 +
 drivers/virt/coco/sev-guest/sev-guest.c |   87 +++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
index da2d7ca531f0..bce43d4639ce 100644
--- a/drivers/virt/coco/sev-guest/Kconfig
+++ b/drivers/virt/coco/sev-guest/Kconfig
@@ -2,9 +2,11 @@ config SEV_GUEST
 	tristate "AMD SEV Guest driver"
 	default m
 	depends on AMD_MEM_ENCRYPT
+	depends on KEYS
 	select CRYPTO
 	select CRYPTO_AEAD2
 	select CRYPTO_GCM
+	select TSM_KEYS
 	help
 	  SEV-SNP firmware provides the guest a mechanism to communicate with
 	  the PSP without risk from a malicious hypervisor who wishes to read,
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index f48c4764a7a2..2bdca268272d 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -21,6 +21,7 @@
 #include <linux/psp-sev.h>
 #include <uapi/linux/sev-guest.h>
 #include <uapi/linux/psp-sev.h>
+#include <keys/tsm.h>
 
 #include <asm/svm.h>
 #include <asm/sev.h>
@@ -769,6 +770,84 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
 	return key;
 }
 
+static int sev_auth_new(struct tsm_key_payload *t, void *provider_data)
+{
+	struct snp_guest_dev *snp_dev = provider_data;
+	const int report_size = SZ_16K;
+	const int ext_size =
+		PAGE_ALIGN_DOWN(TSM_DATA_MAX - report_size - sizeof(*t));
+	int ret;
+
+	if (t->pubkey_len != 64)
+		return -EINVAL;
+
+	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;

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

[Peter Gonda]

On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> The sevguest driver was a first mover in the confidential computing
> space. As a first mover that afforded some leeway to build the driver
> without concern for common infrastructure.
>
> Now that sevguest is no longer a singleton [1] the common operation of
> building and transmitting attestation report blobs can / should be made
> common. In this model the so called "TSM-provider" implementations can
> share a common envelope ABI even if the contents of that envelope remain
> vendor-specific. When / if the industry agrees on an attestation record
> format, that definition can also fit in the same ABI. In the meantime
> the kernel's maintenance burden is reduced and collaboration on the
> commons is increased.
>
> Convert sevguest to use TSM keys to retrieve the blobs that the
> SNP_{GET,GET_EXT}_REPORT ioctls produce. The flow for retrieving the
> SNP_GET_REPORT blob via the keyctl utility would be:
>
>     dd if=/dev/urandom of=pubkey bs=1 count=64
>     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u
>     keyctl print $key_id | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
>
> ...while the SNP_GET_EXT_REPORT flow adds the "format=extended" option
> to the request flow:
>
>     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2 format=extended" @u
>
> The output format from 'keyctl print' is:
>
>     <pubkey blob> <auth blob desc[:format]> <auth blob>
>
> ...where the blobs are hex encoded and the descriptor string is either
> "sev" or "sev:extended" in this case.
>
> Note, the Keys subsystem frontend for the functionality that
> SNP_GET_DERIVED_KEY represents is saved for follow-on work that likely
> needs to become a new trusted-keys type. The old ioctls can be lazily
> deprecated, the main motivation of this effort is to stop the
> proliferation of new ioctls, and to increase cross-vendor colloboration.

collaboration

>
> Note, only compile-tested.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  drivers/virt/coco/sev-guest/Kconfig     |    2 +
>  drivers/virt/coco/sev-guest/sev-guest.c |   87 +++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> index da2d7ca531f0..bce43d4639ce 100644
> --- a/drivers/virt/coco/sev-guest/Kconfig
> +++ b/drivers/virt/coco/sev-guest/Kconfig
> @@ -2,9 +2,11 @@ config SEV_GUEST
>         tristate "AMD SEV Guest driver"
>         default m
>         depends on AMD_MEM_ENCRYPT
> +       depends on KEYS
>         select CRYPTO
>         select CRYPTO_AEAD2
>         select CRYPTO_GCM
> +       select TSM_KEYS
>         help
>           SEV-SNP firmware provides the guest a mechanism to communicate with
>           the PSP without risk from a malicious hypervisor who wishes to read,
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index f48c4764a7a2..2bdca268272d 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -21,6 +21,7 @@
>  #include <linux/psp-sev.h>
>  #include <uapi/linux/sev-guest.h>
>  #include <uapi/linux/psp-sev.h>
> +#include <keys/tsm.h>
>
>  #include <asm/svm.h>
>  #include <asm/sev.h>
> @@ -769,6 +770,84 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
>         return key;
>  }
>
> +static int sev_auth_new(struct tsm_key_payload *t, void *provider_data)
> +{
> +       struct snp_guest_dev *snp_dev = provider_data;
> +       const int report_size = SZ_16K;
> +       const int ext_size =
> +               PAGE_ALIGN_DOWN(TSM_DATA_MAX - report_size - sizeof(*t));
> +       int ret;
> +
> +       if (t->pubkey_len != 64)
> +               return -EINVAL;

Magic number?

We only support asymmetric keys with public keys exactly equal to 64
bytes? Is that only p256? SNP uses p384 can we atleast allow that
curve too? But why not let userspace what key type they want to use?

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

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

[Dan Williams]

Peter Gonda wrote:
> On Fri, Jul 28, 2023 at 1:31 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > The sevguest driver was a first mover in the confidential computing
> > space. As a first mover that afforded some leeway to build the driver
> > without concern for common infrastructure.
> >
> > Now that sevguest is no longer a singleton [1] the common operation of
> > building and transmitting attestation report blobs can / should be made
> > common. In this model the so called "TSM-provider" implementations can
> > share a common envelope ABI even if the contents of that envelope remain
> > vendor-specific. When / if the industry agrees on an attestation record
> > format, that definition can also fit in the same ABI. In the meantime
> > the kernel's maintenance burden is reduced and collaboration on the
> > commons is increased.
> >
> > Convert sevguest to use TSM keys to retrieve the blobs that the
> > SNP_{GET,GET_EXT}_REPORT ioctls produce. The flow for retrieving the
> > SNP_GET_REPORT blob via the keyctl utility would be:
> >
> >     dd if=/dev/urandom of=pubkey bs=1 count=64
> >     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2" @u
> >     keyctl print $key_id | awk '{ print $3 }' | xxd -p -c 0 -r | hexdump -C
> >
> > ...while the SNP_GET_EXT_REPORT flow adds the "format=extended" option
> > to the request flow:
> >
> >     keyctl add tsm tsm_test "auth $(xxd -p -c 0 < pubkey) privlevel=2 format=extended" @u
> >
> > The output format from 'keyctl print' is:
> >
> >     <pubkey blob> <auth blob desc[:format]> <auth blob>
> >
> > ...where the blobs are hex encoded and the descriptor string is either
> > "sev" or "sev:extended" in this case.
> >
> > Note, the Keys subsystem frontend for the functionality that
> > SNP_GET_DERIVED_KEY represents is saved for follow-on work that likely
> > needs to become a new trusted-keys type. The old ioctls can be lazily
> > deprecated, the main motivation of this effort is to stop the
> > proliferation of new ioctls, and to increase cross-vendor colloboration.
> 
> collaboration

got it.

> 
> >
> > Note, only compile-tested.
> >
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  drivers/virt/coco/sev-guest/Kconfig     |    2 +
> >  drivers/virt/coco/sev-guest/sev-guest.c |   87 +++++++++++++++++++++++++++++++
> >  2 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
> > index da2d7ca531f0..bce43d4639ce 100644
> > --- a/drivers/virt/coco/sev-guest/Kconfig
> > +++ b/drivers/virt/coco/sev-guest/Kconfig
> > @@ -2,9 +2,11 @@ config SEV_GUEST
> >         tristate "AMD SEV Guest driver"
> >         default m
> >         depends on AMD_MEM_ENCRYPT
> > +       depends on KEYS
> >         select CRYPTO
> >         select CRYPTO_AEAD2
> >         select CRYPTO_GCM
> > +       select TSM_KEYS
> >         help
> >           SEV-SNP firmware provides the guest a mechanism to communicate with
> >           the PSP without risk from a malicious hypervisor who wishes to read,
> > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > index f48c4764a7a2..2bdca268272d 100644
> > --- a/drivers/virt/coco/sev-guest/sev-guest.c
> > +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/psp-sev.h>
> >  #include <uapi/linux/sev-guest.h>
> >  #include <uapi/linux/psp-sev.h>
> > +#include <keys/tsm.h>
> >
> >  #include <asm/svm.h>
> >  #include <asm/sev.h>
> > @@ -769,6 +770,84 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> >         return key;
> >  }
> >
> > +static int sev_auth_new(struct tsm_key_payload *t, void *provider_data)
> > +{
> > +       struct snp_guest_dev *snp_dev = provider_data;
> > +       const int report_size = SZ_16K;
> > +       const int ext_size =
> > +               PAGE_ALIGN_DOWN(TSM_DATA_MAX - report_size - sizeof(*t));
> > +       int ret;
> > +
> > +       if (t->pubkey_len != 64)
> > +               return -EINVAL;
> 
> Magic number?
> 
> We only support asymmetric keys with public keys exactly equal to 64
> bytes? Is that only p256? SNP uses p384 can we atleast allow that
> curve too? But why not let userspace what key type they want to use?

The kernel has no control here. See Table 20 MSG_REPORT_REQ Message
Structure (https://www.amd.com/system/files/TechDocs/56860.pdf)

...only 64-byte payloads are accepted. I assume one could specify less
than 64-bytes and zero-fill the rest, but that's a contract between the
requester and the attester.

> 
> > +
> > +       if (t->auth_blob_format[0] &&
> > +           strcmp(t->auth_blob_format, "extended") != 0)
> > +               return -EINVAL;
> > +
> > +       if (t->auth_blob_format[0]) {
> > +               u8 *buf __free(kvfree) =
> > +                       kvzalloc(report_size + ext_size, GFP_KERNEL);
> > +
> > +               struct snp_ext_report_req req = {
> > +                       .data = { .vmpl = t->privlevel },
> > +                       .certs_address = (__u64)buf + report_size,
> > +                       .certs_len = ext_size,
> > +               };
> > +               memcpy(&req.data.user_data, t->pubkey, 64);
> 
> Again without any freshness from the remote party, of what use is this
> attestation report?

This interface is just marshaling the same data that could be retrieved
via SNP_GET_REPORT ioctl on the sevguest chardev today. So I would point
you back to vendor documentation for how this report is used. See "VM
Launch and Attestation" here:

https://www.amd.com/system/files/TechDocs/SEV-SNP-strengthening-vm-isolation-with-integrity-protection-and-more.pdf

I am just here to stanch the proliferation of new chardevs and new
ioctls for this TSM-common operation. This effort was started when TDX
patches showed up to take a 64-byte input payload and wrap it in a
attestation report with its own chardev and ioctls.

> 
> > +
> > +               struct snp_guest_request_ioctl input = {
> > +                       .msg_version = 1,
> > +                       .req_data = (__u64) &req,
> > +                       .resp_data = (__u64) buf,
> > +               };
> > +
> > +               ret = get_ext_report(snp_dev, &input, SNP_KARG);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               no_free_ptr(buf);
> > +               t->auth_blob = buf;
> > +               t->auth_blob_len = report_size + ext_size;
> > +               t->auth_blob_desc = "sev";
> > +       } else {
> > +               u8 *buf __free(kvfree) = kvzalloc(report_size, GFP_KERNEL);
> > +
> > +               struct snp_report_req req = {
> > +                       .vmpl = t->privlevel,
> > +               };
> > +               memcpy(&req.user_data, t->pubkey, 64);
> > +
> > +               struct snp_guest_request_ioctl input = {
> > +                       .msg_version = 1,
> > +                       .req_data = (__u64) &req,
> > +                       .resp_data = (__u64) buf,
> > +               };
> > +
> > +               ret = get_report(snp_dev, &input, SNP_KARG);
> > +               if (ret)
> > +                       return ret;
> > +
> > +               no_free_ptr(buf);
> > +               t->auth_blob = buf;
> > +               t->auth_blob_len = report_size;
> > +               t->auth_blob_desc = "sev";
> > +       }
> 
> Can we reduce the code duplication between the branches here?

I'll take a look.

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

[Peter Gonda]

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

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

[Jarkko Sakkinen]

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

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

[Dan Williams]

Jarkko Sakkinen wrote:
> 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?

TPMs and TEEs are covered by trusted-keys. I do think a "TSM" flavor of
trusted-keys is in scope for where some of these implementations are
headed, but that comes later. I talk about that in the changelog that
functionality like SNP_GET_DERIVED_KEY likely wants to have a
trusted-keys frontend and not isolated behind a vendor-specific ioctl
interface.

This facility is different, it is just aiming to unify this attestation
report flow. It scales to any driver that can provide the ->auth_new()
operation. I have the sev-guest conversion in this set, and Sathya has
tested this with tdx-guest. I am hoping Samuel can evaluate it for
cove-guest or whatever that driver ends up being called.

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

[Jarkko Sakkinen]

On Fri Jul 28, 2023 at 7:44 PM UTC, Dan Williams wrote:
> Jarkko Sakkinen wrote:
> > 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?
>
> TPMs and TEEs are covered by trusted-keys. I do think a "TSM" flavor of
> trusted-keys is in scope for where some of these implementations are
> headed, but that comes later. I talk about that in the changelog that
> functionality like SNP_GET_DERIVED_KEY likely wants to have a
> trusted-keys frontend and not isolated behind a vendor-specific ioctl
> interface.

TEE's and TPM's are not the exact same thing. Are we sure that any
future ARM SMC like TEE interface what you say will hold?

Why do we need a new key type, when we have already trusted keys?

This whole territory should be better defined so that everything
will fit together.

> This facility is different, it is just aiming to unify this attestation
> report flow. It scales to any driver that can provide the ->auth_new()
> operation. I have the sev-guest conversion in this set, and Sathya has
> tested this with tdx-guest. I am hoping Samuel can evaluate it for
> cove-guest or whatever that driver ends up being called.

What about SGX without TDX?

BR, Jarkko

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

[Dan Williams]

Jarkko Sakkinen wrote:
> On Fri Jul 28, 2023 at 7:44 PM UTC, Dan Williams wrote:
> > Jarkko Sakkinen wrote:
> > > 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?
> >
> > TPMs and TEEs are covered by trusted-keys. I do think a "TSM" flavor of
> > trusted-keys is in scope for where some of these implementations are
> > headed, but that comes later. I talk about that in the changelog that
> > functionality like SNP_GET_DERIVED_KEY likely wants to have a
> > trusted-keys frontend and not isolated behind a vendor-specific ioctl
> > interface.
> 
> TEE's and TPM's are not the exact same thing. Are we sure that any
> future ARM SMC like TEE interface what you say will hold?

Agree, they are not the same thing, I assume that's why trusted-keys has
a TEE and a TPM backend. Also that's the point of common interface
proposals for the per vendor experts to take a look and make sure it
fits their needs. If you have contacts there, please highlight this
thread to them.

> Why do we need a new key type, when we have already trusted keys?

As I mentioned to James to the comment from him about vTPM, if that ends
up just looking like a standard TPM to Linux then nothing new is needed.

> This whole territory should be better defined so that everything
> will fit together.

Yes, the per-vendor differentiation in this space is an impediment to
kernel interface design.

> > This facility is different, it is just aiming to unify this attestation
> > report flow. It scales to any driver that can provide the ->auth_new()
> > operation. I have the sev-guest conversion in this set, and Sathya has
> > tested this with tdx-guest. I am hoping Samuel can evaluate it for
> > cove-guest or whatever that driver ends up being called.
> 
> What about SGX without TDX?

My hope would be that anything that can not be fronted by TPM2_Quote
directly can by frontend by this "TSM" class device (as I will be
switching from Keyring to sysfs).

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

[Huang, Kai]

On Mon, 2023-07-31 at 10:09 +0000, Jarkko Sakkinen wrote:
> > This facility is different, it is just aiming to unify this attestation
> > report flow. It scales to any driver that can provide the ->auth_new()
> > operation. I have the sev-guest conversion in this set, and Sathya has
> > tested this with tdx-guest. I am hoping Samuel can evaluate it for
> > cove-guest or whatever that driver ends up being called.
> 
> What about SGX without TDX?

SGX attestation is completely among userspace enclaves, and the existing SGX
userspace stack has fully adopted what is needed to do attestation.  Why do we
need to cover SGX?

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

[Jarkko Sakkinen]

On Tue Aug 1, 2023 at 1:41 AM EEST, Huang, Kai wrote:
> On Mon, 2023-07-31 at 10:09 +0000, Jarkko Sakkinen wrote:
> > > This facility is different, it is just aiming to unify this attestation
> > > report flow. It scales to any driver that can provide the ->auth_new()
> > > operation. I have the sev-guest conversion in this set, and Sathya has
> > > tested this with tdx-guest. I am hoping Samuel can evaluate it for
> > > cove-guest or whatever that driver ends up being called.
> > 
> > What about SGX without TDX?
>
> SGX attestation is completely among userspace enclaves, and the existing SGX
> userspace stack has fully adopted what is needed to do attestation.  Why do we
> need to cover SGX?

I have no answer to that. I'm merely trying to understand what this is.

BR, Jarkko

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

[James Bottomley]

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

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

[Dan Williams]

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.

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

[James Bottomley]

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

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

[Dan Williams]

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.

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

[Huang, Kai]

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.

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

[James Bottomley]

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

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

[James Bottomley]

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

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

[Huang, Kai]

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.

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

[James Bottomley]

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

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

[Huang, Kai]

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!

> > 

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

[Dan Williams]

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.

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

[Dan Williams]

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.

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

[Daniel P. Berrangé]

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 :|

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

[Huang, Kai]

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.

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

[James Bottomley]

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

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

[Dan Williams]

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/ZM0lEvYJ+5IgybLT@redhat.com 

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

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

[James Bottomley]

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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/

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

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

[Dan Williams]

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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
> 
> 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.

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

[James Bottomley]

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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
> > 
> > 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.

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

[Peter Gonda]

On Tue, Aug 8, 2023 at 8:19 AM James Bottomley
<James.Bottomley@hansenpartnership.com> 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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
> > >
> > > 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.

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

[Sathyanarayanan Kuppuswamy]

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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
>>>
>>> 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

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

[Dan Williams]

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/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
> >>>
> >>> 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.

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

[Dionna Amalie Glaze]

>
> 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)

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

[Dan Williams]

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/

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

[Dionna Amalie Glaze]

>
> 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)

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

[Dan Williams]

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

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

[Huang, Kai]

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.

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

[Dan Williams]

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.

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

[Peter Gonda]

On Tue, Aug 8, 2023 at 9:28 PM Dan Williams <dan.j.williams@intel.com> 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.

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

[James Bottomley]

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

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

[Dionna Amalie Glaze]

> 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)

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

[James Bottomley]

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

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

[Dionna Amalie Glaze]

> 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)

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

[James Bottomley]

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

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

[Dionna Amalie Glaze]

> 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)

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

[Dan Williams]

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.

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

[Jarkko Sakkinen]

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