On 3/6/25 19:37, 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
> Cc: Nikunj A Dadhania <nikunj@xxxxxxx>
> Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
> ---
> arch/x86/include/asm/sev.h | 6 ++--
> arch/x86/coco/sev/core.c | 23 +++++--------
> drivers/virt/coco/sev-guest/sev-guest.c | 34 ++++++++++++++++----
> 3 files changed, 39 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 1581246491b5..ba7999f66abe 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -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;
> };
>
> /*
> @@ -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;
>
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 82492efc5d94..d02eea5e3d50 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -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:
> @@ -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));
> @@ -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:
> /*
> @@ -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;
>
> /*
> @@ -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;
> }
> @@ -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));
>
> + /* 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 &&
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 4699fdc9ed44..cf3fb61f4d5b 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -177,6 +177,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;
> @@ -210,8 +211,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));
Not sure if it is worth using alloc_pages_exact() (and free_pages_exact())
here instead, since you only end up performing set_memory_decrypted()
against npages vs the actual number allocated. It's not an issue, just
looks a bit odd to my eye.
> + 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));
You can't be sure at what stage the failure occurred, so you need to leak
the pages, just like below where you call set_memory_encrypted().
And similar to below, maybe do a WARN_ONCE() instead of pr_err()?
> + return -EFAULT;
> + }
> +
> cmd:
> /*
> * The intermediate response buffer is used while decrypting the
> @@ -220,10 +233,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;
> @@ -238,7 +253,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;
> @@ -247,7 +262,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;
> }
> @@ -257,6 +272,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));
Can't report_req->certs_len have been updated with a new value at this
point (from the "if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN)")
check and you'll attempt to possibly free more than you allocated?
This would be covered if you stick with npages and use alloc_pages_exact()
and free_pages_exact() using npages.
Thanks,
Tom
> + }
> return ret;
> }
>
