Re: [PATCH 1/4] tsm: Add TVM Measurement Register support

[Huang, Kai]

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

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

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

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

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

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

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

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.