On 11/11/2024 4:21 PM, Borislav Petkov wrote:
> On Mon, Nov 11, 2024 at 02:16:00PM +0530, Nikunj A. Dadhania wrote:
>> That was the reason I had not implemented "free" counterpart.
>
> Then let's simplify this too because it is kinda silly right now:
When snp_msg_alloc() is called by the sev-guest driver, secrets will
be reinitialized and buffers will be re-allocated, leaking memory
allocated during snp_get_tsc_info()::snp_msg_alloc().
As you suggested, implementing a "free" counterpart will be better.
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 8ecac0ca419b..12b167fd6475 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -488,14 +488,7 @@ static inline bool snp_is_vmpck_empty(struct snp_msg_desc *mdesc)
int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id);
struct snp_msg_desc *snp_msg_alloc(void);
-
-static inline void snp_msg_cleanup(struct snp_msg_desc *mdesc)
-{
- mdesc->vmpck = NULL;
- mdesc->os_area_msg_seqno = NULL;
- kfree(mdesc->ctx);
-}
-
+void snp_msg_free(struct snp_msg_desc *mdesc);
int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
struct snp_guest_request_ioctl *rio);
@@ -541,7 +534,7 @@ static inline void snp_kexec_begin(void) { }
static inline bool snp_is_vmpck_empty(struct snp_msg_desc *mdesc) { return false; }
static inline int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id) { return -1; }
static inline struct snp_msg_desc *snp_msg_alloc(void) { return NULL; }
-static inline void snp_msg_cleanup(struct snp_msg_desc *mdesc) { }
+static inline void snp_msg_free(struct snp_msg_desc *mdesc) { }
static inline int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req,
struct snp_guest_request_ioctl *rio) { return -ENODEV; }
static inline void __init snp_secure_tsc_prepare(void) { }
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index d40d4528c1eb..25fb8e79eb9b 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -96,8 +96,6 @@ static u64 sev_hv_features __ro_after_init;
/* Secrets page physical address from the CC blob */
static u64 secrets_pa __ro_after_init;
-static struct snp_msg_desc *snp_mdesc;
-
/* Secure TSC values read using TSC_INFO SNP Guest request */
static u64 snp_tsc_scale __ro_after_init;
static u64 snp_tsc_offset __ro_after_init;
@@ -2818,9 +2816,6 @@ struct snp_msg_desc *snp_msg_alloc(void)
BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
- if (snp_mdesc)
- return snp_mdesc;
-
mdesc = kzalloc(sizeof(struct snp_msg_desc), GFP_KERNEL);
if (!mdesc)
return ERR_PTR(-ENOMEM);
@@ -2848,8 +2843,6 @@ struct snp_msg_desc *snp_msg_alloc(void)
mdesc->input.resp_gpa = __pa(mdesc->response);
mdesc->input.data_gpa = __pa(mdesc->certs_data);
- snp_mdesc = mdesc;
-
return mdesc;
e_free_response:
@@ -2858,11 +2851,29 @@ struct snp_msg_desc *snp_msg_alloc(void)
free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
e_unmap:
iounmap((__force void __iomem *)mdesc->secrets);
+ kfree(mdesc);
return ERR_PTR(-ENOMEM);
}
EXPORT_SYMBOL_GPL(snp_msg_alloc);
+void snp_msg_free(struct snp_msg_desc *mdesc)
+{
+ if (!mdesc)
+ return;
+
+ mdesc->vmpck = NULL;
+ mdesc->os_area_msg_seqno = NULL;
+ kfree(mdesc->ctx);
+
+ free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
+ free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
+ iounmap((__force void __iomem *)mdesc->secrets);
+
+ kfree(mdesc);
+}
+EXPORT_SYMBOL_GPL(snp_msg_free);
+
/* Mutex to serialize the shared buffer access and command handling. */
static DEFINE_MUTEX(snp_cmd_mutex);
@@ -3179,8 +3190,10 @@ static int __init snp_get_tsc_info(void)
return rc;
tsc_req = kzalloc(sizeof(struct snp_tsc_info_req), GFP_KERNEL);
- if (!tsc_req)
- return -ENOMEM;
+ if (!tsc_req) {
+ rc = -ENOMEM;
+ goto e_free_mdesc;
+ }
memset(&req, 0, sizeof(req));
memset(&rio, 0, sizeof(rio));
@@ -3197,7 +3210,7 @@ static int __init snp_get_tsc_info(void)
rc = snp_send_guest_request(mdesc, &req, &rio);
if (rc)
- goto err_req;
+ goto e_request;
memcpy(&tsc_resp, buf, sizeof(tsc_resp));
pr_debug("%s: response status %x scale %llx offset %llx factor %x\n",
@@ -3212,11 +3225,14 @@ static int __init snp_get_tsc_info(void)
rc = -EIO;
}
-err_req:
+e_request:
/* The response buffer contains the sensitive data, explicitly clear it. */
memzero_explicit(buf, sizeof(buf));
memzero_explicit(&tsc_resp, sizeof(tsc_resp));
+e_free_mdesc:
+ snp_msg_free(mdesc);
+
return rc;
}
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index d64efc489686..084ea9499e75 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -647,7 +647,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
return 0;
e_msg_init:
- snp_msg_cleanup(mdesc);
+ snp_msg_free(mdesc);
return ret;
}
@@ -656,7 +656,7 @@ static void __exit sev_guest_remove(struct platform_device *pdev)
{
struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
- snp_msg_cleanup(snp_dev->msg_desc);
+ snp_msg_free(snp_dev->msg_desc);
misc_deregister(&snp_dev->misc);
}
