Skip to content

Commit

Permalink
virt: sev-guest: Move SNP Guest Request data pages handling under snp…
Browse files Browse the repository at this point in the history 
…_cmd_mutex

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

  ae59661 ("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: ae59661 ("virt: sev-guest: Reduce the scope of SNP command mutex")
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20250307013700.437505-3-aik@amd.com
  • Loading branch information
Alexey Kardashevskiy authored and Borislav Petkov (AMD) committed Mar 7, 2025
1 parent ac7c06a commit 3e385c0
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 24 deletions.
23 changes: 8 additions & 15 deletions arch/x86/coco/sev/core.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2853,19 +2853,8 @@ struct snp_msg_desc *snp_msg_alloc(void)
if (!mdesc->response)
goto e_free_request;

mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE);
if (!mdesc->certs_data)
goto e_free_response;

/* initial the input address 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);

return mdesc;

e_free_response:
free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
e_free_request:
free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
e_unmap:
Expand All @@ -2885,7 +2874,6 @@ void snp_msg_free(struct snp_msg_desc *mdesc)
kfree(mdesc->ctx);
free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE);
iounmap((__force void __iomem *)mdesc->secrets);

memset(mdesc, 0, sizeof(*mdesc));
Expand Down Expand Up @@ -3054,7 +3042,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:
/*
Expand All @@ -3064,7 +3052,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;

/*
Expand Down Expand Up @@ -3120,7 +3108,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;
}
Expand Down Expand Up @@ -3158,6 +3146,11 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req
*/
memcpy(mdesc->request, &mdesc->secret_request, sizeof(mdesc->secret_request));

/* Initialize 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 &&
Expand Down
6 changes: 3 additions & 3 deletions arch/x86/include/asm/sev.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,9 @@ struct snp_guest_req {
unsigned int vmpck_id;
u8 msg_version;
u8 msg_type;

struct snp_req_data input;
void *certs_data;
};

/*
Expand Down Expand Up @@ -263,9 +266,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;

Expand Down
34 changes: 28 additions & 6 deletions drivers/virt/coco/sev-guest/sev-guest.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,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;
Expand Down Expand Up @@ -209,8 +210,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
Expand All @@ -219,10 +232,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;
Expand All @@ -237,7 +252,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;
Expand All @@ -246,7 +261,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;
}
Expand All @@ -256,6 +271,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;
}

Expand Down

0 comments on commit 3e385c0

Please sign in to comment.