* [PATCH v2 0/5] tsm: Attestation Report ABI
@ 2023-08-14 7:43 Dan Williams
2023-08-14 7:43 ` [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
` (5 more replies)
0 siblings, 6 replies; 47+ messages in thread
From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw)
To: linux-coco
Cc: Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra,
Tom Lendacky, Peter Gonda, Borislav Petkov, Dionna Amalie Glaze,
Samuel Ortiz, Dionna Glaze, Greg Kroah-Hartman, Andrew Morton,
James Bottomley, x86, linux-kernel
Changes since v1:
- Switch from Keyring to sysfs (James)
An attestation report is signed evidence of how a Trusted Virtual
Machine (TVM) was launched and its current state. A verifying party uses
the report to make judgements of the confidentiality and integrity of
that execution environment. Upon successful attestation the verifying
party may, for example, proceed to deploy secrets to the TVM to carry
out a workload. Multiple confidential computing platforms share this
similar flow.
The approach of adding adding new char devs and new ioctls, for what
amounts to the same logical functionality with minor formatting
differences across vendors [1], is untenable. Common concepts and the
community benefit from common infrastructure.
Use sysfs for this facility for maintainability compared to ioctl(). The
expectation is that this interface is a boot time, configure once, get
report, and done flow. I.e. not something that receives ongoing
transactions at runtime. However, runtime retrieval is not precluded and
a mechanism to detect potential configuration conflicts from
multiple-threads using this interface is included.
The keyring@ list is dropped on this posting since a new key-type is no
longer being pursued.
Link: http://lore.kernel.org/r/cover.1684048511.git.sathyanarayanan.kuppuswamy@linux.intel.com
---
Dan Williams (5):
virt: coco: Add a coco/Makefile and coco/Kconfig
tsm: Introduce a shared ABI for attestation reports
virt: sevguest: Prep for kernel internal {get,get_ext}_report()
mm/slab: Add __free() support for kvfree
virt: sevguest: Add TSM_REPORTS support for SNP_{GET,GET_EXT}_REPORT
Documentation/ABI/testing/sysfs-class-tsm | 47 +++++
MAINTAINERS | 8 +
drivers/virt/Kconfig | 6 -
drivers/virt/Makefile | 4
drivers/virt/coco/Kconfig | 13 +
drivers/virt/coco/Makefile | 8 +
drivers/virt/coco/sev-guest/Kconfig | 1
drivers/virt/coco/sev-guest/sev-guest.c | 129 ++++++++++++-
drivers/virt/coco/tdx-guest/Kconfig | 1
drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++
include/linux/slab.h | 2
include/linux/tsm.h | 45 +++++
12 files changed, 535 insertions(+), 19 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-class-tsm
create mode 100644 drivers/virt/coco/Kconfig
create mode 100644 drivers/virt/coco/Makefile
create mode 100644 drivers/virt/coco/tsm.c
create mode 100644 include/linux/tsm.h
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
^ permalink raw reply [flat|nested] 47+ messages in thread* [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams @ 2023-08-14 7:43 ` Dan Williams 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams ` (4 subsequent siblings) 5 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw) To: linux-coco; +Cc: peterz, x86, linux-kernel In preparation for adding another coco build target, relieve drivers/virt/Makefile of the responsibility to track new compilation unit additions to drivers/virt/coco/, and do the same for drivers/virt/Kconfig. Signed-off-by: Dan Williams <dan.j.williams@intel.com> --- drivers/virt/Kconfig | 6 +----- drivers/virt/Makefile | 4 +--- drivers/virt/coco/Kconfig | 9 +++++++++ drivers/virt/coco/Makefile | 7 +++++++ 4 files changed, 18 insertions(+), 8 deletions(-) create mode 100644 drivers/virt/coco/Kconfig create mode 100644 drivers/virt/coco/Makefile diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig index f79ab13a5c28..40129b6f0eca 100644 --- a/drivers/virt/Kconfig +++ b/drivers/virt/Kconfig @@ -48,10 +48,6 @@ source "drivers/virt/nitro_enclaves/Kconfig" source "drivers/virt/acrn/Kconfig" -source "drivers/virt/coco/efi_secret/Kconfig" - -source "drivers/virt/coco/sev-guest/Kconfig" - -source "drivers/virt/coco/tdx-guest/Kconfig" +source "drivers/virt/coco/Kconfig" endif diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile index e9aa6fc96fab..f29901bd7820 100644 --- a/drivers/virt/Makefile +++ b/drivers/virt/Makefile @@ -9,6 +9,4 @@ obj-y += vboxguest/ obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ obj-$(CONFIG_ACRN_HSM) += acrn/ -obj-$(CONFIG_EFI_SECRET) += coco/efi_secret/ -obj-$(CONFIG_SEV_GUEST) += coco/sev-guest/ -obj-$(CONFIG_INTEL_TDX_GUEST) += coco/tdx-guest/ +obj-y += coco/ diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig new file mode 100644 index 000000000000..fc5c64f04c4a --- /dev/null +++ b/drivers/virt/coco/Kconfig @@ -0,0 +1,9 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Confidential computing related collateral +# +source "drivers/virt/coco/efi_secret/Kconfig" + +source "drivers/virt/coco/sev-guest/Kconfig" + +source "drivers/virt/coco/tdx-guest/Kconfig" diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile new file mode 100644 index 000000000000..55302ef719ad --- /dev/null +++ b/drivers/virt/coco/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Confidential computing related collateral +# +obj-$(CONFIG_EFI_SECRET) += efi_secret/ +obj-$(CONFIG_SEV_GUEST) += sev-guest/ +obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/ ^ permalink raw reply related [flat|nested] 47+ messages in thread
* [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams 2023-08-14 7:43 ` [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams @ 2023-08-14 7:43 ` Dan Williams 2023-08-14 8:24 ` Jeremi Piotrowski ` (5 more replies) 2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams ` (3 subsequent siblings) 5 siblings, 6 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw) To: linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel 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 launch state, sign it and submit it to a verifying party. Upon successful attestation that verifies the integrity of the TVM additional secrets may be deployed. 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 sysfs interface to retrieve the TSM-specific blob. echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex hexdump /sys/class/tsm/tsm0/outblob This approach later allows for the standardization of the attestation blob format without needing to change the Linux ABI. Until then, the format of 'outblob' is determined by the parent device for 'tsm0'. The expectation is that this is a boot time exchange that need not be regenerated, making it amenable to a sysfs interface. In case userspace does try to generate multiple attestation reports it includes conflict detection so userspace can be sure no other thread changed the parameters from its last configuration step to the blob retrieval. TSM specific options are encoded as 'extra' attributes on the TSM device with the expectation that vendors reuse the same options for similar concepts. The current options are defined by SEV-SNP's need for a 'privilege level' concept (VMPL), and the option to retrieve a certificate chain in addition to the attestation report ("extended" format). Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Cc: Dionna Amalie Glaze <dionnaglaze@google.com> Cc: James Bottomley <James.Bottomley@HansenPartnership.com> Cc: Peter Gonda <pgonda@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> --- Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ MAINTAINERS | 8 + drivers/virt/coco/Kconfig | 4 drivers/virt/coco/Makefile | 1 drivers/virt/coco/tdx-guest/Kconfig | 1 drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ include/linux/tsm.h | 45 +++++ 7 files changed, 396 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-class-tsm create mode 100644 drivers/virt/coco/tsm.c create mode 100644 include/linux/tsm.h diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm new file mode 100644 index 000000000000..37017bde626d --- /dev/null +++ b/Documentation/ABI/testing/sysfs-class-tsm @@ -0,0 +1,47 @@ +What: /sys/class/tsm/tsm0/inhex +Date: August, 2023 +KernelVersion: v6.6 +Contact: linux-cxl@vger.kernel.org +Description: + (RW) Hex encoded userdata to be included in the attestation + report. For replay protection this should include a nonce, but + the kernel does not place any restrictions on the content. + +What: /sys/class/tsm/tsm0/outblob +Date: August, 2023 +KernelVersion: v6.6 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) Binary attestation report generated from @inhex translated + to binary and any options. The format of the report is vendor + specific and determined by the parent device of 'tsm0'. + +What: /sys/class/tsm/tsm0/generation +Date: August, 2023 +KernelVersion: v6.6 +Contact: linux-cxl@vger.kernel.org +Description: + (RO) The value in this attribute increments each time @inhex or + any option is written. Userspace can detect conflicts by + checking generation before writing to any attribute and making + sure the number of writes matches expectations after reading + @outblob. + +What: /sys/class/tsm/tsm0/privlevel +Date: August, 2023 +KernelVersion: v6.6 +Contact: linux-cxl@vger.kernel.org +Description: + (RW) If a TSM implementation supports the concept of attestation + reports for TVMs running at different privilege levels, like + SEV-SNP "VMPL", specify the privilege level via this attribute. + +What: /sys/class/tsm/tsm0/format +Date: August, 2023 +KernelVersion: v6.6 +Contact: linux-cxl@vger.kernel.org +Description: + (RW) If a TSM implementation supports the concept of attestation + reports with "extended" contents, like SEV-SNP extended reports + with certificate chains, specify "extended" vs "default" via + this attribute. diff --git a/MAINTAINERS b/MAINTAINERS index 3be1bdfe8ecc..97f74d344c8a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21625,6 +21625,14 @@ W: https://github.com/srcres258/linux-doc T: git git://github.com/srcres258/linux-doc.git doc-zh-tw F: Documentation/translations/zh_TW/ +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS +M: Dan Williams <dan.j.williams@intel.com> +L: linux-coco@lists.linux.dev +S: Maintained +F: Documentation/ABI/testing/sysfs-class-tsm +F: drivers/virt/coco/tsm.c +F: include/linux/tsm.h + TTY LAYER M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> M: Jiri Slaby <jirislaby@kernel.org> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig index fc5c64f04c4a..d92f07019f38 100644 --- a/drivers/virt/coco/Kconfig +++ b/drivers/virt/coco/Kconfig @@ -2,6 +2,10 @@ # # Confidential computing related collateral # + +config TSM_REPORTS + tristate + source "drivers/virt/coco/efi_secret/Kconfig" source "drivers/virt/coco/sev-guest/Kconfig" diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile index 55302ef719ad..18c1aba5edb7 100644 --- a/drivers/virt/coco/Makefile +++ b/drivers/virt/coco/Makefile @@ -2,6 +2,7 @@ # # Confidential computing related collateral # +obj-$(CONFIG_TSM_REPORTS) += tsm.o obj-$(CONFIG_EFI_SECRET) += efi_secret/ obj-$(CONFIG_SEV_GUEST) += sev-guest/ obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/ diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig index 14246fc2fb02..22dd59e19431 100644 --- a/drivers/virt/coco/tdx-guest/Kconfig +++ b/drivers/virt/coco/tdx-guest/Kconfig @@ -1,6 +1,7 @@ config TDX_GUEST_DRIVER tristate "TDX Guest driver" depends on INTEL_TDX_GUEST + select TSM_REPORTS help The driver provides userspace interface to communicate with the TDX module to request the TDX guest details like attestation diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c new file mode 100644 index 000000000000..1bf2ee82eb94 --- /dev/null +++ b/drivers/virt/coco/tsm.c @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/tsm.h> +#include <linux/err.h> +#include <linux/slab.h> +#include <linux/rwsem.h> +#include <linux/device.h> +#include <linux/string.h> +#include <linux/module.h> +#include <linux/cleanup.h> + +struct class *tsm_class; +static struct tsm_provider { + const struct tsm_ops *ops; + struct device *dev; +} provider; +static DECLARE_RWSEM(tsm_rwsem); + +/** + * DOC: Trusted Security Module (TSM) Attestation Report Interface + * + * The TSM report interface is a common provider of blobs that facilitate + * attestation of a TVM (confidential computing guest) by an attestation + * service. A TSM report combines a user-defined blob (likely a public-key with + * a nonce for a key-exchange 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. The expectation is that this interface is + * invoked infrequently, likely only once at TVM boot time. + * + * The attestation report format is TSM provider specific, when / if a standard + * materializes that can be published instead of the vendor layout. + */ + +/** + * struct tsm_report - track state of report generation relative to options + * @desc: report generation options / cached report state + * @outblob: generated evidence to provider to the attestation agent + * @outblob_len: sizeof(outblob) + * @write_generation: conflict detection, and report regeneration tracking + * @read_generation: cached report invalidation tracking + */ +struct tsm_report { + struct tsm_desc desc; + size_t outblob_len; + u8 *outblob; + unsigned long write_generation; + unsigned long read_generation; +} tsm_report; + +static ssize_t privlevel_store(struct device *dev, + struct device_attribute *attr, const char *buf, + size_t len) +{ + unsigned int val; + int rc; + + rc = kstrtouint(buf, 0, &val); + if (rc) + return rc; + + guard(rwsem_write)(&tsm_rwsem); + if (tsm_report.desc.privlevel == val) + return len; + tsm_report.desc.privlevel = val; + tsm_report.write_generation++; + + return len; +} + +static ssize_t privlevel_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%u\n", tsm_report.desc.privlevel); +} + +static DEVICE_ATTR_RW(privlevel); + +static ssize_t format_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t len) +{ + enum tsm_format format; + + if (sysfs_streq(buf, "default")) + format = TSM_FORMAT_DEFAULT; + else if (sysfs_streq(buf, "extended")) + format = TSM_FORMAT_EXTENDED; + else + return -EINVAL; + + guard(rwsem_write)(&tsm_rwsem); + if (tsm_report.desc.outblob_format == format) + return len; + tsm_report.desc.outblob_format = format; + tsm_report.write_generation++; + + return len; +} + +static ssize_t format_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + if (tsm_report.desc.outblob_format == TSM_FORMAT_DEFAULT) + return sysfs_emit(buf, "default\n"); + return sysfs_emit(buf, "extended\n"); +} + +static DEVICE_ATTR_RW(format); + +static struct attribute *tsm_extra_attributes[] = { + &dev_attr_format.attr, + &dev_attr_privlevel.attr, + NULL, +}; + +struct attribute_group tsm_extra_attribute_group = { + .attrs = tsm_extra_attributes, +}; +EXPORT_SYMBOL_GPL(tsm_extra_attribute_group); + +/* + * Input is a small hex blob, rather than a writable binary attribute, so that + * it is conveyed atomically. + */ +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t len) +{ + u8 inblob[TSM_INBLOB_MAX]; + size_t inblob_len; + int rc; + + inblob_len = len; + if (buf[len - 1] == '\n') + inblob_len--; + if (inblob_len & 1) + return -EINVAL; + inblob_len /= 2; + if (inblob_len > TSM_INBLOB_MAX) + return -EINVAL; + + rc = hex2bin(inblob, buf, inblob_len); + if (rc < 0) + return rc; + + guard(rwsem_write)(&tsm_rwsem); + if (memcmp(tsm_report.desc.inblob, inblob, inblob_len) == 0) + return len; + memcpy(tsm_report.desc.inblob, inblob, inblob_len); + tsm_report.desc.inblob_len = inblob_len; + tsm_report.write_generation++; + + return len; +} + +static ssize_t inhex_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + char *end; + + guard(rwsem_read)(&tsm_rwsem); + if (!tsm_report.desc.inblob_len) + return 0; + end = bin2hex(buf, tsm_report.desc.inblob, tsm_report.desc.inblob_len); + *end++ = '\n'; + return end - buf; +} +static DEVICE_ATTR_RW(inhex); + +static ssize_t generation_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + guard(rwsem_read)(&tsm_rwsem); + return sysfs_emit(buf, "%lu\n", tsm_report.write_generation); +} +static DEVICE_ATTR_RO(generation); + +static struct attribute *tsm_attributes[] = { + &dev_attr_inhex.attr, + &dev_attr_generation.attr, + NULL, +}; + +static ssize_t outblob_read(struct file *f, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t offset, size_t count) +{ + guard(rwsem_read)(&tsm_rwsem); + if (!tsm_report.desc.inblob_len) + return -EINVAL; + + if (!tsm_report.outblob || + tsm_report.read_generation != tsm_report.write_generation) { + const struct tsm_ops *ops = provider.ops; + size_t outblob_len; + u8 *outblob; + + kvfree(tsm_report.outblob); + outblob = ops->report_new(provider.dev->parent, + &tsm_report.desc, &outblob_len); + if (IS_ERR(outblob)) + return PTR_ERR(outblob); + tsm_report.outblob_len = outblob_len; + tsm_report.outblob = outblob; + tsm_report.read_generation = tsm_report.write_generation; + } + + return memory_read_from_buffer(buf, count, &offset, + tsm_report.outblob, + tsm_report.outblob_len); +} +static BIN_ATTR_RO(outblob, 0); + +static struct bin_attribute *tsm_bin_attributes[] = { + &bin_attr_outblob, + NULL, +}; + +struct attribute_group tsm_default_attribute_group = { + .bin_attrs = tsm_bin_attributes, + .attrs = tsm_attributes, +}; +EXPORT_SYMBOL_GPL(tsm_default_attribute_group); + +static const struct attribute_group *tsm_default_attribute_groups[] = { + &tsm_default_attribute_group, + NULL, +}; + +int register_tsm(const struct tsm_ops *ops, struct device *parent, + const struct attribute_group **groups) +{ + const struct tsm_ops *conflict; + struct device *dev; + int rc; + + if (!parent) + return -EINVAL; + + if (!groups) + groups = tsm_default_attribute_groups; + + guard(rwsem_write)(&tsm_rwsem); + conflict = provider.ops; + if (conflict) { + pr_err("\"%s\" ops already registered\n", conflict->name); + return rc; + } + + dev = device_create_with_groups(tsm_class, parent, 0, NULL, groups, + "tsm0"); + if (IS_ERR(dev)) + return PTR_ERR(dev); + + provider.ops = ops; + provider.dev = dev; + return 0; +} +EXPORT_SYMBOL_GPL(register_tsm); + +int unregister_tsm(const struct tsm_ops *ops) +{ + guard(rwsem_write)(&tsm_rwsem); + if (ops != provider.ops) + return -EBUSY; + provider.ops = NULL; + device_unregister(provider.dev); + provider.dev = NULL; + kvfree(tsm_report.outblob); + tsm_report.outblob = NULL; + return 0; +} +EXPORT_SYMBOL_GPL(unregister_tsm); + +static int __init tsm_init(void) +{ + tsm_class = class_create("tsm"); + return PTR_ERR_OR_ZERO(tsm_class); +} +module_init(tsm_init); + +static void __exit tsm_exit(void) +{ + class_destroy(tsm_class); +} +module_exit(tsm_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via sysfs"); diff --git a/include/linux/tsm.h b/include/linux/tsm.h new file mode 100644 index 000000000000..6dc2f07543b8 --- /dev/null +++ b/include/linux/tsm.h @@ -0,0 +1,45 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef __TSM_H +#define __TSM_H + +#include <linux/types.h> +#include <linux/device.h> + +#define TSM_INBLOB_MAX 64 + +enum tsm_format { + TSM_FORMAT_DEFAULT, + TSM_FORMAT_EXTENDED, +}; + +/** + * struct tsm_desc - option descriptor for generating tsm report blobs + * @privlevel: optional privilege level to associate with @outblob + * @inblob_len: sizeof @inblob + * @inblob: arbitrary input data + * @outblob_format: for TSMs with an "extended" format + */ +struct tsm_desc { + unsigned int privlevel; + size_t inblob_len; + u8 inblob[TSM_INBLOB_MAX]; + enum tsm_format outblob_format; +}; + +/* + * arch specific ops, only one is expected to be registered at a time + * i.e. only one of SEV, TDX, COVE, etc. + */ +struct tsm_ops { + const char *name; + u8 *(*report_new)(struct device *dev, const struct tsm_desc *desc, + size_t *outblob_len); +}; + +extern struct attribute_group tsm_default_attribute_group; +extern struct attribute_group tsm_extra_attribute_group; + +int register_tsm(const struct tsm_ops *ops, struct device *parent, + const struct attribute_group **groups); +int unregister_tsm(const struct tsm_ops *ops); +#endif /* __TSM_H */ ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams @ 2023-08-14 8:24 ` Jeremi Piotrowski 2023-08-14 16:21 ` Dan Williams 2023-08-14 15:38 ` Greg Kroah-Hartman ` (4 subsequent siblings) 5 siblings, 1 reply; 47+ messages in thread From: Jeremi Piotrowski @ 2023-08-14 8:24 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel On 8/14/2023 9:43 AM, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob > > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm > new file mode 100644 > index 000000000000..37017bde626d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-tsm > @@ -0,0 +1,47 @@ > +What: /sys/class/tsm/tsm0/inhex > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) Hex encoded userdata to be included in the attestation > + report. For replay protection this should include a nonce, but > + the kernel does not place any restrictions on the content. > + > +What: /sys/class/tsm/tsm0/outblob > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) Binary attestation report generated from @inhex translated > + to binary and any options. The format of the report is vendor > + specific and determined by the parent device of 'tsm0'. > + > +What: /sys/class/tsm/tsm0/generation > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The value in this attribute increments each time @inhex or > + any option is written. Userspace can detect conflicts by > + checking generation before writing to any attribute and making > + sure the number of writes matches expectations after reading > + @outblob. > + > +What: /sys/class/tsm/tsm0/privlevel > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) If a TSM implementation supports the concept of attestation > + reports for TVMs running at different privilege levels, like > + SEV-SNP "VMPL", specify the privilege level via this attribute. > + > +What: /sys/class/tsm/tsm0/format > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) If a TSM implementation supports the concept of attestation > + reports with "extended" contents, like SEV-SNP extended reports > + with certificate chains, specify "extended" vs "default" via > + this attribute. > diff --git a/MAINTAINERS b/MAINTAINERS > index 3be1bdfe8ecc..97f74d344c8a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21625,6 +21625,14 @@ W: https://github.com/srcres258/linux-doc > T: git git://github.com/srcres258/linux-doc.git doc-zh-tw > F: Documentation/translations/zh_TW/ > > +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS > +M: Dan Williams <dan.j.williams@intel.com> > +L: linux-coco@lists.linux.dev > +S: Maintained > +F: Documentation/ABI/testing/sysfs-class-tsm > +F: drivers/virt/coco/tsm.c > +F: include/linux/tsm.h > + > TTY LAYER > M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > M: Jiri Slaby <jirislaby@kernel.org> > diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig > index fc5c64f04c4a..d92f07019f38 100644 > --- a/drivers/virt/coco/Kconfig > +++ b/drivers/virt/coco/Kconfig > @@ -2,6 +2,10 @@ > # > # Confidential computing related collateral > # > + > +config TSM_REPORTS > + tristate > + > source "drivers/virt/coco/efi_secret/Kconfig" > > source "drivers/virt/coco/sev-guest/Kconfig" > diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile > index 55302ef719ad..18c1aba5edb7 100644 > --- a/drivers/virt/coco/Makefile > +++ b/drivers/virt/coco/Makefile > @@ -2,6 +2,7 @@ > # > # Confidential computing related collateral > # > +obj-$(CONFIG_TSM_REPORTS) += tsm.o > obj-$(CONFIG_EFI_SECRET) += efi_secret/ > obj-$(CONFIG_SEV_GUEST) += sev-guest/ > obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/ > diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig > index 14246fc2fb02..22dd59e19431 100644 > --- a/drivers/virt/coco/tdx-guest/Kconfig > +++ b/drivers/virt/coco/tdx-guest/Kconfig > @@ -1,6 +1,7 @@ > config TDX_GUEST_DRIVER > tristate "TDX Guest driver" > depends on INTEL_TDX_GUEST > + select TSM_REPORTS > help > The driver provides userspace interface to communicate with > the TDX module to request the TDX guest details like attestation > diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c > new file mode 100644 > index 000000000000..1bf2ee82eb94 > --- /dev/null > +++ b/drivers/virt/coco/tsm.c > @@ -0,0 +1,290 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/tsm.h> > +#include <linux/err.h> > +#include <linux/slab.h> > +#include <linux/rwsem.h> > +#include <linux/device.h> > +#include <linux/string.h> > +#include <linux/module.h> > +#include <linux/cleanup.h> > + > +struct class *tsm_class; > +static struct tsm_provider { > + const struct tsm_ops *ops; > + struct device *dev; > +} provider; > +static DECLARE_RWSEM(tsm_rwsem); > + > +/** > + * DOC: Trusted Security Module (TSM) Attestation Report Interface > + * > + * The TSM report interface is a common provider of blobs that facilitate > + * attestation of a TVM (confidential computing guest) by an attestation > + * service. A TSM report combines a user-defined blob (likely a public-key with > + * a nonce for a key-exchange 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. The expectation is that this interface is > + * invoked infrequently, likely only once at TVM boot time. > + * > + * The attestation report format is TSM provider specific, when / if a standard > + * materializes that can be published instead of the vendor layout. > + */ > + > +/** > + * struct tsm_report - track state of report generation relative to options > + * @desc: report generation options / cached report state > + * @outblob: generated evidence to provider to the attestation agent > + * @outblob_len: sizeof(outblob) > + * @write_generation: conflict detection, and report regeneration tracking > + * @read_generation: cached report invalidation tracking > + */ > +struct tsm_report { > + struct tsm_desc desc; > + size_t outblob_len; > + u8 *outblob; > + unsigned long write_generation; > + unsigned long read_generation; > +} tsm_report; > + > +static ssize_t privlevel_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t len) > +{ > + unsigned int val; > + int rc; > + > + rc = kstrtouint(buf, 0, &val); > + if (rc) > + return rc; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (tsm_report.desc.privlevel == val) > + return len; > + tsm_report.desc.privlevel = val; > + tsm_report.write_generation++; > + > + return len; > +} > + > +static ssize_t privlevel_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%u\n", tsm_report.desc.privlevel); > +} > + > +static DEVICE_ATTR_RW(privlevel); > + > +static ssize_t format_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + enum tsm_format format; > + > + if (sysfs_streq(buf, "default")) > + format = TSM_FORMAT_DEFAULT; > + else if (sysfs_streq(buf, "extended")) > + format = TSM_FORMAT_EXTENDED; > + else > + return -EINVAL; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (tsm_report.desc.outblob_format == format) > + return len; > + tsm_report.desc.outblob_format = format; > + tsm_report.write_generation++; > + > + return len; > +} > + > +static ssize_t format_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + if (tsm_report.desc.outblob_format == TSM_FORMAT_DEFAULT) > + return sysfs_emit(buf, "default\n"); > + return sysfs_emit(buf, "extended\n"); > +} > + > +static DEVICE_ATTR_RW(format); > + > +static struct attribute *tsm_extra_attributes[] = { > + &dev_attr_format.attr, > + &dev_attr_privlevel.attr, > + NULL, > +}; > + > +struct attribute_group tsm_extra_attribute_group = { > + .attrs = tsm_extra_attributes, > +}; > +EXPORT_SYMBOL_GPL(tsm_extra_attribute_group); > + > +/* > + * Input is a small hex blob, rather than a writable binary attribute, so that > + * it is conveyed atomically. > + */ > +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + u8 inblob[TSM_INBLOB_MAX]; > + size_t inblob_len; > + int rc; > + > + inblob_len = len; > + if (buf[len - 1] == '\n') > + inblob_len--; > + if (inblob_len & 1) > + return -EINVAL; > + inblob_len /= 2; > + if (inblob_len > TSM_INBLOB_MAX) > + return -EINVAL; > + > + rc = hex2bin(inblob, buf, inblob_len); > + if (rc < 0) > + return rc; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (memcmp(tsm_report.desc.inblob, inblob, inblob_len) == 0) > + return len; > + memcpy(tsm_report.desc.inblob, inblob, inblob_len); > + tsm_report.desc.inblob_len = inblob_len; > + tsm_report.write_generation++; > + > + return len; > +} > + > +static ssize_t inhex_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + char *end; > + > + guard(rwsem_read)(&tsm_rwsem); > + if (!tsm_report.desc.inblob_len) > + return 0; > + end = bin2hex(buf, tsm_report.desc.inblob, tsm_report.desc.inblob_len); > + *end++ = '\n'; > + return end - buf; > +} > +static DEVICE_ATTR_RW(inhex); > + > +static ssize_t generation_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + guard(rwsem_read)(&tsm_rwsem); > + return sysfs_emit(buf, "%lu\n", tsm_report.write_generation); > +} > +static DEVICE_ATTR_RO(generation); > + > +static struct attribute *tsm_attributes[] = { > + &dev_attr_inhex.attr, > + &dev_attr_generation.attr, > + NULL, > +}; > + > +static ssize_t outblob_read(struct file *f, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t offset, size_t count) > +{ > + guard(rwsem_read)(&tsm_rwsem); This is unfortunate but it would need to be a rwsem_write otherwise two processes can race to reach the kvfree and both call report_new at the same time (unlikely as it may be). Jeremi > + if (!tsm_report.desc.inblob_len) > + return -EINVAL; > + > + if (!tsm_report.outblob || > + tsm_report.read_generation != tsm_report.write_generation) { > + const struct tsm_ops *ops = provider.ops; > + size_t outblob_len; > + u8 *outblob; > + > + kvfree(tsm_report.outblob); > + outblob = ops->report_new(provider.dev->parent, > + &tsm_report.desc, &outblob_len); > + if (IS_ERR(outblob)) > + return PTR_ERR(outblob); > + tsm_report.outblob_len = outblob_len; > + tsm_report.outblob = outblob; > + tsm_report.read_generation = tsm_report.write_generation; > + } > + > + return memory_read_from_buffer(buf, count, &offset, > + tsm_report.outblob, > + tsm_report.outblob_len); > +} > +static BIN_ATTR_RO(outblob, 0); > + > +static struct bin_attribute *tsm_bin_attributes[] = { > + &bin_attr_outblob, > + NULL, > +}; > + > +struct attribute_group tsm_default_attribute_group = { > + .bin_attrs = tsm_bin_attributes, > + .attrs = tsm_attributes, > +}; > +EXPORT_SYMBOL_GPL(tsm_default_attribute_group); > + > +static const struct attribute_group *tsm_default_attribute_groups[] = { > + &tsm_default_attribute_group, > + NULL, > +}; > + > +int register_tsm(const struct tsm_ops *ops, struct device *parent, > + const struct attribute_group **groups) > +{ > + const struct tsm_ops *conflict; > + struct device *dev; > + int rc; > + > + if (!parent) > + return -EINVAL; > + > + if (!groups) > + groups = tsm_default_attribute_groups; > + > + guard(rwsem_write)(&tsm_rwsem); > + conflict = provider.ops; > + if (conflict) { > + pr_err("\"%s\" ops already registered\n", conflict->name); > + return rc; > + } > + > + dev = device_create_with_groups(tsm_class, parent, 0, NULL, groups, > + "tsm0"); > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + > + provider.ops = ops; > + provider.dev = dev; > + return 0; > +} > +EXPORT_SYMBOL_GPL(register_tsm); > + > +int unregister_tsm(const struct tsm_ops *ops) > +{ > + guard(rwsem_write)(&tsm_rwsem); > + if (ops != provider.ops) > + return -EBUSY; > + provider.ops = NULL; > + device_unregister(provider.dev); > + provider.dev = NULL; > + kvfree(tsm_report.outblob); > + tsm_report.outblob = NULL; > + return 0; > +} > +EXPORT_SYMBOL_GPL(unregister_tsm); > + > +static int __init tsm_init(void) > +{ > + tsm_class = class_create("tsm"); > + return PTR_ERR_OR_ZERO(tsm_class); > +} > +module_init(tsm_init); > + > +static void __exit tsm_exit(void) > +{ > + class_destroy(tsm_class); > +} > +module_exit(tsm_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via sysfs"); > diff --git a/include/linux/tsm.h b/include/linux/tsm.h > new file mode 100644 > index 000000000000..6dc2f07543b8 > --- /dev/null > +++ b/include/linux/tsm.h > @@ -0,0 +1,45 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __TSM_H > +#define __TSM_H > + > +#include <linux/types.h> > +#include <linux/device.h> > + > +#define TSM_INBLOB_MAX 64 > + > +enum tsm_format { > + TSM_FORMAT_DEFAULT, > + TSM_FORMAT_EXTENDED, > +}; > + > +/** > + * struct tsm_desc - option descriptor for generating tsm report blobs > + * @privlevel: optional privilege level to associate with @outblob > + * @inblob_len: sizeof @inblob > + * @inblob: arbitrary input data > + * @outblob_format: for TSMs with an "extended" format > + */ > +struct tsm_desc { > + unsigned int privlevel; > + size_t inblob_len; > + u8 inblob[TSM_INBLOB_MAX]; > + enum tsm_format outblob_format; > +}; > + > +/* > + * arch specific ops, only one is expected to be registered at a time > + * i.e. only one of SEV, TDX, COVE, etc. > + */ > +struct tsm_ops { > + const char *name; > + u8 *(*report_new)(struct device *dev, const struct tsm_desc *desc, > + size_t *outblob_len); > +}; > + > +extern struct attribute_group tsm_default_attribute_group; > +extern struct attribute_group tsm_extra_attribute_group; > + > +int register_tsm(const struct tsm_ops *ops, struct device *parent, > + const struct attribute_group **groups); > +int unregister_tsm(const struct tsm_ops *ops); > +#endif /* __TSM_H */ > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 8:24 ` Jeremi Piotrowski @ 2023-08-14 16:21 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 16:21 UTC (permalink / raw) To: Jeremi Piotrowski, Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel Jeremi Piotrowski wrote: > On 8/14/2023 9:43 AM, 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 launch state, sign it and > > submit it to a verifying party. Upon successful attestation that > > verifies the integrity of the TVM additional secrets may be deployed. > > 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 sysfs interface to retrieve the TSM-specific blob. > > > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > > hexdump /sys/class/tsm/tsm0/outblob > > > > This approach later allows for the standardization of the attestation > > blob format without needing to change the Linux ABI. Until then, the > > format of 'outblob' is determined by the parent device for 'tsm0'. > > > > The expectation is that this is a boot time exchange that need not be > > regenerated, making it amenable to a sysfs interface. In case userspace > > does try to generate multiple attestation reports it includes conflict > > detection so userspace can be sure no other thread changed the > > parameters from its last configuration step to the blob retrieval. > > > > TSM specific options are encoded as 'extra' attributes on the TSM device > > with the expectation that vendors reuse the same options for similar > > concepts. The current options are defined by SEV-SNP's need for a > > 'privilege level' concept (VMPL), and the option to retrieve a > > certificate chain in addition to the attestation report ("extended" > > format). > > > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > > Cc: Peter Gonda <pgonda@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> > > --- [..] > > +static ssize_t outblob_read(struct file *f, struct kobject *kobj, > > + struct bin_attribute *bin_attr, char *buf, > > + loff_t offset, size_t count) > > +{ > > + guard(rwsem_read)(&tsm_rwsem); > > This is unfortunate but it would need to be a rwsem_write otherwise two > processes can race to reach the kvfree and both call report_new at the > same time (unlikely as it may be). Ugh, yup, good eye, will fix. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams 2023-08-14 8:24 ` Jeremi Piotrowski @ 2023-08-14 15:38 ` Greg Kroah-Hartman 2023-08-14 16:43 ` Dan Williams 2023-08-15 19:51 ` Tom Lendacky ` (3 subsequent siblings) 5 siblings, 1 reply; 47+ messages in thread From: Greg Kroah-Hartman @ 2023-08-14 15:38 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Samuel Ortiz, peterz, x86, linux-kernel On Mon, Aug 14, 2023 at 12:43:21AM -0700, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob Why is one way a hex-encode file, that the kernel has to parse, and the other not? Binary sysfs files should be "pass through" if at all possible, why not make them both binary and not mess with hex at all? That keeps the kernel simpler, and if userspace wants the hex format, they can provide it much easier (with less potential parsing errors). > > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm > new file mode 100644 > index 000000000000..37017bde626d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-tsm > @@ -0,0 +1,47 @@ > +What: /sys/class/tsm/tsm0/inhex > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) Hex encoded userdata to be included in the attestation > + report. For replay protection this should include a nonce, but > + the kernel does not place any restrictions on the content. "inhex" and it's read/write? Naming is hard :( > + > +What: /sys/class/tsm/tsm0/outblob > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) Binary attestation report generated from @inhex translated > + to binary and any options. The format of the report is vendor > + specific and determined by the parent device of 'tsm0'. > + > +What: /sys/class/tsm/tsm0/generation > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The value in this attribute increments each time @inhex or > + any option is written. Userspace can detect conflicts by > + checking generation before writing to any attribute and making > + sure the number of writes matches expectations after reading > + @outblob. > + > +What: /sys/class/tsm/tsm0/privlevel > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) If a TSM implementation supports the concept of attestation > + reports for TVMs running at different privilege levels, like > + SEV-SNP "VMPL", specify the privilege level via this attribute. Where is the list of potential values for this file at? > + > +What: /sys/class/tsm/tsm0/format > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RW) If a TSM implementation supports the concept of attestation > + reports with "extended" contents, like SEV-SNP extended reports > + with certificate chains, specify "extended" vs "default" via > + this attribute. > diff --git a/MAINTAINERS b/MAINTAINERS > index 3be1bdfe8ecc..97f74d344c8a 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -21625,6 +21625,14 @@ W: https://github.com/srcres258/linux-doc > T: git git://github.com/srcres258/linux-doc.git doc-zh-tw > F: Documentation/translations/zh_TW/ > > +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS > +M: Dan Williams <dan.j.williams@intel.com> > +L: linux-coco@lists.linux.dev > +S: Maintained > +F: Documentation/ABI/testing/sysfs-class-tsm > +F: drivers/virt/coco/tsm.c > +F: include/linux/tsm.h > + > TTY LAYER > M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > M: Jiri Slaby <jirislaby@kernel.org> > diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig > index fc5c64f04c4a..d92f07019f38 100644 > --- a/drivers/virt/coco/Kconfig > +++ b/drivers/virt/coco/Kconfig > @@ -2,6 +2,10 @@ > # > # Confidential computing related collateral > # > + > +config TSM_REPORTS > + tristate > + > source "drivers/virt/coco/efi_secret/Kconfig" > > source "drivers/virt/coco/sev-guest/Kconfig" > diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile > index 55302ef719ad..18c1aba5edb7 100644 > --- a/drivers/virt/coco/Makefile > +++ b/drivers/virt/coco/Makefile > @@ -2,6 +2,7 @@ > # > # Confidential computing related collateral > # > +obj-$(CONFIG_TSM_REPORTS) += tsm.o > obj-$(CONFIG_EFI_SECRET) += efi_secret/ > obj-$(CONFIG_SEV_GUEST) += sev-guest/ > obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/ > diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig > index 14246fc2fb02..22dd59e19431 100644 > --- a/drivers/virt/coco/tdx-guest/Kconfig > +++ b/drivers/virt/coco/tdx-guest/Kconfig > @@ -1,6 +1,7 @@ > config TDX_GUEST_DRIVER > tristate "TDX Guest driver" > depends on INTEL_TDX_GUEST > + select TSM_REPORTS > help > The driver provides userspace interface to communicate with > the TDX module to request the TDX guest details like attestation > diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c > new file mode 100644 > index 000000000000..1bf2ee82eb94 > --- /dev/null > +++ b/drivers/virt/coco/tsm.c > @@ -0,0 +1,290 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/tsm.h> > +#include <linux/err.h> > +#include <linux/slab.h> > +#include <linux/rwsem.h> > +#include <linux/device.h> > +#include <linux/string.h> > +#include <linux/module.h> > +#include <linux/cleanup.h> > + > +struct class *tsm_class; Nit, we are moving away from using class_create(), please make this a const static class and register it with the driver core instead. That way we don't have to fix it up in the future when we finally catch up with all of the existing class_create() calls we have left. See the patches in this -rc cycle for a bunch of them already, with many more on lkml for examples of how to convert this. Here's one example: https://lore.kernel.org/r/20230810174618.7844-1-ivan.orlov0322@gmail.com > +static struct tsm_provider { > + const struct tsm_ops *ops; > + struct device *dev; > +} provider; > +static DECLARE_RWSEM(tsm_rwsem); > + > +/** > + * DOC: Trusted Security Module (TSM) Attestation Report Interface > + * > + * The TSM report interface is a common provider of blobs that facilitate > + * attestation of a TVM (confidential computing guest) by an attestation > + * service. A TSM report combines a user-defined blob (likely a public-key with > + * a nonce for a key-exchange 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. The expectation is that this interface is > + * invoked infrequently, likely only once at TVM boot time. > + * > + * The attestation report format is TSM provider specific, when / if a standard > + * materializes that can be published instead of the vendor layout. Published where? > +/* > + * Input is a small hex blob, rather than a writable binary attribute, so that > + * it is conveyed atomically. > + */ > +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + u8 inblob[TSM_INBLOB_MAX]; Can this get too big for the stack? > +static ssize_t inhex_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + char *end; > + > + guard(rwsem_read)(&tsm_rwsem); I like seeing the guard() usage, very nice :) Overall, the sysfs api is ok, except for the hex values, which is easy to change. The usage of sysfs is ok as well, no complaints from me there. thanks, greg k-h ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 15:38 ` Greg Kroah-Hartman @ 2023-08-14 16:43 ` Dan Williams 2023-08-14 18:43 ` Greg Kroah-Hartman 0 siblings, 1 reply; 47+ messages in thread From: Dan Williams @ 2023-08-14 16:43 UTC (permalink / raw) To: Greg Kroah-Hartman, Dan Williams Cc: linux-coco, Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Samuel Ortiz, peterz, x86, linux-kernel Greg Kroah-Hartman wrote: > On Mon, Aug 14, 2023 at 12:43:21AM -0700, 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 launch state, sign it and > > submit it to a verifying party. Upon successful attestation that > > verifies the integrity of the TVM additional secrets may be deployed. > > 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 sysfs interface to retrieve the TSM-specific blob. > > > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > > hexdump /sys/class/tsm/tsm0/outblob > > Why is one way a hex-encode file, that the kernel has to parse, and the > other not? Binary sysfs files should be "pass through" if at all > possible, why not make them both binary and not mess with hex at all? > That keeps the kernel simpler, and if userspace wants the hex format, > they can provide it much easier (with less potential parsing errors). I can do that. The concern was the contract around what to do with partial writes since binary attributes allow writing the middle of the buffer. So either the attribute needs to enforce that @offset is always zero, or that the unwritten portion of the buffer is zeroed. I will go with just enforcing offset=zero writes. > > This approach later allows for the standardization of the attestation > > blob format without needing to change the Linux ABI. Until then, the > > format of 'outblob' is determined by the parent device for 'tsm0'. > > > > The expectation is that this is a boot time exchange that need not be > > regenerated, making it amenable to a sysfs interface. In case userspace > > does try to generate multiple attestation reports it includes conflict > > detection so userspace can be sure no other thread changed the > > parameters from its last configuration step to the blob retrieval. > > > > TSM specific options are encoded as 'extra' attributes on the TSM device > > with the expectation that vendors reuse the same options for similar > > concepts. The current options are defined by SEV-SNP's need for a > > 'privilege level' concept (VMPL), and the option to retrieve a > > certificate chain in addition to the attestation report ("extended" > > format). > > > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > > Cc: Peter Gonda <pgonda@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> > > --- > > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > > MAINTAINERS | 8 + > > drivers/virt/coco/Kconfig | 4 > > drivers/virt/coco/Makefile | 1 > > drivers/virt/coco/tdx-guest/Kconfig | 1 > > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > > include/linux/tsm.h | 45 +++++ > > 7 files changed, 396 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > > create mode 100644 drivers/virt/coco/tsm.c > > create mode 100644 include/linux/tsm.h > > > > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm > > new file mode 100644 > > index 000000000000..37017bde626d > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-class-tsm > > @@ -0,0 +1,47 @@ > > +What: /sys/class/tsm/tsm0/inhex > > +Date: August, 2023 > > +KernelVersion: v6.6 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + (RW) Hex encoded userdata to be included in the attestation > > + report. For replay protection this should include a nonce, but > > + the kernel does not place any restrictions on the content. > > "inhex" and it's read/write? Naming is hard :( True, could be write-only. > > > > + > > +What: /sys/class/tsm/tsm0/outblob > > +Date: August, 2023 > > +KernelVersion: v6.6 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + (RO) Binary attestation report generated from @inhex translated > > + to binary and any options. The format of the report is vendor > > + specific and determined by the parent device of 'tsm0'. > > + > > +What: /sys/class/tsm/tsm0/generation > > +Date: August, 2023 > > +KernelVersion: v6.6 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + (RO) The value in this attribute increments each time @inhex or > > + any option is written. Userspace can detect conflicts by > > + checking generation before writing to any attribute and making > > + sure the number of writes matches expectations after reading > > + @outblob. > > + > > +What: /sys/class/tsm/tsm0/privlevel > > +Date: August, 2023 > > +KernelVersion: v6.6 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + (RW) If a TSM implementation supports the concept of attestation > > + reports for TVMs running at different privilege levels, like > > + SEV-SNP "VMPL", specify the privilege level via this attribute. > > Where is the list of potential values for this file at? Good idea, let me bound this with some reasonable values. > > > + > > +What: /sys/class/tsm/tsm0/format > > +Date: August, 2023 > > +KernelVersion: v6.6 > > +Contact: linux-cxl@vger.kernel.org > > +Description: > > + (RW) If a TSM implementation supports the concept of attestation > > + reports with "extended" contents, like SEV-SNP extended reports > > + with certificate chains, specify "extended" vs "default" via > > + this attribute. > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 3be1bdfe8ecc..97f74d344c8a 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -21625,6 +21625,14 @@ W: https://github.com/srcres258/linux-doc > > T: git git://github.com/srcres258/linux-doc.git doc-zh-tw > > F: Documentation/translations/zh_TW/ > > > > +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS > > +M: Dan Williams <dan.j.williams@intel.com> > > +L: linux-coco@lists.linux.dev > > +S: Maintained > > +F: Documentation/ABI/testing/sysfs-class-tsm > > +F: drivers/virt/coco/tsm.c > > +F: include/linux/tsm.h > > + > > TTY LAYER > > M: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > M: Jiri Slaby <jirislaby@kernel.org> > > diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig > > index fc5c64f04c4a..d92f07019f38 100644 > > --- a/drivers/virt/coco/Kconfig > > +++ b/drivers/virt/coco/Kconfig > > @@ -2,6 +2,10 @@ > > # > > # Confidential computing related collateral > > # > > + > > +config TSM_REPORTS > > + tristate > > + > > source "drivers/virt/coco/efi_secret/Kconfig" > > > > source "drivers/virt/coco/sev-guest/Kconfig" > > diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile > > index 55302ef719ad..18c1aba5edb7 100644 > > --- a/drivers/virt/coco/Makefile > > +++ b/drivers/virt/coco/Makefile > > @@ -2,6 +2,7 @@ > > # > > # Confidential computing related collateral > > # > > +obj-$(CONFIG_TSM_REPORTS) += tsm.o > > obj-$(CONFIG_EFI_SECRET) += efi_secret/ > > obj-$(CONFIG_SEV_GUEST) += sev-guest/ > > obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/ > > diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig > > index 14246fc2fb02..22dd59e19431 100644 > > --- a/drivers/virt/coco/tdx-guest/Kconfig > > +++ b/drivers/virt/coco/tdx-guest/Kconfig > > @@ -1,6 +1,7 @@ > > config TDX_GUEST_DRIVER > > tristate "TDX Guest driver" > > depends on INTEL_TDX_GUEST > > + select TSM_REPORTS > > help > > The driver provides userspace interface to communicate with > > the TDX module to request the TDX guest details like attestation > > diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c > > new file mode 100644 > > index 000000000000..1bf2ee82eb94 > > --- /dev/null > > +++ b/drivers/virt/coco/tsm.c > > @@ -0,0 +1,290 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */ > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > + > > +#include <linux/tsm.h> > > +#include <linux/err.h> > > +#include <linux/slab.h> > > +#include <linux/rwsem.h> > > +#include <linux/device.h> > > +#include <linux/string.h> > > +#include <linux/module.h> > > +#include <linux/cleanup.h> > > + > > +struct class *tsm_class; > > Nit, we are moving away from using class_create(), please make this a > const static class and register it with the driver core instead. That > way we don't have to fix it up in the future when we finally catch up > with all of the existing class_create() calls we have left. > > See the patches in this -rc cycle for a bunch of them already, with many > more on lkml for examples of how to convert this. Here's one example: > https://lore.kernel.org/r/20230810174618.7844-1-ivan.orlov0322@gmail.com Got it. > > > +static struct tsm_provider { > > + const struct tsm_ops *ops; > > + struct device *dev; > > +} provider; > > +static DECLARE_RWSEM(tsm_rwsem); > > + > > +/** > > + * DOC: Trusted Security Module (TSM) Attestation Report Interface > > + * > > + * The TSM report interface is a common provider of blobs that facilitate > > + * attestation of a TVM (confidential computing guest) by an attestation > > + * service. A TSM report combines a user-defined blob (likely a public-key with > > + * a nonce for a key-exchange 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. The expectation is that this interface is > > + * invoked infrequently, likely only once at TVM boot time. > > + * > > + * The attestation report format is TSM provider specific, when / if a standard > > + * materializes that can be published instead of the vendor layout. > > Published where? Likely via a new attribute so that old scripts dependent on vendor specific attestation formats do not break. > > > +/* > > + * Input is a small hex blob, rather than a writable binary attribute, so that > > + * it is conveyed atomically. > > + */ > > +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + u8 inblob[TSM_INBLOB_MAX]; > > Can this get too big for the stack? As it stands the current implementations only allow 64-byte inputs. The question would be whether RISC-V or ARM need more space for their attestation report input buffers. Will keep an eye on it. > > +static ssize_t inhex_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + char *end; > > + > > + guard(rwsem_read)(&tsm_rwsem); > > I like seeing the guard() usage, very nice :) > > Overall, the sysfs api is ok, except for the hex values, which is easy > to change. The usage of sysfs is ok as well, no complaints from me > there. Thanks Greg. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 16:43 ` Dan Williams @ 2023-08-14 18:43 ` Greg Kroah-Hartman 0 siblings, 0 replies; 47+ messages in thread From: Greg Kroah-Hartman @ 2023-08-14 18:43 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Samuel Ortiz, peterz, x86, linux-kernel On Mon, Aug 14, 2023 at 09:43:37AM -0700, Dan Williams wrote: > Greg Kroah-Hartman wrote: > > On Mon, Aug 14, 2023 at 12:43:21AM -0700, 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 launch state, sign it and > > > submit it to a verifying party. Upon successful attestation that > > > verifies the integrity of the TVM additional secrets may be deployed. > > > 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 sysfs interface to retrieve the TSM-specific blob. > > > > > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > > > hexdump /sys/class/tsm/tsm0/outblob > > > > Why is one way a hex-encode file, that the kernel has to parse, and the > > other not? Binary sysfs files should be "pass through" if at all > > possible, why not make them both binary and not mess with hex at all? > > That keeps the kernel simpler, and if userspace wants the hex format, > > they can provide it much easier (with less potential parsing errors). > > I can do that. The concern was the contract around what to do with > partial writes since binary attributes allow writing the middle of the > buffer. So either the attribute needs to enforce that @offset is always > zero, or that the unwritten portion of the buffer is zeroed. I will go > with just enforcing offset=zero writes. Enforcing that sounds sane, thanks. greg k-h ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams 2023-08-14 8:24 ` Jeremi Piotrowski 2023-08-14 15:38 ` Greg Kroah-Hartman @ 2023-08-15 19:51 ` Tom Lendacky 2023-08-16 14:44 ` Tom Lendacky ` (2 subsequent siblings) 5 siblings, 0 replies; 47+ messages in thread From: Tom Lendacky @ 2023-08-15 19:51 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel On 8/14/23 02:43, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob > > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > +/* > + * Input is a small hex blob, rather than a writable binary attribute, so that > + * it is conveyed atomically. > + */ > +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + u8 inblob[TSM_INBLOB_MAX]; > + size_t inblob_len; > + int rc; > + > + inblob_len = len; > + if (buf[len - 1] == '\n') > + inblob_len--; > + if (inblob_len & 1) > + return -EINVAL; > + inblob_len /= 2; > + if (inblob_len > TSM_INBLOB_MAX) > + return -EINVAL; Is an array_index_nospec() call needed after this check? > + > + rc = hex2bin(inblob, buf, inblob_len); > + if (rc < 0) > + return rc; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (memcmp(tsm_report.desc.inblob, inblob, inblob_len) == 0) > + return len; > + memcpy(tsm_report.desc.inblob, inblob, inblob_len); Should the portion of tsm_report.desc.inblob that is not updated be cleared if inblob_len != TSM_INBLOB_MAX? > + tsm_report.desc.inblob_len = inblob_len; > + tsm_report.write_generation++; > + > + return len; > +} > + > +int register_tsm(const struct tsm_ops *ops, struct device *parent, > + const struct attribute_group **groups) > +{ > + const struct tsm_ops *conflict; > + struct device *dev; > + int rc; > + > + if (!parent) > + return -EINVAL; > + > + if (!groups) > + groups = tsm_default_attribute_groups; > + > + guard(rwsem_write)(&tsm_rwsem); > + conflict = provider.ops; > + if (conflict) { > + pr_err("\"%s\" ops already registered\n", conflict->name); > + return rc; Looks like rc is used uninitialized. > + } > + > + dev = device_create_with_groups(tsm_class, parent, 0, NULL, groups, > + "tsm0"); You can go out to 100 characters now, so this could all be one line. Thanks, Tom > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + > + provider.ops = ops; > + provider.dev = dev; > + return 0; > +} > +EXPORT_SYMBOL_GPL(register_tsm); > + ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams ` (2 preceding siblings ...) 2023-08-15 19:51 ` Tom Lendacky @ 2023-08-16 14:44 ` Tom Lendacky 2023-08-16 15:12 ` Dan Williams 2023-08-22 7:29 ` Roy Hopkins 2023-08-23 13:49 ` Samuel Ortiz 5 siblings, 1 reply; 47+ messages in thread From: Tom Lendacky @ 2023-08-16 14:44 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel On 8/14/23 02:43, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob > > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > +static ssize_t privlevel_store(struct device *dev, > + struct device_attribute *attr, const char *buf, > + size_t len) > +{ > + unsigned int val; > + int rc; > + > + rc = kstrtouint(buf, 0, &val); > + if (rc) > + return rc; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (tsm_report.desc.privlevel == val) > + return len; > + tsm_report.desc.privlevel = val; > + tsm_report.write_generation++; So I'm wondering if this use of write_generation helps or not. Since it isn't incremented if the levels are the same, that might present race conditions. What if user A requests vmpl 2 and privlevel is already 2, then write_generation is not incremented. But before user A can read back the generation value user B can request vmpl 3 and cause write_generation to be incremented. This may not be a problem for VMPL, since that can be checked in the returned attestation report, but it could be for the report format. If the extended format is requested but changed to default, then the additional certs might not be returned and the guest may think there aren't any...? Maybe incrementing the write_generation no matter what is best. Thanks, Tom > + > + return len; > +} > + > +static ssize_t privlevel_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + return sysfs_emit(buf, "%u\n", tsm_report.desc.privlevel); > +} > + > +static DEVICE_ATTR_RW(privlevel); > + > +static ssize_t format_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + enum tsm_format format; > + > + if (sysfs_streq(buf, "default")) > + format = TSM_FORMAT_DEFAULT; > + else if (sysfs_streq(buf, "extended")) > + format = TSM_FORMAT_EXTENDED; > + else > + return -EINVAL; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (tsm_report.desc.outblob_format == format) > + return len; > + tsm_report.desc.outblob_format = format; > + tsm_report.write_generation++; > + > + return len; > +} > + > +static ssize_t format_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + if (tsm_report.desc.outblob_format == TSM_FORMAT_DEFAULT) > + return sysfs_emit(buf, "default\n"); > + return sysfs_emit(buf, "extended\n"); > +} > + > +static DEVICE_ATTR_RW(format); > + > +static struct attribute *tsm_extra_attributes[] = { > + &dev_attr_format.attr, > + &dev_attr_privlevel.attr, > + NULL, > +}; > + > +struct attribute_group tsm_extra_attribute_group = { > + .attrs = tsm_extra_attributes, > +}; > +EXPORT_SYMBOL_GPL(tsm_extra_attribute_group); > + > +/* > + * Input is a small hex blob, rather than a writable binary attribute, so that > + * it is conveyed atomically. > + */ > +static ssize_t inhex_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + u8 inblob[TSM_INBLOB_MAX]; > + size_t inblob_len; > + int rc; > + > + inblob_len = len; > + if (buf[len - 1] == '\n') > + inblob_len--; > + if (inblob_len & 1) > + return -EINVAL; > + inblob_len /= 2; > + if (inblob_len > TSM_INBLOB_MAX) > + return -EINVAL; > + > + rc = hex2bin(inblob, buf, inblob_len); > + if (rc < 0) > + return rc; > + > + guard(rwsem_write)(&tsm_rwsem); > + if (memcmp(tsm_report.desc.inblob, inblob, inblob_len) == 0) > + return len; > + memcpy(tsm_report.desc.inblob, inblob, inblob_len); > + tsm_report.desc.inblob_len = inblob_len; > + tsm_report.write_generation++; > + > + return len; > +} > + > +static ssize_t inhex_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + char *end; > + > + guard(rwsem_read)(&tsm_rwsem); > + if (!tsm_report.desc.inblob_len) > + return 0; > + end = bin2hex(buf, tsm_report.desc.inblob, tsm_report.desc.inblob_len); > + *end++ = '\n'; > + return end - buf; > +} > +static DEVICE_ATTR_RW(inhex); > + > +static ssize_t generation_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + guard(rwsem_read)(&tsm_rwsem); > + return sysfs_emit(buf, "%lu\n", tsm_report.write_generation); > +} > +static DEVICE_ATTR_RO(generation); > + > +static struct attribute *tsm_attributes[] = { > + &dev_attr_inhex.attr, > + &dev_attr_generation.attr, > + NULL, > +}; > + > +static ssize_t outblob_read(struct file *f, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t offset, size_t count) > +{ > + guard(rwsem_read)(&tsm_rwsem); > + if (!tsm_report.desc.inblob_len) > + return -EINVAL; > + > + if (!tsm_report.outblob || > + tsm_report.read_generation != tsm_report.write_generation) { > + const struct tsm_ops *ops = provider.ops; > + size_t outblob_len; > + u8 *outblob; > + > + kvfree(tsm_report.outblob); > + outblob = ops->report_new(provider.dev->parent, > + &tsm_report.desc, &outblob_len); > + if (IS_ERR(outblob)) > + return PTR_ERR(outblob); > + tsm_report.outblob_len = outblob_len; > + tsm_report.outblob = outblob; > + tsm_report.read_generation = tsm_report.write_generation; > + } > + > + return memory_read_from_buffer(buf, count, &offset, > + tsm_report.outblob, > + tsm_report.outblob_len); > +} > +static BIN_ATTR_RO(outblob, 0); > + > +static struct bin_attribute *tsm_bin_attributes[] = { > + &bin_attr_outblob, > + NULL, > +}; > + > +struct attribute_group tsm_default_attribute_group = { > + .bin_attrs = tsm_bin_attributes, > + .attrs = tsm_attributes, > +}; > +EXPORT_SYMBOL_GPL(tsm_default_attribute_group); > + > +static const struct attribute_group *tsm_default_attribute_groups[] = { > + &tsm_default_attribute_group, > + NULL, > +}; > + > +int register_tsm(const struct tsm_ops *ops, struct device *parent, > + const struct attribute_group **groups) > +{ > + const struct tsm_ops *conflict; > + struct device *dev; > + int rc; > + > + if (!parent) > + return -EINVAL; > + > + if (!groups) > + groups = tsm_default_attribute_groups; > + > + guard(rwsem_write)(&tsm_rwsem); > + conflict = provider.ops; > + if (conflict) { > + pr_err("\"%s\" ops already registered\n", conflict->name); > + return rc; > + } > + > + dev = device_create_with_groups(tsm_class, parent, 0, NULL, groups, > + "tsm0"); > + if (IS_ERR(dev)) > + return PTR_ERR(dev); > + > + provider.ops = ops; > + provider.dev = dev; > + return 0; > +} > +EXPORT_SYMBOL_GPL(register_tsm); > + > +int unregister_tsm(const struct tsm_ops *ops) > +{ > + guard(rwsem_write)(&tsm_rwsem); > + if (ops != provider.ops) > + return -EBUSY; > + provider.ops = NULL; > + device_unregister(provider.dev); > + provider.dev = NULL; > + kvfree(tsm_report.outblob); > + tsm_report.outblob = NULL; > + return 0; > +} > +EXPORT_SYMBOL_GPL(unregister_tsm); > + > +static int __init tsm_init(void) > +{ > + tsm_class = class_create("tsm"); > + return PTR_ERR_OR_ZERO(tsm_class); > +} > +module_init(tsm_init); > + > +static void __exit tsm_exit(void) > +{ > + class_destroy(tsm_class); > +} > +module_exit(tsm_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via sysfs"); > diff --git a/include/linux/tsm.h b/include/linux/tsm.h > new file mode 100644 > index 000000000000..6dc2f07543b8 > --- /dev/null > +++ b/include/linux/tsm.h > @@ -0,0 +1,45 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __TSM_H > +#define __TSM_H > + > +#include <linux/types.h> > +#include <linux/device.h> > + > +#define TSM_INBLOB_MAX 64 > + > +enum tsm_format { > + TSM_FORMAT_DEFAULT, > + TSM_FORMAT_EXTENDED, > +}; > + > +/** > + * struct tsm_desc - option descriptor for generating tsm report blobs > + * @privlevel: optional privilege level to associate with @outblob > + * @inblob_len: sizeof @inblob > + * @inblob: arbitrary input data > + * @outblob_format: for TSMs with an "extended" format > + */ > +struct tsm_desc { > + unsigned int privlevel; > + size_t inblob_len; > + u8 inblob[TSM_INBLOB_MAX]; > + enum tsm_format outblob_format; > +}; > + > +/* > + * arch specific ops, only one is expected to be registered at a time > + * i.e. only one of SEV, TDX, COVE, etc. > + */ > +struct tsm_ops { > + const char *name; > + u8 *(*report_new)(struct device *dev, const struct tsm_desc *desc, > + size_t *outblob_len); > +}; > + > +extern struct attribute_group tsm_default_attribute_group; > +extern struct attribute_group tsm_extra_attribute_group; > + > +int register_tsm(const struct tsm_ops *ops, struct device *parent, > + const struct attribute_group **groups); > +int unregister_tsm(const struct tsm_ops *ops); > +#endif /* __TSM_H */ > > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-16 14:44 ` Tom Lendacky @ 2023-08-16 15:12 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-16 15:12 UTC (permalink / raw) To: Tom Lendacky, Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel Tom Lendacky wrote: > > +static ssize_t privlevel_store(struct device *dev, > > + struct device_attribute *attr, const char *buf, > > + size_t len) > > +{ > > + unsigned int val; > > + int rc; > > + > > + rc = kstrtouint(buf, 0, &val); > > + if (rc) > > + return rc; > > + > > + guard(rwsem_write)(&tsm_rwsem); > > + if (tsm_report.desc.privlevel == val) > > + return len; > > + tsm_report.desc.privlevel = val; > > + tsm_report.write_generation++; > > So I'm wondering if this use of write_generation helps or not. Since it > isn't incremented if the levels are the same, that might present race > conditions. > > What if user A requests vmpl 2 and privlevel is already 2, then > write_generation is not incremented. But before user A can read back the > generation value user B can request vmpl 3 and cause write_generation to > be incremented. > > This may not be a problem for VMPL, since that can be checked in the > returned attestation report, but it could be for the report format. If the > extended format is requested but changed to default, then the additional > certs might not be returned and the guest may think there aren't any...? > > Maybe incrementing the write_generation no matter what is best. True, and good eye. If write_generation does not always increment once per write there is no way to assume the state of the parameters. Will fix. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams ` (3 preceding siblings ...) 2023-08-16 14:44 ` Tom Lendacky @ 2023-08-22 7:29 ` Roy Hopkins 2023-08-23 13:49 ` Samuel Ortiz 5 siblings, 0 replies; 47+ messages in thread From: Roy Hopkins @ 2023-08-22 7:29 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, peterz, x86, linux-kernel On Mon, 2023-08-14 at 00:43 -0700, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob > > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: > http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch > [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 > +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > +static ssize_t outblob_read(struct file *f, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t offset, size_t count) > +{ > + guard(rwsem_read)(&tsm_rwsem); > + if (!tsm_report.desc.inblob_len) > + return -EINVAL; > + > + if (!tsm_report.outblob || > + tsm_report.read_generation != tsm_report.write_generation) { > + const struct tsm_ops *ops = provider.ops; > + size_t outblob_len; > + u8 *outblob; > + > + kvfree(tsm_report.outblob); I think tsm_report.outblob needs to be set to NULL here otherwise there is the possibility of a double-free if ops->report_new returns an error. Roy > + outblob = ops->report_new(provider.dev->parent, > + &tsm_report.desc, &outblob_len); > + if (IS_ERR(outblob)) > + return PTR_ERR(outblob); > + tsm_report.outblob_len = outblob_len; > + tsm_report.outblob = outblob; > + tsm_report.read_generation = tsm_report.write_generation; > + } > + > + return memory_read_from_buffer(buf, count, &offset, > + tsm_report.outblob, > + tsm_report.outblob_len); > +} ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams ` (4 preceding siblings ...) 2023-08-22 7:29 ` Roy Hopkins @ 2023-08-23 13:49 ` Samuel Ortiz 2023-08-28 10:46 ` Dr. Greg 5 siblings, 1 reply; 47+ messages in thread From: Samuel Ortiz @ 2023-08-23 13:49 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, peterz, x86, linux-kernel Hi Dan, On Mon, Aug 14, 2023 at 12:43:21AM -0700, 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 launch state, sign it and > submit it to a verifying party. Upon successful attestation that > verifies the integrity of the TVM additional secrets may be deployed. > 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 sysfs interface to retrieve the TSM-specific blob. > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > hexdump /sys/class/tsm/tsm0/outblob My concern with that interface is that one could easily get an attestation report with a nonce set by another userspace component or thread. I realize there is a generation counter to detect if a configuration changed between the caller's last config setting and the report it got, but I think that this shows that this may not be the best interface. IMHO an attestation report request from userspace should be an atomic call that includes multiple platform independent attibutes like e.g. an attestation nonce. > This approach later allows for the standardization of the attestation > blob format without needing to change the Linux ABI. Until then, the > format of 'outblob' is determined by the parent device for 'tsm0'. > > The expectation is that this is a boot time exchange that need not be > regenerated, This works well with the encrypted boot disk that's decrypted through attestation use-case, but this is just one use case. Although I don't expect attestation requests to be frequent, we should not assume this is only a boot time operation. Not only it can happen after the guest is fully booted, but it can also happen multiple times. An attestation flow where a guest gets an attestation token back from a validated report is something we'd want to support. Those token's validity are time limited, and userspace would want to regenerate a report, with a fresh, attestation service provided nonce. Another thing to keep in mind is that an attestation report could be amended by userspace itself, for TEE that support runtime measurement (The RTMR things...). So the TVM measurement itself could change during the lifecycle of a TVM. > making it amenable to a sysfs interface. In case userspace > does try to generate multiple attestation reports it includes conflict > detection so userspace can be sure no other thread changed the > parameters from its last configuration step to the blob retrieval. > > TSM specific options are encoded as 'extra' attributes on the TSM device > with the expectation that vendors reuse the same options for similar > concepts. The current options are defined by SEV-SNP's need for a > 'privilege level' concept (VMPL), and the option to retrieve a > certificate chain in addition to the attestation report ("extended" > format). > > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1] > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Peter Gonda <pgonda@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> > --- > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/coco/Kconfig | 4 > drivers/virt/coco/Makefile | 1 > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/tsm.h | 45 +++++ > 7 files changed, 396 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm > new file mode 100644 > index 000000000000..37017bde626d > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-tsm > @@ -0,0 +1,47 @@ > +What: /sys/class/tsm/tsm0/inhex > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org Did you mean linux-coco@ ? > +Description: > + (RW) Hex encoded userdata to be included in the attestation > + report. For replay protection this should include a nonce, but > + the kernel does not place any restrictions on the content. > + > +What: /sys/class/tsm/tsm0/outblob > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) Binary attestation report generated from @inhex translated > + to binary and any options. The format of the report is vendor > + specific and determined by the parent device of 'tsm0'. > + > +What: /sys/class/tsm/tsm0/generation > +Date: August, 2023 > +KernelVersion: v6.6 > +Contact: linux-cxl@vger.kernel.org > +Description: > + (RO) The value in this attribute increments each time @inhex or > + any option is written. Userspace can detect conflicts by > + checking generation before writing to any attribute and making > + sure the number of writes matches expectations after reading > + @outblob. One note here is that an attestation userspace thread could get an invalid counter although the report is valid, if e.g. a separate thread modified a config between the first thread report generation and it reading the counter back. So an attestation report generating thread could get false negatives with that interface. Cheers, Samuel. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports 2023-08-23 13:49 ` Samuel Ortiz @ 2023-08-28 10:46 ` Dr. Greg 0 siblings, 0 replies; 47+ messages in thread From: Dr. Greg @ 2023-08-28 10:46 UTC (permalink / raw) To: Samuel Ortiz Cc: Dan Williams, linux-coco, Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley, Peter Gonda, Greg Kroah-Hartman, peterz, x86, linux-kernel On Wed, Aug 23, 2023 at 03:49:17PM +0200, Samuel Ortiz wrote: Good morning, I hope the week is starting well for everyone. Some further background to hopefully assist decision making on how to handle the export of attestation information for COCO TSM's. > Hi Dan, > > On Mon, Aug 14, 2023 at 12:43:21AM -0700, 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 launch state, sign it and > > submit it to a verifying party. Upon successful attestation that > > verifies the integrity of the TVM additional secrets may be deployed. > > 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 sysfs interface to retrieve the TSM-specific blob. > > > > echo $hex_encoded_userdata_plus_nonce > /sys/class/tsm/tsm0/inhex > > hexdump /sys/class/tsm/tsm0/outblob > My concern with that interface is that one could easily get an > attestation report with a nonce set by another userspace component > or thread. I realize there is a generation counter to detect if a > configuration changed between the caller's last config setting and > the report it got, but I think that this shows that this may not be > the best interface. IMHO an attestation report request from > userspace should be an atomic call that includes multiple platform > independent attibutes like e.g. an attestation nonce. The challenge in all of this would seem to be the need to get a 20+ year ABI 'right' on the first try, so some issues to consider based on our experiences building trusted systems, albeit with a focus on endpoint devices. The issue about needing to implement attestation atomically is on point. This requirement would seem to suggest that the optimum path forward for confidential computing will be to have attestation support implemented in the resource orchestration infrastructure inside the TVM. Since the goal is confidentiality, you need to trust everything that executes inside the execution domain/VM. This is particularly true given the reality of micro-architectural side channel issues, which don't seem to be going away and which Intel explicitly documents that you must address with TDX. Given that, there would seem to be no reason not to have the attestation support all in one place and then provide access to that functionality to anything in the TVM that would want to make an integrity statement to a relying party. A sysfs interface is perfectly suited to the this model, since the trust orchestrator, if we use our terminology, can address the concurrency and atomicity issues presented by a sysfs interface. From an Intel/SGX centric perspective, which is our area of expertise when it comes to the notion of a TSM, the attestation process is reasonably complex, even more so in a TDX environment, since you need to go outside the trusted execution domain in order to conduct the full attestation process. I don't know how many people have direct experience with all of this, but we can offer it on good authority that the COCO model will benefit from having all the necessary procedures done correctly and in one place > > This approach later allows for the standardization of the > > attestation blob format without needing to change the Linux > > ABI. Until then, the format of 'outblob' is determined by the > > parent device for 'tsm0'. > > > > The expectation is that this is a boot time exchange that need not > > be regenerated, > This works well with the encrypted boot disk that's decrypted > through attestation use-case, but this is just one use > case. Although I don't expect attestation requests to be frequent, > we should not assume this is only a boot time operation. Not only it > can happen after the guest is fully booted, but it can also happen > multiple times. An attestation flow where a guest gets an > attestation token back from a validated report is something we'd > want to support. Those token's validity are time limited, and > userspace would want to regenerate a report, with a fresh, > attestation service provided nonce. There would certainly seem to be no argument that a trusted execution domain may want to conduct multiple attestations, Jeremi suggested this is something the Confidential Containers initiative would want as well. The model of having attestation support centralized in the resource orchestration infrastructure is not only consistent with that need but would also appear to be the optimal approach. Let us consider TDX as an example, since it will undoubtedly be a major player in COCO. In the Data Center Attestation Primitives (DCAP) model, one needs to choregraph a dance between the Provisioning Certification Enclave (PCE) and the Quoting Enclave (QE) in order to conduct the attestation process. That requires access to SGX functionality, something that is not supported by Intel in a TDX mediated virtual machine. This imposes a need to go outside of the TD guest, to the host, in order to get access to SGX in order to load and run the PCE and QE enclaves. The call into the kernel to access the hypervisor interface is simply the starting point for the attestation process. The purpose is to have the TDX module generate a CPU specific report (SEAMREPORT) that will ultimately authenticate material, usually ECDH key exchange components, as coming from an execution domain with known provenance. That report then needs to be processed through the PCE and QE enclave infrastructure in order to fully prove the identity of the report and thus authenticate its contents. Mixed into all of this is a need to verify the MROWNER and MRSIGNER signatures on the involved enclaves as well as the Security Version Numbers (SVN's) on the enclaves and the TDX infrastructure. This process ends up authenticating a certificate chain which proves that a secret to be conveyed into the TD is actually being injected into a valid TD, with known provenance and one that is 'Genuine Intel' in origin. All of this doesn't seem to speak of infrastructure you would want to replicate multiple times over. > Another thing to keep in mind is that an attestation report could be > amended by userspace itself, for TEE that support runtime > measurement (The RTMR things...). So the TVM measurement itself > could change during the lifecycle of a TVM. Once again, speaking only to TDX, it isn't apparent that the RTMR registers are going to be useful for the task of confirming general application level integrity once the OS is started. Their primary utility will be to provide a root of trust for the resource orchestration infrastructure and in turn VTPM's, or something like TSEM's root security modeling namespace, if the goal is to provide a foundation for generalized trust/integrity mechanisms inside of the TVM. To wit: The formal usage recommendations for the TDX RTMR registers are as follows: RTMR[0]: PCR[1,7] RTMR[1]: PCR[4,5] RTMR[2]: PCR[8,15] RTMR[3]: Special use Where RTMR[2] is specified for usage by the OS/application for measurement purposes, so only the equivalent of one PCR is available. This would automatically preclude the use of systemd in a 'trusted' VM implementation, given that the latest Linux TPM PCR registry has systemd consuming PCR registers 11-15 for its own use. Once again, if you are really serious about confidentiality, you need to have an attestable integrity guarantee for everything that happens inside the trusted VM. IMA uses register 10 if you go that route and we will be using a register above what systemd is using for the TSEM root namespace. So we believe the lone trusted virtual machine measurement register will need to stay constant, in order to easily validate whatever root of trust that is in turn designated to guarantee the integrity of what goes on inside of the TVM from an OS/application perspective. A functional attestation scheme is thus going to require not only attestation of the state of the TVM but also what has gone on inside of the VM after it has booted. Another reason for having a single shopping site for the attestation needs of whatever runs inside of the trusted VM. FWIW, since we copied the LSM list on this, we haven't come by the design and implementation of TSEM and the notion of trust orchestration idly. If COCO is going to become a relevant reality there needs to be consideration given to the concept of trust orchestraton and management. Once gain, FWIW, and something that Jeremi from Microsoft might be able to comment on. It is unclear how COCO based Confidential Containers fits into the standard IMA attestation model, where one has to review an event log documented by a linear extension trust measurement to determine whether or not the system is to be trusted. At a minimum, this would require that a relying party for attestation of one container be able to verify what has gone on in all other container invocations. Unless the containers are being run for only a single entity, this would seem to imply a violation of the notion of privacy and confidentiality. > Cheers, > Samuel. Hopefully all of the above is of assistance in getting the ABI right and the scope of issues involved. Best wishes for a productive week. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams 2023-08-14 7:43 ` [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams 2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams @ 2023-08-14 7:43 ` Dan Williams 2023-08-14 16:58 ` Dionna Amalie Glaze 2023-08-15 20:20 ` Tom Lendacky 2023-08-14 7:43 ` [PATCH v2 4/5] mm/slab: Add __free() support for kvfree Dan Williams ` (2 subsequent siblings) 5 siblings, 2 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw) To: linux-coco Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel 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; ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams @ 2023-08-14 16:58 ` Dionna Amalie Glaze 2023-08-14 23:24 ` Dan Williams 2023-08-15 20:20 ` Tom Lendacky 1 sibling, 1 reply; 47+ messages in thread From: Dionna Amalie Glaze @ 2023-08-14 16:58 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Brijesh Singh, peterz, x86, linux-kernel > > 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; Do we have an agreement around the continued existence of sev-guest for supporting derived keys, is that similarly slated for the chopping block, or is it left undecided? It appears your choice to not include the uarg/karg extension here is deliberate. > case SNP_GET_EXT_REPORT: > - ret = get_ext_report(snp_dev, &input); > + ret = get_ext_report(snp_dev, &input, SNP_UARG); > break; > default: > break; > Reviewed-by: Dionna Glaze <dionnaglaze@google.com> -- -Dionna Glaze, PhD (she/her) ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-14 16:58 ` Dionna Amalie Glaze @ 2023-08-14 23:24 ` Dan Williams 2023-08-15 20:11 ` Tom Lendacky 0 siblings, 1 reply; 47+ messages in thread From: Dan Williams @ 2023-08-14 23:24 UTC (permalink / raw) To: Dionna Amalie Glaze, Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Brijesh Singh, peterz, x86, linux-kernel Dionna Amalie Glaze wrote: > > > > 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; > > Do we have an agreement around the continued existence of sev-guest > for supporting derived keys, is that similarly slated for the chopping > block, or is it left undecided? > It appears your choice to not include the uarg/karg extension here is > deliberate. I do want to understand the argument from James a bit more, but the exlcusion here was simply because there is currently no support for this concept from other vendors. That said, if it remains a vendor unique concept and continues getting suspicious looks from folks like James, it may indeed be something the kernel wants to jettison. > > case SNP_GET_EXT_REPORT: > > - ret = get_ext_report(snp_dev, &input); > > + ret = get_ext_report(snp_dev, &input, SNP_UARG); > > break; > > default: > > break; > > > > Reviewed-by: Dionna Glaze <dionnaglaze@google.com> Thanks for all your help on this! ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-14 23:24 ` Dan Williams @ 2023-08-15 20:11 ` Tom Lendacky 2023-08-15 21:03 ` Dan Williams 0 siblings, 1 reply; 47+ messages in thread From: Tom Lendacky @ 2023-08-15 20:11 UTC (permalink / raw) To: Dan Williams, Dionna Amalie Glaze Cc: linux-coco, Borislav Petkov, Brijesh Singh, peterz, x86, linux-kernel On 8/14/23 18:24, Dan Williams wrote: > Dionna Amalie Glaze wrote: >>> >>> 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; >> >> Do we have an agreement around the continued existence of sev-guest >> for supporting derived keys, is that similarly slated for the chopping >> block, or is it left undecided? >> It appears your choice to not include the uarg/karg extension here is >> deliberate. > > I do want to understand the argument from James a bit more, but the > exlcusion here was simply because there is currently no support for this > concept from other vendors. > > That said, if it remains a vendor unique concept and continues getting > suspicious looks from folks like James, it may indeed be something the > kernel wants to jettison. I'm not sure why we would want to jettison it. Just because other vendors don't have a key derivation function doesn't mean it can't be useful to customers that want to use it on AMD platforms. Thanks, Tom > >>> case SNP_GET_EXT_REPORT: >>> - ret = get_ext_report(snp_dev, &input); >>> + ret = get_ext_report(snp_dev, &input, SNP_UARG); >>> break; >>> default: >>> break; >>> >> >> Reviewed-by: Dionna Glaze <dionnaglaze@google.com> > > Thanks for all your help on this! ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-15 20:11 ` Tom Lendacky @ 2023-08-15 21:03 ` Dan Williams 2023-08-16 19:38 ` Dionna Amalie Glaze 0 siblings, 1 reply; 47+ messages in thread From: Dan Williams @ 2023-08-15 21:03 UTC (permalink / raw) To: Tom Lendacky, Dan Williams, Dionna Amalie Glaze Cc: linux-coco, Borislav Petkov, Brijesh Singh, peterz, x86, linux-kernel Tom Lendacky wrote: > On 8/14/23 18:24, Dan Williams wrote: > > Dionna Amalie Glaze wrote: > >>> > >>> 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; > >> > >> Do we have an agreement around the continued existence of sev-guest > >> for supporting derived keys, is that similarly slated for the chopping > >> block, or is it left undecided? > >> It appears your choice to not include the uarg/karg extension here is > >> deliberate. > > > > I do want to understand the argument from James a bit more, but the > > exlcusion here was simply because there is currently no support for this > > concept from other vendors. > > > > That said, if it remains a vendor unique concept and continues getting > > suspicious looks from folks like James, it may indeed be something the > > kernel wants to jettison. > > I'm not sure why we would want to jettison it. Just because other vendors > don't have a key derivation function doesn't mean it can't be useful to > customers that want to use it on AMD platforms. Definitely, instead it was this comment from James that gave me pause: "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)." http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-15 21:03 ` Dan Williams @ 2023-08-16 19:38 ` Dionna Amalie Glaze 0 siblings, 0 replies; 47+ messages in thread From: Dionna Amalie Glaze @ 2023-08-16 19:38 UTC (permalink / raw) To: Dan Williams Cc: Tom Lendacky, linux-coco, Borislav Petkov, Brijesh Singh, peterz, x86, linux-kernel > Definitely, instead it was this comment from James that gave me pause: > > "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)." > Key derivation on AMD SEV-SNP is not necessarily tied to a physical system with the introduction of VLEK-based attestation. It's now tied to a CSP's fleet of machines. We can use key derivation in the SVSM as a basis for further key derivation based on measurement registers, so the utility increases to provide something like persisted sealed data that can only be unsealed when the SVSM witnesses a particular runtime measurement configuration. We can use NIST 800-90A Rev. 1 for combining keys from the PSP with measurement register values for example. > http://lore.kernel.org/r/c6576d1682b576ba47556478a98f397ed518a177.camel@HansenPartnership.com -- -Dionna Glaze, PhD (she/her) ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams 2023-08-14 16:58 ` Dionna Amalie Glaze @ 2023-08-15 20:20 ` Tom Lendacky 2023-08-15 21:37 ` Dan Williams 1 sibling, 1 reply; 47+ messages in thread From: Tom Lendacky @ 2023-08-15 20:20 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel On 8/14/23 02:43, Dan Williams wrote: > 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 s/confidenital/confidential/ > 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); I'm a fan of blank lines to make reading functions easier. A blank line here and below after the memcpy() would be nice. Ditto in the copy_to() function. > + 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) You can go out to 100 characters now, so you can put "struct .. *arg" on the top line and just put the enum on a new line. > { > 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) Ditto here on the 100 characters. > { > 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)) { This can also be a single line now. Thanks, Tom > 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; > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() 2023-08-15 20:20 ` Tom Lendacky @ 2023-08-15 21:37 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-15 21:37 UTC (permalink / raw) To: Tom Lendacky, Dan Williams, linux-coco Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel Tom Lendacky wrote: > On 8/14/23 02:43, Dan Williams wrote: > > 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 > > s/confidenital/confidential/ > > > 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); > > I'm a fan of blank lines to make reading functions easier. A blank line > here and below after the memcpy() would be nice. > > Ditto in the copy_to() function. > > > + 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) > > You can go out to 100 characters now, so you can put "struct .. *arg" on > the top line and just put the enum on a new line. > > > { > > 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) > > Ditto here on the 100 characters. > > > { > > 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)) { > > This can also be a single line now. So the kernel's .clang-format template still enforces the 80 column limit. I am ok to bump that to 100 temporarily for my edits to this file, but in general I tend to just let clang-format do its thing. One less thing to worry about. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams ` (2 preceding siblings ...) 2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams @ 2023-08-14 7:43 ` Dan Williams 2023-08-14 15:31 ` Greg Kroah-Hartman 2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams 2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski 5 siblings, 1 reply; 47+ messages in thread From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw) To: linux-coco Cc: Andrew Morton, Peter Zijlstra, Greg Kroah-Hartman, x86, linux-kernel 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); ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 7:43 ` [PATCH v2 4/5] mm/slab: Add __free() support for kvfree Dan Williams @ 2023-08-14 15:31 ` Greg Kroah-Hartman 2023-08-14 16:17 ` Peter Zijlstra 0 siblings, 1 reply; 47+ messages in thread From: Greg Kroah-Hartman @ 2023-08-14 15:31 UTC (permalink / raw) To: Dan Williams; +Cc: linux-coco, Andrew Morton, Peter Zijlstra, x86, linux-kernel On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > 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)) No need to check _T before calling this, right (as was also pointed out earlier). thanks, greg k-h ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 15:31 ` Greg Kroah-Hartman @ 2023-08-14 16:17 ` Peter Zijlstra 2023-08-14 18:44 ` Greg Kroah-Hartman 2024-01-04 6:57 ` Lukas Wunner 0 siblings, 2 replies; 47+ messages in thread From: Peter Zijlstra @ 2023-08-14 16:17 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Dan Williams, linux-coco, Andrew Morton, x86, linux-kernel On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote: > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > > 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)) > > No need to check _T before calling this, right (as was also pointed out > earlier). Well, that does mean you get an unconditional call to kvfree() in the success case. Linus argued against this. This way the compiler sees: buf = NULL; if (buf) kvfree(buf); and goes: 'let me clean that up for you'. And all is well. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 16:17 ` Peter Zijlstra @ 2023-08-14 18:44 ` Greg Kroah-Hartman 2023-08-14 18:45 ` Greg Kroah-Hartman 2024-01-04 6:57 ` Lukas Wunner 1 sibling, 1 reply; 47+ messages in thread From: Greg Kroah-Hartman @ 2023-08-14 18:44 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Dan Williams, linux-coco, Andrew Morton, x86, linux-kernel On Mon, Aug 14, 2023 at 06:17:31PM +0200, Peter Zijlstra wrote: > On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > > > 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)) > > > > No need to check _T before calling this, right (as was also pointed out > > earlier). > > Well, that does mean you get an unconditional call to kvfree() in the > success case. Linus argued against this. > > This way the compiler sees: > > buf = NULL; > if (buf) > kvfree(buf); > > and goes: 'let me clean that up for you'. And all is well. Ah, didn't realize that, ok, nevermind :) ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 18:44 ` Greg Kroah-Hartman @ 2023-08-14 18:45 ` Greg Kroah-Hartman 0 siblings, 0 replies; 47+ messages in thread From: Greg Kroah-Hartman @ 2023-08-14 18:45 UTC (permalink / raw) To: Peter Zijlstra; +Cc: Dan Williams, linux-coco, Andrew Morton, x86, linux-kernel On Mon, Aug 14, 2023 at 08:44:43PM +0200, Greg Kroah-Hartman wrote: > On Mon, Aug 14, 2023 at 06:17:31PM +0200, Peter Zijlstra wrote: > > On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > > > > 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)) > > > > > > No need to check _T before calling this, right (as was also pointed out > > > earlier). > > > > Well, that does mean you get an unconditional call to kvfree() in the > > success case. Linus argued against this. > > > > This way the compiler sees: > > > > buf = NULL; > > if (buf) > > kvfree(buf); > > > > and goes: 'let me clean that up for you'. And all is well. > > Ah, didn't realize that, ok, nevermind :) Note, a comment should be added because in a year or so someone is going to come along and try to "clean this up" and we will have forgotten why it's a bad idea to do so. thanks, greg k-h ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2023-08-14 16:17 ` Peter Zijlstra 2023-08-14 18:44 ` Greg Kroah-Hartman @ 2024-01-04 6:57 ` Lukas Wunner 2024-01-04 18:29 ` Dan Williams 1 sibling, 1 reply; 47+ messages in thread From: Lukas Wunner @ 2024-01-04 6:57 UTC (permalink / raw) To: Peter Zijlstra Cc: Greg Kroah-Hartman, Dan Williams, linux-coco, Andrew Morton, x86, linux-kernel On Mon, Aug 14, 2023 at 06:17:31PM +0200, Peter Zijlstra wrote: > On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote: > > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > > > 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)) > > > > No need to check _T before calling this, right (as was also pointed out > > earlier). > > Well, that does mean you get an unconditional call to kvfree() in the > success case. Linus argued against this. > > This way the compiler sees: > > buf = NULL; > if (buf) > kvfree(buf); > > and goes: 'let me clean that up for you'. And all is well. Have you actually verified that assumption in the generated Assembler code? The kernel is compiled with -fno-delete-null-pointer-checks since commit a3ca86aea507 ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS"). So NULL pointer checks are *not* optimized away even if the compiler knows that a pointer is NULL. Background story: https://lwn.net/Articles/342330/ Thanks, Lukas ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree 2024-01-04 6:57 ` Lukas Wunner @ 2024-01-04 18:29 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2024-01-04 18:29 UTC (permalink / raw) To: Lukas Wunner, Peter Zijlstra Cc: Greg Kroah-Hartman, Dan Williams, linux-coco, Andrew Morton, x86, linux-kernel Lukas Wunner wrote: > On Mon, Aug 14, 2023 at 06:17:31PM +0200, Peter Zijlstra wrote: > > On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote: > > > > 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)) > > > > > > No need to check _T before calling this, right (as was also pointed out > > > earlier). > > > > Well, that does mean you get an unconditional call to kvfree() in the > > success case. Linus argued against this. > > > > This way the compiler sees: > > > > buf = NULL; > > if (buf) > > kvfree(buf); > > > > and goes: 'let me clean that up for you'. And all is well. > > Have you actually verified that assumption in the generated Assembler code? > > The kernel is compiled with -fno-delete-null-pointer-checks since commit > a3ca86aea507 ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS"). > > So NULL pointer checks are *not* optimized away even if the compiler > knows that a pointer is NULL. Interesting, I am not sure how -fno-delete-null-pointer-checks plays into this, but I can confirm that Peter's expectations are being met in a routine with: DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T)) ...without that conditional the assembly is: 0xffffffff819ad129 <+41>: call 0xffffffff81800840 <pci_get_domain_bus_and_slot> 0xffffffff819ad12e <+46>: mov %rax,%r12 0xffffffff819ad131 <+49>: test %rax,%rax 0xffffffff819ad134 <+52>: je 0xffffffff819ad154 <cxl_cper_event_call+84> 0xffffffff819ad136 <+54>: mov %rax,%rdi 0xffffffff819ad139 <+57>: call 0xffffffff817f5f10 <pci_dev_lock> 0xffffffff819ad13e <+62>: cmpq $0xffffffff82c681c0,0x80(%r12) 0xffffffff819ad14a <+74>: je 0xffffffff819ad160 <cxl_cper_event_call+96> 0xffffffff819ad14c <+76>: mov %r12,%rdi 0xffffffff819ad14f <+79>: call 0xffffffff817f5fa0 <pci_dev_unlock> 0xffffffff819ad154 <+84>: pop %rbx 0xffffffff819ad155 <+85>: mov %r12,%rdi 0xffffffff819ad158 <+88>: pop %rbp 0xffffffff819ad159 <+89>: pop %r12 0xffffffff819ad15b <+91>: jmp 0xffffffff817fe1e0 <pci_dev_put> ...i.e. the check for NULL at 0xffffffff819ad134 jumps to do an unnecessary pci_dev_put(). With the conditional in the macro the sequence is: 0xffffffff819ad129 <+41>: call 0xffffffff81800840 <pci_get_domain_bus_and_slot> 0xffffffff819ad12e <+46>: test %rax,%rax 0xffffffff819ad131 <+49>: je 0xffffffff819ad18c <cxl_cper_event_call+140> 0xffffffff819ad133 <+51>: mov %rax,%r12 0xffffffff819ad136 <+54>: mov %rax,%rdi 0xffffffff819ad139 <+57>: call 0xffffffff817f5f10 <pci_dev_lock> 0xffffffff819ad13e <+62>: cmpq $0xffffffff82c681c0,0x80(%r12) 0xffffffff819ad14a <+74>: je 0xffffffff819ad160 <cxl_cper_event_call+96> 0xffffffff819ad14c <+76>: mov %r12,%rdi 0xffffffff819ad14f <+79>: call 0xffffffff817f5fa0 <pci_dev_unlock> ... 0xffffffff819ad18c <+140>: pop %rbx 0xffffffff819ad18d <+141>: pop %rbp 0xffffffff819ad18e <+142>: pop %r12 0xffffffff819ad190 <+144>: jmp 0xffffffff81efc6a0 <__x86_return_thunk> ...i.e. optimize away the pci_dev_put() and return directly when @pdev is already known to be NULL. So empirically -fno-delete-null-pointer-checks still allows for redundant NULL checks to be optimized. ^ permalink raw reply [flat|nested] 47+ messages in thread
* [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams ` (3 preceding siblings ...) 2023-08-14 7:43 ` [PATCH v2 4/5] mm/slab: Add __free() support for kvfree Dan Williams @ 2023-08-14 7:43 ` Dan Williams 2023-08-14 8:30 ` Jeremi Piotrowski ` (2 more replies) 2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski 5 siblings, 3 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 7:43 UTC (permalink / raw) To: linux-coco Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel 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 CONFIG_TSM_REPORTS to retrieve the blobs that the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for retrieving the SNP_GET_REPORT blob via the TSM interface utility, assuming no nonce and VMPL==2: echo 2 > /sys/class/tsm/tsm0/privlevel dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex hexdump -C /sys/class/tsm/tsm0/outblob ...while the SNP_GET_EXT_REPORT flow needs to additionally set the format to "extended": echo 2 > /sys/class/tsm/tsm0/privlevel echo extended > /sys/class/tsm/tsm0/format dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex hexdump -C /sys/class/tsm/tsm0/outblob 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 | 1 drivers/virt/coco/sev-guest/sev-guest.c | 81 +++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig index da2d7ca531f0..1cffc72c41cb 100644 --- a/drivers/virt/coco/sev-guest/Kconfig +++ b/drivers/virt/coco/sev-guest/Kconfig @@ -5,6 +5,7 @@ config SEV_GUEST select CRYPTO select CRYPTO_AEAD2 select CRYPTO_GCM + select TSM_REPORTS 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..5941081502e8 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -16,6 +16,7 @@ #include <linux/miscdevice.h> #include <linux/set_memory.h> #include <linux/fs.h> +#include <linux/tsm.h> #include <crypto/aead.h> #include <linux/scatterlist.h> #include <linux/psp-sev.h> @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno return key; } +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, + size_t *outblob_len) +{ + struct snp_guest_dev *snp_dev = dev_get_drvdata(dev); + const int report_size = SZ_16K; + const int ext_size = SZ_16K; + int ret, size; + + if (desc->inblob_len != 64) + return ERR_PTR(-EINVAL); + + if (desc->outblob_format == TSM_FORMAT_EXTENDED) + size = report_size + ext_size; + else + size = report_size; + + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); + + if (desc->outblob_format == TSM_FORMAT_EXTENDED) { + struct snp_ext_report_req ext_req = { + .data = { .vmpl = desc->privlevel }, + .certs_address = (__u64)buf + report_size, + .certs_len = ext_size, + }; + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len); + + struct snp_guest_request_ioctl input = { + .msg_version = 1, + .req_data = (__u64)&ext_req, + .resp_data = (__u64)buf, + }; + + ret = get_ext_report(snp_dev, &input, SNP_KARG); + } else { + struct snp_report_req req = { + .vmpl = desc->privlevel, + }; + memcpy(&req.user_data, desc->inblob, desc->inblob_len); + + 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 ERR_PTR(ret); + + *outblob_len = size; + no_free_ptr(buf); + return buf; +} + +static const struct tsm_ops sev_tsm_ops = { + .name = KBUILD_MODNAME, + .report_new = sev_report_new, +}; + +static const struct attribute_group *sev_tsm_attribute_groups[] = { + &tsm_default_attribute_group, + &tsm_extra_attribute_group, + NULL, +}; + +static void unregister_sev_tsm(void *data) +{ + unregister_tsm(&sev_tsm_ops); +} + static int __init sev_guest_probe(struct platform_device *pdev) { struct snp_secrets_page_layout *layout; @@ -842,6 +915,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(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups); + 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; ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams @ 2023-08-14 8:30 ` Jeremi Piotrowski 2023-08-14 16:22 ` Dan Williams 2023-08-14 11:21 ` Peter Zijlstra 2023-08-15 20:50 ` Tom Lendacky 2 siblings, 1 reply; 47+ messages in thread From: Jeremi Piotrowski @ 2023-08-14 8:30 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel On 8/14/2023 9:43 AM, Dan Williams 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 CONFIG_TSM_REPORTS to retrieve the blobs that > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for > retrieving the SNP_GET_REPORT blob via the TSM interface utility, > assuming no nonce and VMPL==2: > > echo 2 > /sys/class/tsm/tsm0/privlevel > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > hexdump -C /sys/class/tsm/tsm0/outblob > > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the > format to "extended": > > echo 2 > /sys/class/tsm/tsm0/privlevel > echo extended > /sys/class/tsm/tsm0/format > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > hexdump -C /sys/class/tsm/tsm0/outblob > > 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 | 1 > drivers/virt/coco/sev-guest/sev-guest.c | 81 +++++++++++++++++++++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig > index da2d7ca531f0..1cffc72c41cb 100644 > --- a/drivers/virt/coco/sev-guest/Kconfig > +++ b/drivers/virt/coco/sev-guest/Kconfig > @@ -5,6 +5,7 @@ config SEV_GUEST > select CRYPTO > select CRYPTO_AEAD2 > select CRYPTO_GCM > + select TSM_REPORTS > 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..5941081502e8 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -16,6 +16,7 @@ > #include <linux/miscdevice.h> > #include <linux/set_memory.h> > #include <linux/fs.h> > +#include <linux/tsm.h> > #include <crypto/aead.h> > #include <linux/scatterlist.h> > #include <linux/psp-sev.h> > @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno > return key; > } > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > + size_t *outblob_len) get_report and get_ext_report both have a: lockdep_assert_held(&snp_cmd_mutex); so you'd need to take that lock somewhere here. Jeremi > +{ > + struct snp_guest_dev *snp_dev = dev_get_drvdata(dev); > + const int report_size = SZ_16K; > + const int ext_size = SZ_16K; > + int ret, size; > + > + if (desc->inblob_len != 64) > + return ERR_PTR(-EINVAL); > + > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) > + size = report_size + ext_size; > + else > + size = report_size; > + > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > + > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) { > + struct snp_ext_report_req ext_req = { > + .data = { .vmpl = desc->privlevel }, > + .certs_address = (__u64)buf + report_size, > + .certs_len = ext_size, > + }; > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len); > + > + struct snp_guest_request_ioctl input = { > + .msg_version = 1, > + .req_data = (__u64)&ext_req, > + .resp_data = (__u64)buf, > + }; > + > + ret = get_ext_report(snp_dev, &input, SNP_KARG); > + } else { > + struct snp_report_req req = { > + .vmpl = desc->privlevel, > + }; > + memcpy(&req.user_data, desc->inblob, desc->inblob_len); > + > + 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 ERR_PTR(ret); > + > + *outblob_len = size; > + no_free_ptr(buf); > + return buf; > +} > + > +static const struct tsm_ops sev_tsm_ops = { > + .name = KBUILD_MODNAME, > + .report_new = sev_report_new, > +}; > + > +static const struct attribute_group *sev_tsm_attribute_groups[] = { > + &tsm_default_attribute_group, > + &tsm_extra_attribute_group, > + NULL, > +}; > + > +static void unregister_sev_tsm(void *data) > +{ > + unregister_tsm(&sev_tsm_ops); > +} > + > static int __init sev_guest_probe(struct platform_device *pdev) > { > struct snp_secrets_page_layout *layout; > @@ -842,6 +915,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(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups); > + 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; > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 8:30 ` Jeremi Piotrowski @ 2023-08-14 16:22 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 16:22 UTC (permalink / raw) To: Jeremi Piotrowski, Dan Williams, linux-coco Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel Jeremi Piotrowski wrote: > On 8/14/2023 9:43 AM, Dan Williams 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 CONFIG_TSM_REPORTS to retrieve the blobs that > > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for > > retrieving the SNP_GET_REPORT blob via the TSM interface utility, > > assuming no nonce and VMPL==2: > > > > echo 2 > /sys/class/tsm/tsm0/privlevel > > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > > hexdump -C /sys/class/tsm/tsm0/outblob > > > > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the > > format to "extended": > > > > echo 2 > /sys/class/tsm/tsm0/privlevel > > echo extended > /sys/class/tsm/tsm0/format > > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > > hexdump -C /sys/class/tsm/tsm0/outblob > > > > 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 | 1 > > drivers/virt/coco/sev-guest/sev-guest.c | 81 +++++++++++++++++++++++++++++++ > > 2 files changed, 82 insertions(+) > > > > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig > > index da2d7ca531f0..1cffc72c41cb 100644 > > --- a/drivers/virt/coco/sev-guest/Kconfig > > +++ b/drivers/virt/coco/sev-guest/Kconfig > > @@ -5,6 +5,7 @@ config SEV_GUEST > > select CRYPTO > > select CRYPTO_AEAD2 > > select CRYPTO_GCM > > + select TSM_REPORTS > > 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..5941081502e8 100644 > > --- a/drivers/virt/coco/sev-guest/sev-guest.c > > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > > @@ -16,6 +16,7 @@ > > #include <linux/miscdevice.h> > > #include <linux/set_memory.h> > > #include <linux/fs.h> > > +#include <linux/tsm.h> > > #include <crypto/aead.h> > > #include <linux/scatterlist.h> > > #include <linux/psp-sev.h> > > @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno > > return key; > > } > > > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > > + size_t *outblob_len) > > get_report and get_ext_report both have a: > > lockdep_assert_held(&snp_cmd_mutex); > > so you'd need to take that lock somewhere here. Got it, and in general this needs testing from someone with actual hardware. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams 2023-08-14 8:30 ` Jeremi Piotrowski @ 2023-08-14 11:21 ` Peter Zijlstra 2023-08-14 16:25 ` Dan Williams 2023-08-15 20:50 ` Tom Lendacky 2 siblings, 1 reply; 47+ messages in thread From: Peter Zijlstra @ 2023-08-14 11:21 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, x86, linux-kernel On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote: > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > + size_t *outblob_len) > +{ > + > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > + > + > + *outblob_len = size; > + no_free_ptr(buf); > + return buf; This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no atomics). So the above would end up being: return NULL; What you want to write is somehting like: return no_free_ptr(buf); or, a convenient shorthand: return_ptr(buf); ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 11:21 ` Peter Zijlstra @ 2023-08-14 16:25 ` Dan Williams 2023-08-14 16:48 ` Peter Zijlstra 0 siblings, 1 reply; 47+ messages in thread From: Dan Williams @ 2023-08-14 16:25 UTC (permalink / raw) To: Peter Zijlstra, Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, x86, linux-kernel Peter Zijlstra wrote: > On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote: > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > > > + size_t *outblob_len) > > +{ > > > + > > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > > + > > > + > > + *outblob_len = size; > > + no_free_ptr(buf); > > + return buf; > > This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no > atomics). So the above would end up being: > > return NULL; > > What you want to write is somehting like: > > return no_free_ptr(buf); > > or, a convenient shorthand: > > return_ptr(buf); > Oh, I indeed did not realize that no_free_ptr() had side effects beyond canceling the free when the variable goes out of scope. Will switch to return_ptr(). ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 16:25 ` Dan Williams @ 2023-08-14 16:48 ` Peter Zijlstra 2023-08-14 22:15 ` Peter Zijlstra 0 siblings, 1 reply; 47+ messages in thread From: Peter Zijlstra @ 2023-08-14 16:48 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, x86, linux-kernel On Mon, Aug 14, 2023 at 09:25:06AM -0700, Dan Williams wrote: > Peter Zijlstra wrote: > > On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote: > > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > > > > > + size_t *outblob_len) > > > +{ > > > > > + > > > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > > > + > > > > > + > > > + *outblob_len = size; > > > + no_free_ptr(buf); > > > + return buf; > > > > This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no > > atomics). So the above would end up being: > > > > return NULL; > > > > What you want to write is somehting like: > > > > return no_free_ptr(buf); > > > > or, a convenient shorthand: > > > > return_ptr(buf); > > > > Oh, I indeed did not realize that no_free_ptr() had side effects beyond > canceling the free when the variable goes out of scope. Will switch to > return_ptr(). Indeed -- ideally no_free_ptr() would be combined with __must_check, but I'm not immediately sure how to pull that off. Let me stick that on the to-do list. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 16:48 ` Peter Zijlstra @ 2023-08-14 22:15 ` Peter Zijlstra 2023-08-15 8:37 ` Peter Zijlstra 0 siblings, 1 reply; 47+ messages in thread From: Peter Zijlstra @ 2023-08-14 22:15 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, x86, linux-kernel, Linus Torvalds, Nick Desaulniers On Mon, Aug 14, 2023 at 06:48:04PM +0200, Peter Zijlstra wrote: > On Mon, Aug 14, 2023 at 09:25:06AM -0700, Dan Williams wrote: > > Peter Zijlstra wrote: > > > On Mon, Aug 14, 2023 at 12:43:38AM -0700, Dan Williams wrote: > > > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > > > > > > > + size_t *outblob_len) > > > > +{ > > > > > > > + > > > > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > > > > + > > > > > > > + > > > > + *outblob_len = size; > > > > + no_free_ptr(buf); > > > > + return buf; > > > > > > This seems broken, no_free_ptr(x) is basically xchg(X, NULL) (except no > > > atomics). So the above would end up being: > > > > > > return NULL; > > > > > > What you want to write is somehting like: > > > > > > return no_free_ptr(buf); > > > > > > or, a convenient shorthand: > > > > > > return_ptr(buf); > > > > > > > Oh, I indeed did not realize that no_free_ptr() had side effects beyond > > canceling the free when the variable goes out of scope. Will switch to > > return_ptr(). > > Indeed -- ideally no_free_ptr() would be combined with __must_check, but > I'm not immediately sure how to pull that off. Let me stick that on the > to-do list. This seems to actually work. I'll try and write some coherent comments tomorrow -- it's definitely too late for that now. diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h index 53f1a7a932b0..162052cef4c7 100644 --- a/include/linux/cleanup.h +++ b/include/linux/cleanup.h @@ -39,8 +39,12 @@ #define __free(_name) __cleanup(__free_##_name) +static inline __must_check void * __no_free_ptr(void **pp) +{ void *p = *pp; *pp = NULL; return p; } + #define no_free_ptr(p) \ - ({ __auto_type __ptr = (p); (p) = NULL; __ptr; }) + ((void)__builtin_types_compatible_p(void *, typeof(p)), \ + ((typeof(p))__no_free_ptr((void **)&(p)))) #define return_ptr(p) return no_free_ptr(p) ^ permalink raw reply related [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 22:15 ` Peter Zijlstra @ 2023-08-15 8:37 ` Peter Zijlstra 0 siblings, 0 replies; 47+ messages in thread From: Peter Zijlstra @ 2023-08-15 8:37 UTC (permalink / raw) To: Dan Williams Cc: linux-coco, Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh, x86, linux-kernel, Linus Torvalds, Nick Desaulniers On Tue, Aug 15, 2023 at 12:15:03AM +0200, Peter Zijlstra wrote: > This seems to actually work. I'll try and write some coherent comments > tomorrow -- it's definitely too late for that now. Clearly I should have gone to bed before sending this patch, not after. Let me try that again. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams 2023-08-14 8:30 ` Jeremi Piotrowski 2023-08-14 11:21 ` Peter Zijlstra @ 2023-08-15 20:50 ` Tom Lendacky 2023-08-15 21:40 ` Dan Williams 2 siblings, 1 reply; 47+ messages in thread From: Tom Lendacky @ 2023-08-15 20:50 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel On 8/14/23 02:43, Dan Williams 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 CONFIG_TSM_REPORTS to retrieve the blobs that > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for > retrieving the SNP_GET_REPORT blob via the TSM interface utility, > assuming no nonce and VMPL==2: > > echo 2 > /sys/class/tsm/tsm0/privlevel > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > hexdump -C /sys/class/tsm/tsm0/outblob > > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the > format to "extended": > > echo 2 > /sys/class/tsm/tsm0/privlevel > echo extended > /sys/class/tsm/tsm0/format > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > hexdump -C /sys/class/tsm/tsm0/outblob > > 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. I just got back from vacation, so I'll apply and test as soon as I get a chance. > > 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 | 1 > drivers/virt/coco/sev-guest/sev-guest.c | 81 +++++++++++++++++++++++++++++++ > 2 files changed, 82 insertions(+) > > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig > index da2d7ca531f0..1cffc72c41cb 100644 > --- a/drivers/virt/coco/sev-guest/Kconfig > +++ b/drivers/virt/coco/sev-guest/Kconfig > @@ -5,6 +5,7 @@ config SEV_GUEST > select CRYPTO > select CRYPTO_AEAD2 > select CRYPTO_GCM > + select TSM_REPORTS > 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..5941081502e8 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -16,6 +16,7 @@ > #include <linux/miscdevice.h> > #include <linux/set_memory.h> > #include <linux/fs.h> > +#include <linux/tsm.h> > #include <crypto/aead.h> > #include <linux/scatterlist.h> > #include <linux/psp-sev.h> > @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno > return key; > } > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > + size_t *outblob_len) > +{ > + struct snp_guest_dev *snp_dev = dev_get_drvdata(dev); > + const int report_size = SZ_16K; The response buffer from the PSP is limited to 4K, so the report size can be SZ_4K. > + const int ext_size = SZ_16K; > + int ret, size; > + > + if (desc->inblob_len != 64) > + return ERR_PTR(-EINVAL); > + > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) > + size = report_size + ext_size; > + else > + size = report_size; > + > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > + > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) { > + struct snp_ext_report_req ext_req = { > + .data = { .vmpl = desc->privlevel }, > + .certs_address = (__u64)buf + report_size, > + .certs_len = ext_size, > + }; > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len); > + > + struct snp_guest_request_ioctl input = { > + .msg_version = 1, > + .req_data = (__u64)&ext_req, > + .resp_data = (__u64)buf, > + }; Won't the compiler complain about this declaration being after the memcpy()? > + > + ret = get_ext_report(snp_dev, &input, SNP_KARG); > + } else { > + struct snp_report_req req = { > + .vmpl = desc->privlevel, > + }; > + memcpy(&req.user_data, desc->inblob, desc->inblob_len); > + > + struct snp_guest_request_ioctl input = { > + .msg_version = 1, > + .req_data = (__u64) &req, > + .resp_data = (__u64) buf, > + }; Ditto here. Thanks, Tom > + > + ret = get_report(snp_dev, &input, SNP_KARG); > + } > + > + if (ret) > + return ERR_PTR(ret); > + > + *outblob_len = size; > + no_free_ptr(buf); > + return buf; > +} > + > +static const struct tsm_ops sev_tsm_ops = { > + .name = KBUILD_MODNAME, > + .report_new = sev_report_new, > +}; > + > +static const struct attribute_group *sev_tsm_attribute_groups[] = { > + &tsm_default_attribute_group, > + &tsm_extra_attribute_group, > + NULL, > +}; > + > +static void unregister_sev_tsm(void *data) > +{ > + unregister_tsm(&sev_tsm_ops); > +} > + > static int __init sev_guest_probe(struct platform_device *pdev) > { > struct snp_secrets_page_layout *layout; > @@ -842,6 +915,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(&sev_tsm_ops, &pdev->dev, sev_tsm_attribute_groups); > + 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; > ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT 2023-08-15 20:50 ` Tom Lendacky @ 2023-08-15 21:40 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-15 21:40 UTC (permalink / raw) To: Tom Lendacky, Dan Williams, linux-coco Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, peterz, x86, linux-kernel Tom Lendacky wrote: > On 8/14/23 02:43, Dan Williams 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 CONFIG_TSM_REPORTS to retrieve the blobs that > > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow for > > retrieving the SNP_GET_REPORT blob via the TSM interface utility, > > assuming no nonce and VMPL==2: > > > > echo 2 > /sys/class/tsm/tsm0/privlevel > > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > > hexdump -C /sys/class/tsm/tsm0/outblob > > > > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the > > format to "extended": > > > > echo 2 > /sys/class/tsm/tsm0/privlevel > > echo extended > /sys/class/tsm/tsm0/format > > dd if=/dev/urandom bs=64 count=1 | xxd -p -c 0 > /sys/class/tsm/tsm0/inhex > > hexdump -C /sys/class/tsm/tsm0/outblob > > > > 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. > > I just got back from vacation, so I'll apply and test as soon as I get a Appreciate it! Hold off on testing until v3 though since Peter highlighted I am misusing no_free_ptr(), Jeremi pointed out that sev-guest locking is being violated, and configfs may need to be deployed for this to future proof the ABI for future use cases. > chance. > > > > > 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 | 1 > > drivers/virt/coco/sev-guest/sev-guest.c | 81 +++++++++++++++++++++++++++++++ > > 2 files changed, 82 insertions(+) > > > > diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig > > index da2d7ca531f0..1cffc72c41cb 100644 > > --- a/drivers/virt/coco/sev-guest/Kconfig > > +++ b/drivers/virt/coco/sev-guest/Kconfig > > @@ -5,6 +5,7 @@ config SEV_GUEST > > select CRYPTO > > select CRYPTO_AEAD2 > > select CRYPTO_GCM > > + select TSM_REPORTS > > 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..5941081502e8 100644 > > --- a/drivers/virt/coco/sev-guest/sev-guest.c > > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > > @@ -16,6 +16,7 @@ > > #include <linux/miscdevice.h> > > #include <linux/set_memory.h> > > #include <linux/fs.h> > > +#include <linux/tsm.h> > > #include <crypto/aead.h> > > #include <linux/scatterlist.h> > > #include <linux/psp-sev.h> > > @@ -769,6 +770,78 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno > > return key; > > } > > > > +static u8 *sev_report_new(struct device *dev, const struct tsm_desc *desc, > > + size_t *outblob_len) > > +{ > > + struct snp_guest_dev *snp_dev = dev_get_drvdata(dev); > > + const int report_size = SZ_16K; > > The response buffer from the PSP is limited to 4K, so the report size can > be SZ_4K. Oh, ok, what about the extended case? > > > + const int ext_size = SZ_16K; > > + int ret, size; > > + > > + if (desc->inblob_len != 64) > > + return ERR_PTR(-EINVAL); > > + > > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) > > + size = report_size + ext_size; > > + else > > + size = report_size; > > + > > + u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL); > > + > > + if (desc->outblob_format == TSM_FORMAT_EXTENDED) { > > + struct snp_ext_report_req ext_req = { > > + .data = { .vmpl = desc->privlevel }, > > + .certs_address = (__u64)buf + report_size, > > + .certs_len = ext_size, > > + }; > > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len); > > + > > + struct snp_guest_request_ioctl input = { > > + .msg_version = 1, > > + .req_data = (__u64)&ext_req, > > + .resp_data = (__u64)buf, > > + }; > > Won't the compiler complain about this declaration being after the memcpy()? The memcpy is into @ext_req, @input is just referencing @ext_req. > > > + > > + ret = get_ext_report(snp_dev, &input, SNP_KARG); > > + } else { > > + struct snp_report_req req = { > > + .vmpl = desc->privlevel, > > + }; > > + memcpy(&req.user_data, desc->inblob, desc->inblob_len); > > + > > + struct snp_guest_request_ioctl input = { > > + .msg_version = 1, > > + .req_data = (__u64) &req, > > + .resp_data = (__u64) buf, > > + }; > > Ditto here. > I think its ok, but let me know if you think I am missing something. Thanks for taking a look! ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams ` (4 preceding siblings ...) 2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams @ 2023-08-14 9:04 ` Jeremi Piotrowski 2023-08-14 17:12 ` Dan Williams 2023-08-23 11:21 ` Dr. Greg 5 siblings, 2 replies; 47+ messages in thread From: Jeremi Piotrowski @ 2023-08-14 9:04 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Peter Gonda, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel On 8/14/2023 9:43 AM, Dan Williams wrote: > Changes since v1: > - Switch from Keyring to sysfs (James) > > An attestation report is signed evidence of how a Trusted Virtual > Machine (TVM) was launched and its current state. A verifying party uses > the report to make judgements of the confidentiality and integrity of > that execution environment. Upon successful attestation the verifying > party may, for example, proceed to deploy secrets to the TVM to carry > out a workload. Multiple confidential computing platforms share this > similar flow. > > The approach of adding adding new char devs and new ioctls, for what > amounts to the same logical functionality with minor formatting > differences across vendors [1], is untenable. Common concepts and the > community benefit from common infrastructure. > > Use sysfs for this facility for maintainability compared to ioctl(). The > expectation is that this interface is a boot time, configure once, get > report, and done flow. I.e. not something that receives ongoing > transactions at runtime. However, runtime retrieval is not precluded and > a mechanism to detect potential configuration conflicts from > multiple-threads using this interface is included. > I wanted to speak up to say that this does not align with the needs we have in the Confidential Containers project. We want to be able to perform attestation not just once during boot but during the lifecycle of the confidential VM. We may need to fetch a fresh attestation report from a trusted agent but also from arbitrary applications running in containers. The trusted agent might need attestation when launching a new container from an encrypted container image or when a new secret is being added to the VM - both of these events may happen at any time (also when containerized applications are already executing). Container applications have their own uses for attestation, such as when they need to fetch keys required to decrypt payloads. We also have things like performing attestation when establishing a TLS or ssh connection to provide an attested e2e encrypted communication channel. I don't think sysfs is suitable for such concurrent transactions. Also if you think about exposing the sysfs interface to an application in a container, this requires bind mounting rw part of the sysfs tree into the mount namespace - not ideal. Jeremi > The keyring@ list is dropped on this posting since a new key-type is no > longer being pursued. > > Link: http://lore.kernel.org/r/cover.1684048511.git.sathyanarayanan.kuppuswamy@linux.intel.com > > --- > > Dan Williams (5): > virt: coco: Add a coco/Makefile and coco/Kconfig > tsm: Introduce a shared ABI for attestation reports > virt: sevguest: Prep for kernel internal {get,get_ext}_report() > mm/slab: Add __free() support for kvfree > virt: sevguest: Add TSM_REPORTS support for SNP_{GET,GET_EXT}_REPORT > > > Documentation/ABI/testing/sysfs-class-tsm | 47 +++++ > MAINTAINERS | 8 + > drivers/virt/Kconfig | 6 - > drivers/virt/Makefile | 4 > drivers/virt/coco/Kconfig | 13 + > drivers/virt/coco/Makefile | 8 + > drivers/virt/coco/sev-guest/Kconfig | 1 > drivers/virt/coco/sev-guest/sev-guest.c | 129 ++++++++++++- > drivers/virt/coco/tdx-guest/Kconfig | 1 > drivers/virt/coco/tsm.c | 290 +++++++++++++++++++++++++++++ > include/linux/slab.h | 2 > include/linux/tsm.h | 45 +++++ > 12 files changed, 535 insertions(+), 19 deletions(-) > create mode 100644 Documentation/ABI/testing/sysfs-class-tsm > create mode 100644 drivers/virt/coco/Kconfig > create mode 100644 drivers/virt/coco/Makefile > create mode 100644 drivers/virt/coco/tsm.c > create mode 100644 include/linux/tsm.h > > base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5 ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski @ 2023-08-14 17:12 ` Dan Williams 2023-08-15 14:27 ` Peter Gonda 2023-08-16 9:42 ` Jeremi Piotrowski 2023-08-23 11:21 ` Dr. Greg 1 sibling, 2 replies; 47+ messages in thread From: Dan Williams @ 2023-08-14 17:12 UTC (permalink / raw) To: Jeremi Piotrowski, Dan Williams, linux-coco Cc: Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Peter Gonda, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel Jeremi Piotrowski wrote: > On 8/14/2023 9:43 AM, Dan Williams wrote: > > Changes since v1: > > - Switch from Keyring to sysfs (James) > > > > An attestation report is signed evidence of how a Trusted Virtual > > Machine (TVM) was launched and its current state. A verifying party uses > > the report to make judgements of the confidentiality and integrity of > > that execution environment. Upon successful attestation the verifying > > party may, for example, proceed to deploy secrets to the TVM to carry > > out a workload. Multiple confidential computing platforms share this > > similar flow. > > > > The approach of adding adding new char devs and new ioctls, for what > > amounts to the same logical functionality with minor formatting > > differences across vendors [1], is untenable. Common concepts and the > > community benefit from common infrastructure. > > > > Use sysfs for this facility for maintainability compared to ioctl(). The > > expectation is that this interface is a boot time, configure once, get > > report, and done flow. I.e. not something that receives ongoing > > transactions at runtime. However, runtime retrieval is not precluded and > > a mechanism to detect potential configuration conflicts from > > multiple-threads using this interface is included. > > > > I wanted to speak up to say that this does not align with the needs we have > in the Confidential Containers project. We want to be able to perform attestation > not just once during boot but during the lifecycle of the confidential VM. We > may need to fetch a fresh attestation report from a trusted agent but also from > arbitrary applications running in containers. > > The trusted agent might need attestation when launching a new container from an > encrypted container image or when a new secret is being added to the VM - both > of these events may happen at any time (also when containerized applications > are already executing). > > Container applications have their own uses for attestation, such as when they need > to fetch keys required to decrypt payloads. We also have things like performing > attestation when establishing a TLS or ssh connection to provide an attested e2e > encrypted communication channel. ...and you expect that the boot time attestation becomes invalidated later at run time such that ongoing round trips to the TSM are needed? I am looking at "Table 21. ATTESTATION_REPORT Structure" for example and not seeing data there that changes from one request to the next. Runtime validation likely looks more like the vTPM use case with PCRs. That will leverage the existing / common TPM ABI. > I don't think sysfs is suitable for such concurrent transactions. Also if you think > about exposing the sysfs interface to an application in a container, this requires > bind mounting rw part of the sysfs tree into the mount namespace - not ideal. sysfs is not suitable for concurrent transactions. The container would need to have an alternate path to request that the singleton owner of the interface generate new reports, or use the boot time attestation to derive per container communication sessions to the attestation agent. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-14 17:12 ` Dan Williams @ 2023-08-15 14:27 ` Peter Gonda 2023-08-15 17:16 ` Dionna Amalie Glaze 2023-08-15 18:13 ` Dan Williams 2023-08-16 9:42 ` Jeremi Piotrowski 1 sibling, 2 replies; 47+ messages in thread From: Peter Gonda @ 2023-08-15 14:27 UTC (permalink / raw) To: Dan Williams Cc: Jeremi Piotrowski, linux-coco, Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel On Mon, Aug 14, 2023 at 11:12 AM Dan Williams <dan.j.williams@intel.com> wrote: > > Jeremi Piotrowski wrote: > > On 8/14/2023 9:43 AM, Dan Williams wrote: > > > Changes since v1: > > > - Switch from Keyring to sysfs (James) > > > > > > An attestation report is signed evidence of how a Trusted Virtual > > > Machine (TVM) was launched and its current state. A verifying party uses > > > the report to make judgements of the confidentiality and integrity of > > > that execution environment. Upon successful attestation the verifying > > > party may, for example, proceed to deploy secrets to the TVM to carry > > > out a workload. Multiple confidential computing platforms share this > > > similar flow. > > > > > > The approach of adding adding new char devs and new ioctls, for what > > > amounts to the same logical functionality with minor formatting > > > differences across vendors [1], is untenable. Common concepts and the > > > community benefit from common infrastructure. > > > > > > Use sysfs for this facility for maintainability compared to ioctl(). The > > > expectation is that this interface is a boot time, configure once, get > > > report, and done flow. I.e. not something that receives ongoing > > > transactions at runtime. However, runtime retrieval is not precluded and > > > a mechanism to detect potential configuration conflicts from > > > multiple-threads using this interface is included. > > > > > > > I wanted to speak up to say that this does not align with the needs we have > > in the Confidential Containers project. We want to be able to perform attestation > > not just once during boot but during the lifecycle of the confidential VM. We > > may need to fetch a fresh attestation report from a trusted agent but also from > > arbitrary applications running in containers. > > > > The trusted agent might need attestation when launching a new container from an > > encrypted container image or when a new secret is being added to the VM - both > > of these events may happen at any time (also when containerized applications > > are already executing). > > > > Container applications have their own uses for attestation, such as when they need > > to fetch keys required to decrypt payloads. We also have things like performing > > attestation when establishing a TLS or ssh connection to provide an attested e2e > > encrypted communication channel. > > ...and you expect that the boot time attestation becomes invalidated > later at run time such that ongoing round trips to the TSM are needed? I > am looking at "Table 21. ATTESTATION_REPORT Structure" for example and > not seeing data there that changes from one request to the next. Runtime > validation likely looks more like the vTPM use case with PCRs. That will > leverage the existing / common TPM ABI. I thought we were dropping the TSM acronym as requested by Jarkko? Why do we need to be so prescriptive about "boot time" vs "runtime" attestations? A user may wish to attest to several requests as Jeremi notes. And why should users be forced into using a vTPM interface if their usecase doesn't require all the features and complexity of a vTPM? Some users may prefer less overall code within their Trusted Computer Base (TCB) and a TPM emulate is a significant code base. It seems like you are just reading the SNP spec, if you read the TDX spec you'll see there are RTMRs which can be extended with new data. This leads to a different data in the attestation. Similar there are REMs in the ARM CCA spec. > > > I don't think sysfs is suitable for such concurrent transactions. Also if you think > > about exposing the sysfs interface to an application in a container, this requires > > bind mounting rw part of the sysfs tree into the mount namespace - not ideal. > > sysfs is not suitable for concurrent transactions. The container would > need to have an alternate path to request that the singleton owner of > the interface generate new reports, or use the boot time attestation to > derive per container communication sessions to the attestation agent. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-15 14:27 ` Peter Gonda @ 2023-08-15 17:16 ` Dionna Amalie Glaze 2023-08-15 21:13 ` Dan Williams 2023-08-15 18:13 ` Dan Williams 1 sibling, 1 reply; 47+ messages in thread From: Dionna Amalie Glaze @ 2023-08-15 17:16 UTC (permalink / raw) To: Peter Gonda Cc: Dan Williams, Jeremi Piotrowski, linux-coco, Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Borislav Petkov, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel > > Why do we need to be so prescriptive about "boot time" vs "runtime" > attestations? A user may wish to attest to several requests as Jeremi > notes. And why should users be forced into using a vTPM interface if > their usecase doesn't require all the features and complexity of a > vTPM? Some users may prefer less overall code within their Trusted > Computer Base (TCB) and a TPM emulate is a significant code base. > I agree, and I was a bit too hasty to acquiesce to sysfs due to the TPM argument that really only applies for SEV-SNP without a whole lot of extra work for other backends (not to say SVSM isn't itself a whole lot of extra work). > It seems like you are just reading the SNP spec, if you read the TDX > spec you'll see there are RTMRs which can be extended with new data. > This leads to a different data in the attestation. Similar there are > REMs in the ARM CCA spec. > I'll add a note here that measurement registers are extensible at any point by ring0, and there should be an API for doing so, the way that there is for /dev/tpmX. It could be /dev/teemr or something to unify TDX, COVE, ARM CCA, and potentially a measurement register protocol extension to SEV-SNP's SVSM. I'm not sure how Intel is going to propose abstracting TCG Canonical Event Log measurements to reuse measurement-to-PCR<X> code points in the kernel as measurement-to-MR<f(X)>, or whatnot, but each technology should have that implementation option to extend their own measurement registers (and event log, potentially). I (and probably James) object with just saying the PCRs are going to xyz-measurement-register for simulating that integrity part of a TPM to get just the quote aspect and not the rest of TPM 2.0 to hide everything behind the TPM abstraction. It doesn't follow the Tcg spec. But I repeat myself. If we use any ioctl, we'll end up multiplexing the input per-technology, and at that point we essentially have manufacturer-specific devices much to Dan's dismay. Sysfs will certainly not be okay for measurement register-only technology, since there's no way to not use a hardware attestation to securely track measurement changes past "the static boot" (PCRs 0-7). I don't want to have to rely on enclave-like peer VMs that perform the TPM behavior. -- -Dionna Glaze, PhD (she/her) ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-15 17:16 ` Dionna Amalie Glaze @ 2023-08-15 21:13 ` Dan Williams 0 siblings, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-15 21:13 UTC (permalink / raw) To: Dionna Amalie Glaze, Peter Gonda Cc: Dan Williams, Jeremi Piotrowski, linux-coco, Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Borislav Petkov, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel Dionna Amalie Glaze wrote: > > > > Why do we need to be so prescriptive about "boot time" vs "runtime" > > attestations? A user may wish to attest to several requests as Jeremi > > notes. And why should users be forced into using a vTPM interface if > > their usecase doesn't require all the features and complexity of a > > vTPM? Some users may prefer less overall code within their Trusted > > Computer Base (TCB) and a TPM emulate is a significant code base. > > > > I agree, and I was a bit too hasty to acquiesce to sysfs due to the > TPM argument that really only applies for SEV-SNP without a whole lot > of extra work for other backends (not to say SVSM isn't itself a whole > lot of extra work). > > > It seems like you are just reading the SNP spec, if you read the TDX > > spec you'll see there are RTMRs which can be extended with new data. > > This leads to a different data in the attestation. Similar there are > > REMs in the ARM CCA spec. > > > > I'll add a note here that measurement registers are extensible at any > point by ring0, and there should be an API for doing so, the way that > there is for /dev/tpmX. > > It could be /dev/teemr or something to unify TDX, COVE, ARM CCA, and > potentially a measurement register protocol extension to SEV-SNP's > SVSM. > I'm not sure how Intel is going to propose abstracting TCG Canonical > Event Log measurements to reuse measurement-to-PCR<X> code points in > the kernel as measurement-to-MR<f(X)>, or whatnot, but each technology > should have that implementation option to extend their own measurement > registers (and event log, potentially). > > I (and probably James) object with just saying the PCRs are going to > xyz-measurement-register for simulating that integrity part of a TPM > to get just the quote aspect and not the rest of TPM 2.0 to hide > everything behind the TPM abstraction. It doesn't follow the Tcg spec. > > But I repeat myself. If we use any ioctl, we'll end up multiplexing > the input per-technology, and at that point we essentially have > manufacturer-specific devices much to Dan's dismay. I think the ioctl() question is separate from the question of how to scale an attestation ABI. While I am not yet convinced that RTMR deserves an ABI separate from vTPM (or at least a trusted-keys abstraction to reuse the kernel's existing PCR ABI for TPM-like facilities) the selection of sysfs precludes the "RTMR and repeated measurements by multiple containers" use case. One way to buy time for that future conversation while moving ahead on this base reporting ABI is to switch from sysfs to configfs. That would allow for per container submission ABI. It could be approximated with sysfs by having an instance creation attribute, but it is more natural to just use mkdir to create more interfaces that can be bind mounted by per-container if need be. > Sysfs will certainly not be okay for measurement register-only > technology, since there's no way to not use a hardware attestation to > securely track measurement changes past "the static boot" (PCRs 0-7). > I don't want to have to rely on enclave-like peer VMs that perform the > TPM behavior. Understood. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-15 14:27 ` Peter Gonda 2023-08-15 17:16 ` Dionna Amalie Glaze @ 2023-08-15 18:13 ` Dan Williams 1 sibling, 0 replies; 47+ messages in thread From: Dan Williams @ 2023-08-15 18:13 UTC (permalink / raw) To: Peter Gonda, Dan Williams Cc: Jeremi Piotrowski, linux-coco, Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel Peter Gonda wrote: > On Mon, Aug 14, 2023 at 11:12 AM Dan Williams <dan.j.williams@intel.com> wrote: > > > > Jeremi Piotrowski wrote: > > > On 8/14/2023 9:43 AM, Dan Williams wrote: > > > > Changes since v1: > > > > - Switch from Keyring to sysfs (James) > > > > > > > > An attestation report is signed evidence of how a Trusted Virtual > > > > Machine (TVM) was launched and its current state. A verifying party uses > > > > the report to make judgements of the confidentiality and integrity of > > > > that execution environment. Upon successful attestation the verifying > > > > party may, for example, proceed to deploy secrets to the TVM to carry > > > > out a workload. Multiple confidential computing platforms share this > > > > similar flow. > > > > > > > > The approach of adding adding new char devs and new ioctls, for what > > > > amounts to the same logical functionality with minor formatting > > > > differences across vendors [1], is untenable. Common concepts and the > > > > community benefit from common infrastructure. > > > > > > > > Use sysfs for this facility for maintainability compared to ioctl(). The > > > > expectation is that this interface is a boot time, configure once, get > > > > report, and done flow. I.e. not something that receives ongoing > > > > transactions at runtime. However, runtime retrieval is not precluded and > > > > a mechanism to detect potential configuration conflicts from > > > > multiple-threads using this interface is included. > > > > > > > > > > I wanted to speak up to say that this does not align with the needs we have > > > in the Confidential Containers project. We want to be able to perform attestation > > > not just once during boot but during the lifecycle of the confidential VM. We > > > may need to fetch a fresh attestation report from a trusted agent but also from > > > arbitrary applications running in containers. > > > > > > The trusted agent might need attestation when launching a new container from an > > > encrypted container image or when a new secret is being added to the VM - both > > > of these events may happen at any time (also when containerized applications > > > are already executing). > > > > > > Container applications have their own uses for attestation, such as when they need > > > to fetch keys required to decrypt payloads. We also have things like performing > > > attestation when establishing a TLS or ssh connection to provide an attested e2e > > > encrypted communication channel. > > > > ...and you expect that the boot time attestation becomes invalidated > > later at run time such that ongoing round trips to the TSM are needed? I > > am looking at "Table 21. ATTESTATION_REPORT Structure" for example and > > not seeing data there that changes from one request to the next. Runtime > > validation likely looks more like the vTPM use case with PCRs. That will > > leverage the existing / common TPM ABI. > > I thought we were dropping the TSM acronym as requested by Jarkko? I read that in the context of the key-type name. The key-type proposal was dropped. > Why do we need to be so prescriptive about "boot time" vs "runtime" > attestations? A user may wish to attest to several requests as Jeremi > notes. And why should users be forced into using a vTPM interface if > their usecase doesn't require all the features and complexity of a > vTPM? When I said "like vTPM" I did not mean to infer "only vTPM". There are three scenarios base attestation reports, attestation reports with runtime measurement values, and vTPM. This patchset is only about the first scenario. RTMRs have similarities with PCRs, does that mean they need to be intergrated behind the existing TPM ABI or deserve something new? That is a question that the RTMR enabling effort will need to answer. > Some users may prefer less overall code within their Trusted > Computer Base (TCB) and a TPM emulate is a significant code base. Yes. > It seems like you are just reading the SNP spec, Nope, fully aware of the TDX spec. This patchset is around wrapping the existing sev-guest capability with a shared ABI for the TDX equivalen. > if you read the TDX > spec you'll see there are RTMRs which can be extended with new data. > This leads to a different data in the attestation. Similar there are > REMs in the ARM CCA spec. Again, RTMRs are not part of this current proposal. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-14 17:12 ` Dan Williams 2023-08-15 14:27 ` Peter Gonda @ 2023-08-16 9:42 ` Jeremi Piotrowski 1 sibling, 0 replies; 47+ messages in thread From: Jeremi Piotrowski @ 2023-08-16 9:42 UTC (permalink / raw) To: Dan Williams, linux-coco Cc: Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Peter Gonda, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel On 8/14/2023 7:12 PM, Dan Williams wrote: > Jeremi Piotrowski wrote: >> On 8/14/2023 9:43 AM, Dan Williams wrote: >>> Changes since v1: >>> - Switch from Keyring to sysfs (James) >>> >>> An attestation report is signed evidence of how a Trusted Virtual >>> Machine (TVM) was launched and its current state. A verifying party uses >>> the report to make judgements of the confidentiality and integrity of >>> that execution environment. Upon successful attestation the verifying >>> party may, for example, proceed to deploy secrets to the TVM to carry >>> out a workload. Multiple confidential computing platforms share this >>> similar flow. >>> >>> The approach of adding adding new char devs and new ioctls, for what >>> amounts to the same logical functionality with minor formatting >>> differences across vendors [1], is untenable. Common concepts and the >>> community benefit from common infrastructure. >>> >>> Use sysfs for this facility for maintainability compared to ioctl(). The >>> expectation is that this interface is a boot time, configure once, get >>> report, and done flow. I.e. not something that receives ongoing >>> transactions at runtime. However, runtime retrieval is not precluded and >>> a mechanism to detect potential configuration conflicts from >>> multiple-threads using this interface is included. >>> >> >> I wanted to speak up to say that this does not align with the needs we have >> in the Confidential Containers project. We want to be able to perform attestation >> not just once during boot but during the lifecycle of the confidential VM. We >> may need to fetch a fresh attestation report from a trusted agent but also from >> arbitrary applications running in containers. >> >> The trusted agent might need attestation when launching a new container from an >> encrypted container image or when a new secret is being added to the VM - both >> of these events may happen at any time (also when containerized applications >> are already executing). >> >> Container applications have their own uses for attestation, such as when they need >> to fetch keys required to decrypt payloads. We also have things like performing >> attestation when establishing a TLS or ssh connection to provide an attested e2e >> encrypted communication channel. > > ...and you expect that the boot time attestation becomes invalidated > later at run time such that ongoing round trips to the TSM are needed? It's not that it would become invalidated - it's that it will have served its purpose. Attestation is used to establish trust with a relying party, for every other relying party you'll need to generate a fresh attestation report. So we can't lock ourselves into a specific protocol in the kernel here that only assumes a single party. The one shot "decrypt disk image during boot" attestation use case is relevant elsewhere, but not so much for containers. > I am looking at "Table 21. ATTESTATION_REPORT Structure" for example and > not seeing data there that changes from one request to the next. REPORT_DATA in SNP or REPORTDATA in TDX. You want to have a nonce/challenge or short lived session keys covered by the report, so you hash some data structure that includes them and request an attestation report with the hash in "report data". Here's an example of the verifying side: https://github.com/confidential-containers/attestation-service/blob/main/attestation-service/src/verifier/tdx/mod.rs#L40-L52 > Runtime validation likely looks more like the vTPM use case with PCRs. That will > leverage the existing / common TPM ABI.> Not at all, these two things are orthogonal. PCRs can be extended at runtime but you'll struggle to use them as described above. You'd have to designate a PCR for this purpose, lock against concurrent users (across multiple TPM commands) and reset it before every use. Highly impractical. TPM2 has a similar concept to "REPORTDATA" called "qualifying data" which is passed when requesting a quote. This highlights the need for an interface to regenerate evidence (attestation report or quote) with user defined data mixed in. >> I don't think sysfs is suitable for such concurrent transactions. Also if you think >> about exposing the sysfs interface to an application in a container, this requires >> bind mounting rw part of the sysfs tree into the mount namespace - not ideal. > > sysfs is not suitable for concurrent transactions. The container would > need to have an alternate path to request that the singleton owner of > the interface generate new reports, or use the boot time attestation to > derive per container communication sessions to the attestation agent. It would be possible to use a userspace agent to coordinate access to generating reports, but that takes us further away from standardization - applications would have to be tailored to a specific environment instead of relying on the same kernel (hardware) interface everywhere. I don't follow this part: "use the boot time attestation to derive per container communication sessions to the attestation agent". In general we want the application attestation report to be linked directly to a hardware root of trust, without chaining through some intermediate entity. ^ permalink raw reply [flat|nested] 47+ messages in thread
* Re: [PATCH v2 0/5] tsm: Attestation Report ABI 2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski 2023-08-14 17:12 ` Dan Williams @ 2023-08-23 11:21 ` Dr. Greg 1 sibling, 0 replies; 47+ messages in thread From: Dr. Greg @ 2023-08-23 11:21 UTC (permalink / raw) To: Jeremi Piotrowski Cc: Dan Williams, linux-coco, Brijesh Singh, Kuppuswamy Sathyanarayanan, Peter Zijlstra, Tom Lendacky, Peter Gonda, Borislav Petkov, Dionna Amalie Glaze, Samuel Ortiz, Greg Kroah-Hartman, Andrew Morton, James Bottomley, x86, linux-kernel, linux-security-module On Mon, Aug 14, 2023 at 11:04:52AM +0200, Jeremi Piotrowski wrote: Good morning, I hope the week is going well for everyone. > On 8/14/2023 9:43 AM, Dan Williams wrote: > > Changes since v1: > > - Switch from Keyring to sysfs (James) > > > > An attestation report is signed evidence of how a Trusted Virtual > > Machine (TVM) was launched and its current state. A verifying party uses > > the report to make judgements of the confidentiality and integrity of > > that execution environment. Upon successful attestation the verifying > > party may, for example, proceed to deploy secrets to the TVM to carry > > out a workload. Multiple confidential computing platforms share this > > similar flow. > > > > The approach of adding adding new char devs and new ioctls, for what > > amounts to the same logical functionality with minor formatting > > differences across vendors [1], is untenable. Common concepts and the > > community benefit from common infrastructure. > > > > Use sysfs for this facility for maintainability compared to ioctl(). The > > expectation is that this interface is a boot time, configure once, get > > report, and done flow. I.e. not something that receives ongoing > > transactions at runtime. However, runtime retrieval is not precluded and > > a mechanism to detect potential configuration conflicts from > > multiple-threads using this interface is included. > > > I wanted to speak up to say that this does not align with the needs > we have in the Confidential Containers project. We want to be able > to perform attestation not just once during boot but during the > lifecycle of the confidential VM. We may need to fetch a fresh > attestation report from a trusted agent but also from arbitrary > applications running in containers. > > The trusted agent might need attestation when launching a new > container from an encrypted container image or when a new secret is > being added to the VM - both of these events may happen at any time > (also when containerized applications are already executing). > > Container applications have their own uses for attestation, such as > when they need to fetch keys required to decrypt payloads. We also > have things like performing attestation when establishing a TLS or > ssh connection to provide an attested e2e encrypted communication > channel. > > I don't think sysfs is suitable for such concurrent > transactions. Also if you think about exposing the sysfs interface > to an application in a container, this requires bind mounting rw > part of the sysfs tree into the mount namespace - not ideal. We don't have a dog in this fight regarding TDX [1], but we do have a significant body of experience with the concepts and challenges involved. The issue at hand is that trust is a resource that needs to be orchestrated, just like any other resource. A concept, based on our experiences, that seems to be significantly outside of mainstream thought. The notion of the need to orchestrate trust seems to be particularly important with a concept such as Confidential Containers. FWIW, we have pushed forward a second round of patches for the kernel infrastructure that make the concepts of trust orchestration and containerization tenable: https://lore.kernel.org/linux-security-module/20230710102319.19716-1-greg@enjellic.com/T/#t In addition, FWIW, we have actually built systems that implement these principals, obviously not on TDX hardware, see [1], but we do have a significant body of experience with using SGX as a trust root. We have even provided a substantial set of initial userspace tooling that implement these concepts to support the proposed patches. I only say all of this to convey the notion that we have actually done work on all of these concepts and are not just waving our hands around. The notion of surfacing this information through /sysfs becomes less problematic if one approaches the issue through the lens of having trust orchestrators that are responsible for managing the security or trust status of the execution platform at large and any subordinate workloads. This concept is true, even if the platform/VM is only hosting a single workload. Which may be a necessity for some security contracts, where there is literally no trust in the fact that side-channel disclosure threats can be properly mitigated, ie. there cannot be workloads with possible adversarial intents. From the outside looking in, unless there are some fundamental conversations regarding how trusted systems and workloads can be developed with the architectures being proposed, it is completely unclear how durable API's, on the order of 20+ years are ever going to be attained. No criticism, just observation. > Jeremi Best wishes for continued progress on all of this, it is important stuff, both in the cloud and on the edge. As always, Dr. Greg The Quixote Project - Flailing at the Travails of Cybersecurity [1]: TDX enabled hardware is difficult, if not impossible to obtain. If there is any doubt, simply search for TDX hardware availability and cringe at the conversations on the Intel forums about people trying to get experience with the technology. This is the same problem that plagued TXT and SGX and results in enabling infrastructure development being done in an echo chamber. A concept that may have proven successful when all of this work was expected to be implementated and enabled by OEM providers but seems problematic in an ostensibly 'community' driven project such as Linux. ^ permalink raw reply [flat|nested] 47+ messages in thread
end of thread, other threads:[~2024-01-04 18:29 UTC | newest]
Thread overview: 47+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams
2023-08-14 7:43 ` [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-08-14 8:24 ` Jeremi Piotrowski
2023-08-14 16:21 ` Dan Williams
2023-08-14 15:38 ` Greg Kroah-Hartman
2023-08-14 16:43 ` Dan Williams
2023-08-14 18:43 ` Greg Kroah-Hartman
2023-08-15 19:51 ` Tom Lendacky
2023-08-16 14:44 ` Tom Lendacky
2023-08-16 15:12 ` Dan Williams
2023-08-22 7:29 ` Roy Hopkins
2023-08-23 13:49 ` Samuel Ortiz
2023-08-28 10:46 ` Dr. Greg
2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
2023-08-14 16:58 ` Dionna Amalie Glaze
2023-08-14 23:24 ` Dan Williams
2023-08-15 20:11 ` Tom Lendacky
2023-08-15 21:03 ` Dan Williams
2023-08-16 19:38 ` Dionna Amalie Glaze
2023-08-15 20:20 ` Tom Lendacky
2023-08-15 21:37 ` Dan Williams
2023-08-14 7:43 ` [PATCH v2 4/5] mm/slab: Add __free() support for kvfree Dan Williams
2023-08-14 15:31 ` Greg Kroah-Hartman
2023-08-14 16:17 ` Peter Zijlstra
2023-08-14 18:44 ` Greg Kroah-Hartman
2023-08-14 18:45 ` Greg Kroah-Hartman
2024-01-04 6:57 ` Lukas Wunner
2024-01-04 18:29 ` Dan Williams
2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
2023-08-14 8:30 ` Jeremi Piotrowski
2023-08-14 16:22 ` Dan Williams
2023-08-14 11:21 ` Peter Zijlstra
2023-08-14 16:25 ` Dan Williams
2023-08-14 16:48 ` Peter Zijlstra
2023-08-14 22:15 ` Peter Zijlstra
2023-08-15 8:37 ` Peter Zijlstra
2023-08-15 20:50 ` Tom Lendacky
2023-08-15 21:40 ` Dan Williams
2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski
2023-08-14 17:12 ` Dan Williams
2023-08-15 14:27 ` Peter Gonda
2023-08-15 17:16 ` Dionna Amalie Glaze
2023-08-15 21:13 ` Dan Williams
2023-08-15 18:13 ` Dan Williams
2023-08-16 9:42 ` Jeremi Piotrowski
2023-08-23 11:21 ` Dr. Greg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).