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