Message-ID: <f031cf8e34ef82649e32c8060ef4a42a2e185783.camel@intel.com>
Date: Wed, 12 Mar 2025 23:11:57 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "bp@...en8.de" <bp@...en8.de>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "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>, "Xing, Cedric" <cedric.xing@...el.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>, "dan.middleton@...ux.intel.com"
	<dan.middleton@...ux.intel.com>, "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>
Subject: Re: [PATCH v2 1/4] tsm: Add TVM Measurement Register support

On Wed, 2025-03-12 at 13:26 -0500, Xing, Cedric wrote:
> Hi Kai,
> 
> Thanks for your comments and my apologies for my late response!

Thanks for replying back!

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

This makes sense.  Could you put those into the changelog?

I still have a slight concern (more like a question) though:

From attestation's point of view, ultimately, those MRs serves the same purpose
as the static ones -- to be included in a verifiable attestation report to
provide trustiness.  I think in most use cases the runtime MRs will be just
extended once at early stage, e.g., during system boots (since they are global
as you mentioned above).  And after that, they will just be read by applications
(e.g., containers).

So the question is whether do we see any requirement for containers to *read*
those MRs independently w/o the full report? From attestation's point of view, I
don't think there is, because those MRs alone are not verifiable.  But I am not
sure in your mind whether there's other use cases in which providing MRs for
read in configfs-tsm would be helpful?

> 
> 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; 
> 

I don't quite follow what's the value of leaving report0 w/o reportdata.

> 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; 
> 

Ditto. W/o reportdata, what value are you going to fill into the report0?

I think the confusion is _why_ do you want to provide the full report0 via
sysfs?  Is it for local verification, presumably?  In which case, probably you
don't need to care about the reportdata?

I can understand the purpose of exporting runtime MRs, I can even understand
(sort of) the purpose of exporting other files like 'mrowner' etc, but I am not
sure the purpose of exporting report0.

> or (3) take away report0/reportdata altogether. Which one 
> do you think is the most reasonable? 
> 

3) Seems more reasonable to me, but I am not certain because I don't fully
understand the purpose (use case).

> In all cases, I'll incorporate 
> Mikko's patch into this series to allow per-container TDREPORT under 
> configfs.

Sorry I might have missed, where can I find this patch?

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

I think it is still valid question that whether we need to make those MRs
consistent for all vendors for the purpose of providing a unified ABI to
userspace.

IIUC, Dan has been wanting unified ABIs around attestation.  It would be great
if Dan can provide guidance here.

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

I interpret this as there's no requirement for containers to *read* those MRs
independently via configfs-tsm. :-)

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

Yeah agreed.  But eventually they are for attestation, right?

> They are vendor specific because they are _indeed_ vendor 
> specific. The intention is to unlock access to all of them for user 
> mode. 
> 

Agreed.

> The semantics (i.e., which MR stores what measurement) is 
> application specific and will be assigned by the application.

This doesn't mean the kernel shouldn't provide a unified ABI to userspace
AFAICT.

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

OK.

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

I think the number is fine.  E.g., in the above case, the application could have
a policy to map a given container measurement to one RTMR (e.g., container0 ->
rtmr0 and so on).

And I am not sure why hash algorithm matters?  If needed, there could be a
policy to query the hash algorithm for a given RTMR and feed extended data based
on the algo in each loop.

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

As above, I don't think I am convinced that a unified ABI doesn't work, or isn't
necessary.

Again, no blocker from me, but I am hoping Dan can provide guidance here.

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

Thanks.

It will be definitely useful for newbies like me :-)

> 
> -Cedric

Powered by blists - more mailing lists