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

[Alexey Kardashevskiy <aik@amd.com>]

On 17/10/23 13:19, Dan Williams wrote:
> 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.

I did not understand why memcpy() and not copy_from_user(), for example, 
had to grep. Commenting on the configfs api outside of it imho is a bit 
too much but ok.


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

I asked as at first I thought it is stored right after rwsem_write in 
@ops for a reason but then it was not even checked for NULL so it could 
be initialized where declared.


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

The order is mixed now:

TSM_DEFAULT_ATTRS
tsm_report_attrs
TSM_DEFAULT_BIN_ATTRS
tsm_report_bin_attrs
tsm_report_bin_extra_attrs (*)
tsm_report_extra_attrs  (*)
tsm_report_item_release
tsm_report_item_ops
tsm_report_default_type
tsm_report_ext_type (*)

Grouping (*) together would make the superset thing clearer imho.

tsm_report_ distract too as they could be just tsm_ (as those in capital 
letters). Either _ext_ or _extra_. I am not insisting though, just 
whining :) Thanks,


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

-- 
Alexey