Re: [PATCH v14 03/13] x86/sev: Add Secure TSC support for SNP guests

["Nikunj A. Dadhania" <nikunj@amd.com>]

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);
 }