| Message ID | 20221116175558.2373112-1-pgonda@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V5] virt: sev: Prevent IV reuse in SNP guest driver | expand |
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)
On Wed, Nov 16, 2022 at 12:02 PM Tom Lendacky <thomas.lendacky@amd.com> wrote: > > 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> Thanks Tom. I'll update with all the feedback after Boris chimes in.
On Thu, Nov 17, 2022 at 07:19:17AM -0700, Peter Gonda wrote:
> Thanks Tom. I'll update with all the feedback after Boris chimes in.
No need - it looks pretty good to me. I'll queue it next week with Tom's
comments incorporated.
Thx.
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. + */ + 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. + */ + 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)
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 the host, then handle_guest_request() retry the request without 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> 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(-)