Re: [PATCH Part1 v5 35/38] x86/sev: Register SNP guest request platform device

On 02/09/2021 22:58, Brijesh Singh wrote:

On 9/2/21 11:40 AM, Borislav Petkov wrote:

[...]

quoted

quoted

+static u64 find_secrets_paddr(void)
+{
+    u64 pa_data = boot_params.cc_blob_address;
+    struct cc_blob_sev_info info;
+    void *map;
+
+    /*
+     * The CC blob contains the address of the secrets page, check
if the
+     * blob is present.
+     */
+    if (!pa_data)
+        return 0;
+
+    map = early_memremap(pa_data, sizeof(info));
+    memcpy(&info, map, sizeof(info));
+    early_memunmap(map, sizeof(info));
+
+    /* Verify that secrets page address is passed */

That's hardly verifying something - if anything, it should say

    /* smoke-test the secrets page passed */

Noted.

quoted

quoted

+    if (info.secrets_phys && info.secrets_len == PAGE_SIZE)
+        return info.secrets_phys;

... which begs the question: how do we verify the HV is not passing some
garbage instead of an actual secrets page?

Unfortunately, the secrets page does not contain a magic header or uuid
which a guest can read to verify that the page is actually populated by
the PSP. 

In the SNP FW ABI document section 8.14.2.5 there's a Table 61 titled
Secrets Page Format, which states that the first field in that page is a
u32 VERSION field which should equal 2h.

While not as strict as GUID header, this can help detect early that the
content of the SNP secrets page is invalid.

-Dov

But since the page is encrypted before the launch so this page
is always accessed encrypted. If hypervisor is tricking us then all that
means is guest OS will get a wrong key and will not be able to
communicate with the PSP to get the attestation reports etc.

quoted

I guess it is that:

"SNP_LAUNCH_UPDATE can insert two special pages into the guest’s
memory: the secrets page and the CPUID page. The secrets page contains
encryption keys used by the guest to interact with the firmware. Because
the secrets page is encrypted with the guest’s memory encryption
key, the hypervisor cannot read the keys. The CPUID page contains
hypervisor provided CPUID function values that it passes to the guest.
The firmware validates these values to ensure the hypervisor is not
providing out-of-range values."

 From "4.5 Launching a Guest" in the SNP FW ABI spec.

I think that explanation above is very important wrt to explaining the
big picture how this all works with those pages injected into the guest
so I guess somewhere around here a comment should say

I will add more explanation.

quoted

"See section 4.5 Launching a Guest in the SNP FW ABI spec for details
about those special pages."

or so.