Re: SVSM Attestation and vTPM specification additions - v0.60

[Christophe de Dinechin <dinechin@redhat.com>]

Hi Tom,


On 2023-01-10 at 12:54 -06, Tom Lendacky <thomas.lendacky@amd.com> wrote...
> Attached is an updated draft version of the SVSM specification with added
> support for an attestation protocol and a vTPM protocol as well as other
> miscellaneous changes (all identified by change bar). Please take a look and
> reply with any feedback you may have.
>
> Thanks,
> Tom

Thanks for sharing.

This is the first time I actually review that document, so my feedback will
be a bit longer than most. Also, I read it at a time where I had lost
network access to Internet, so RTFM wasn't an option...

First, the actual errors:
p9: Typo VMLP1+ instead of VMPL1+
p18: "bit1=0" and "bit1=1": That seems to be bit 2

Then the more mundane comments

p9: "expected that, but not limited to": the wording sounds strange to me
p9: Undefined acronym (*) VMSA

p10: "certain forms of RMPADJUST": The body of the document seems to
     indicate that the required RMPADJUST are performed as part of the
     various other services. There is no explicit need (apparently) for a
     separate guest-accessible RMPADJUST. Maybe expand a little bit on this
     topic, and explain if the guest is supposed to do any RMPADJUST if
     running at VMPL1.
p10: gPA space of the guest: "of the guest" seems redundant, since it's gPAs
p10: lower VMPL -> less privileged VMPL, or explain that "lower" privilege
     levels have a higher number.
p10: "The initial SVSM memory configuration...": Unclear what "required"
     means in that sentence. Is that the core protocol? Can "create VCPU"
     request additional memory, for example?

p11: No explanation of how the SVSM knows where the secrets page is.
     Probably need an xref to some other doc.
p11: Who does the initial construction of the secrets page? What happens if
     that other actor does not write zeroes? What attacks can the host
     perform on the secrets page if any?
p11: undefined acronym VMPCK
p11: Why should the guest OS "capture" the SVSM_CAA value? In what sense?
     Is it because it can change afterwards, or because the secrets page
     becomes unavailable, or for another reason?
p11: Byte offset in secrets page fields starts at 0x140. Explain why this is
     safe, and how other possible users of the secrets page would avoid
     stomping on that area.

p12: How does the SVSM "terminate"? Is it a regular guest shutdown, or some
     other mechanism, or is unspecified? Can the SVSM log anything, and if
     so, where would it be found? (I assume that would be host platform
     specific, but would still be noteworthy in this specification)

p13: "Use of the Calling Area is necessary..." effectively, this is a single
     byte in the calling area, right? So it's not really "ensuring", maybe
     "detect" spurious invokation (1). I think malicious invokations are not
     made too difficult by this mechanism, since there are only two states,
     and the host still controls when vCPUs run.

p14: Undefined acronyms: GHCB, MSR
p14: I think GHCB Specification is an external reference, worthy having
     italics and a precise reference / link (can't check, no network ATM)
p14: "If the host illegally entered the SVSM, this field will be zero": I
     believe that the conditions enforcing this should be precisely spelled
     out, including for a host with malicious intent. If the mechanism is
     indeed robust, then we are not protecting against "spurious" calls but
     against "spurious or malicious" calls. Otherwise, "will be zero" should
     be replaced by "should normally be zero".
p14: "only after VMSA.RAX and SVSM_CALL_PENDING": this suggests that there
     is some kind of possible race condition here. If that is true, then
     maybe there is a need to specify memory ordering semantics on the three
     relevant fields?

p16: PVALIDATE: What happens if a guest is unaware of SVSM and executes
     PVALIDATE directly? Is the SVSM supposed to emulate that, or to punish
     the guest, or something else? Is that even possible for the SVSM to
     trap and emulate relevant VMPL1 instructions? Also see note on page 10
     regarding RMPADJUST.
p16: "It affects the Calling Area for calling vCPU only": This seems slighly
     inconsistent with page 9 "other SVSM implementations may choose a
     single execution context that services all guest VCPUs".
p16: Alignment for RCX is 8 bytes, but alignment for RDX in
     SVSM_CORE_CREATE_VCPU is 4K. Is that not the same calling area?
     Also, what is the use case for moving the calling area?

p17: The table links appear in a strange colour (some kind of weird cyan).
     It seems clickable too, so I suspect a hyperlink, but since the link is
     always on the same page, it's not super-useful.

p18: Is the CAA seen as assigned to the SVSM? I believe the answer is no
p18: For increased readability, I suggest naming the error codes for the
     SVSM_CORE_PVALIDATE call, and putting them in a table.
     Also, why tag the specific errors right after the architectural
     PVALIDATE errors? In case of architectural extension, you'd always get
     0x8000_1011, which is not super helpful. Instead, you could reserve
     0x8000_1xxx for protocol errors, and put PVALIDATE errors at
     0x9nnn_nnnn, which probably gives you enough room at least for the
     coming 6 months.
p18: "VMPL of the VCPU making the request": wouldn't it make sense to add a
     VMPL field in the PVALIDATE operation descriptor, so that the guest
     could control less-privileged VMPLs?
p18: As indicated earlier, I'm confused by the 4K alginment requirement for
     RDX (Calling Area gPA)
p18: What is the APIC ID of the vCPU used for? I see no mention in the
     explanatory text. Is that an internal index, or does the SVSM
     implementation need it for some reason (I was not clever enough to
     imagine why)

p19: What specification defines FAIL_INUSE? Add xref?

p20: Evidently, SVSM_CORE_DEPOSIT_MEM is intended to be used when another
     service returns 0x4mmm_mmmm. However, in the presence of a flag
     indicating "I may no longer need this memory", and given the
     limitations "cannot cross a page", I am concerned about possible lack
     of forward progress if two vCPUs start parallel operations where one
     vCPU says "Hey, I need X terabytes of RAM to do that" (which will then
     be split into umteen DEPOSIT_MEM calls due to page limit), while
     another says "Hey guys, I'm done" and sets the MEM_AVAILABLE flag. I
     see nothing in the spec that would prevent the second CPU from actively
     withdrawing the memory that the first one is trying to deposit. I think
     that the spec should clarify a forward-progress logic that prevents
     that from happening.
p20: It would be interesting to have at least a vague idea of what
     operations can actually request more memory, just to set expectations.
p20: Suppose that CREATE_VCPU requests more memory. It has no obvious
     "restart" field, unlike things like PVALIDATE. That means that there
     should be a rather strong guarantee that all SVSM calls that can
     potentially return 0x4mmm_mmmm either have no effect when they return
     such a request, or are idempotent if called again after providing more
     memory.

p21: The MEM_AVAILABLE flag is set in the calling area of the startup vCPU.
     Is there any requirement that WITHDRAW_MEM should only be called from
     the startup vCPU, or from only one vCPU at a time?

p22: The writable area ends at a page boundary. What could be a vald
     rationale for setting the RCX pointer in the middle of a page?
     Maybe simpler to require that the pointer be page-aligned than have a
     spec that mentions page offset 0xFF8 as a special case...
p22: Rationale for not returning incomplete? I'm trying to see how the guest
     could efficiently let secondary vCPUs withdraw memory with the protocol
     as specified, without a little additional wording regarding either the
     memory semantics of the MEM_AVAILABLE flag, and telling if there is
     indeed more work to be done by this vCPU using SVSM_ERR_INCOMPLETE.

p23: Table 9 is mnissing RDX, R8 and R9 rows (as input for configuration)
p23: Table 9 title should be "configuration or query"
p23: RCX result is the same for query and configuration, or is that for
     query only?
p23: At the specified RIP. If Bit 3 is not set in the configuration case,
     does it return after the VMGEXIT?

p29: Table 14: How can RAX be used as command ordinal and command response
     size if it's already used for call identifier / result value?

p30: Why would you need 4 bytes for the TPM command ordinal? This causes the
     TPM command size to be misaligned. What about 2 bytes for command
     ordinal, one byte for locality, and one reserved byte?


(*) Like most readers of this document, I know what it means, but since you
defined VMM just above, or VM the page before, I interpreted your intent to
be that every acronym should be defined on first use.

(1) As I am writing this, I have a doubt what happens if the host writes to
    the secrets page, and can't verify easily without a network.

>
> [2. application/pdf; 58019-Secure_VM_Service_Module_Specification.pdf]...


--
Cheers,
Christophe de Dinechin (https://c3d.github.io)
Theory of Incomplete Measurements (https://c3d.github.io/TIM)