diff mbox series

[v15,01/13] x86/sev: Carve out and export SNP guest messaging init routines

Message ID 20241203090045.942078-2-nikunj@amd.com (mailing list archive)
State New
Headers show
Series Add Secure TSC support for SNP guests | expand

Commit Message

Nikunj A. Dadhania Dec. 3, 2024, 9 a.m. UTC
Currently, the sev-guest driver is the only user of SNP guest messaging.
All routines for initializing SNP guest messaging are implemented within
the sev-guest driver and are not available during early boot. In
prepratation for adding Secure TSC guest support, carve out APIs to
allocate and initialize guest messaging descriptor context and make it part
of coco/sev/core.c. As there is no user of sev_guest_platform_data anymore,
remove the structure.

Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/sev.h              |  24 ++-
 arch/x86/coco/sev/core.c                | 183 +++++++++++++++++++++-
 drivers/virt/coco/sev-guest/sev-guest.c | 197 +++---------------------
 arch/x86/Kconfig                        |   1 +
 drivers/virt/coco/sev-guest/Kconfig     |   1 -
 5 files changed, 220 insertions(+), 186 deletions(-)

Comments

Borislav Petkov Dec. 3, 2024, 2:19 p.m. UTC | #1
On Tue, Dec 03, 2024 at 02:30:33PM +0530, Nikunj A Dadhania wrote:
> Currently, the sev-guest driver is the only user of SNP guest messaging.
> All routines for initializing SNP guest messaging are implemented within
> the sev-guest driver and are not available during early boot. In
> prepratation for adding Secure TSC guest support, carve out APIs to

Unknown word [prepratation] in commit message.
Suggestions: ['preparation', 'preparations', 'reparation', 'perpetration', 'reputation', 'perpetuation', 'peroration', 'presentation', 'repatriation', 'propagation', "preparation's"]

Please introduce a spellchecker into your patch creation workflow.

> allocate and initialize guest messaging descriptor context and make it part
> of coco/sev/core.c. As there is no user of sev_guest_platform_data anymore,
> remove the structure.
> 
> Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  arch/x86/include/asm/sev.h              |  24 ++-
>  arch/x86/coco/sev/core.c                | 183 +++++++++++++++++++++-
>  drivers/virt/coco/sev-guest/sev-guest.c | 197 +++---------------------
>  arch/x86/Kconfig                        |   1 +
>  drivers/virt/coco/sev-guest/Kconfig     |   1 -
>  5 files changed, 220 insertions(+), 186 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
> index 91f08af31078..f78c94e29c74 100644
> --- a/arch/x86/include/asm/sev.h
> +++ b/arch/x86/include/asm/sev.h
> @@ -14,6 +14,7 @@
>  #include <asm/insn.h>
>  #include <asm/sev-common.h>
>  #include <asm/coco.h>
> +#include <asm/set_memory.h>
>  
>  #define GHCB_PROTOCOL_MIN	1ULL
>  #define GHCB_PROTOCOL_MAX	2ULL
> @@ -170,10 +171,6 @@ struct snp_guest_msg {
>  	u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];
>  } __packed;
>  
> -struct sev_guest_platform_data {
> -	u64 secrets_gpa;
> -};
> -
>  struct snp_guest_req {
>  	void *req_buf;
>  	size_t req_sz;
> @@ -253,6 +250,7 @@ struct snp_msg_desc {
>  
>  	u32 *os_area_msg_seqno;
>  	u8 *vmpck;
> +	int vmpck_id;
>  };
>  
>  /*
> @@ -458,6 +456,20 @@ void set_pte_enc_mask(pte_t *kpte, unsigned long pfn, pgprot_t new_prot);
>  void snp_kexec_finish(void);
>  void snp_kexec_begin(void);
>  
> +static inline bool snp_is_vmpck_empty(struct snp_msg_desc *mdesc)
> +{
> +	static const char zero_key[VMPCK_KEY_LEN] = {0};
> +
> +	if (mdesc->vmpck)
> +		return !memcmp(mdesc->vmpck, zero_key, VMPCK_KEY_LEN);
> +
> +	return true;
> +}

This function looks silly in a header with that array allocation.

I think you should simply do:

	if (memchr_inv(mdesc->vmpck, 0, VMPCK_KEY_LEN))

at the call sites and not have this helper at all.

But please do verify whether what I'm saying actually makes sense and if it
does, this can be a cleanup pre-patch.


> +
> +int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id);
> +struct snp_msg_desc *snp_msg_alloc(void);
> +void snp_msg_free(struct snp_msg_desc *mdesc);
> +
>  #else	/* !CONFIG_AMD_MEM_ENCRYPT */
>  
>  #define snp_vmpl 0
> @@ -498,6 +510,10 @@ static inline int prepare_pte_enc(struct pte_enc_desc *d) { return 0; }
>  static inline void set_pte_enc_mask(pte_t *kpte, unsigned long pfn, pgprot_t new_prot) { }
>  static inline void snp_kexec_finish(void) { }
>  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_free(struct snp_msg_desc *mdesc) { }
>  
>  #endif	/* CONFIG_AMD_MEM_ENCRYPT */
>  
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index c5b0148b8c0a..3cc741eefd06 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -25,6 +25,7 @@
>  #include <linux/psp-sev.h>
>  #include <linux/dmi.h>
>  #include <uapi/linux/sev-guest.h>
> +#include <crypto/gcm.h>
>  
>  #include <asm/init.h>
>  #include <asm/cpu_entry_area.h>
> @@ -2580,15 +2581,9 @@ static struct platform_device sev_guest_device = {
>  
>  static int __init snp_init_platform_device(void)
>  {
> -	struct sev_guest_platform_data data;
> -
>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>  		return -ENODEV;
>  
> -	data.secrets_gpa = secrets_pa;
> -	if (platform_device_add_data(&sev_guest_device, &data, sizeof(data)))
> -		return -ENODEV;
> -
>  	if (platform_device_register(&sev_guest_device))
>  		return -ENODEV;
>  
> @@ -2667,3 +2662,179 @@ static int __init sev_sysfs_init(void)
>  }
>  arch_initcall(sev_sysfs_init);
>  #endif // CONFIG_SYSFS
> +
> +static void free_shared_pages(void *buf, size_t sz)
> +{
> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> +	int ret;
> +
> +	if (!buf)
> +		return;
> +
> +	ret = set_memory_encrypted((unsigned long)buf, npages);
> +	if (ret) {
> +		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");

Looking at where this lands:

set_memory_encrypted
|-> __set_memory_enc_dec

and that doing now:

        if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
                if (!down_read_trylock(&mem_enc_lock))
                        return -EBUSY;


after

859e63b789d6 ("x86/tdx: Convert shared memory back to private on kexec")

we probably should pay attention to this here firing and maybe turning that
_trylock() into a normal down_read*

Anyway, just something to pay attention to in the future.

> +		return;
> +	}
> +
> +	__free_pages(virt_to_page(buf), get_order(sz));
> +}

...

> +struct snp_msg_desc *snp_msg_alloc(void)
> +{
> +	struct snp_msg_desc *mdesc;
> +	void __iomem *mem;
> +
> +	BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
> +
> +	mdesc = kzalloc(sizeof(struct snp_msg_desc), GFP_KERNEL);

The above ones use GFP_KERNEL_ACCOUNT. What's the difference?

> +	if (!mdesc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mem = ioremap_encrypted(secrets_pa, PAGE_SIZE);
> +	if (!mem)
> +		goto e_free_mdesc;
> +
> +	mdesc->secrets = (__force struct snp_secrets_page *)mem;
> +
> +	/* Allocate the shared page used for the request and response message. */
> +	mdesc->request = alloc_shared_pages(sizeof(struct snp_guest_msg));
> +	if (!mdesc->request)
> +		goto e_unmap;
> +
> +	mdesc->response = alloc_shared_pages(sizeof(struct snp_guest_msg));
> +	if (!mdesc->response)
> +		goto e_free_request;
> +
> +	mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE);
> +	if (!mdesc->certs_data)
> +		goto e_free_response;
> +
> +	/* initial the input address for guest request */
> +	mdesc->input.req_gpa = __pa(mdesc->request);
> +	mdesc->input.resp_gpa = __pa(mdesc->response);
> +	mdesc->input.data_gpa = __pa(mdesc->certs_data);
> +
> +	return mdesc;
> +
> +e_free_response:
> +	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
> +e_free_request:
> +	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
> +e_unmap:
> +	iounmap(mem);
> +e_free_mdesc:
> +	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;

	memset(mdesc, ...);

at the end instead of those assignments.

> +	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);
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index b699771be029..5268511bc9b8 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c

...

> @@ -993,115 +898,57 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>  		return -ENODEV;
>  
> -	if (!dev->platform_data)
> -		return -ENODEV;
> -
> -	data = (struct sev_guest_platform_data *)dev->platform_data;
> -	mapping = ioremap_encrypted(data->secrets_gpa, PAGE_SIZE);
> -	if (!mapping)
> -		return -ENODEV;
> -
> -	secrets = (__force void *)mapping;
> -
> -	ret = -ENOMEM;
>  	snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
>  	if (!snp_dev)
> -		goto e_unmap;
> -
> -	mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
> -	if (!mdesc)
> -		goto e_unmap;
> -
> -	/* Adjust the default VMPCK key based on the executing VMPL level */
> -	if (vmpck_id == -1)
> -		vmpck_id = snp_vmpl;
> +		return -ENOMEM;
>  
> -	ret = -EINVAL;
> -	mdesc->vmpck = get_vmpck(vmpck_id, secrets, &mdesc->os_area_msg_seqno);
> -	if (!mdesc->vmpck) {
> -		dev_err(dev, "Invalid VMPCK%d communication key\n", vmpck_id);
> -		goto e_unmap;
> -	}
> +	mdesc = snp_msg_alloc();
> +	if (IS_ERR_OR_NULL(mdesc))
> +		return -ENOMEM;
>  
> -	/* Verify that VMPCK is not zero. */
> -	if (is_vmpck_empty(mdesc)) {
> -		dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id);
> -		goto e_unmap;
> -	}
> +	ret = snp_msg_init(mdesc, vmpck_id);
> +	if (ret)
> +		return -EIO;

You just leaked mdesc here.

Audit all your error paths.

Thx.
Nikunj A. Dadhania Dec. 3, 2024, 2:35 p.m. UTC | #2
On 12/3/2024 7:49 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 02:30:33PM +0530, Nikunj A Dadhania wrote:
>> Currently, the sev-guest driver is the only user of SNP guest messaging.
>> All routines for initializing SNP guest messaging are implemented within
>> the sev-guest driver and are not available during early boot. In
>> prepratation for adding Secure TSC guest support, carve out APIs to
> 
> Unknown word [prepratation] in commit message.
> Suggestions: ['preparation', 'preparations', 'reparation', 'perpetration', 'reputation', 'perpetuation', 'peroration', 'presentation', 'repatriation', 'propagation', "preparation's"]
> 
> Please introduce a spellchecker into your patch creation workflow.

This is what I use with checkpatch, that didnt catch the wrong spelling. Do you suggest using something else ?

./scripts/checkpatch.pl --codespell < sectsc_v15/v15-0001-x86-sev-Carve-out-and-export-SNP-guest-messaging.patch
total: 0 errors, 0 warnings, 569 lines checked

"[PATCH v15 01/13] x86/sev: Carve out and export SNP guest messaging" has no obvious style problems and is ready for submission.

Regards
Nikunj
Borislav Petkov Dec. 3, 2024, 2:50 p.m. UTC | #3
On Tue, Dec 03, 2024 at 08:05:32PM +0530, Nikunj A. Dadhania wrote:
> This is what I use with checkpatch, that didnt catch the wrong spelling.

Not surprised.

> Do you suggest using something else ?

You can enable spellchecking in your editor with which you write the commit
messages. For example:

https://www.linux.com/training-tutorials/using-spell-checking-vim/

Or, you can use my tool:

https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=vp

You'd need to fish it out of the repo.

It doesn't completely replace checkpatch yet but I am extending it with
features as I go. But it does spellcheck:

$ ~/dev/vp/.tip/bin/vp.py ~/tmp/review/new
prepratation for adding Secure TSC guest support, carve out APIs to
Unknown word [prepratation] in commit message.
Suggestions: ['preparation', 'preparations', 'reparation', 'perpetration', 'reputation', 'perpetuation', 'peroration', 'presentation', 'repatriation', 'propagation', "preparation's"]

Class patch:
    original subject: [[PATCH v15 01/13] x86/sev: Carve out and export SNP guest messaging init routines]
             subject: [x86/sev: Carve out and export SNP guest messaging init routines]
              sender: [Nikunj A Dadhania <nikunj@amd.com>]
              author: [Nikunj A Dadhania <nikunj@amd.com>]
             version: [15]
              number: [1]
                name: [x86-sev-carve_out_and_export_snp_guest_messaging_init_routines]
                date: [Tue, 03 Dec 2024 14:30:33 +0530]
          message-id: [20241203090045.942078-2-nikunj@amd.com]

I'm sure there are gazillion other ways to automate it, ofc.

HTH.
Nikunj A. Dadhania Dec. 3, 2024, 2:52 p.m. UTC | #4
On 12/3/2024 8:20 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 08:05:32PM +0530, Nikunj A. Dadhania wrote:
>> This is what I use with checkpatch, that didnt catch the wrong spelling.
> 
> Not surprised.
> 
>> Do you suggest using something else ?
> 
> You can enable spellchecking in your editor with which you write the commit
> messages. For example:
> 
> https://www.linux.com/training-tutorials/using-spell-checking-vim/
> 
> Or, you can use my tool:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=vp
> 
> You'd need to fish it out of the repo.

Sure will give it a try.

Regards
Nikunj
Nikunj A. Dadhania Dec. 4, 2024, 9:30 a.m. UTC | #5
On 12/3/2024 7:49 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 02:30:33PM +0530, Nikunj A Dadhania wrote:
>> @@ -458,6 +456,20 @@ void set_pte_enc_mask(pte_t *kpte, unsigned long pfn, pgprot_t new_prot);
>>  void snp_kexec_finish(void);
>>  void snp_kexec_begin(void);
>>  
>> +static inline bool snp_is_vmpck_empty(struct snp_msg_desc *mdesc)
>> +{
>> +	static const char zero_key[VMPCK_KEY_LEN] = {0};
>> +
>> +	if (mdesc->vmpck)
>> +		return !memcmp(mdesc->vmpck, zero_key, VMPCK_KEY_LEN);
>> +
>> +	return true;
>> +}
> 
> This function looks silly in a header with that array allocation.
> 
> I think you should simply do:
> 
> 	if (memchr_inv(mdesc->vmpck, 0, VMPCK_KEY_LEN))

Just a minor nit, it will need a negation:

 	if (!memchr_inv(mdesc->vmpck, 0, VMPCK_KEY_LEN))
 
> at the call sites and not have this helper at all.
> 
> But please do verify whether what I'm saying actually makes sense and if it
> does, this can be a cleanup pre-patch.

Yes, it makes sense. I have verified the code and below is the cleanup pre-patch.

From 4d249f393aeba7bed7fb99778b8ee8a24a33b5b7 Mon Sep 17 00:00:00 2001
From: Nikunj A Dadhania <nikunj@amd.com>
Date: Tue, 3 Dec 2024 20:48:28 +0530
Subject: [PATCH] virt: sev-guest: Remove is_vmpck_empty() helper

Remove the is_vmpck_empty() helper function, which uses a local array
allocation to check if the VMPCK is empty. Replace it with memchr_inv() to
directly determine if the VMPCK is empty without additional memory
allocation.

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index b699771be029..62328d0b2cb6 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -63,16 +63,6 @@ MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP.
 /* Mutex to serialize the shared buffer access and command handling. */
 static DEFINE_MUTEX(snp_cmd_mutex);
 
-static bool is_vmpck_empty(struct snp_msg_desc *mdesc)
-{
-	char zero_key[VMPCK_KEY_LEN] = {0};
-
-	if (mdesc->vmpck)
-		return !memcmp(mdesc->vmpck, zero_key, VMPCK_KEY_LEN);
-
-	return true;
-}
-
 /*
  * If an error is received from the host or AMD Secure Processor (ASP) there
  * are two options. Either retry the exact same encrypted request or discontinue
@@ -335,7 +325,7 @@ static int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	guard(mutex)(&snp_cmd_mutex);
 
 	/* Check if the VMPCK is not empty */
-	if (is_vmpck_empty(mdesc)) {
+	if (!mdesc->vmpck || !memchr_inv(mdesc->vmpck, 0, VMPCK_KEY_LEN)) {
 		pr_err_ratelimited("VMPCK is disabled\n");
 		return -ENOTTY;
 	}
@@ -1024,7 +1014,7 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 	}
 
 	/* Verify that VMPCK is not zero. */
-	if (is_vmpck_empty(mdesc)) {
+	if (!memchr_inv(mdesc->vmpck, 0, VMPCK_KEY_LEN)) {
 		dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id);
 		goto e_unmap;
 	}
Nikunj A. Dadhania Dec. 4, 2024, 10 a.m. UTC | #6
On 12/3/2024 7:49 PM, Borislav Petkov wrote:
> On Tue, Dec 03, 2024 at 02:30:33PM +0530, Nikunj A Dadhania wrote:
 
>> @@ -2667,3 +2662,179 @@ static int __init sev_sysfs_init(void)
>>  }
>>  arch_initcall(sev_sysfs_init);
>>  #endif // CONFIG_SYSFS
>> +
>> +static void free_shared_pages(void *buf, size_t sz)
>> +{
>> +	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>> +	int ret;
>> +
>> +	if (!buf)
>> +		return;
>> +
>> +	ret = set_memory_encrypted((unsigned long)buf, npages);
>> +	if (ret) {
>> +		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
> 
> Looking at where this lands:
> 
> set_memory_encrypted
> |-> __set_memory_enc_dec
> 
> and that doing now:
> 
>         if (cc_platform_has(CC_ATTR_MEM_ENCRYPT)) {
>                 if (!down_read_trylock(&mem_enc_lock))
>                         return -EBUSY;
> 
> 
> after
> 
> 859e63b789d6 ("x86/tdx: Convert shared memory back to private on kexec")
> 
> we probably should pay attention to this here firing and maybe turning that
> _trylock() into a normal down_read*
> 
> Anyway, just something to pay attention to in the future.

Yes, will keep an eye.

> 
>> +		return;
>> +	}
>> +
>> +	__free_pages(virt_to_page(buf), get_order(sz));
>> +}
> 
> ...
> 
>> +struct snp_msg_desc *snp_msg_alloc(void)
>> +{
>> +	struct snp_msg_desc *mdesc;
>> +	void __iomem *mem;
>> +
>> +	BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
>> +
>> +	mdesc = kzalloc(sizeof(struct snp_msg_desc), GFP_KERNEL);
> 
> The above ones use GFP_KERNEL_ACCOUNT. What's the difference?

The above ones I have retained old code.

GFP_KERNEL_ACCOUNT allocation are accounted in kmemcg and the below note from[1]
----------------------------------------------------------------------------
Untrusted allocations triggered from userspace should be a subject of kmem
accounting and must have __GFP_ACCOUNT bit set. There is the handy
GFP_KERNEL_ACCOUNT shortcut for GFP_KERNEL allocations that should be accounted.
----------------------------------------------------------------------------

For mdesc, I had kept it similar to snp_dev allocation, that is why it is 
having GFP_KERNEL.

        snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
        if (!snp_dev)
-               goto e_unmap;
-
-       mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);

Let me know if mdesc allocation need to be GFP_KERNEL_ACCOUNT.

>> +void snp_msg_free(struct snp_msg_desc *mdesc)
>> +{
>> +	if (!mdesc)
>> +		return;
>> +
>> +	mdesc->vmpck = NULL;
>> +	mdesc->os_area_msg_seqno = NULL;
> 
> 	memset(mdesc, ...);
> 
> at the end instead of those assignments.

Sure.

> 
>> +	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);
>> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
>> index b699771be029..5268511bc9b8 100644
>> --- a/drivers/virt/coco/sev-guest/sev-guest.c
>> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> 
> ...
> 
>> @@ -993,115 +898,57 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>>  	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
>>  		return -ENODEV;
>>  
>> -	if (!dev->platform_data)
>> -		return -ENODEV;
>> -
>> -	data = (struct sev_guest_platform_data *)dev->platform_data;
>> -	mapping = ioremap_encrypted(data->secrets_gpa, PAGE_SIZE);
>> -	if (!mapping)
>> -		return -ENODEV;
>> -
>> -	secrets = (__force void *)mapping;
>> -
>> -	ret = -ENOMEM;
>>  	snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
>>  	if (!snp_dev)
>> -		goto e_unmap;
>> -
>> -	mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
>> -	if (!mdesc)
>> -		goto e_unmap;
>> -
>> -	/* Adjust the default VMPCK key based on the executing VMPL level */
>> -	if (vmpck_id == -1)
>> -		vmpck_id = snp_vmpl;
>> +		return -ENOMEM;
>>  
>> -	ret = -EINVAL;
>> -	mdesc->vmpck = get_vmpck(vmpck_id, secrets, &mdesc->os_area_msg_seqno);
>> -	if (!mdesc->vmpck) {
>> -		dev_err(dev, "Invalid VMPCK%d communication key\n", vmpck_id);
>> -		goto e_unmap;
>> -	}
>> +	mdesc = snp_msg_alloc();
>> +	if (IS_ERR_OR_NULL(mdesc))
>> +		return -ENOMEM;
>>  
>> -	/* Verify that VMPCK is not zero. */
>> -	if (is_vmpck_empty(mdesc)) {
>> -		dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id);
>> -		goto e_unmap;
>> -	}
>> +	ret = snp_msg_init(mdesc, vmpck_id);
>> +	if (ret)
>> +		return -EIO;
> 
> You just leaked mdesc here.

Right

> Audit all your error paths.

Sure I will audit and send updated patch.

Regards
Nikunj


1) https://www.kernel.org/doc/html/v6.12/core-api/memory-allocation.html
Borislav Petkov Dec. 4, 2024, 8:02 p.m. UTC | #7
On Wed, Dec 04, 2024 at 03:30:13PM +0530, Nikunj A. Dadhania wrote:
> The above ones I have retained old code.

Right.

> GFP_KERNEL_ACCOUNT allocation are accounted in kmemcg and the below note from[1]
> ----------------------------------------------------------------------------
> Untrusted allocations triggered from userspace should be a subject of kmem
> accounting and must have __GFP_ACCOUNT bit set. There is the handy
> GFP_KERNEL_ACCOUNT shortcut for GFP_KERNEL allocations that should be accounted.
> ----------------------------------------------------------------------------

Interesting.

> For mdesc, I had kept it similar to snp_dev allocation, that is why it is 
> having GFP_KERNEL.
> 
>         snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
>         if (!snp_dev)
> -               goto e_unmap;
> -
> -       mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
> 
> Let me know if mdesc allocation need to be GFP_KERNEL_ACCOUNT.

Let's audit that thing:

* snp_init_crypto - not really untrusted allocation. It is on the driver probe
path.

* get_report - I don't think so:

        /*      
         * 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(report_resp->data) + mdesc->ctx->authsize;
        report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);

That resp_len is limited and that's on the guest_ioctl path which cannot
happen concurrently?

* get_ext_report - ditto

* alloc_shared_pages - all the allocations are limited but I guess that could
remain _ACCOUNT as a measure for future robustness.

And that was it.

So AFAICT, only one use case is semi-valid.

So maybe we should convert those remaining ones to boring GFP_KERNEL...
Nikunj A. Dadhania Dec. 5, 2024, 6:23 a.m. UTC | #8
On 12/5/2024 1:32 AM, Borislav Petkov wrote:
> On Wed, Dec 04, 2024 at 03:30:13PM +0530, Nikunj A. Dadhania wrote:
>> The above ones I have retained old code.
> 
> Right.
> 
>> GFP_KERNEL_ACCOUNT allocation are accounted in kmemcg and the below note from[1]
>> ----------------------------------------------------------------------------
>> Untrusted allocations triggered from userspace should be a subject of kmem
>> accounting and must have __GFP_ACCOUNT bit set. There is the handy
>> GFP_KERNEL_ACCOUNT shortcut for GFP_KERNEL allocations that should be accounted.
>> ----------------------------------------------------------------------------
> 
> Interesting.
> 
>> For mdesc, I had kept it similar to snp_dev allocation, that is why it is 
>> having GFP_KERNEL.
>>
>>         snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
>>         if (!snp_dev)
>> -               goto e_unmap;
>> -
>> -       mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
>>
>> Let me know if mdesc allocation need to be GFP_KERNEL_ACCOUNT.
> 
> Let's audit that thing:
> 
> * snp_init_crypto - not really untrusted allocation. It is on the driver probe
> path.
> 
> * get_report - I don't think so:
> 
>         /*      
>          * 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(report_resp->data) + mdesc->ctx->authsize;
>         report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
> 
> That resp_len is limited and that's on the guest_ioctl path which cannot
> happen concurrently?

It is a trusted allocation, but should it be accounted as it is part of
the userspace ioctl path ?

> 
> * get_ext_report - ditto
> 
> * alloc_shared_pages - all the allocations are limited but I guess that could
> remain _ACCOUNT as a measure for future robustness.

Ok.

> And that was it.
> 
> So AFAICT, only one use case is semi-valid.
> 
> So maybe we should convert those remaining ones to boring GFP_KERNEL...
> 

Sure, let me add this as a pre-patch.

Regards,
Nikunj
Borislav Petkov Dec. 6, 2024, 8:27 p.m. UTC | #9
On Thu, Dec 05, 2024 at 11:53:53AM +0530, Nikunj A. Dadhania wrote:
> > * get_report - I don't think so:
> > 
> >         /*      
> >          * 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(report_resp->data) + mdesc->ctx->authsize;
> >         report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
> > 
> > That resp_len is limited and that's on the guest_ioctl path which cannot
> > happen concurrently?
> 
> It is a trusted allocation, but should it be accounted as it is part of
> the userspace ioctl path ?

Well, it is unlocked_ioctl() and snp_guest_ioctl() is not taking any locks.
What's stopping anyone from writing a nasty little program which hammers the
sev-guest on the ioctl interface until the OOM killer activates?

IOW, this should probably remain _ACCOUNT AFAICT.
Dionna Amalie Glaze Dec. 7, 2024, 12:27 a.m. UTC | #10
>
> Well, it is unlocked_ioctl() and snp_guest_ioctl() is not taking any locks.
> What's stopping anyone from writing a nasty little program which hammers the
> sev-guest on the ioctl interface until the OOM killer activates?
>

Given sev-guest requires heightened privileges, can we not assume a
reasonable user space? I thought that was an organizing principle.

> IOW, this should probably remain _ACCOUNT AFAICT.
>
> --
> Regards/Gruss,
>     Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
>
Nikunj A. Dadhania Dec. 9, 2024, 6:16 a.m. UTC | #11
On 12/7/2024 1:57 AM, Borislav Petkov wrote:
> On Thu, Dec 05, 2024 at 11:53:53AM +0530, Nikunj A. Dadhania wrote:
>>> * get_report - I don't think so:
>>>
>>>         /*      
>>>          * 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(report_resp->data) + mdesc->ctx->authsize;
>>>         report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT);
>>>
>>> That resp_len is limited and that's on the guest_ioctl path which cannot
>>> happen concurrently?
>>
>> It is a trusted allocation, but should it be accounted as it is part of
>> the userspace ioctl path ?
> 
> Well, it is unlocked_ioctl() and snp_guest_ioctl() is not taking any locks.
> What's stopping anyone from writing a nasty little program which hammers the
> sev-guest on the ioctl interface until the OOM killer activates?
> 
> IOW, this should probably remain _ACCOUNT AFAICT.

Both get_report()/get_ext_report() are in the unlocked_ioctl(), we will
retain the _ACCOUNT

That leaves us with only one site: snp_init_crypto(), should I fold this change
in current patch ?
Regards
Nikunj
Borislav Petkov Dec. 9, 2024, 3:36 p.m. UTC | #12
On Fri, Dec 06, 2024 at 04:27:40PM -0800, Dionna Amalie Glaze wrote:
> Given sev-guest requires heightened privileges, can we not assume a
> reasonable user space?

And that sev-guest driver runs in the guest so the worst that can happen is,
the guest gets killed for misbehaving. Oh well...

I guess that's ok.
Borislav Petkov Dec. 9, 2024, 3:38 p.m. UTC | #13
On Mon, Dec 09, 2024 at 11:46:44AM +0530, Nikunj A. Dadhania wrote:
> That leaves us with only one site: snp_init_crypto(), should I fold this
> change in current patch ?

Nah, a pre-patch pls.

Along with an explanation summing up our discussion in the commit message.
This patch is already doing enough.

Thx.
Nikunj A. Dadhania Dec. 10, 2024, 6:38 a.m. UTC | #14
On 12/9/2024 9:08 PM, Borislav Petkov wrote:
> On Mon, Dec 09, 2024 at 11:46:44AM +0530, Nikunj A. Dadhania wrote:
>> That leaves us with only one site: snp_init_crypto(), should I fold this
>> change in current patch ?
> 
> Nah, a pre-patch pls.
> 
> Along with an explanation summing up our discussion in the commit message.
> This patch is already doing enough.

From: Nikunj A Dadhania <nikunj@amd.com>
Date: Thu, 5 Dec 2024 12:00:56 +0530
Subject: [PATCH] virt: sev-guest: Replace GFP_KERNEL_ACCOUNT with GFP_KERNEL

Replace GFP_KERNEL_ACCOUNT with GFP_KERNEL in the sev-guest driver code.
GFP_KERNEL_ACCOUNT is typically used for accounting untrusted userspace
allocations. After auditing the sev-guest code, the following changes are
necessary:

  * snp_init_crypto(): Use GFP_KERNEL as this is a trusted device probe
    path.

Retain GFP_KERNEL_ACCOUNT in the following cases for robustness and
specific path requirements:

  * alloc_shared_pages(): Although all allocations are limited, retain
    GFP_KERNEL_ACCOUNT for future robustness.

  * get_report() and get_ext_report(): These functions are on the unlocked
    ioctl path and should continue using GFP_KERNEL_ACCOUNT.

Suggested-by: Borislav Petkov <bp@alien8.de>
Signed-off-by: Nikunj A Dadhania <nikunj@amd.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c
b/drivers/virt/coco/sev-guest/sev-guest.c
index 62328d0b2cb6..250ce92d816b 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -141,7 +141,7 @@ static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t
keylen)
 {
 	struct aesgcm_ctx *ctx;

-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return NULL;
diff mbox series

Patch

diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 91f08af31078..f78c94e29c74 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -14,6 +14,7 @@ 
 #include <asm/insn.h>
 #include <asm/sev-common.h>
 #include <asm/coco.h>
+#include <asm/set_memory.h>
 
 #define GHCB_PROTOCOL_MIN	1ULL
 #define GHCB_PROTOCOL_MAX	2ULL
@@ -170,10 +171,6 @@  struct snp_guest_msg {
 	u8 payload[PAGE_SIZE - sizeof(struct snp_guest_msg_hdr)];
 } __packed;
 
-struct sev_guest_platform_data {
-	u64 secrets_gpa;
-};
-
 struct snp_guest_req {
 	void *req_buf;
 	size_t req_sz;
@@ -253,6 +250,7 @@  struct snp_msg_desc {
 
 	u32 *os_area_msg_seqno;
 	u8 *vmpck;
+	int vmpck_id;
 };
 
 /*
@@ -458,6 +456,20 @@  void set_pte_enc_mask(pte_t *kpte, unsigned long pfn, pgprot_t new_prot);
 void snp_kexec_finish(void);
 void snp_kexec_begin(void);
 
+static inline bool snp_is_vmpck_empty(struct snp_msg_desc *mdesc)
+{
+	static const char zero_key[VMPCK_KEY_LEN] = {0};
+
+	if (mdesc->vmpck)
+		return !memcmp(mdesc->vmpck, zero_key, VMPCK_KEY_LEN);
+
+	return true;
+}
+
+int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id);
+struct snp_msg_desc *snp_msg_alloc(void);
+void snp_msg_free(struct snp_msg_desc *mdesc);
+
 #else	/* !CONFIG_AMD_MEM_ENCRYPT */
 
 #define snp_vmpl 0
@@ -498,6 +510,10 @@  static inline int prepare_pte_enc(struct pte_enc_desc *d) { return 0; }
 static inline void set_pte_enc_mask(pte_t *kpte, unsigned long pfn, pgprot_t new_prot) { }
 static inline void snp_kexec_finish(void) { }
 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_free(struct snp_msg_desc *mdesc) { }
 
 #endif	/* CONFIG_AMD_MEM_ENCRYPT */
 
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index c5b0148b8c0a..3cc741eefd06 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -25,6 +25,7 @@ 
 #include <linux/psp-sev.h>
 #include <linux/dmi.h>
 #include <uapi/linux/sev-guest.h>
+#include <crypto/gcm.h>
 
 #include <asm/init.h>
 #include <asm/cpu_entry_area.h>
@@ -2580,15 +2581,9 @@  static struct platform_device sev_guest_device = {
 
 static int __init snp_init_platform_device(void)
 {
-	struct sev_guest_platform_data data;
-
 	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
 		return -ENODEV;
 
-	data.secrets_gpa = secrets_pa;
-	if (platform_device_add_data(&sev_guest_device, &data, sizeof(data)))
-		return -ENODEV;
-
 	if (platform_device_register(&sev_guest_device))
 		return -ENODEV;
 
@@ -2667,3 +2662,179 @@  static int __init sev_sysfs_init(void)
 }
 arch_initcall(sev_sysfs_init);
 #endif // CONFIG_SYSFS
+
+static void free_shared_pages(void *buf, size_t sz)
+{
+	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
+	int ret;
+
+	if (!buf)
+		return;
+
+	ret = set_memory_encrypted((unsigned long)buf, npages);
+	if (ret) {
+		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
+		return;
+	}
+
+	__free_pages(virt_to_page(buf), get_order(sz));
+}
+
+static void *alloc_shared_pages(size_t sz)
+{
+	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
+	struct page *page;
+	int ret;
+
+	page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(sz));
+	if (!page)
+		return NULL;
+
+	ret = set_memory_decrypted((unsigned long)page_address(page), npages);
+	if (ret) {
+		pr_err("failed to mark page shared, ret=%d\n", ret);
+		__free_pages(page, get_order(sz));
+		return NULL;
+	}
+
+	return page_address(page);
+}
+
+static u8 *get_vmpck(int id, struct snp_secrets_page *secrets, u32 **seqno)
+{
+	u8 *key = NULL;
+
+	switch (id) {
+	case 0:
+		*seqno = &secrets->os_area.msg_seqno_0;
+		key = secrets->vmpck0;
+		break;
+	case 1:
+		*seqno = &secrets->os_area.msg_seqno_1;
+		key = secrets->vmpck1;
+		break;
+	case 2:
+		*seqno = &secrets->os_area.msg_seqno_2;
+		key = secrets->vmpck2;
+		break;
+	case 3:
+		*seqno = &secrets->os_area.msg_seqno_3;
+		key = secrets->vmpck3;
+		break;
+	default:
+		break;
+	}
+
+	return key;
+}
+
+static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
+{
+	struct aesgcm_ctx *ctx;
+
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
+	if (!ctx)
+		return NULL;
+
+	if (aesgcm_expandkey(ctx, key, keylen, AUTHTAG_LEN)) {
+		pr_err("Crypto context initialization failed\n");
+		kfree(ctx);
+		return NULL;
+	}
+
+	return ctx;
+}
+
+int snp_msg_init(struct snp_msg_desc *mdesc, int vmpck_id)
+{
+	/* Adjust the default VMPCK key based on the executing VMPL level */
+	if (vmpck_id == -1)
+		vmpck_id = snp_vmpl;
+
+	mdesc->vmpck = get_vmpck(vmpck_id, mdesc->secrets, &mdesc->os_area_msg_seqno);
+	if (!mdesc->vmpck) {
+		pr_err("Invalid VMPCK%d communication key\n", vmpck_id);
+		return -EINVAL;
+	}
+
+	/* Verify that VMPCK is not zero. */
+	if (snp_is_vmpck_empty(mdesc)) {
+		pr_err("Empty VMPCK%d communication key\n", vmpck_id);
+		return -EINVAL;
+	}
+
+	mdesc->vmpck_id = vmpck_id;
+
+	mdesc->ctx = snp_init_crypto(mdesc->vmpck, VMPCK_KEY_LEN);
+	if (!mdesc->ctx)
+		return -ENOMEM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(snp_msg_init);
+
+struct snp_msg_desc *snp_msg_alloc(void)
+{
+	struct snp_msg_desc *mdesc;
+	void __iomem *mem;
+
+	BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
+
+	mdesc = kzalloc(sizeof(struct snp_msg_desc), GFP_KERNEL);
+	if (!mdesc)
+		return ERR_PTR(-ENOMEM);
+
+	mem = ioremap_encrypted(secrets_pa, PAGE_SIZE);
+	if (!mem)
+		goto e_free_mdesc;
+
+	mdesc->secrets = (__force struct snp_secrets_page *)mem;
+
+	/* Allocate the shared page used for the request and response message. */
+	mdesc->request = alloc_shared_pages(sizeof(struct snp_guest_msg));
+	if (!mdesc->request)
+		goto e_unmap;
+
+	mdesc->response = alloc_shared_pages(sizeof(struct snp_guest_msg));
+	if (!mdesc->response)
+		goto e_free_request;
+
+	mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE);
+	if (!mdesc->certs_data)
+		goto e_free_response;
+
+	/* initial the input address for guest request */
+	mdesc->input.req_gpa = __pa(mdesc->request);
+	mdesc->input.resp_gpa = __pa(mdesc->response);
+	mdesc->input.data_gpa = __pa(mdesc->certs_data);
+
+	return mdesc;
+
+e_free_response:
+	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
+e_free_request:
+	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
+e_unmap:
+	iounmap(mem);
+e_free_mdesc:
+	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);
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index b699771be029..5268511bc9b8 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -63,16 +63,6 @@  MODULE_PARM_DESC(vmpck_id, "The VMPCK ID to use when communicating with the PSP.
 /* Mutex to serialize the shared buffer access and command handling. */
 static DEFINE_MUTEX(snp_cmd_mutex);
 
-static bool is_vmpck_empty(struct snp_msg_desc *mdesc)
-{
-	char zero_key[VMPCK_KEY_LEN] = {0};
-
-	if (mdesc->vmpck)
-		return !memcmp(mdesc->vmpck, zero_key, VMPCK_KEY_LEN);
-
-	return true;
-}
-
 /*
  * If an error is received from the host or AMD Secure Processor (ASP) there
  * are two options. Either retry the exact same encrypted request or discontinue
@@ -93,7 +83,7 @@  static bool is_vmpck_empty(struct snp_msg_desc *mdesc)
 static void snp_disable_vmpck(struct snp_msg_desc *mdesc)
 {
 	pr_alert("Disabling VMPCK%d communication key to prevent IV reuse.\n",
-		  vmpck_id);
+		  mdesc->vmpck_id);
 	memzero_explicit(mdesc->vmpck, VMPCK_KEY_LEN);
 	mdesc->vmpck = NULL;
 }
@@ -147,23 +137,6 @@  static inline struct snp_guest_dev *to_snp_dev(struct file *file)
 	return container_of(dev, struct snp_guest_dev, misc);
 }
 
-static struct aesgcm_ctx *snp_init_crypto(u8 *key, size_t keylen)
-{
-	struct aesgcm_ctx *ctx;
-
-	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL_ACCOUNT);
-	if (!ctx)
-		return NULL;
-
-	if (aesgcm_expandkey(ctx, key, keylen, AUTHTAG_LEN)) {
-		pr_err("Crypto context initialization failed\n");
-		kfree(ctx);
-		return NULL;
-	}
-
-	return ctx;
-}
-
 static int verify_and_dec_payload(struct snp_msg_desc *mdesc, struct snp_guest_req *req)
 {
 	struct snp_guest_msg *resp_msg = &mdesc->secret_response;
@@ -335,7 +308,7 @@  static int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r
 	guard(mutex)(&snp_cmd_mutex);
 
 	/* Check if the VMPCK is not empty */
-	if (is_vmpck_empty(mdesc)) {
+	if (snp_is_vmpck_empty(mdesc)) {
 		pr_err_ratelimited("VMPCK is disabled\n");
 		return -ENOTTY;
 	}
@@ -414,7 +387,7 @@  static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 
 	req.msg_version = arg->msg_version;
 	req.msg_type = SNP_MSG_REPORT_REQ;
-	req.vmpck_id = vmpck_id;
+	req.vmpck_id = mdesc->vmpck_id;
 	req.req_buf = report_req;
 	req.req_sz = sizeof(*report_req);
 	req.resp_buf = report_resp->data;
@@ -461,7 +434,7 @@  static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 
 	req.msg_version = arg->msg_version;
 	req.msg_type = SNP_MSG_KEY_REQ;
-	req.vmpck_id = vmpck_id;
+	req.vmpck_id = mdesc->vmpck_id;
 	req.req_buf = derived_key_req;
 	req.req_sz = sizeof(*derived_key_req);
 	req.resp_buf = buf;
@@ -539,7 +512,7 @@  static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 
 	req.msg_version = arg->msg_version;
 	req.msg_type = SNP_MSG_REPORT_REQ;
-	req.vmpck_id = vmpck_id;
+	req.vmpck_id = mdesc->vmpck_id;
 	req.req_buf = &report_req->data;
 	req.req_sz = sizeof(report_req->data);
 	req.resp_buf = report_resp->data;
@@ -616,76 +589,11 @@  static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 	return ret;
 }
 
-static void free_shared_pages(void *buf, size_t sz)
-{
-	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
-	int ret;
-
-	if (!buf)
-		return;
-
-	ret = set_memory_encrypted((unsigned long)buf, npages);
-	if (ret) {
-		WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n");
-		return;
-	}
-
-	__free_pages(virt_to_page(buf), get_order(sz));
-}
-
-static void *alloc_shared_pages(struct device *dev, size_t sz)
-{
-	unsigned int npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
-	struct page *page;
-	int ret;
-
-	page = alloc_pages(GFP_KERNEL_ACCOUNT, get_order(sz));
-	if (!page)
-		return NULL;
-
-	ret = set_memory_decrypted((unsigned long)page_address(page), npages);
-	if (ret) {
-		dev_err(dev, "failed to mark page shared, ret=%d\n", ret);
-		__free_pages(page, get_order(sz));
-		return NULL;
-	}
-
-	return page_address(page);
-}
-
 static const struct file_operations snp_guest_fops = {
 	.owner	= THIS_MODULE,
 	.unlocked_ioctl = snp_guest_ioctl,
 };
 
-static u8 *get_vmpck(int id, struct snp_secrets_page *secrets, u32 **seqno)
-{
-	u8 *key = NULL;
-
-	switch (id) {
-	case 0:
-		*seqno = &secrets->os_area.msg_seqno_0;
-		key = secrets->vmpck0;
-		break;
-	case 1:
-		*seqno = &secrets->os_area.msg_seqno_1;
-		key = secrets->vmpck1;
-		break;
-	case 2:
-		*seqno = &secrets->os_area.msg_seqno_2;
-		key = secrets->vmpck2;
-		break;
-	case 3:
-		*seqno = &secrets->os_area.msg_seqno_3;
-		key = secrets->vmpck3;
-		break;
-	default:
-		break;
-	}
-
-	return key;
-}
-
 struct snp_msg_report_resp_hdr {
 	u32 status;
 	u32 report_size;
@@ -979,13 +887,10 @@  static void unregister_sev_tsm(void *data)
 
 static int __init sev_guest_probe(struct platform_device *pdev)
 {
-	struct sev_guest_platform_data *data;
-	struct snp_secrets_page *secrets;
 	struct device *dev = &pdev->dev;
 	struct snp_guest_dev *snp_dev;
 	struct snp_msg_desc *mdesc;
 	struct miscdevice *misc;
-	void __iomem *mapping;
 	int ret;
 
 	BUILD_BUG_ON(sizeof(struct snp_guest_msg) > PAGE_SIZE);
@@ -993,115 +898,57 @@  static int __init sev_guest_probe(struct platform_device *pdev)
 	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
 		return -ENODEV;
 
-	if (!dev->platform_data)
-		return -ENODEV;
-
-	data = (struct sev_guest_platform_data *)dev->platform_data;
-	mapping = ioremap_encrypted(data->secrets_gpa, PAGE_SIZE);
-	if (!mapping)
-		return -ENODEV;
-
-	secrets = (__force void *)mapping;
-
-	ret = -ENOMEM;
 	snp_dev = devm_kzalloc(&pdev->dev, sizeof(struct snp_guest_dev), GFP_KERNEL);
 	if (!snp_dev)
-		goto e_unmap;
-
-	mdesc = devm_kzalloc(&pdev->dev, sizeof(struct snp_msg_desc), GFP_KERNEL);
-	if (!mdesc)
-		goto e_unmap;
-
-	/* Adjust the default VMPCK key based on the executing VMPL level */
-	if (vmpck_id == -1)
-		vmpck_id = snp_vmpl;
+		return -ENOMEM;
 
-	ret = -EINVAL;
-	mdesc->vmpck = get_vmpck(vmpck_id, secrets, &mdesc->os_area_msg_seqno);
-	if (!mdesc->vmpck) {
-		dev_err(dev, "Invalid VMPCK%d communication key\n", vmpck_id);
-		goto e_unmap;
-	}
+	mdesc = snp_msg_alloc();
+	if (IS_ERR_OR_NULL(mdesc))
+		return -ENOMEM;
 
-	/* Verify that VMPCK is not zero. */
-	if (is_vmpck_empty(mdesc)) {
-		dev_err(dev, "Empty VMPCK%d communication key\n", vmpck_id);
-		goto e_unmap;
-	}
+	ret = snp_msg_init(mdesc, vmpck_id);
+	if (ret)
+		return -EIO;
 
 	platform_set_drvdata(pdev, snp_dev);
 	snp_dev->dev = dev;
-	mdesc->secrets = secrets;
-
-	/* Allocate the shared page used for the request and response message. */
-	mdesc->request = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
-	if (!mdesc->request)
-		goto e_unmap;
-
-	mdesc->response = alloc_shared_pages(dev, sizeof(struct snp_guest_msg));
-	if (!mdesc->response)
-		goto e_free_request;
-
-	mdesc->certs_data = alloc_shared_pages(dev, SEV_FW_BLOB_MAX_SIZE);
-	if (!mdesc->certs_data)
-		goto e_free_response;
-
-	ret = -EIO;
-	mdesc->ctx = snp_init_crypto(mdesc->vmpck, VMPCK_KEY_LEN);
-	if (!mdesc->ctx)
-		goto e_free_cert_data;
 
 	misc = &snp_dev->misc;
 	misc->minor = MISC_DYNAMIC_MINOR;
 	misc->name = DEVICE_NAME;
 	misc->fops = &snp_guest_fops;
 
-	/* Initialize the input addresses for guest request */
-	mdesc->input.req_gpa = __pa(mdesc->request);
-	mdesc->input.resp_gpa = __pa(mdesc->response);
-	mdesc->input.data_gpa = __pa(mdesc->certs_data);
-
 	/* Set the privlevel_floor attribute based on the vmpck_id */
-	sev_tsm_ops.privlevel_floor = vmpck_id;
+	sev_tsm_ops.privlevel_floor = mdesc->vmpck_id;
 
 	ret = tsm_register(&sev_tsm_ops, snp_dev);
 	if (ret)
-		goto e_free_cert_data;
+		goto e_msg_init;
 
 	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
 	if (ret)
-		goto e_free_cert_data;
+		goto e_msg_init;
 
 	ret =  misc_register(misc);
 	if (ret)
-		goto e_free_ctx;
+		goto e_msg_init;
 
 	snp_dev->msg_desc = mdesc;
-	dev_info(dev, "Initialized SEV guest driver (using VMPCK%d communication key)\n", vmpck_id);
+	dev_info(dev, "Initialized SEV guest driver (using VMPCK%d communication key)\n",
+		 mdesc->vmpck_id);
 	return 0;
 
-e_free_ctx:
-	kfree(mdesc->ctx);
-e_free_cert_data:
-	free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE);
-e_free_response:
-	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
-e_free_request:
-	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
-e_unmap:
-	iounmap(mapping);
+e_msg_init:
+	snp_msg_free(mdesc);
+
 	return ret;
 }
 
 static void __exit sev_guest_remove(struct platform_device *pdev)
 {
 	struct snp_guest_dev *snp_dev = platform_get_drvdata(pdev);
-	struct snp_msg_desc *mdesc = snp_dev->msg_desc;
 
-	free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE);
-	free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg));
-	free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg));
-	kfree(mdesc->ctx);
+	snp_msg_free(snp_dev->msg_desc);
 	misc_deregister(&snp_dev->misc);
 }
 
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 9d7bd0ae48c4..0f7e3acf37e3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1559,6 +1559,7 @@  config AMD_MEM_ENCRYPT
 	select ARCH_HAS_CC_PLATFORM
 	select X86_MEM_ENCRYPT
 	select UNACCEPTED_MEMORY
+	select CRYPTO_LIB_AESGCM
 	help
 	  Say yes to enable support for the encryption of system memory.
 	  This requires an AMD processor that supports Secure Memory
diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
index 0b772bd921d8..a6405ab6c2c3 100644
--- a/drivers/virt/coco/sev-guest/Kconfig
+++ b/drivers/virt/coco/sev-guest/Kconfig
@@ -2,7 +2,6 @@  config SEV_GUEST
 	tristate "AMD SEV Guest driver"
 	default m
 	depends on AMD_MEM_ENCRYPT
-	select CRYPTO_LIB_AESGCM
 	select TSM_REPORTS
 	help
 	  SEV-SNP firmware provides the guest a mechanism to communicate with