6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Dan Williams <dan.j.williams@xxxxxxxxx>
[ Upstream commit db10cb9b574675402bfd8fe1a31aafdd45b002df ]
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@xxxxxxxxxx>
Closes: http://lore.kernel.org/r/CAMkAt6r2VPPMZ__SQfJse8qWsUyYW3AgYbOUVM0S_Vtk=KvkxQ@xxxxxxxxxxxxxx
Fixes: fce96cf04430 ("virt: Add SEV-SNP guest driver")
Cc: Borislav Petkov <bp@xxxxxxxxx>
Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
Cc: Dionna Glaze <dionnaglaze@xxxxxxxxxx>
Cc: Jeremi Piotrowski <jpiotrowski@xxxxxxxxxxxxxxxxxxx>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---
drivers/virt/coco/sev-guest/sev-guest.c | 45 ++++++++++++++-----------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 5cdc972e5ef4a..c47e54b2a865e 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -57,6 +57,11 @@ struct snp_guest_dev {
struct snp_secrets_page_layout *layout;
struct snp_req_data input;
+ union {
+ struct snp_report_req report;
+ struct snp_derived_key_req derived_key;
+ struct snp_ext_report_req ext_report;
+ } req;
u32 *os_area_msg_seqno;
u8 *vmpck;
};
@@ -475,8 +480,8 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
{
struct snp_guest_crypto *crypto = snp_dev->crypto;
+ struct snp_report_req *req = &snp_dev->req.report;
struct snp_report_resp *resp;
- struct snp_report_req req;
int rc, resp_len;
lockdep_assert_held(&snp_cmd_mutex);
@@ -484,7 +489,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
if (!arg->req_data || !arg->resp_data)
return -EINVAL;
- if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
return -EFAULT;
/*
@@ -498,7 +503,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
return -ENOMEM;
rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
- SNP_MSG_REPORT_REQ, &req, sizeof(req), resp->data,
+ SNP_MSG_REPORT_REQ, req, sizeof(*req), resp->data,
resp_len);
if (rc)
goto e_free;
@@ -513,9 +518,9 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
{
+ struct snp_derived_key_req *req = &snp_dev->req.derived_key;
struct snp_guest_crypto *crypto = snp_dev->crypto;
struct snp_derived_key_resp resp = {0};
- struct snp_derived_key_req req;
int rc, resp_len;
/* Response data is 64 bytes and max authsize for GCM is 16 bytes. */
u8 buf[64 + 16];
@@ -534,11 +539,11 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
if (sizeof(buf) < resp_len)
return -ENOMEM;
- if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
return -EFAULT;
rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
- SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len);
+ SNP_MSG_KEY_REQ, req, sizeof(*req), buf, resp_len);
if (rc)
return rc;
@@ -554,8 +559,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
{
+ struct snp_ext_report_req *req = &snp_dev->req.ext_report;
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;
@@ -564,18 +569,18 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
if (!arg->req_data || !arg->resp_data)
return -EINVAL;
- if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
return -EFAULT;
/* userspace does not want certificate data */
- if (!req.certs_len || !req.certs_address)
+ if (!req->certs_len || !req->certs_address)
goto cmd;
- if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
- !IS_ALIGNED(req.certs_len, PAGE_SIZE))
+ if (req->certs_len > SEV_FW_BLOB_MAX_SIZE ||
+ !IS_ALIGNED(req->certs_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->certs_len))
return -EFAULT;
/*
@@ -584,8 +589,8 @@ 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, req->certs_len);
+ npages = req->certs_len >> PAGE_SHIFT;
cmd:
/*
* The intermediate response buffer is used while decrypting the
@@ -599,14 +604,14 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
snp_dev->input.data_npages = npages;
ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
- SNP_MSG_REPORT_REQ, &req.data,
- sizeof(req.data), resp->data, resp_len);
+ SNP_MSG_REPORT_REQ, &req->data,
+ sizeof(req->data), resp->data, resp_len);
/* If certs length is invalid then copy the returned length */
if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
- req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
+ req->certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
- if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
+ if (copy_to_user((void __user *)arg->req_data, req, sizeof(*req)))
ret = -EFAULT;
}
@@ -614,8 +619,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
goto e_free;
if (npages &&
- copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
- req.certs_len)) {
+ copy_to_user((void __user *)req->certs_address, snp_dev->certs_data,
+ req->certs_len)) {
ret = -EFAULT;
goto e_free;
}
--
2.42.0
