Re: [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports

[Roy Hopkins]

On Mon, 2023-08-14 at 00:43 -0700, 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@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch
>  [1]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
> Cc: Dionna Amalie Glaze <dionnaglaze@xxxxxxxxxx>
> Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Peter Gonda <pgonda@xxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Samuel Ortiz <sameo@xxxxxxxxxxxx>
> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> ---
>  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 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);

I think tsm_report.outblob needs to be set to NULL here otherwise there is
the possibility of a double-free if ops->report_new returns an error.

Roy

> +               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);
> +}