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." > 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. Thx. -- Regards/Gruss, Boris. SUSE Software Solutions Germany GmbH GF: Ivo Totev, Andrew Myers, Andrew McDonald, Martje Boudien Moerman (HRB 36809, AG Nürnberg)
