Skip to content

Commit db10cb9

Browse files
djbwdjbw
committed
virt: sevguest: Fix passing a stack buffer as a scatterlist target
CONFIG_DEBUG_SG highlights that get_{report,ext_report,derived_key)()} are passing stack buffers as the @req_buf argument to handle_guest_request(), generating a Call Trace of the following form: WARNING: CPU: 0 PID: 1175 at include/linux/scatterlist.h:187 enc_dec_message+0x518/0x5b0 [sev_guest] [..] Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023 RIP: 0010:enc_dec_message+0x518/0x5b0 [sev_guest] Call Trace: <TASK> [..] handle_guest_request+0x135/0x520 [sev_guest] get_ext_report+0x1ec/0x3e0 [sev_guest] snp_guest_ioctl+0x157/0x200 [sev_guest] Note that the above Call Trace was with the DEBUG_SG BUG_ON()s converted to WARN_ON()s. This is benign as long as there are no hardware crypto accelerators loaded for the aead cipher, and no subsequent dma_map_sg() is performed on the scatterlist. However, sev-guest can not assume the presence of an aead accelerator nor can it assume that CONFIG_DEBUG_SG is disabled. Resolve this bug by allocating virt_addr_valid() memory, similar to the other buffers am @snp_dev instance carries, to marshal requests from user buffers to kernel buffers. Reported-by: Peter Gonda <pgonda@google.com> Closes: http://lore.kernel.org/r/CAMkAt6r2VPPMZ__SQfJse8qWsUyYW3AgYbOUVM0S_Vtk=KvkxQ@mail.gmail.com Fixes: fce96cf ("virt: Add SEV-SNP guest driver") Cc: Borislav Petkov <bp@alien8.de> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Dionna Glaze <dionnaglaze@google.com> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
1 parent 6465e26 commit db10cb9

File tree

1 file changed

+25
-20
lines changed

1 file changed

+25
-20
lines changed

‎drivers/virt/coco/sev-guest/sev-guest.c

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,11 @@ struct snp_guest_dev {
5757

5858
struct snp_secrets_page_layout *layout;
5959
struct snp_req_data input;
60+
union {
61+
struct snp_report_req report;
62+
struct snp_derived_key_req derived_key;
63+
struct snp_ext_report_req ext_report;
64+
} req;
6065
u32 *os_area_msg_seqno;
6166
u8 *vmpck;
6267
};
@@ -473,16 +478,16 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
473478
static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
474479
{
475480
struct snp_guest_crypto *crypto = snp_dev->crypto;
481+
struct snp_report_req *req = &snp_dev->req.report;
476482
struct snp_report_resp *resp;
477-
struct snp_report_req req;
478483
int rc, resp_len;
479484

480485
lockdep_assert_held(&snp_cmd_mutex);
481486

482487
if (!arg->req_data || !arg->resp_data)
483488
return -EINVAL;
484489

485-
if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
490+
if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
486491
return -EFAULT;
487492

488493
/*
@@ -496,7 +501,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
496501
return -ENOMEM;
497502

498503
rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
499-
SNP_MSG_REPORT_REQ, &req, sizeof(req), resp->data,
504+
SNP_MSG_REPORT_REQ, req, sizeof(*req), resp->data,
500505
resp_len);
501506
if (rc)
502507
goto e_free;
@@ -511,9 +516,9 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
511516

512517
static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
513518
{
519+
struct snp_derived_key_req *req = &snp_dev->req.derived_key;
514520
struct snp_guest_crypto *crypto = snp_dev->crypto;
515521
struct snp_derived_key_resp resp = {0};
516-
struct snp_derived_key_req req;
517522
int rc, resp_len;
518523
/* Response data is 64 bytes and max authsize for GCM is 16 bytes. */
519524
u8 buf[64 + 16];
@@ -532,11 +537,11 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
532537
if (sizeof(buf) < resp_len)
533538
return -ENOMEM;
534539

535-
if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
540+
if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
536541
return -EFAULT;
537542

538543
rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
539-
SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len);
544+
SNP_MSG_KEY_REQ, req, sizeof(*req), buf, resp_len);
540545
if (rc)
541546
return rc;
542547

@@ -552,8 +557,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
552557

553558
static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
554559
{
560+
struct snp_ext_report_req *req = &snp_dev->req.ext_report;
555561
struct snp_guest_crypto *crypto = snp_dev->crypto;
556-
struct snp_ext_report_req req;
557562
struct snp_report_resp *resp;
558563
int ret, npages = 0, resp_len;
559564

@@ -562,18 +567,18 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
562567
if (!arg->req_data || !arg->resp_data)
563568
return -EINVAL;
564569

565-
if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
570+
if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
566571
return -EFAULT;
567572

568573
/* userspace does not want certificate data */
569-
if (!req.certs_len || !req.certs_address)
574+
if (!req->certs_len || !req->certs_address)
570575
goto cmd;
571576

572-
if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
573-
!IS_ALIGNED(req.certs_len, PAGE_SIZE))
577+
if (req->certs_len > SEV_FW_BLOB_MAX_SIZE ||
578+
!IS_ALIGNED(req->certs_len, PAGE_SIZE))
574579
return -EINVAL;
575580

576-
if (!access_ok((const void __user *)req.certs_address, req.certs_len))
581+
if (!access_ok((const void __user *)req->certs_address, req->certs_len))
577582
return -EFAULT;
578583

579584
/*
@@ -582,8 +587,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
582587
* the host. If host does not supply any certs in it, then copy
583588
* zeros to indicate that certificate data was not provided.
584589
*/
585-
memset(snp_dev->certs_data, 0, req.certs_len);
586-
npages = req.certs_len >> PAGE_SHIFT;
590+
memset(snp_dev->certs_data, 0, req->certs_len);
591+
npages = req->certs_len >> PAGE_SHIFT;
587592
cmd:
588593
/*
589594
* The intermediate response buffer is used while decrypting the
@@ -597,23 +602,23 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
597602

598603
snp_dev->input.data_npages = npages;
599604
ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
600-
SNP_MSG_REPORT_REQ, &req.data,
601-
sizeof(req.data), resp->data, resp_len);
605+
SNP_MSG_REPORT_REQ, &req->data,
606+
sizeof(req->data), resp->data, resp_len);
602607

603608
/* If certs length is invalid then copy the returned length */
604609
if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
605-
req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
610+
req->certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
606611

607-
if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
612+
if (copy_to_user((void __user *)arg->req_data, req, sizeof(*req)))
608613
ret = -EFAULT;
609614
}
610615

611616
if (ret)
612617
goto e_free;
613618

614619
if (npages &&
615-
copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
616-
req.certs_len)) {
620+
copy_to_user((void __user *)req->certs_address, snp_dev->certs_data,
621+
req->certs_len)) {
617622
ret = -EFAULT;
618623
goto e_free;
619624
}

0 commit comments

Comments
 (0)