Re: [PATCH] qapi, i386/sev: Add debug-launch-digest to launch-measure response

[Dov Murik]

On 31/01/2022 13:44, Daniel P. Berrangé wrote:
> On Mon, Jan 31, 2022 at 11:15:39AM +0000, Dov Murik wrote:
>> Currently the responses of QMP commands query-sev-launch-measure and
>> query-sev-attestation-report return just the signed measurement. In
>> order to validate it, the Guest Owner must know the exact guest launch
>> digest, besides other host and guest properties which are included in
>> the measurement.
>>
>> The other host and guest details (SEV API major, SEV API minor, SEV
>> build, and guest policy) are all available via query-sev QMP command.
>> However, the launch digest is not available.  This makes checking the
>> measurement harder for the Guest Owner, as it has to iterate through all
>> allowed launch digests and compute the expected measurement.
> 
> So more specifically to validate the measurement, the guest owner is
> currently calculating:
> 
>    expected_measurement = HMAC(0x04 || API_MAJOR || API_MINOR || BUILD ||
>                                GCTX.POLICY || GCTX.LD || MNONCE; GCTX.TIK)
> 
> where GCTX.LD is
> 
>     SHA256(FIRMWARE-CODE || KERNEL-HASHES-TABLE || VMSA-FOREACH-VCPU)
> 
> and comparing expected_measurement to the actual measurement from
> query-sev-launch-measure.
> 
>> Add a new field debug-launch-digest to the response of
>> query-sev-launch-measure and query-sev-attestation-report which includes
>> the guest launch digest, which is the SHA256 of all initial memory added
>> to the guest via sev_launch_update_data().
> 
> So this new 'debug-launch-digest' field is returning GCTX.LD value
> above.
> 

Yes. Maybe rename it to gctx-ld ?

hmmm, except at the moment it doesn't include the VMSAs, because I don't
know an easy way to do it within QEMU :-( .  So the content of
debug-launch-digest is currently incorrect for SEV-ES guests.



>> Note that the value of debug-launch-digest should not be used for
>> verifying the measurement, because it is not signed.  Hence the choice
>> of the 'debug-' prefix for the field's name.
> 
> The earlier paragraph talks about making it easier for the guest
> owner to verify the measurement, but here is saying this new field
> should not be used to verify the measurement.
> 
> So I'm confused as to what is the benefit of returning this info ?
> 
> Due to the lack of guarantees about this data, it can't be used
> for a real production use case. AFAIK it can only be useful if
> debugging your code logic used for validating measurwements.
> Am I missing something about the motivation here ?
> 
> 
> If the guest owner is comparing expected and actual measurements
> and they get a mis-match, all they'll see is two big hex strings
> representing the HMAC value, and they'll have to work backwards
> to figure out whether one of their expected inputs had a mistake,
> or their algorithm was buggy.
> 
> If the guest owner is comparing the expectd and actual launch
> digest and they get a mis-match, again they'll just huave two
> big hex strings representing the SHA256 value, and they'll have
> to work backwards to figure out whether one of their expected
> inputs had a mistake, or their algorithm was buggy.
> 
> By having this 'debug-launch-digest' field, you can slightly
> more quickly determine whether your mistake lies in your impl
> of the HMAC alg, or API_MAJOR || API_MINOR || BUILD || GCTX.POLICY
> values, vs a mistake in your calc of the debug-launch-digest
> field. Basically it gives you one step in bisecting the mistake
> location.
> 

The scenario we are encountering is that there's not one allowed
assignment to the parameters, but rather a more relaxed policy along the
lines of:

API_MAJOR.API_MINOR should be >= 12.34
BUILD should be >= 66
GCTX.POLICY should be 0x0 or 0x2
GCTX.LD can be one of these allowed OVMFs: {hash-a, hash-b, hash-c}


Having the values of API*, BUILD, POLICY from query-sev is very
comfortable for verifying they are in the allowed ranges.  But since the
Guest Owner doesn't have GCTX.LD, they have to compute the measurement
for each of the allowed OVMF images.  Once the values are known and
"allowed", then the HMAC must be computed to see that the signed
measurement does indeed match.

Note: I guess that adding the hashes of kernel/initrd/cmdline here might
help too (for direct boot with kernel-hashes=on), and maybe the hash of
OVMF itself (on its own).

More generally: within space/network constraints, give the Guest Owner
all the information it needs to compute the launch measurement.  There's
a problem with OVMF there (we don't want to send the whole 4 MB over the
QMP response, but given its hash we can't "extend" it to include the
kernel-hashes struct).



> Is that really compelling enough to justify adding this extra
> info to the QAPI commands ? IME of writing code to verify the SEV
> measurement, there were many little ways I screwed up at first
> and having this field would not have made a significant difference
> to my debugging efforts.

I think that adding the extra field is not that bad, even if it's useful
just for some cases.

I'll be glad to get rid of the launch_memory buffer in my implementation
and replace it with a SHA256-context variable (but found none in QEMU).
 Other than that I don't think the suggested addition is too bad (in
fact, I would add some more info like kernel hashes.).


> 
> What was/would have been useful was having a known good reference
> implementation of the algorithm which dumped out all the key
> values for the different steps of process. I used James Bottomley's
> python script as my reference and hacked it to dump out key values
> so I could see what step I went wrong in. I was still working blind
> for doing the SEV-ES VMSA and kernel hashes table work.

Another point of reference is the session::verify() function in the sev
crate [1], but it doesn't deal with VMSAs and kernel-hashes either.

[1] https://github.com/enarx/sev/blob/main/src/session/mod.rs#L122


-Dov