Message-ID: <aa492474-e975-4121-8e0f-54414a7f5e65@intel.com>
Date: Wed, 12 Mar 2025 13:26:37 -0500
From: "Xing, Cedric" <cedric.xing@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "bp@...en8.de" <bp@...en8.de>,
	"x86@...nel.org" <x86@...nel.org>, "mingo@...hat.com" <mingo@...hat.com>,
	"Williams, Dan J" <dan.j.williams@...el.com>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"tglx@...utronix.de" <tglx@...utronix.de>, "hpa@...or.com" <hpa@...or.com>
CC: "sathyanarayanan.kuppuswamy@...ux.intel.com"
	<sathyanarayanan.kuppuswamy@...ux.intel.com>, "dionnaglaze@...gle.com"
	<dionnaglaze@...gle.com>, "linux-coco@...ts.linux.dev"
	<linux-coco@...ts.linux.dev>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "James.Bottomley@...senPartnership.com"
	<James.Bottomley@...senPartnership.com>, "mikko.ylinen@...ux.intel.com"
	<mikko.ylinen@...ux.intel.com>, "dan.middleton@...ux.intel.com"
	<dan.middleton@...ux.intel.com>
Subject: Re: [PATCH v2 1/4] tsm: Add TVM Measurement Register support

Hi Kai,

Thanks for your comments and my apologies for my late response!

On 3/5/2025 7:20 PM, Huang, Kai wrote:
> On Sun, 2025-02-23 at 21:20 -0600, Cedric Xing wrote:
>> Add new TSM APIs for TVM guest drivers to register and expose measurement
>> registers (MRs) as sysfs attributes (files).
> 
> Hi Cedric,
> 
> The current TSM is done in configfs, but not sysfs.  The reason, quoted from
> commit 70e6f7e2b9857 ("configfs-tsm: Introduce a shared ABI for attestation
> reports"), is:
> 
>      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.
> 
> And the link [2] (where you can find the relevant discussion) is:
> 
> http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com
> 
> Could you elaborate why do you choose to expose MRs via sysfs rather than
> configfs?  Is the above reason not valid anymore?
> 	
The key difference between MRs and reports/quotes is the lack of 
context. Reports/quotes benefit from having a separate context for each 
container, ensuring they don't interfere with each other. However, MRs 
are global, and creating separate contexts would be confusing since 
changes/extensions to MRs by one container will always be visible to others.

Below is TDX specific:

Report0/reportdata is an exception, as report0 serves as a comprehensive 
list of all measurements rather than a container-specific report. 
reportdata provides an easy way to request a report if inter-container 
race isn't a concern for your application.

I can see the confusion here though (both Mikko and you raised the same 
concern). I can (1) take away reportdata but leave report0 as it; or (2) 
take away reportdata and break down report0 into tee_tcb_info and tdinfo 
(and strip off report_mac_struct) so user can still have a comprehensive 
list of MRs; or (3) take away report0/reportdata altogether. Which one 
do you think is the most reasonable? In all cases, I'll incorporate 
Mikko's patch into this series to allow per-container TDREPORT under 
configfs.

> 
>>
>> New TSM APIs:
>>
>> - `tsm_register_measurement(struct tsm_measurement *)`: Register a set of
>>    MRs with the TSM core.
>> - `tsm_unregister_measurement(struct tsm_measurement *)`: Unregister a
>>    previously registered set of MRs.
>>
>> These APIs are centered around `struct tsm_measurement`, which includes an
>> array of `struct tsm_measurement_register`s with properties
>> (`tsm_measurement_register::mr_flags`) like *Readable* (`TSM_MR_F_R`) and
>> *Extensible* (`TSM_MR_F_X`). For details, see include/linux/tsm.h.
> 
> Nit:
> 
> We can see those details from the code.  Personally I think you don't need to
> describe them again in the changelog.  It would be more helpful if you could put
> more _why_ here.
> 
Did I describe them in the changelog? I'm not sure...

> E.g., Wwhat is userspace's requirement/flow that involves reading/extending
> those MRs?  An example is even better.
> 
The intention of exposing MRs as files is to make it obvious to users 
how to read/extend MRs. But from your feedback I can tell it isn't 
obvious enough and I'll update the commit message in the next revision.

>>
>> Upon successful registration, the TSM core exposes MRs in sysfs at
>> /sys/kernel/tsm/MR_PROVIDER/, where MR_PROVIDER is the measurement
>> provider's name (`tsm_measurement::name`). Each MR is accessible either as
>> a file (/sys/kernel/tsm/MR_PROVIDER/MR_NAME contains the MR value) or a
>> directory (/sys/kernel/tsm/MR_PROVIDER/MR_NAME/HASH/digest contains the MR
>> value) depending on whether `TSM_MR_F_F` is set or cleared (in
>> `tsm_measurement_register::mr_flags`). MR_NAME is the name
>> (`tsm_measurement_register::mr_name`) of the MR, while HASH is the hash
>> algorithm (`tsm_measurement_register::mr_hash`) name in the latter case.
> 
> Please correct me if I am wrong: in my understanding, the purpose is to provide
> a "unified ABI for usrspace" for MRs, but not just some common infrastructure in
> the kernel to support exposing MRs, right?
> 
> Configfs-tsm provides consistent names for all attributes for all vendors:
> 'inblob', 'outblob', 'generation', 'provider' etc, so it provides a unified ABI
> for userspace.
> 
"attestation reports" in this configfs context refers to opaque blobs 
consumed by external parties, while the guest acts as the "wire" for 
transporting the reports.

> But here actually each vendor will have its own directory.  E.g., for TDX we
> have:
> 
> 	/sys/kernel/tsm/tdx/...
> 
> And the actual MRs under the vendor-specific directory are completely vendor-
> specific.  E.g., as shown in the last patch, for TDX we have: mrconfigid,
> mrowner etc.  And for other vendors they are free to register MRs on their own.
> 
In contrast, MRs (especially extensible/RT MRs) are consumed by the 
guest itself. They are vendor specific because they are _indeed_ vendor 
specific. The intention is to unlock access to all of them for user 
mode. The semantics (i.e., which MR stores what measurement) is 
application specific and will be assigned by the application.

> Could you elaborate how userspace is supposed to use those MRs in a common way?
>   
> Or this is not the purpose?
> 
Sure. For example, CoCo may require storing container measurements into 
an RTMR. Then, a potential implementation could extend those 
measurements to an "RTMR file" named "container_mr", which could be a 
symlink pointing to different RTMRs on different archs.

Of course, we are hand-waiving the potential difference in the 
number/naming of the MRs and the hash algorithms they use in the example 
above.

Generally, as shown in the example above, common names (e.g., 
"container_mr") don't provide common semantics (e.g., different hash, or 
different measurements may be extended to the same or different MRs on 
different archs), so we avoid using them. A full solution would require 
a log-oriented ABI and a virtual measurement stack. We're laying the 
groundwork for this today.

>>
>> *Crypto Agility* is supported by merging independent MRs with a common name
>> into a single directory, each represented by its HASH/digest file. Note
>> that HASH must be distinct or behavior is undefined.
> 
> Ditto.  I think it would be more helpful if you can provide _why_ we need to
> support crypto agility rather than _how_ is it implemented, which can be seen
> from the actual code.  Once merged, the _why_ will be helpful when some random
> guy in the future tries to git blame and figure out the story behind.
> 
The reason for crypto agility is to allow introducing new hash 
algorithms without sacrificing compatibility. It's a lessen learned from 
the TPM 1.x to 2.x transition. I thought it was obvious, but will add 
clarification in the next revision.

-Cedric

Powered by blists - more mailing lists