[PATCH V3 0/2] Fix security issue in SNP guest AES-GCM usage

[PATCH V3 0/2] Fix security issue in SNP guest AES-GCM usage

[Peter Gonda]

Currently the ASP and SNP guest use an AES-GCM bases secure channel to
communicate with each other. The IV for this encryption scheme is a
sequence that each party maintains. Currently the ASP requires the
sequence number of the request to be exactly one more than its saved
sequence number and the ASP only increments its saved sequence number
after a successful command. That means if the guest request ever fails
it can only ever retry that exact encrypted command or discontinue its
use of that VMPCK. If it were to try another command it would either
need to reuse the sequence number which is the IC. That can lead to the
encryption scheme failing with AES-GCM. Or if it incremented the
sequence number the ASP would never accept the command due to sequence
number mismatch.

https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf

Cc: Dionna Glaze <dionnaglaze@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: Ashish Kalra <Ashish.Kalra@amd.com>
Cc: linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org

Peter Gonda (2):
  virt: sev: Prevent IV reuse in SNP guest driver
  virt: sev: Allow for retrying SNP extended requests

 arch/x86/include/asm/svm.h              |  6 ++
 arch/x86/kernel/sev.c                   | 28 ++++++--
 drivers/virt/coco/sev-guest/sev-guest.c | 93 ++++++++++++++++---------
 3 files changed, 91 insertions(+), 36 deletions(-)

-- 
2.38.0.135.g90850a2211-goog

[PATCH V3 1/2] virt: sev: Prevent IV reuse in SNP guest driver

[Peter Gonda]

The 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:
https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf

To handle userspace querying the cert_data length. Instead of requesting
the cert length from userspace use the size of the drivers allocated
shared buffer. Then copy that buffer to userspace, or give userspace an
error depending on the size of the buffer given by userspace.

Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver")
Signed-off-by: Peter Gonda <pgonda@google.com>
Reported-by: Peter Gonda <pgonda@google.com>
Reviewed-by: Dionna Glaze <dionnaglaze@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: Ashish Kalra <Ashish.Kalra@amd.com>
Cc: linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org
---
 drivers/virt/coco/sev-guest/sev-guest.c | 93 ++++++++++++++++---------
 1 file changed, 62 insertions(+), 31 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index f422f9c58ba7..8c54ea84bc57 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
+ * 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;
 }
@@ -326,29 +345,29 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
 	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)
@@ -437,7 +456,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_ext_report_req req;
 	struct snp_report_resp *resp;
-	int ret, npages = 0, resp_len;
+	int ret, resp_len, req_cert_len, resp_cert_len;
 
 	lockdep_assert_held(&snp_cmd_mutex);
 
@@ -448,14 +467,15 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 		return -EFAULT;
 
 	/* userspace does not want certificate data */
-	if (!req.certs_len || !req.certs_address)
+	req_cert_len = req.certs_len;
+	if (!req_cert_len || !req.certs_address)
 		goto cmd;
 
-	if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
-	    !IS_ALIGNED(req.certs_len, PAGE_SIZE))
+	if (req_cert_len > sizeof(*snp_dev->certs_data) ||
+	    !IS_ALIGNED(req_cert_len, PAGE_SIZE))
 		return -EINVAL;
 
-	if (!access_ok((const void __user *)req.certs_address, req.certs_len))
+	if (!access_ok((const void __user *)req.certs_address, req_cert_len))
 		return -EFAULT;
 
 	/*
@@ -464,8 +484,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	 * the host. If host does not supply any certs in it, then copy
 	 * zeros to indicate that certificate data was not provided.
 	 */
-	memset(snp_dev->certs_data, 0, req.certs_len);
-	npages = req.certs_len >> PAGE_SHIFT;
+	memset(snp_dev->certs_data, 0, sizeof(*snp_dev->certs_data));
 cmd:
 	/*
 	 * The intermediate response buffer is used while decrypting the
@@ -477,25 +496,37 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (!resp)
 		return -ENOMEM;
 
-	snp_dev->input.data_npages = npages;
+	snp_dev->input.data_npages = sizeof(*snp_dev->certs_data) >> PAGE_SHIFT;
 	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version,
 				   SNP_MSG_REPORT_REQ, &req.data,
 				   sizeof(req.data), resp->data, resp_len, &arg->fw_err);
 
+	resp_cert_len = snp_dev->input.data_npages << PAGE_SHIFT;
+
 	/* If certs length is invalid then copy the returned length */
 	if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
-		req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
+		dev_alert(snp_dev->dev,
+			  "Certificate data from host: %d, Max size allocated by driver: %lu.\n",
+			  resp_cert_len, sizeof(*snp_dev->certs_data));
+		ret = -EFAULT;
+	}
+
+	if (ret)
+		goto e_free;
+
+	/* Pass the actual certificate data size back to userspace */
+	req.certs_len = resp_cert_len;
+	if (resp_cert_len > req_cert_len) {
+		arg->fw_err = SNP_GUEST_REQ_INVALID_LEN;
 
 		if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
 			ret = -EFAULT;
-	}
 
-	if (ret)
 		goto e_free;
+	}
 
-	if (npages &&
-	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
-			 req.certs_len)) {
+	if (copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
+			 resp_cert_len)) {
 		ret = -EFAULT;
 		goto e_free;
 	}
@@ -676,7 +707,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));
 	if (!snp_dev->certs_data)
 		goto e_free_response;
 
@@ -703,7 +734,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 	return 0;
 
 e_free_cert_data:
-	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
+	free_shared_pages(snp_dev->certs_data, sizeof(*snp_dev->certs_data));
 e_free_response:
 	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
 e_free_request:
@@ -717,7 +748,7 @@ static int __exit sev_guest_remove(struct platform_device *pdev)
 {
 	struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
 
-	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
+	free_shared_pages(snp_dev->certs_data, sizeof(*snp_dev->certs_data));
 	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
 	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
 	deinit_crypto(snp_dev->crypto);
-- 
2.38.0.135.g90850a2211-goog

[PATCH V3 2/2] virt: sev: Allow for retrying SNP extended requests

[Peter Gonda]

If an SNP Extended Request is placed without enough data pages the host
will return an error to the guest and tell it the number of required
data pages. If we place an extended request without enogh data page we
can retry the command portion of the request using an SNP Request. This
allows us to keep our command sequence numbers with the ASP in sync
while also supporting the SNP Extended Request's data size querying
capability. This happens inside of snp_issue_guest_request() to keep the
safety of the sequence numbers easy. Any failure
snp_issue_guest_request() should result in no further of the VMCPK due
to issues with the sequence number being the IV in the AES-GCM
communication channel. IV reuse, meaning sequence number reuse in this
driver, can result in the secure channel being compromised.

Signed-off-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: Ashish Kalra <Ashish.Kalra@amd.com>
Cc: linux-kernel@vger.kernel.org
Cc: kvm@vger.kernel.org
---
 arch/x86/include/asm/svm.h              |  6 ++++++
 arch/x86/kernel/sev.c                   | 28 ++++++++++++++++++++-----
 drivers/virt/coco/sev-guest/sev-guest.c |  2 +-
 3 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 0361626841bc..3886b8ea18ae 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -585,6 +585,12 @@ struct vmcb {
 				(unsigned long *)&ghcb->save.valid_bitmap);	\
 	}									\
 										\
+	static __always_inline void ghcb_clear_##field(const struct ghcb *ghcb) \
+	{									\
+		clear_bit(GHCB_BITMAP_IDX(field),				\
+			  (unsigned long *)&ghcb->save.valid_bitmap);		\
+	}									\
+										\
 	static __always_inline u64 ghcb_get_##field(struct ghcb *ghcb)		\
 	{									\
 		return ghcb->save.field;					\
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index a428c62330d3..3f7e2105ef97 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -2213,12 +2213,30 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned
 		goto e_put;
 
 	if (ghcb->save.sw_exit_info_2) {
-		/* Number of expected pages are returned in RBX */
+		/* For a SNP Extended Request, if the request was placed with
+		 * insufficient data pages. The host will return the number of
+		 * pages required using RBX in the GHCB. We can than retry the
+		 * call as an SNP Request to fulfill the command without getting
+		 * the extended request data.
+		 */
 		if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
-		    ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
-			input->data_npages = ghcb_get_rbx(ghcb);
-
-		*fw_err = ghcb->save.sw_exit_info_2;
+		    ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) {
+			int npages = ghcb_get_rbx(ghcb);
+
+			ghcb_clear_rax(ghcb);
+			ghcb_clear_rbx(ghcb);
+
+			ret = sev_es_ghcb_hv_call(ghcb, &ctxt,
+						  SVM_VMGEXIT_GUEST_REQUEST,
+						  input->req_gpa,
+						  input->resp_gpa);
+			if (ret)
+				goto e_put;
+
+			input->data_npages = npages;
+			*fw_err = SNP_GUEST_REQ_INVALID_LEN;
+		} else
+			*fw_err = ghcb->save.sw_exit_info_2;
 
 		ret = -EIO;
 	}
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 8c54ea84bc57..ede07c0ec0c3 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -496,7 +496,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (!resp)
 		return -ENOMEM;
 
-	snp_dev->input.data_npages = sizeof(*snp_dev->certs_data) >> PAGE_SHIFT;
+	snp_dev->input.data_npages = req_cert_len  >> PAGE_SHIFT;
 	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version,
 				   SNP_MSG_REPORT_REQ, &req.data,
 				   sizeof(req.data), resp->data, resp_len, &arg->fw_err);
-- 
2.38.0.135.g90850a2211-goog

Re: [PATCH V3 2/2] virt: sev: Allow for retrying SNP extended requests

[Dionna Amalie Glaze]

I think just no to this patch? Reasons below.

>
>         if (ghcb->save.sw_exit_info_2) {
> -               /* Number of expected pages are returned in RBX */
> +               /* For a SNP Extended Request, if the request was placed with
> +                * insufficient data pages. The host will return the number of
> +                * pages required using RBX in the GHCB. We can than retry the
> +                * call as an SNP Request to fulfill the command without getting
> +                * the extended request data.
> +                */
>                 if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&
> -                   ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN)
> -                       input->data_npages = ghcb_get_rbx(ghcb);
> -
> -               *fw_err = ghcb->save.sw_exit_info_2;
> +                   ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) {
> +                       int npages = ghcb_get_rbx(ghcb);
> +
> +                       ghcb_clear_rax(ghcb);
> +                       ghcb_clear_rbx(ghcb);
> +
> +                       ret = sev_es_ghcb_hv_call(ghcb, &ctxt,
> +                                                 SVM_VMGEXIT_GUEST_REQUEST,
> +                                                 input->req_gpa,
> +                                                 input->resp_gpa);
> +                       if (ret)
> +                               goto e_put;
> +

I'm not keen on reissuing the call in this function. I think
issue_request should do its job of sending a request to the host and
returning the specified data, in this case the number of pages in RBX.
I know it's not particularly fun to interpret exitinfo2 in a couple
places, but it serves a purpose. We don't want this function to grow
to have special cases for all the commands that can be sent to the psp
if they don't involve data passed back through the GHCB. The
get_ghcb/put_ghcb frame is the only thing we really need to respect in
here.

The sev-guest device owns the VMPCKn keys, the message sequence
number, and the responsibility of sending a coherent response back to
user space. When we account for the host changing the certificate page
length during the request and not wanting to return to the guest
without completing the firmware call, the length might grow past the
4KiB max constant we have so far. The driver can make the choice of
issuing the request without extended data like you do here, or to
reallocate its cert_data buffer and ask for the extended data again.
It shouldn't matter to the core functionality of issuing a single
request.

When throttling comes into play and retrying needs to happen more than
once, then we're in another situation where the sev-guest driver also
owns the responsibility of trying not to get throttled too hard. My
patches suggest a 2HZ rate limit to avoid any big penalties of running
requests in a tight loop and looking like a DoS antagonist, but that
doesn't belong in arch/x86/kernel/sev.c due to the variability of
strategies.

> +                       input->data_npages = npages;
> +                       *fw_err = SNP_GUEST_REQ_INVALID_LEN;
> +               } else
> +                       *fw_err = ghcb->save.sw_exit_info_2;

I think in both branches of the conditional, fw_err gets set to
exit_info_2. See v4 of my throttling patch series.

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH V3 1/2] virt: sev: Prevent IV reuse in SNP guest driver

[Tom Lendacky]

On 10/27/22 10:05, Peter Gonda wrote:
> The 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:
> https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf
> 
> To handle userspace querying the cert_data length. Instead of requesting
> the cert length from userspace use the size of the drivers allocated
> shared buffer. Then copy that buffer to userspace, or give userspace an
> error depending on the size of the buffer given by userspace.
> 
> Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver")
> Signed-off-by: Peter Gonda <pgonda@google.com>
> Reported-by: Peter Gonda <pgonda@google.com>
> Reviewed-by: Dionna Glaze <dionnaglaze@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: Ashish Kalra <Ashish.Kalra@amd.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: kvm@vger.kernel.org
> ---
>   drivers/virt/coco/sev-guest/sev-guest.c | 93 ++++++++++++++++---------
>   1 file changed, 62 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index f422f9c58ba7..8c54ea84bc57 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
> + * 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;
>   }
> @@ -326,29 +345,29 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
>   	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;
> +	}

Realize that snp_issue_guest_request() will return -EIO in the case that 
the returned SW_EXITINFO2 value is SNP_GUEST_REQ_INVALID_LEN. So all the 
work you do below in get_ext_report() doesn't matter because you end up 
disabling the key here.

So maybe this patch should be split up and parts of it added to the second 
patch (but that patch seems like it would still hit this issue because 
-EIO is still returned.

Thanks,
Tom

>   
> -	/*
> -	 * 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)
> @@ -437,7 +456,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   	struct snp_guest_crypto *crypto = snp_dev->crypto;
>   	struct snp_ext_report_req req;
>   	struct snp_report_resp *resp;
> -	int ret, npages = 0, resp_len;
> +	int ret, resp_len, req_cert_len, resp_cert_len;
>   
>   	lockdep_assert_held(&snp_cmd_mutex);
>   
> @@ -448,14 +467,15 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   		return -EFAULT;
>   
>   	/* userspace does not want certificate data */
> -	if (!req.certs_len || !req.certs_address)
> +	req_cert_len = req.certs_len;
> +	if (!req_cert_len || !req.certs_address)
>   		goto cmd;
>   
> -	if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
> -	    !IS_ALIGNED(req.certs_len, PAGE_SIZE))
> +	if (req_cert_len > sizeof(*snp_dev->certs_data) ||
> +	    !IS_ALIGNED(req_cert_len, PAGE_SIZE))
>   		return -EINVAL;
>   
> -	if (!access_ok((const void __user *)req.certs_address, req.certs_len))
> +	if (!access_ok((const void __user *)req.certs_address, req_cert_len))
>   		return -EFAULT;
>   
>   	/*
> @@ -464,8 +484,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   	 * the host. If host does not supply any certs in it, then copy
>   	 * zeros to indicate that certificate data was not provided.
>   	 */
> -	memset(snp_dev->certs_data, 0, req.certs_len);
> -	npages = req.certs_len >> PAGE_SHIFT;
> +	memset(snp_dev->certs_data, 0, sizeof(*snp_dev->certs_data));
>   cmd:
>   	/*
>   	 * The intermediate response buffer is used while decrypting the
> @@ -477,25 +496,37 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
>   	if (!resp)
>   		return -ENOMEM;
>   
> -	snp_dev->input.data_npages = npages;
> +	snp_dev->input.data_npages = sizeof(*snp_dev->certs_data) >> PAGE_SHIFT;
>   	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version,
>   				   SNP_MSG_REPORT_REQ, &req.data,
>   				   sizeof(req.data), resp->data, resp_len, &arg->fw_err);
>   
> +	resp_cert_len = snp_dev->input.data_npages << PAGE_SHIFT;
> +
>   	/* If certs length is invalid then copy the returned length */
>   	if (arg->fw_err == SNP_GUEST_REQ_INVALID_LEN) {
> -		req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
> +		dev_alert(snp_dev->dev,
> +			  "Certificate data from host: %d, Max size allocated by driver: %lu.\n",
> +			  resp_cert_len, sizeof(*snp_dev->certs_data));
> +		ret = -EFAULT;
> +	}
> +
> +	if (ret)
> +		goto e_free;
> +
> +	/* Pass the actual certificate data size back to userspace */
> +	req.certs_len = resp_cert_len;
> +	if (resp_cert_len > req_cert_len) {
> +		arg->fw_err = SNP_GUEST_REQ_INVALID_LEN;
>   
>   		if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
>   			ret = -EFAULT;
> -	}
>   
> -	if (ret)
>   		goto e_free;
> +	}
>   
> -	if (npages &&
> -	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
> -			 req.certs_len)) {
> +	if (copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
> +			 resp_cert_len)) {
>   		ret = -EFAULT;
>   		goto e_free;
>   	}
> @@ -676,7 +707,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));
>   	if (!snp_dev->certs_data)
>   		goto e_free_response;
>   
> @@ -703,7 +734,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	return 0;
>   
>   e_free_cert_data:
> -	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
> +	free_shared_pages(snp_dev->certs_data, sizeof(*snp_dev->certs_data));
>   e_free_response:
>   	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
>   e_free_request:
> @@ -717,7 +748,7 @@ static int __exit sev_guest_remove(struct platform_device *pdev)
>   {
>   	struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
>   
> -	free_shared_pages(snp_dev->certs_data, SEV_FW_BLOB_MAX_SIZE);
> +	free_shared_pages(snp_dev->certs_data, sizeof(*snp_dev->certs_data));
>   	free_shared_pages(snp_dev->response, sizeof(struct snp_guest_msg));
>   	free_shared_pages(snp_dev->request, sizeof(struct snp_guest_msg));
>   	deinit_crypto(snp_dev->crypto);

Re: [PATCH V3 1/2] virt: sev: Prevent IV reuse in SNP guest driver

[Peter Gonda]

On Thu, Oct 27, 2022 at 12:06 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 10/27/22 10:05, Peter Gonda wrote:
> > The 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:
> > https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf
> >
> > To handle userspace querying the cert_data length. Instead of requesting
> > the cert length from userspace use the size of the drivers allocated
> > shared buffer. Then copy that buffer to userspace, or give userspace an
> > error depending on the size of the buffer given by userspace.
> >
> > Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver")
> > Signed-off-by: Peter Gonda <pgonda@google.com>
> > Reported-by: Peter Gonda <pgonda@google.com>
> > Reviewed-by: Dionna Glaze <dionnaglaze@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: Ashish Kalra <Ashish.Kalra@amd.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: kvm@vger.kernel.org
> > ---
> >   drivers/virt/coco/sev-guest/sev-guest.c | 93 ++++++++++++++++---------
> >   1 file changed, 62 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > index f422f9c58ba7..8c54ea84bc57 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
> > + * 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;
> >   }
> > @@ -326,29 +345,29 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> >       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;
> > +     }
>
> Realize that snp_issue_guest_request() will return -EIO in the case that
> the returned SW_EXITINFO2 value is SNP_GUEST_REQ_INVALID_LEN. So all the
> work you do below in get_ext_report() doesn't matter because you end up
> disabling the key here.
>
> So maybe this patch should be split up and parts of it added to the second
> patch (but that patch seems like it would still hit this issue because
> -EIO is still returned.
>

Ack I see that. My testing didn't catch this since I realized I didn't
actually load any certificate data into the host. After doing so my
testing catches this bug.

I agree with Dionna's comments on 2/2. My suggestion would be to keep
the constraint that either handle_guest_request() leaves the sequence
number in a good state or disables the VMPCK. After seeing her V4
series I suggest we take this patch and follow up on the certificate
querying with the further changes to snp_issue_guest_request().
Thoughts?

Re: [PATCH V3 1/2] virt: sev: Prevent IV reuse in SNP guest driver

[Peter Gonda]

On Thu, Oct 27, 2022 at 2:10 PM Peter Gonda <pgonda@google.com> wrote:
>
> On Thu, Oct 27, 2022 at 12:06 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >
> > On 10/27/22 10:05, Peter Gonda wrote:
> > > The 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:
> > > https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/800-38-series-drafts/gcm/joux_comments.pdf
> > >
> > > To handle userspace querying the cert_data length. Instead of requesting
> > > the cert length from userspace use the size of the drivers allocated
> > > shared buffer. Then copy that buffer to userspace, or give userspace an
> > > error depending on the size of the buffer given by userspace.
> > >
> > > Fixes: fce96cf044308 ("virt: Add SEV-SNP guest driver")
> > > Signed-off-by: Peter Gonda <pgonda@google.com>
> > > Reported-by: Peter Gonda <pgonda@google.com>
> > > Reviewed-by: Dionna Glaze <dionnaglaze@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: Ashish Kalra <Ashish.Kalra@amd.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: kvm@vger.kernel.org
> > > ---
> > >   drivers/virt/coco/sev-guest/sev-guest.c | 93 ++++++++++++++++---------
> > >   1 file changed, 62 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> > > index f422f9c58ba7..8c54ea84bc57 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
> > > + * 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;
> > >   }
> > > @@ -326,29 +345,29 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in
> > >       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;
> > > +     }
> >
> > Realize that snp_issue_guest_request() will return -EIO in the case that
> > the returned SW_EXITINFO2 value is SNP_GUEST_REQ_INVALID_LEN. So all the
> > work you do below in get_ext_report() doesn't matter because you end up
> > disabling the key here.
> >
> > So maybe this patch should be split up and parts of it added to the second
> > patch (but that patch seems like it would still hit this issue because
> > -EIO is still returned.
> >
>
> Ack I see that. My testing didn't catch this since I realized I didn't
> actually load any certificate data into the host. After doing so my
> testing catches this bug.
>
> I agree with Dionna's comments on 2/2. My suggestion would be to keep
> the constraint that either handle_guest_request() leaves the sequence
> number in a good state or disables the VMPCK. After seeing her V4
> series I suggest we take this patch and follow up on the certificate
> querying with the further changes to snp_issue_guest_request().
> Thoughts?

Actually we want the V2 version of this patch, which forces userspace
to use 4 pages and therefore doesn't let a short userspace request
corrupt the sequence numbers.