On Fri, Nov 11, 2022 at 9:46 AM Borislav Petkov <bp@xxxxxxx> wrote: > > On Thu, Nov 03, 2022 at 08:23:18AM -0700, Peter Gonda wrote: > > The ASP and an SNP guest use a series of AES-GCM keys called VMPCKs to > > ASP? > > That must be the AMD Secure Processor or so but pls write it out. > > > communicate securely with each other. The IV to this scheme is a > > sequence number that both the ASP and the guest track. Currently this > > sequence number in a guest request must exactly match the sequence > > number tracked by the ASP. This means that if the guest sees an error > > from the host during a request it can only retry that exact request or > > disable the VMPCK to prevent an IV reuse. AES-GCM cannot tolerate IV > > reuse see: > > https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf > > Right, how stable will that link be? > > IOW, perhaps quote the paper name and authors so that people can find it > on their own. > > > To handle userspace querying the cert_data length handle_guest_request() > > now: saves the number of pages required by the host, retries the request > > This needs to sound like this: > > "In order to address this, save the number of pages ..." > > IOW, as the docs say: > > "Describe your changes in imperative mood, e.g. "make xyzzy do frotz" > instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > to do frotz", as if you are giving orders to the codebase to change > its behaviour." Thanks for detailed review. Working on cleaning up the text for a V5. > > > without requesting the extended data, then returns the number of pages > > required. > > > > Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver") > > I'm guessing this needs to go to stable? > > > Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx> > > Reported-by: Peter Gonda <pgonda@xxxxxxxxxx> > > Cc: Dionna Glaze <dionnaglaze@xxxxxxxxxx> > > Cc: Borislav Petkov <bp@xxxxxxx> > > Cc: Tom Lendacky <thomas.lendacky@xxxxxxx> > > Cc: Michael Roth <michael.roth@xxxxxxx> > > Cc: Haowen Bai <baihaowen@xxxxxxxxx> > > Cc: Yang Yingliang <yangyingliang@xxxxxxxxxx> > > Cc: Marc Orr <marcorr@xxxxxxxxxx> > > Cc: David Rientjes <rientjes@xxxxxxxxxx> > > Cc: Ashish Kalra <Ashish.Kalra@xxxxxxx> > > Cc: linux-kernel@xxxxxxxxxxxxxxx > > Cc: kvm@xxxxxxxxxxxxxxx > > --- > > Tested by placing each of the guest requests: attestation quote, > > extended attestation quote, and get key. Then tested the extended > > attestation quote certificate length querying. > > > > V4 > > * As suggested by Dionna moved the extended request retry logic into > > the driver. > > * Due to big change in patch dropped any reviewed-by tags. > > > > --- > > drivers/virt/coco/sev-guest/sev-guest.c | 70 +++++++++++++++++++------ > > 1 file changed, 53 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > > index f422f9c58ba79..7dd6337ebdd5b 100644 > > --- a/drivers/virt/coco/sev-guest/sev-guest.c > > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > > @@ -41,7 +41,7 @@ struct snp_guest_dev { > > struct device *dev; > > struct miscdevice misc; > > > > - void *certs_data; > > + u8 (*certs_data)[SEV_FW_BLOB_MAX_SIZE]; > > struct snp_guest_crypto *crypto; > > struct snp_guest_msg *request, *response; > > struct snp_secrets_page_layout *layout; > > @@ -67,8 +67,27 @@ static bool is_vmpck_empty(struct snp_guest_dev *snp_dev) > > return true; > > } > > > > +/* > > + * If we receive an error from the host or ASP we have two options. We can > > Please use passive voice in your commit message: no "we" or "I", etc, > and describe your changes in imperative mood. > > Bottom line is: personal pronouns are ambiguous in text, especially with > so many parties/companies/etc developing the kernel so let's avoid them > please. > > > + * either retry the exact same encrypted request or we can discontinue using the > > + * VMPCK. > > + * > > + * This is because in the current encryption scheme GHCB v2 uses AES-GCM to > > + * encrypt the requests. The IV for this scheme is the sequence number. GCM > > + * cannot tolerate IV reuse. > > + * > > + * The ASP FW v1.51 only increments the sequence numbers on a successful > > + * guest<->ASP back and forth and only accepts messages at its exact sequence > > + * number. > > + * > > + * So if we were to reuse the sequence number the encryption scheme is > > + * vulnerable. If we encrypt the sequence number for a fresh IV the ASP will > > + * reject our request. > > + */ > > static void snp_disable_vmpck(struct snp_guest_dev *snp_dev) > > { > > + dev_alert(snp_dev->dev, "Disabling vmpck_id: %d to prevent IV reuse.\n", > > + vmpck_id); > > memzero_explicit(snp_dev->vmpck, VMPCK_KEY_LEN); > > snp_dev->vmpck = NULL; > > } > > @@ -323,32 +342,49 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in > > > > /* Call firmware to process the request */ > > rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > > + > > + /* > > + * If the extended guest request fails due to having to small of a > > "... too small... " > > > + * certificate data buffer retry the same guest request without the > > + * extended data request. > > + */ > > + if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST && > > + err == SNP_GUEST_REQ_INVALID_LEN) { > > + const unsigned int certs_npages = snp_dev->input.data_npages; > > + > > + exit_code = SVM_VMGEXIT_GUEST_REQUEST; > > + rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > > + > > + err = SNP_GUEST_REQ_INVALID_LEN; > > Huh, why are we overwriting err here? > > > + snp_dev->input.data_npages = certs_npages; > > + } > > + > > if (fw_err) > > *fw_err = err; > > > > - if (rc) > > - return rc; > > + if (rc) { > > + dev_alert(snp_dev->dev, > > + "Detected error from ASP request. rc: %d, fw_err: %llu\n", > > + rc, *fw_err); > > + goto disable_vmpck; > > + } > > > > - /* > > - * The verify_and_dec_payload() will fail only if the hypervisor is > > - * actively modifying the message header or corrupting the encrypted payload. > > - * This hints that hypervisor is acting in a bad faith. Disable the VMPCK so that > > - * the key cannot be used for any communication. The key is disabled to ensure > > - * that AES-GCM does not use the same IV while encrypting the request payload. > > - */ > > rc = verify_and_dec_payload(snp_dev, resp_buf, resp_sz); > > if (rc) { > > dev_alert(snp_dev->dev, > > - "Detected unexpected decode failure, disabling the vmpck_id %d\n", > > - vmpck_id); > > - snp_disable_vmpck(snp_dev); > > - return rc; > > + "Detected unexpected decode failure from ASP. rc: %d\n", > > + rc); > > + goto disable_vmpck; > > } > > > > /* Increment to new message sequence after payload decryption was successful. */ > > snp_inc_msg_seqno(snp_dev); > > > > return 0; > > + > > +disable_vmpck: > > + snp_disable_vmpck(snp_dev); > > + return rc; > > } > > > > static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > > @@ -676,7 +712,7 @@ static int __init sev_guest_probe(struct platform_device *pdev) > > if (!snp_dev->response) > > goto e_free_request; > > > > - snp_dev->certs_data = alloc_shared_pages(dev, SEV_FW_BLOB_MAX_SIZE); > > + snp_dev->certs_data = alloc_shared_pages(dev, sizeof(*snp_dev->certs_data)); > > What's that change for? > > I went searching for that ->certs_data only ot realize that it is an > array of size of SEV_FW_BLOB_MAX_SIZE elems. Do you want this change reverted? I liked the extra readability of the sizeof(*snp_dev->certs_data) but its unnecessary for this change. > > Thx. > > -- > Regards/Gruss, > Boris. > > SUSE Software Solutions Germany GmbH > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman > (HRB 36809, AG Nürnberg)
