* Brijesh Singh (brijesh.singh@amd.com) wrote:

I see that Tom answered few comments. I will cover others.


On 6/9/21 2:24 PM, Dr. David Alan Gilbert wrote:
+ /*

quoted

quoted

+	 * The message sequence counter for the SNP guest request is a 64-bit value
+	 * but the version 2 of GHCB specification defines the 32-bit storage for the
+	 * it.
+	 */
+	if ((count + 1) >= INT_MAX)
+		return 0;

Is that UINT_MAX?

Good catch. It should be UINT_MAX.

OK, but I'm also confused by two things:
  a) Why +1 given that Tom's reply says this gets incremented by 2 each
time (once for the message, once for the reply)
  b) Why >= ? I think here is count was INT_MAX-1 you'd skip to 0,
skipping INT_MAX - is that what you want?

quoted

+	/*
+	 * The secret page contains the VM encryption key used for encrypting the
+	 * messages between the guest and the PSP. The secrets page location is
+	 * available either through the setup_data or EFI configuration table.
+	 */
+	if (hdr->cc_blob_address) {
+		paddr = hdr->cc_blob_address;
Can you trust the paddr the host has given you or do you need to do some
form of validation?

The paddr is mapped encrypted. That means that data  in the paddr must
be encrypted either through the guest or PSP. After locating the paddr,
we perform a simply sanity check (32-bit magic string "AMDE"). See the
verify header check below. Unfortunately the secrets page itself does
not contain any magic key which we can use to ensure that
hdr->secret_paddr is actually pointing to the secrets pages but all of
these memory is accessed encrypted so its safe to access it. If VMM
lying to us that basically means guest will not be able to communicate
with the PSP and can't do the attestation etc.

OK; that nails pretty much anything bad that can happen - I was just
thinking if the host did something odd like give you an address in the
middle of some other useful structure.

Dave

quoted

Dave
+	} else if (efi_enabled(EFI_CONFIG_TABLES)) {
+#ifdef CONFIG_EFI
+		paddr = cc_blob_phys;
+#else
+		return -ENODEV;
+#endif
+	} else {
+		return -ENODEV;
+	}
+
+	info = memremap(paddr, sizeof(*info), MEMREMAP_WB);
+	if (!info)
+		return -ENOMEM;
+
+	/* Verify the header that its a valid SEV_SNP CC header */
+	if ((info->magic == CC_BLOB_SEV_HDR_MAGIC) &&
+	    info->secrets_phys &&
+	    (info->secrets_len == PAGE_SIZE)) {
+		res->start = info->secrets_phys;
+		res->end = info->secrets_phys + info->secrets_len;
+		res->flags = IORESOURCE_MEM;
+		snp_secrets_phys = info->secrets_phys;
+		ret = 0;
+	}
+
+	memunmap(info);
+	return ret;
+}
+

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK