From dc6d20b900b72bea89ebd8154ba9bde1029f330b Mon Sep 17 00:00:00 2001 From: Nikunj A Dadhania Date: Wed, 31 Jul 2024 20:37:52 +0530 Subject: [PATCH 1/4] virt: sev-guest: Replace dev_dbg() with pr_debug() In preparation for moving code to arch/x86/coco/sev/core.c, replace dev_dbg with pr_debug. No functional change. Signed-off-by: Nikunj A Dadhania Signed-off-by: Borislav Petkov (AMD) Reviewed-by: Tom Lendacky Reviewed-by: Borislav Petkov (AMD) Tested-by: Peter Gonda Link: https://lore.kernel.org/r/20240731150811.156771-2-nikunj@amd.com --- drivers/virt/coco/sev-guest/sev-guest.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 6fc7884ea0a1..7d343f2c6ef8 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -296,8 +296,9 @@ static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload, struct snp_guest_msg_hdr *req_hdr = &req->hdr; struct snp_guest_msg_hdr *resp_hdr = &resp->hdr; - dev_dbg(snp_dev->dev, "response [seqno %lld type %d version %d sz %d]\n", - resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version, resp_hdr->msg_sz); + pr_debug("response [seqno %lld type %d version %d sz %d]\n", + resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version, + resp_hdr->msg_sz); /* Copy response from shared memory to encrypted memory. */ memcpy(resp, snp_dev->response, sizeof(*resp)); @@ -343,8 +344,8 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8 if (!hdr->msg_seqno) return -ENOSR; - dev_dbg(snp_dev->dev, "request [seqno %lld type %d version %d sz %d]\n", - hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz); + pr_debug("request [seqno %lld type %d version %d sz %d]\n", + hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz); return __enc_payload(snp_dev, req, payload, sz); } From a1bbb2236bb97c0afee4cdf8fd732ff5f9cd60ac Mon Sep 17 00:00:00 2001 From: Nikunj A Dadhania Date: Wed, 31 Jul 2024 20:37:53 +0530 Subject: [PATCH 2/4] virt: sev-guest: Rename local guest message variables Rename local guest message variables for more clarity. No functional change. Signed-off-by: Nikunj A Dadhania Signed-off-by: Borislav Petkov (AMD) Link: https://lore.kernel.org/r/20240731150811.156771-3-nikunj@amd.com --- drivers/virt/coco/sev-guest/sev-guest.c | 117 ++++++++++++------------ 1 file changed, 59 insertions(+), 58 deletions(-) diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 7d343f2c6ef8..a72fe1e959c2 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -291,45 +291,45 @@ static int dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg, static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload, u32 sz) { struct snp_guest_crypto *crypto = snp_dev->crypto; - struct snp_guest_msg *resp = &snp_dev->secret_response; - struct snp_guest_msg *req = &snp_dev->secret_request; - struct snp_guest_msg_hdr *req_hdr = &req->hdr; - struct snp_guest_msg_hdr *resp_hdr = &resp->hdr; + struct snp_guest_msg *resp_msg = &snp_dev->secret_response; + struct snp_guest_msg *req_msg = &snp_dev->secret_request; + struct snp_guest_msg_hdr *req_msg_hdr = &req_msg->hdr; + struct snp_guest_msg_hdr *resp_msg_hdr = &resp_msg->hdr; pr_debug("response [seqno %lld type %d version %d sz %d]\n", - resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version, - resp_hdr->msg_sz); + resp_msg_hdr->msg_seqno, resp_msg_hdr->msg_type, resp_msg_hdr->msg_version, + resp_msg_hdr->msg_sz); /* Copy response from shared memory to encrypted memory. */ - memcpy(resp, snp_dev->response, sizeof(*resp)); + memcpy(resp_msg, snp_dev->response, sizeof(*resp_msg)); /* Verify that the sequence counter is incremented by 1 */ - if (unlikely(resp_hdr->msg_seqno != (req_hdr->msg_seqno + 1))) + if (unlikely(resp_msg_hdr->msg_seqno != (req_msg_hdr->msg_seqno + 1))) return -EBADMSG; /* Verify response message type and version number. */ - if (resp_hdr->msg_type != (req_hdr->msg_type + 1) || - resp_hdr->msg_version != req_hdr->msg_version) + if (resp_msg_hdr->msg_type != (req_msg_hdr->msg_type + 1) || + resp_msg_hdr->msg_version != req_msg_hdr->msg_version) return -EBADMSG; /* * If the message size is greater than our buffer length then return * an error. */ - if (unlikely((resp_hdr->msg_sz + crypto->a_len) > sz)) + if (unlikely((resp_msg_hdr->msg_sz + crypto->a_len) > sz)) return -EBADMSG; /* Decrypt the payload */ - return dec_payload(snp_dev, resp, payload, resp_hdr->msg_sz + crypto->a_len); + return dec_payload(snp_dev, resp_msg, payload, resp_msg_hdr->msg_sz + crypto->a_len); } static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8 type, void *payload, size_t sz) { - struct snp_guest_msg *req = &snp_dev->secret_request; - struct snp_guest_msg_hdr *hdr = &req->hdr; + struct snp_guest_msg *msg = &snp_dev->secret_request; + struct snp_guest_msg_hdr *hdr = &msg->hdr; - memset(req, 0, sizeof(*req)); + memset(msg, 0, sizeof(*msg)); hdr->algo = SNP_AEAD_AES_256_GCM; hdr->hdr_version = MSG_HDR_VER; @@ -347,7 +347,7 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8 pr_debug("request [seqno %lld type %d version %d sz %d]\n", hdr->msg_seqno, hdr->msg_type, hdr->msg_version, hdr->msg_sz); - return __enc_payload(snp_dev, req, payload, sz); + return __enc_payload(snp_dev, msg, payload, sz); } static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, @@ -496,8 +496,8 @@ struct snp_req_resp { 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 *report_req = &snp_dev->req.report; + struct snp_report_resp *report_resp; int rc, resp_len; lockdep_assert_held(&snp_cmd_mutex); @@ -505,7 +505,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(report_req, (void __user *)arg->req_data, sizeof(*report_req))) return -EFAULT; /* @@ -513,30 +513,29 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io * response payload. Make sure that it has enough space to cover the * authtag. */ - resp_len = sizeof(resp->data) + crypto->a_len; - resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); - if (!resp) + resp_len = sizeof(report_resp->data) + crypto->a_len; + report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); + if (!report_resp) return -ENOMEM; - rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg, - SNP_MSG_REPORT_REQ, req, sizeof(*req), resp->data, - resp_len); + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg, SNP_MSG_REPORT_REQ, + report_req, sizeof(*report_req), report_resp->data, resp_len); if (rc) goto e_free; - if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp))) + if (copy_to_user((void __user *)arg->resp_data, report_resp, sizeof(*report_resp))) rc = -EFAULT; e_free: - kfree(resp); + kfree(report_resp); return rc; } 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_derived_key_req *derived_key_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_resp derived_key_resp = {0}; int rc, resp_len; /* Response data is 64 bytes and max authsize for GCM is 16 bytes. */ u8 buf[64 + 16]; @@ -551,25 +550,27 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque * response payload. Make sure that it has enough space to cover the * authtag. */ - resp_len = sizeof(resp.data) + crypto->a_len; + resp_len = sizeof(derived_key_resp.data) + crypto->a_len; if (sizeof(buf) < resp_len) return -ENOMEM; - if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req))) + if (copy_from_user(derived_key_req, (void __user *)arg->req_data, + sizeof(*derived_key_req))) return -EFAULT; - rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg, - SNP_MSG_KEY_REQ, req, sizeof(*req), buf, resp_len); + rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg, SNP_MSG_KEY_REQ, + derived_key_req, sizeof(*derived_key_req), buf, resp_len); if (rc) return rc; - memcpy(resp.data, buf, sizeof(resp.data)); - if (copy_to_user((void __user *)arg->resp_data, &resp, sizeof(resp))) + memcpy(derived_key_resp.data, buf, sizeof(derived_key_resp.data)); + if (copy_to_user((void __user *)arg->resp_data, &derived_key_resp, + sizeof(derived_key_resp))) rc = -EFAULT; /* The response buffer contains the sensitive data, explicitly clear it. */ memzero_explicit(buf, sizeof(buf)); - memzero_explicit(&resp, sizeof(resp)); + memzero_explicit(&derived_key_resp, sizeof(derived_key_resp)); return rc; } @@ -577,9 +578,9 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; struct snp_guest_crypto *crypto = snp_dev->crypto; - struct snp_report_resp *resp; + struct snp_report_resp *report_resp; int ret, npages = 0, resp_len; sockptr_t certs_address; @@ -588,22 +589,22 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; - if (copy_from_sockptr(req, io->req_data, sizeof(*req))) + if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) return -EFAULT; /* caller does not want certificate data */ - if (!req->certs_len || !req->certs_address) + if (!report_req->certs_len || !report_req->certs_address) goto cmd; - if (req->certs_len > SEV_FW_BLOB_MAX_SIZE || - !IS_ALIGNED(req->certs_len, PAGE_SIZE)) + if (report_req->certs_len > SEV_FW_BLOB_MAX_SIZE || + !IS_ALIGNED(report_req->certs_len, PAGE_SIZE)) return -EINVAL; if (sockptr_is_kernel(io->resp_data)) { - certs_address = KERNEL_SOCKPTR((void *)req->certs_address); + certs_address = KERNEL_SOCKPTR((void *)report_req->certs_address); } else { - certs_address = USER_SOCKPTR((void __user *)req->certs_address); - if (!access_ok(certs_address.user, req->certs_len)) + certs_address = USER_SOCKPTR((void __user *)report_req->certs_address); + if (!access_ok(certs_address.user, report_req->certs_len)) return -EFAULT; } @@ -613,45 +614,45 @@ 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, report_req->certs_len); + npages = report_req->certs_len >> PAGE_SHIFT; cmd: /* * The intermediate response buffer is used while decrypting the * response payload. Make sure that it has enough space to cover the * authtag. */ - resp_len = sizeof(resp->data) + crypto->a_len; - resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); - if (!resp) + resp_len = sizeof(report_resp->data) + crypto->a_len; + report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); + if (!report_resp) return -ENOMEM; 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); + ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg, SNP_MSG_REPORT_REQ, + &report_req->data, sizeof(report_req->data), + report_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; + report_req->certs_len = snp_dev->input.data_npages << PAGE_SHIFT; - if (copy_to_sockptr(io->req_data, req, sizeof(*req))) + if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req))) ret = -EFAULT; } if (ret) goto e_free; - if (npages && copy_to_sockptr(certs_address, snp_dev->certs_data, req->certs_len)) { + if (npages && copy_to_sockptr(certs_address, snp_dev->certs_data, report_req->certs_len)) { ret = -EFAULT; goto e_free; } - if (copy_to_sockptr(io->resp_data, resp, sizeof(*resp))) + if (copy_to_sockptr(io->resp_data, report_resp, sizeof(*report_resp))) ret = -EFAULT; e_free: - kfree(resp); + kfree(report_resp); return ret; } From 5f7c38f81df206b370d97a827251bd4bc50ff46b Mon Sep 17 00:00:00 2001 From: Nikunj A Dadhania Date: Wed, 31 Jul 2024 20:37:54 +0530 Subject: [PATCH 3/4] virt: sev-guest: Fix user-visible strings User-visible abbreviations should be in capitals, ensure messages are readable and clear. No functional change. Signed-off-by: Nikunj A Dadhania Signed-off-by: Borislav Petkov (AMD) Link: https://lore.kernel.org/r/20240731150811.156771-4-nikunj@amd.com --- drivers/virt/coco/sev-guest/sev-guest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index a72fe1e959c2..3b76cbf78f41 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -114,7 +114,7 @@ static bool is_vmpck_empty(struct snp_guest_dev *snp_dev) */ static void snp_disable_vmpck(struct snp_guest_dev *snp_dev) { - dev_alert(snp_dev->dev, "Disabling vmpck_id %d to prevent IV reuse.\n", + dev_alert(snp_dev->dev, "Disabling VMPCK%d communication key to prevent IV reuse.\n", vmpck_id); memzero_explicit(snp_dev->vmpck, VMPCK_KEY_LEN); snp_dev->vmpck = NULL; @@ -1117,13 +1117,13 @@ static int __init sev_guest_probe(struct platform_device *pdev) ret = -EINVAL; snp_dev->vmpck = get_vmpck(vmpck_id, secrets, &snp_dev->os_area_msg_seqno); if (!snp_dev->vmpck) { - dev_err(dev, "invalid vmpck id %d\n", vmpck_id); + dev_err(dev, "Invalid VMPCK%d communication key\n", vmpck_id); goto e_unmap; } /* Verify that VMPCK is not zero. */ if (is_vmpck_empty(snp_dev)) { - dev_err(dev, "vmpck id %d is null\n", vmpck_id); + dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id); goto e_unmap; } @@ -1174,7 +1174,7 @@ static int __init sev_guest_probe(struct platform_device *pdev) if (ret) goto e_free_cert_data; - dev_info(dev, "Initialized SEV guest driver (using vmpck_id %d)\n", vmpck_id); + dev_info(dev, "Initialized SEV guest driver (using VMPCK%d communication key)\n", vmpck_id); return 0; e_free_cert_data: From 2b9ac0b84c2cae91bbaceab62df4de6d503421ec Mon Sep 17 00:00:00 2001 From: Nikunj A Dadhania Date: Wed, 31 Jul 2024 20:37:55 +0530 Subject: [PATCH 4/4] virt: sev-guest: Ensure the SNP guest messages do not exceed a page Currently, struct snp_guest_msg includes a message header (96 bytes) and a payload (4000 bytes). There is an implicit assumption here that the SNP message header will always be 96 bytes, and with that assumption the payload array size has been set to 4000 bytes - a magic number. If any new member is added to the SNP message header, the SNP guest message will span more than a page. Instead of using a magic number for the payload, declare struct snp_guest_msg in a way that payload plus the message header do not exceed a page. [ bp: Massage. ] Suggested-by: Tom Lendacky Signed-off-by: Nikunj A Dadhania Signed-off-by: Borislav Petkov (AMD) Acked-by: Borislav Petkov (AMD) Link: https://lore.kernel.org/r/20240731150811.156771-5-nikunj@amd.com --- arch/x86/include/asm/sev.h | 2 +- drivers/virt/coco/sev-guest/sev-guest.c | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 79bbe2be900e..ee34ab00a8d6 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -164,7 +164,7 @@ struct snp_guest_msg_hdr { struct snp_guest_msg { struct snp_guest_msg_hdr hdr; - u8 payload[4000]; + u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)]; } __packed; struct sev_guest_platform_data { diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 3b76cbf78f41..89754b019be2 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -1092,6 +1092,8 @@ static int __init sev_guest_probe(struct platform_device *pdev) void __iomem *mapping; int ret; + BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE); + if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) return -ENODEV;