Message-ID: <85eb9541-f33a-4bef-8bfa-d1394e850a7b@intel.com>
Date: Tue, 18 Feb 2025 12:13:35 -0600
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: "linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] tsm: Add TVM Measurement Register support

On 2/18/2025 3:14 AM, Huang, Kai wrote:
> 
>>>> +/**
>>>> + * enum tsm_measurement_register_flag - properties of an MR
>>>> + * @TSM_MR_F_X: this MR supports the extension semantics on write
>>>> + * @TSM_MR_F_W: this MR is writable
>>>
>>> Why a MR can be written w/o being extended?  What is the use case of this?
>>>
>>
>> This is because "write" may not be the only way to extend an RTMR. For
>> example, the current ABI proposed by this patch can be considered "MR
>> centric", meaning it's the application that takes care of what to hash,
>> using what algorithm, and which RTMR to extend.
>>
> 
> [...]
> 
>> However, theoretically,
>> applications should only be concerned the integrity of some sequence of
>> events (the event log). Therefore, there could be a "log centric" ABI
>> that allows applications to integrity-protect its logs in a CC-arch
>> agnostic manner.
>>
> 
> I agree "log centric" ABI could be useful.  I don't know a lot of the format of
> "event log", but I am thinking that making sure "integrity of some sequence of
> events" may not be good enough -- we actually need to make sure "integrity of
> each component" that get involved in those events.
> 
By "some sequence of events", I was talking from the perspective of an 
application. Different applications will generate independent sequences 
of events and because different CC archs offer different types and 
numbers of MRs that use different hash algorithms, what we need is a SW 
arch that can "pack" those sequences into something that can be 
integrity-protected by the HW resource from the underlying CC arch, then 
"unpack" it back to independent sequences to be verified/appraised by 
potentially independent entities. The "log centric" ABI will be part of 
such an architecture. This is too big a topic to solve by this patch. 
For now, what matters is just not to create roadblocks.

> E.g., a guest wants to load some particular kernel module during boot and wants
> to make sure the correct one gets loaded.  Userspace can trigger an event of
> "loading that module" and get this *event log* verified.  But w/o getting the
> kernel module binary itself measured as part of this step, we cannot really be
> sure whether this step is compromised or not.  In this case, the userspace may
> still need to (write and) extend the MR(s).
> 
By the term "event", it usually implies accompanying "event data", which 
could be/contain the measurements of the kernel modules.

You are talking about actual uses of RTMRs and this patch aims to enable 
those exact uses. :-)

>> And if that's the case, RTMRs may be marked RO ("X w/o
>> W") to prevent direct extension.
> 
> Sorry I don't quite follow why RO is enough for "log centric" ABI.  Could you
> elaborate a bit?
> 
My apologies for the confusion again! R/W controls the file permissions 
of the sysfs attributes, while X is the semantics of the MR. Clearing W 
prevents direct "write" to the attribute (by user mode) but doesn't 
prevent the MR from being extended through other interfaces. It isn't 
obvious in this patch because the "log centric" ABI has been taken out. 
It was originally proposed in 
https://lore.kernel.org/all/20240907-tsm-rtmr-v1-0-12fc4d43d4e7@intel.com/

>>
>> The use of "W w/o X" is to support pseudo-MRs. For example, `reportdata`
>> is such a pseudo-MR that is W but not X. So an application can request a
>> TDREPORT by a write to `reportdata` followed by a read from `report0`.
> 
> I am a little bit confused.  This series is about exposing "measurement
> registers" to userspace, so I thought there should be at least some
> "measurement" get involved for any entry that is report to userspace.
> 
> 'reportdata' is more like the nonce embedded to the attestation report, and it
> doesn't involve any measurement.
> 
> I can see why you want to expose 'reportdata' to userspace, but calling
> 'reportdata' as measurement register seems unfit.
> 
Glad that you see why! I called it a pseudo-MR but there could be better 
names. The intention of this patch is to create a sysfs tree that offers 
all measurement related functionalities to user mode applications. 
`reportdata` plays a role here because it usually carries the digest of 
something that the guest wants to attest to.

>>
>>>> + * @TSM_MR_F_R: this MR is readable. This should typically be set
>>>> + * @TSM_MR_F_L: this MR is live - writes to other MRs may change this MR
>>>
>>> Why one MR can be changed by writing to other MRs?
>>>
>>
>> Good catch! I'll fix the comment.
>>
>>>> + * @TSM_MR_F_F: present this MR as a file (instead of a directory)
>>>> + * @TSM_MR_F_LIVE: shorthand for L (live) and R (readable)
>>>> + * @TSM_MR_F_RTMR: shorthand for LIVE and X (extensible)
>>>> + */
>>>> +enum tsm_measurement_register_flag {
>>>> +    TSM_MR_F_X = 1,
>>>> +    TSM_MR_F_W = 2,
>>>> +    TSM_MR_F_R = 4,
>>>> +    TSM_MR_F_L = 8,
>>>> +    TSM_MR_F_F = 16,
>>>> +    TSM_MR_F_LIVE = TSM_MR_F_L | TSM_MR_F_R,
>>>> +    TSM_MR_F_RTMR = TSM_MR_F_LIVE | TSM_MR_F_X,
>>>> +};
>>>
>>> I am not sure whether we need so many flags.  To me seems like we only
>>> need:
>>>
>>>    - TSM_MR_ENABLED:  The MR has been initialized with a certain algo.
>>>    - TSM_MR_UNLOCKED: The MR is writable and any write will extend it.
>>>    - TSM_MR_LOCKED:   The MR is locked and finalized.
>>>
>>
>> W/X are independent and both necessary (see my previous explanation on
>> "X w/o W" and "W w/o X").
>>
>> I'm not sure if there are non-readable MRs. But theoretically,
>> applications inside a TVM (CC guest) may not need to read any MR values.
>> Therefore, there could be CC archs (in future) that do not support
>> reading all MRs within a guest. And because of that, I decided to keep R
>> as an independent bit.
> 
> [...]
> 
>>
>> L is to indicate an MR's value may not match its last write.
> 
> "L" doesn't seem to be able to reflect the MR value is out-of-sync. :-)
> 
> What does it stand for?
> 
L stands for Live. It indicates to the TSM MR core that the value of the 
MR is different than the last value written, hence should issue 
refresh() to read the value back from HW (instead of using the cached 
value obtained from the last write).

>>
>> F is for CC guest to expose (pseudo) MRs that may not have an associated
>> hash algorithm (e.g., `report0` on TDX).
> 
> OK.  But my thinking is such MR actually isn't MR at all.
> 
Besides Measurement Register, MR can also stand for MeasuRement. Not to 
debate what MR stands for, but this patch aims to capture/expose all 
measurement related functionalities. `report0` offers less understood 
yet important info beyond measurement registers, such as CPUSVN and 
measurements of the TDX module, so is considered within the scope.

>>
>> LOCKED/UNLOCKED, from attestation perspective, is NOT a functional but a
>> verifiable security property, which is usually implemented by extending
>> a special token to the RTMR.
>>
>>> The TSM_MR_ENABLED may not be needed either, but I think it's better to
>>> have it so that the kernel can reject both read/write from userspace.
>>>
>> I'm not sure what a "disabled" MR is and its implication from
>> attestation perspective.
> 
> I was thinking from the perspective that userspace may only be interested in one
> particular MR.  If that MR is not used, I suppose it should have default value
> 0.  But I was thinking that "refusing userspace to read" may be better than
> "returning 0 to userspace" for a particular MR, if it is not used.
> 
I'd try not to predict what applications need but focus on what the HW 
offers. HW features are usually motivated by specific usages, but their 
actual uses are usually way beyond.

Moreover, the traditional Linux file ownership/permissions can satisfy 
this need very easily.

> But from attestation's perspective, I tend to agree with you that "disabled MR"
> may not be helpful.  We need to send the whole attestation report to the
> verifier anyway and the verifier should only care about whether the MR values in
> the report matches what the verifier knows.
> 
>>
>>>> +
>>>> +#define TSM_MR_(mr,
>>>> hash)                                                           \
>>>> +    .mr_name = #mr, .mr_size = hash##_DIGEST_SIZE, .mr_hash =
>>>> HASH_ALGO_##hash, \
>>>> +    .mr_flags = TSM_MR_F_R
>>>> +
>>>> +/**
>>>> + * struct tsm_measurement - define CC specific MRs and methods for
>>>> updating them
>>>> + * @name: name of the measurement provider
>>>> + * @mrs: array of MR definitions ending with mr_name set to %NULL
>>>> + * @refresh: invoked to update the specified MR
>>>> + * @extend: invoked to extend the specified MR with mr_size bytes
>>>> + */
>>>> +struct tsm_measurement {
>>>> +    const char *name;
>>>> +    const struct tsm_measurement_register *mrs;
>>>> +    int (*refresh)(struct tsm_measurement *tmr, const struct
>>>> tsm_measurement_register *mr);
>>>> +    int (*extend)(struct tsm_measurement *tmr, const struct
>>>> tsm_measurement_register *mr,
>>>> +              const u8 *data);
>>>> +};
>>>
>>>   From the description above, I don't quite follow what does ->refresh()
>>> do exactly.  Could you clarify why we need it?
>>
>> I'll fix the comment.
> 
> Thanks.
> 
>>
>> Basically, refresh() brings all cached MR values up to date. The
>> parameter `mr` indicate which MR that has triggered the refresh. On TDX,
>> the 1st read after a write to any RTMR will trigger refresh() to reread
>> all MRs by TDG.MR.REPORT, while subsequent reads will simply return the
>> cached values until the next write to any RTMRs.
> 
> Yeah.  I also think adding some comments around the code using pvd->in_sync
> would be helpful, as I mentioned in another reply.
> 

Powered by blists - more mailing lists