Message-ID: <10ab62c7-d2fc-4014-a235-700bef017a3e@intel.com>
Date: Mon, 17 Feb 2025 14:57:01 -0600
From: "Xing, Cedric" <cedric.xing@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>, Dan Williams
	<dan.j.williams@...el.com>, "Kirill A. Shutemov"
	<kirill.shutemov@...ux.intel.com>, Dave Hansen <dave.hansen@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>, <x86@...nel.org>, "H. Peter Anvin"
	<hpa@...or.com>
CC: <linux-kernel@...r.kernel.org>, <linux-coco@...ts.linux.dev>
Subject: Re: [PATCH 1/4] tsm: Add TVM Measurement Register support

Hi Kai,

On 2/16/2025 6:17 PM, Huang, Kai wrote:
> Hi Cedric,
> 
> [...]
> 
>> +static ssize_t tmr_digest_read(struct file *filp, struct kobject 
>> *kobj, struct bin_attribute *attr,
>> +                   char *page, loff_t off, size_t count)
> 
> Better to rename 'page' to 'buffer'?
> 
> Since page normally implies 4KB alignment but I don't see we need the 
> alignment here.
> 

'page' was used here to imply the size of the buffer cannot exceed a 
page (which is the current behavior of the kernel). But I agree with you 
and will make the changes.

[...]

> The logic around using pvd->in_sync is kinda complicated.  MR operations 
> seem like a classic reader/writer contention problem and I am not sure 
> why pvd->in_sync is needed.  Could you help to clarify?
> 
If in_sync is true, then "refresh()" will NOT be invoked on reads from 
"live" MRs.

For example, on TDX, if an RTMR has NOT been extended since the last 
read, then the next read will return the cached copy of the RTMR value - 
i.e., saving a "refresh()" call (which must issue TDCALL[TDG.MR.REPORT] 
to reread all MRs and can be slow).

> [...]
> 
>> +
>> +/**
>> + * struct tsm_measurement_register - describes an architectural 
>> measurement register (MR)
>> + * @mr_name: name of the MR
>> + * @mr_value: buffer containing the current value of the MR
>> + * @mr_size: size of the MR - typically the digest size of @mr_hash
>> + * @mr_flags: bitwise OR of flags defined in enum 
>> tsm_measurement_register_flag
>> + * @mr_hash: optional hash identifier defined in include/uapi/linux/ 
>> hash_info.h
>> + *
>> + * A CC guest driver provides this structure to detail the 
>> measurement facility supported by the
>> + * underlying CC hardware. After registration via 
>> `tsm_register_measurement`, the CC guest driver
>> + * must retain this structure until it is unregistered using 
>> `tsm_unregister_measurement`.
>> + */
>> +struct tsm_measurement_register {
>> +    const char *mr_name;
>> +    void *mr_value;
>> +    u32 mr_size;
>> +    u32 mr_flags;
>> +    enum hash_algo mr_hash;
>> +};
>> +
>> +/**
>> + * 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. And if that's the case, RTMRs may be marked RO ("X w/o 
W") to prevent direct extension.

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

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

F is for CC guest to expose (pseudo) MRs that may not have an associated 
hash algorithm (e.g., `report0` on TDX).

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.

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

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.

-Cedric

Powered by blists - more mailing lists