Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports

[Kuppuswamy Sathyanarayanan]

On 9/26/2023 11:59 AM, Dan Williams wrote:
> 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@xxxxxxxxxxxxxxxxxxxxxxxxx.notmuch [1]
>>> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@xxxxxxxxxxxxxxxxxxx [2]
>>> 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>
>>> Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> Acked-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> [..]
>>> +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".
> 

Got it. Thanks for clarifying it.

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

ok.

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

No, I am talking about the write_generation, read_generation and cfg options.
They are part of struct tsm_report_state, so why document it here?

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

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer