[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <53d6b712-e2e6-9188-51c7-6190a42dcc5f@amd.com>
Date: Tue, 15 Aug 2023 14:51:47 -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
>
> +/*
> + * 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);
> +
Powered by blists - more mailing lists