Message-ID: <20241203141950.GCZ08ThrMOHmDFeaa2@fat_crate.local>
Date: Tue, 3 Dec 2024 15:19:50 +0100
From: Borislav Petkov <bp@...en8.de>
To: Nikunj A Dadhania <nikunj@....com>
Cc: linux-kernel@...r.kernel.org, thomas.lendacky@....com, x86@...nel.org,
	kvm@...r.kernel.org, mingo@...hat.com, tglx@...utronix.de,
	dave.hansen@...ux.intel.com, pgonda@...gle.com, seanjc@...gle.com,
	pbonzini@...hat.com
Subject: Re: [PATCH v15 01/13] x86/sev: Carve out and export SNP guest
 messaging init routines

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@....com>
> Reviewed-by: Tom Lendacky <thomas.lendacky@....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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Powered by blists - more mailing lists