Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams <dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
[..]
> > +What:		/sys/kernel/config/tsm/report/$name/provider
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) A name for the format-specification of @outblob like
> > +		"sev-guest" [1]  or "tdx-guest" [2] in the near term, or a
> > +		common standard format in the future.
> 
> Nit: /sys/kernel/config/tsm/report/report0/provider contains 
> "sev_guest", i.e. "_", not "-".

Yes, will fix either with a follow-on or a respin if more feedback
arrives.

> 
> > +
> > +		[1]: SEV Secure Nested Paging Firmware ABI Specification
> > +		Revision 1.55 Table 22
> > +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
> > +
> > +		[2]: Intel® Trust Domain Extensions Data Center Attestation
> > +		Primitives : Quote Generation Library and Quote Verification
> > +		Library Revision 0.8 Appendix 4,5
> > +		https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/generation
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) The value in this attribute increments each time @inblob 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, or it can prevent conflicts by creating a report
> > +		instance per requesting context.
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/privlevel
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(WO) Attribute is visible if a TSM implementation provider
> > +		supports the concept of attestation reports for TVMs running at
> > +		different privilege levels, like SEV-SNP "VMPL", specify the
> > +		privilege level via this attribute.  The minimum acceptable
> > +		value is conveyed via @privlevel_floor and the maximum
> > +		acceptable value is TSM_PRIVLEVEL_MAX (3).
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/privlevel_floor
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) Indicates the minimum permissible value that can be written
> > +		to @privlevel.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b19995690904..8acbeb029ba1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21889,6 +21889,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/configfs-tsm
> > +F:	drivers/virt/coco/tsm.c
> > +F:	include/linux/tsm.h
> > +
> >   TTY LAYER AND SERIAL DRIVERS
> >   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..87d142c1f932 100644
> > --- a/drivers/virt/coco/Kconfig
> > +++ b/drivers/virt/coco/Kconfig
> > @@ -2,6 +2,11 @@
> >   #
> >   # Confidential computing related collateral
> >   #
> > +
> > +config TSM_REPORTS
> > +	select CONFIGFS_FS
> > +	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/tsm.c b/drivers/virt/coco/tsm.c
> > new file mode 100644
> > index 000000000000..0200a86f1efe
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm.c
> > @@ -0,0 +1,423 @@
> > +// 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/string.h>
> > +#include <linux/module.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/configfs.h>
> > +
> > +static struct tsm_provider {
> > +	const struct tsm_ops *ops;
> > +	const struct config_item_type *type;
> > +	void *data;
> > +} 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, however configfs allows for multiple agents to
> > + * own their own report generation instances to generate reports as
> > + * often as needed.
> > + *
> > + * The attestation report format is TSM provider specific, when / if a standard
> > + * materializes that can be published instead of the vendor layout. Until then
> > + * the 'provider' attribute indicates the format of 'outblob', and optionally
> > + * 'auxblob'.
> > + */
> > +
> > +struct tsm_report_state {
> > +	struct tsm_report report;
> > +	unsigned long write_generation;
> > +	unsigned long read_generation;
> > +	struct config_item cfg;
> > +};
> > +
> > +enum tsm_data_select {
> > +	TSM_REPORT,
> > +	TSM_CERTS,
> > +};
> > +
> > +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> > +{
> > +	struct tsm_report_state *state =
> > +		container_of(cfg, struct tsm_report_state, cfg);
> 
> This be one line of 88 chars (less than allowed 100).
> 
> (I'll comment once on this, feel free to ignore :) )

Unless and until the kernel's .clang-format template is updated to 100
you will find my patches wrapped to its ColumnLimit setting (80). I did
manually fixup my sev_guest changes to 100 since there was precedent in
that file, everything else I just let .clang-format do its thing.

> 
> > +
> > +	return &state->report;
> > +}
> > +
> > +static struct tsm_report_state *to_state(struct tsm_report *report)
> > +{
> > +	return container_of(report, struct tsm_report_state, report);
> > +}
> > +
> > +static int try_advance_write_generation(struct tsm_report *report)
> > +{
> > +	struct tsm_report_state *state = to_state(report);
> > +
> > +	lockdep_assert_held_write(&tsm_rwsem);
> > +
> > +	/*
> > +	 * Malicious or broken userspace has written enough times for
> > +	 * read_generation == write_generation by modular arithmetic without an
> > +	 * interim read. Stop accepting updates until the current report
> > +	 * configuration is read.
> > +	 */
> > +	if (state->write_generation == state->read_generation - 1)
> > +		return -EBUSY;
> > +	state->write_generation++;
> > +	return 0;
> > +}
> > +
> > +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> > +					  const char *buf, size_t len)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	unsigned int val;
> > +	int rc;
> > +
> > +	rc = kstrtouint(buf, 0, &val);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/*
> > +	 * The valid privilege levels that a TSM might accept, if it accepts a
> > +	 * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
> > +	 * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
> > +	 * than 0.
> > +	 */
> 
> Sounds like privlevel_floor should be "unsigned int" rather than "int".

Sure.

> > +	if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
> > +		return -EINVAL;
> > +
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	rc = try_advance_write_generation(report);
> > +	if (rc)
> > +		return rc;
> > +	report->desc.privlevel = val;
> > +
> > +	return len;
> > +}
> > +CONFIGFS_ATTR_WO(tsm_report_, privlevel);
> > +
> > +static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
> > +					       char *buf)
> > +{
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
> 
> %d  or  change the type.

Ok.

> 
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
> > +
> > +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> > +				       const void *buf, size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	int rc;
> > +
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	rc = try_advance_write_generation(report);
> > +	if (rc)
> > +		return rc;
> > +
> > +	report->desc.inblob_len = count;
> > +	memcpy(report->desc.inblob, buf, count);
> > +	return count;
> > +}
> > +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
> > +
> > +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	struct tsm_report_state *state = to_state(report);
> > +
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	return sysfs_emit(buf, "%lu\n", state->write_generation);
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, generation);
> > +
> > +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
> > +{
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	return sysfs_emit(buf, "%s\n", provider.ops->name);
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, provider);
> > +
> > +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> > +			     enum tsm_data_select select)
> > +{
> > +	loff_t offset = 0;
> > +	ssize_t len;
> > +	u8 *out;
> > +
> > +	if (select == TSM_REPORT) {
> > +		out = report->outblob;
> > +		len = report->outblob_len;
> > +	} else {
> > +		out = report->auxblob;
> > +		len = report->auxblob_len;
> > +	}
> > +
> > +	/*
> > +	 * Recall that a NULL @buf is configfs requesting the size of
> > +	 * the buffer.
> > +	 */
> 
> The comment can be one line (or even dropped as it is configfs api).

I got a comment from a reviewer that did not understand why @buf is
allowed to be NULL. It's an oddity compared to sysfs binary attributes,
so I don't mind the comment.

[..]
> > +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> > +			       size_t count, enum tsm_data_select select)
> > +{
> > +	struct tsm_report_state *state = to_state(report);
> > +	const struct tsm_ops *ops;
> > +	ssize_t rc;
> > +
> > +	/* try to read from the existing report if present and valid... */
> > +	rc = read_cached_report(report, buf, count, select);
> > +	if (rc >= 0 || rc != -EWOULDBLOCK)
> > +		return rc;
> > +
> > +	/* slow path, report may need to be regenerated... */
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	ops = provider.ops;
> > +	if (!report->desc.inblob_len)
> > +		return -EINVAL;
> > +
> > +	/* did another thread already generate this report? */
> > +	if (report->outblob &&
> > +	    state->read_generation == state->write_generation)
> > +		goto out;
> > +	kvfree(report->outblob);
> > +	kvfree(report->auxblob);
> > +	report->outblob = NULL;
> > +	report->auxblob = NULL;
> > +	rc = ops->report_new(report, provider.data);
> 
> 
> Drop @ops and use "provider.ops" here?

Shrug, ok.

> 
> > +	if (rc < 0)
> > +		return rc;
> > +	state->read_generation = state->write_generation;
> > +out:
> > +	return __read_report(report, buf, count, select);
> > +}
> > +
> > +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> > +				       size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +
> > +	return tsm_report_read(report, buf, count, TSM_REPORT);
> > +}
> > +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
> > +
> > +static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
> > +				       size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +
> > +	return tsm_report_read(report, buf, count, TSM_CERTS);
> > +}
> > +CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
> > +
> > +#define TSM_DEFAULT_ATTRS() \
> 
> imho this one and TSM_DEFAULT_BIN_ATTRS are not really helping with 
> readability or a size :)

It is meant to do neither, it is only here to make it clear that
tsm_report_bin_extra_attrs[] is a super-set of tsm_report_bin_attrs[].
What I really want is sysfs-style group syntax, but configfs does not
have that same declaration capability.

[..]
> 
> A little confusing thing is that this guy is neither near the beginning 
> of the file (with other statics, this could even be a member of 
> tsm_provider) nor the code which sets/clears it - tsm_init/tsm_unregister.

@tsm_report_group is only used in tsm_{init,exit}(). I'll move its
declartion closer to tsm_init() in a follow-on.

[..]
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > new file mode 100644
> > index 000000000000..5fadc382064d
> > --- /dev/null
> > +++ b/include/linux/tsm.h
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_H
> > +#define __TSM_H
> > +
> > +#include <linux/sizes.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> 
> device.h is not needed.

Yes, an earlier version of this interface referenced devices.