Messages in this thread | | | Date | Wed, 16 Nov 2022 13:02:14 -0600 | Subject | Re: [PATCH V5] virt: sev: Prevent IV reuse in SNP guest driver | From | Tom Lendacky <> |
| |
On 11/16/22 11:55, Peter Gonda wrote: > The AMD Secure Processor (ASP) and an SNP guest use a series of > AES-GCM keys called VMPCKs to 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: "Authentication Failures in NIST > version of GCM" - Antoine Joux et al. > > In order to address this make handle_guest_request() delete the VMPCK > on any non successful return. To allow userspace querying the cert_data > length make handle_guest_request() safe the number of pages required by
s/safe/save/
> the host, then handle_guest_request() retry the request without
... then have handle_guest_request() ...
> requesting the extended data, then return the number of pages required > back to userspace. > > Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver") > Signed-off-by: Peter Gonda <pgonda@google.com> > Reported-by: Peter Gonda <pgonda@google.com>
Just some nits on the commit message and comments below, otherwise
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Borislav Petkov <bp@suse.de> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Michael Roth <michael.roth@amd.com> > Cc: Haowen Bai <baihaowen@meizu.com> > Cc: Yang Yingliang <yangyingliang@huawei.com> > Cc: Marc Orr <marcorr@google.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Dionna Glaze <dionnaglaze@google.com> > Cc: Ashish Kalra <Ashish.Kalra@amd.com> > Cc: stable@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: kvm@vger.kernel.org > --- > drivers/virt/coco/sev-guest/sev-guest.c | 83 ++++++++++++++++++++----- > 1 file changed, 69 insertions(+), 14 deletions(-) > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index f422f9c58ba79..64b4234c14da8 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -67,8 +67,27 @@ static bool is_vmpck_empty(struct snp_guest_dev *snp_dev) > return true; > } > > +/* > + * If an error is received from the host or AMD Secure Processor (ASP) there > + * are two options. Either retry the exact same encrypted request or 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 the sequence number were to be reused the encryption scheme is > + * vulnerable. If the sequence number were incremented for a fresh IV the ASP > + * will reject the 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; > } > @@ -321,34 +340,70 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in > if (rc) > return rc; > > - /* Call firmware to process the request */ > + /* > + * Call firmware to process the request. In this function the encrypted > + * message enters shared memory with the host. So after this call the > + * sequence number must be incremented or the VMPCK must be deleted to > + * prevent reuse of the IV. > + */ > rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > + > + /* > + * If the extended guest request fails due to having too small of a > + * certificate data buffer retry the same guest request without the > + * extended data request in order to not have to reuse the IV.
... in order to increment the sequence number to avoid reuse of the IV.
> + */ > + 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; > + > + /* > + * If this call to the firmware succeeds the sequence number can > + * be incremented allowing for continued use of the VMPCK. If > + * there is an error reflected in the return value, this value > + * is checked further down and the result will be the deletion > + * of the VMPCK and the error code being propagated back to the > + * user as an IOCLT return code.
s/IOCLT/ioctl()/
Thanks, Tom
> + */ > + rc = snp_issue_guest_request(exit_code, &snp_dev->input, &err); > + > + /* > + * Override the error to inform callers the given extended > + * request buffer size was too small and give the caller the > + * required buffer size. > + */ > + err = SNP_GUEST_REQ_INVALID_LEN; > + 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)
| |