Message-ID: <7fada981-bc73-774e-bff3-e9be56c52b66@amd.com>
Date:   Wed, 16 Aug 2023 09:44:07 -0500
From:   Tom Lendacky <thomas.lendacky@....com>
To:     Dan Williams <dan.j.williams@...el.com>, linux-coco@...ts.linux.dev
Cc:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Dionna Amalie Glaze <dionnaglaze@...gle.com>,
        James Bottomley <James.Bottomley@...senPartnership.com>,
        Peter Gonda <pgonda@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Samuel Ortiz <sameo@...osinc.com>, peterz@...radead.org,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation
 reports

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@...ux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@...gle.com>
> Cc: James Bottomley <James.Bottomley@...senPartnership.com>
> Cc: Peter Gonda <pgonda@...gle.com>
> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> Cc: Samuel Ortiz <sameo@...osinc.com>
> Signed-off-by: Dan Williams <dan.j.williams@...el.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 */
> 
> 

Powered by blists - more mailing lists