Message-ID: <651329fed4947_124e929464@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Date:   Tue, 26 Sep 2023 11:59:10 -0700
From:   Dan Williams <dan.j.williams@...el.com>
To:     Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@...ux.intel.com>,
        Dan Williams <dan.j.williams@...el.com>,
        <linux-coco@...ts.linux.dev>
CC:     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>,
        Thomas Gleixner <tglx@...utronix.de>, <peterz@...radead.org>,
        <linux-kernel@...r.kernel.org>, <x86@...nel.org>,
        <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for
 attestation reports

Kuppuswamy Sathyanarayanan wrote:
> Hi Dan,
> 
> On 9/25/2023 9:17 PM, 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-SNP, COVE if
> > not more, is to define a configfs interface to retrieve the TSM-specific
> > blob.
> > 
> >     report=/sys/kernel/config/tsm/report/report0
> >     mkdir $report
> >     dd if=binary_userdata_plus_nonce > $report/inblob
> >     hexdump $report/outblob
> > 
> > This approach later allows for the standardization of the attestation
> > blob format without needing to invent a new ABI. Once standardization
> > happens the standard format can be emitted by $report/outblob and
> > indicated by $report/provider, or a new attribute like
> > "$report/tcg_coco_report" can emit the standard format alongside the
> > vendor format.
> > 
> > Review of previous iterations of this interface identified that there is
> > a need to scale report generation for multiple container environments
> > [2]. Configfs enables a model where each container can bind mount one or
> > more report generation item instances. Still, within a container only a
> > single thread can be manipulating a given configuration instance at a
> > time. A 'generation' count is provided to detect conflicts between
> > multiple threads racing to configure a report instance.
> > 
> > The SEV-SNP concepts of "extended reports" and "privilege levels" are
> > optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> > time. The expectation is that those concepts are generic enough that
> > they may be adopted by other TSM implementations. In other words,
> > configfs-tsm aims to address a superset of TSM specific functionality
> > with a common ABI where attributes may appear, or not appear, based on the set
> > of concepts the implementation supports.
> > 
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> > 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>
> > Acked-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
> > Acked-by: Thomas Gleixner <tglx@...utronix.de>
> > Signed-off-by: Dan Williams <dan.j.williams@...el.com>
[..]
> > +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> > +			     enum tsm_data_select select)
> > +{
> > +	loff_t offset = 0;
> > +	u8 *out, len;
> > +
> > +	if (select == TSM_REPORT) {
> > +		out = report->outblob;
> > +		len = report->outblob_len;
> > +	} else {
> > +		out = report->certs;
> > +		len = report->certs_len;
> > +	}
> > +
> 
> Since we get out and len from arch_ops, I think we can check for null condition before
> attempting the memory_read_from_buffer()
> 
> > +	if (!buf)
> > +		return len;
> 
> buf cannot be NULL, right? Do you want this check? If you want to leave it,
> in NULL condition it should return 0 bytes, right?

No, and this might deserve a comment for folks that are not familiar
with how configfs works, but configfs calls an attribute's ->read()
helper with @buf == NULL to say "please tell me how many bytes are
available, and I will call you back again to fill in the buffer at that
size".

> 
> > +	return memory_read_from_buffer(buf, count, &offset, out, len);
> > +}
> > +
> > +static ssize_t read_cached_report(struct tsm_report *report, void *buf,
> > +				  size_t count, enum tsm_data_select select)
> > +{
> > +	struct tsm_report_state *state = to_state(report);
> > +
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	if (!report->desc.inblob_len)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * A given TSM backend always fills in ->outblob regardless of
> > +	 * whether the report includes certs or not.
> > +	 */
> > +	if (!report->outblob ||
> > +	    state->read_generation != state->write_generation)
> > +		return -EWOULDBLOCK;
> > +
> > +	return __read_report(report, buf, count, select);
> > +}
> > +
> > +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->certs);
> > +	report->outblob = NULL;
> > +	report->certs = NULL;
> 
> Since you are clearing outblob and certs, do you want to reset the outblob_len and certs_len?

Not strictly necessary, nothing in the code is checking _len for whether
the report is ready or not.

[..]
> > +/**
> > + * struct tsm_desc - option descriptor for generating tsm report blobs
> > + * @privlevel: optional privilege level to associate with @outblob
> > + * @inblob_len: sizeof @inblob
> > + * @inblob: arbitrary input data
> > + */
> > +struct tsm_desc {
> > +	unsigned int privlevel;
> > +	size_t inblob_len;
> > +	u8 inblob[TSM_INBLOB_MAX];
> > +};
> > +
> > +/**
> > + * struct tsm_report - track state of report generation relative to options
> > + * @desc: report generation options / cached report state
> > + * @outblob: generated evidence to provider to the attestation agent
> > + * @outblob_len: sizeof(outblob)
> 
> I think following is incorrect. You might want to add info about certs_len
> and certs.

Yeah, missed updating this with certs addition. The outblob_len
definition is correct, or do you mean the kdoc is out of order with
respect to the struct?

> 
> > + * @write_generation: conflict detection, and report regeneration tracking
> > + * @read_generation: cached report invalidation tracking
> > + * @cfg: configfs interface
> > + */
> > +struct tsm_report {
> > +	struct tsm_desc desc;
> > +	size_t outblob_len;
> > +	u8 *outblob;
> > +	size_t certs_len;
> > +	u8 *certs;
> > +};
> > +
> > +/*
> > + * arch specific ops, only one is expected to be registered at a time
> > + * i.e. only one of SEV, TDX, COVE, etc.
> > + */
> 
> Since it is ARCH specific ops, I think adding some info about its members
> will be helpful. Like what is report_new callback and its acceptable
> return values.

Sure.

Will wait for positive test feedback about the sev-guest changes before
spinning this series again.

Powered by blists - more mailing lists