Messages in this thread | | | Date | Tue, 15 Aug 2023 14:51:47 -0500 | Subject | Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports | From | Tom Lendacky <> |
| |
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); > +
| |