Re: [PATCH 6.13.y] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex

Alexey Kardashevskiy <aik@xxxxxxx> · Mon, 10 Mar 2025 21:13:11 +1100

On 10/3/25 21:00, Alexey Kardashevskiy wrote:
Compared to the SNP Guest Request, the "Extended" version adds data pages
for receiving certificates. If not enough pages provided, the HV can
report to the VM how much is needed so the VM can reallocate and repeat.

Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command
mutex") moved handling of the allocated/desired pages number out of scope
of said mutex and create a possibility for a race (multiple instances
trying to trigger Extended request in a VM) as there is just one instance
of snp_msg_desc per /dev/sev-guest and no locking other than snp_cmd_mutex.

Fix the issue by moving the data blob/size and the GHCB input struct
(snp_req_data) into snp_guest_req which is allocated on stack now
and accessed by the GHCB caller under that mutex.

Stop allocating SEV_FW_BLOB_MAX_SIZE in snp_msg_alloc() as only one of
four callers needs it. Free the received blob in get_ext_report() right
after it is copied to the userspace. Possible future users of
snp_send_guest_request() are likely to have different ideas about
the buffer size anyways.

Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex")
Cc: stable@xxxxxxxxxxxxxxx # 6.13
Cc: Nikunj A Dadhania <nikunj@xxxxxxx>
Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>

Missed:

(cherry picked from commit 3e385c0d6ce88ac9916dcf84267bd5855d830748)

I first cherrypicked and sent, then I read about "cherry-oick -x", sorry 
for the noise. thanks,


---
  arch/x86/include/asm/sev.h              |  6 +--
  drivers/virt/coco/sev-guest/sev-guest.c | 63 +++++++++++++++----------
  2 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 91f08af31078..82d9250aac34 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -185,6 +185,9 @@ struct snp_guest_req {
  	unsigned int vmpck_id;
  	u8 msg_version;
  	u8 msg_type;
+
+	struct snp_req_data input;
+	void *certs_data;
  };
  
  /*
@@ -245,9 +248,6 @@ struct snp_msg_desc {
  	struct snp_guest_msg secret_request, secret_response;
  
  	struct snp_secrets_page *secrets;
-	struct snp_req_data input;
-
-	void *certs_data;
  
  	struct aesgcm_ctx *ctx;
  
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index af64e6191f74..480159606434 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -249,7 +249,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
  	 * sequence number must be incremented or the VMPCK must be deleted to
  	 * prevent reuse of the IV.
  	 */
-	rc = snp_issue_guest_request(req, &mdesc->input, rio);
+	rc = snp_issue_guest_request(req, &req->input, rio);
  	switch (rc) {
  	case -ENOSPC:
  		/*
@@ -259,7 +259,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
  		 * order to increment the sequence number and thus avoid
  		 * IV reuse.
  		 */
-		override_npages = mdesc->input.data_npages;
+		override_npages = req->input.data_npages;
  		req->exit_code	= SVM_VMGEXIT_GUEST_REQUEST;
  
  		/*
@@ -315,7 +315,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
  	}
  
  	if (override_npages)
-		mdesc->input.data_npages = override_npages;
+		req->input.data_npages = override_npages;
  
  	return rc;
  }
@@ -354,6 +354,11 @@ static int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
  	memcpy(mdesc->request, &mdesc->secret_request,
  	       sizeof(mdesc->secret_request));
  
+	/* initial the input address for guest request */
+	req->input.req_gpa = __pa(mdesc->request);
+	req->input.resp_gpa = __pa(mdesc->response);
+	req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0;
+
  	rc = __handle_guest_request(mdesc, req, rio);
  	if (rc) {
  		if (rc == -EIO &&
@@ -495,6 +500,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
  	struct snp_guest_req req = {};
  	int ret, npages = 0, resp_len;
  	sockptr_t certs_address;
+	struct page *page;
  
  	if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
  		return -EINVAL;
@@ -528,8 +534,20 @@ 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(mdesc->certs_data, 0, report_req->certs_len);
  	npages = report_req->certs_len >> PAGE_SHIFT;
+	page = alloc_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO,
+			   get_order(report_req->certs_len));
+	if (!page)
+		return -ENOMEM;
+
+	req.certs_data = page_address(page);
+	ret = set_memory_decrypted((unsigned long)req.certs_data, npages);
+	if (ret) {
+		pr_err("failed to mark page shared, ret=%d\n", ret);
+		__free_pages(page, get_order(report_req->certs_len));
+		return -EFAULT;
+	}
+
  cmd:
  	/*
  	 * The intermediate response buffer is used while decrypting the
@@ -538,10 +556,12 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
  	 */
  	resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize;
  	report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
-	if (!report_resp)
-		return -ENOMEM;
+	if (!report_resp) {
+		ret = -ENOMEM;
+		goto e_free_data;
+	}
  
-	mdesc->input.data_npages = npages;
+	req.input.data_npages = npages;
  
  	req.msg_version = arg->msg_version;
  	req.msg_type = SNP_MSG_REPORT_REQ;
@@ -556,7 +576,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
  
  	/* If certs length is invalid then copy the returned length */
  	if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
-		report_req->certs_len = mdesc->input.data_npages << PAGE_SHIFT;
+		report_req->certs_len = req.input.data_npages << PAGE_SHIFT;
  
  		if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req)))
  			ret = -EFAULT;
@@ -565,7 +585,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
  	if (ret)
  		goto e_free;
  
-	if (npages && copy_to_sockptr(certs_address, mdesc->certs_data, report_req->certs_len)) {
+	if (npages && copy_to_sockptr(certs_address, req.certs_data, report_req->certs_len)) {
  		ret = -EFAULT;
  		goto e_free;
  	}
@@ -575,6 +595,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
  
  e_free:
  	kfree(report_resp);
+e_free_data:
+	if (npages) {
+		if (set_memory_encrypted((unsigned long)req.certs_data, npages))
+			WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
+		else
+			__free_pages(page, get_order(report_req->certs_len));
+	}
  	return ret;
  }
  
@@ -1048,35 +1075,26 @@ static int __init sev_guest_probe(struct platform_device *pdev)
  	if (!mdesc->response)
  		goto e_free_request;
  
-	mdesc->certs_data = alloc_shared_pages(dev, SEV_FW_BLOB_MAX_SIZE);
-	if (!mdesc->certs_data)
-		goto e_free_response;
-
  	ret = -EIO;
  	mdesc->ctx = snp_init_crypto(mdesc->vmpck, VMPCK_KEY_LEN);
  	if (!mdesc->ctx)
-		goto e_free_cert_data;
+		goto e_free_response;
  
  	misc = &snp_dev->misc;
  	misc->minor = MISC_DYNAMIC_MINOR;
  	misc->name = DEVICE_NAME;
  	misc->fops = &snp_guest_fops;
  
-	/* Initialize the input addresses for guest request */
-	mdesc->input.req_gpa = __pa(mdesc->request);
-	mdesc->input.resp_gpa = __pa(mdesc->response);
-	mdesc->input.data_gpa = __pa(mdesc->certs_data);
-
  	/* Set the privlevel_floor attribute based on the vmpck_id */
  	sev_tsm_ops.privlevel_floor = vmpck_id;
  
  	ret = tsm_register(&sev_tsm_ops, snp_dev);
  	if (ret)
-		goto e_free_cert_data;
+		goto e_free_response;
  
  	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
  	if (ret)
-		goto e_free_cert_data;
+		goto e_free_response;
  
  	ret =  misc_register(misc);
  	if (ret)
@@ -1088,8 +1106,6 @@ static int __init sev_guest_probe(struct platform_device *pdev)
  
  e_free_ctx:
  	kfree(mdesc->ctx);
-e_free_cert_data:
-	free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE);
  e_free_response:
  	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
  e_free_request:
@@ -1104,7 +1120,6 @@ static void __exit sev_guest_remove(struct platform_device *pdev)
  	struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
  	struct snp_msg_desc *mdesc = snp_dev->msg_desc;
  
-	free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE);
  	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
  	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
  	kfree(mdesc->ctx);

--
Alexey








