| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1da55a62-fea4-c6f8-079b-bc38515520e1@amd.com>
Date: Wed, 3 Aug 2022 13:18:22 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: linux-kernel@...r.kernel.org, x86@...nel.org
Cc: Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
"Kirill A. Shutemov" <kirill@...temov.name>,
"H. Peter Anvin" <hpa@...or.com>,
Michael Roth <michael.roth@....com>,
Joerg Roedel <jroedel@...e.de>,
Andy Lutomirski <luto@...nel.org>,
Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v1.1 1/2] x86/sev: Use per-CPU PSC structure in prep for
unaccepted memory support
On 8/3/22 13:11, Tom Lendacky wrote:
Of course, I'll fix the subject if submitting this for real... ugh.
Thanks,
Tom
> In advance of providing support for unaccepted memory, switch from using
> kmalloc() for allocating the Page State Change (PSC) structure to using a
> static structure. This is needed to avoid a possible recursive call into
> set_pages_state() if the kmalloc() call requires (more) memory to be
> accepted, which would result in a hang.
>
> Page state changes occur whenever DMA memory is allocated or memory needs
> to be shared with the hypervisor (kvmclock, attestation reports, etc.).
> Since most page state changes occur early in boot and are limited in
> number, a single static PSC structure is used and protected by a spin
> lock with interrupts disabled.
>
> Even with interrupts disabled, an NMI can be raised while performing
> memory acceptance. The NMI could then cause further memory acceptance to
> be performed. To prevent a deadlock, use the MSR protocol if executing in
> an NMI context.
>
> Since the set_pages_state() path is the only path into vmgexit_psc(),
> rename vmgexit_psc() to __vmgexit_psc() and remove the calls to disable
> interrupts which are now performed by set_pages_state().
>
> Signed-off-by: Tom Lendacky <thomas.lendacky@....com>
> ---
> arch/x86/kernel/sev.c | 55 +++++++++++++++++++++++++------------------
> 1 file changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index c05f0124c410..84d94fd2ec53 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -66,6 +66,9 @@ static struct ghcb boot_ghcb_page __bss_decrypted __aligned(PAGE_SIZE);
> */
> static struct ghcb *boot_ghcb __section(".data");
>
> +/* Flag to indicate when the first per-CPU GHCB is registered */
> +static bool ghcb_percpu_ready __section(".data");
> +
> /* Bitmap of SEV features supported by the hypervisor */
> static u64 sev_hv_features __ro_after_init;
>
> @@ -122,6 +125,15 @@ struct sev_config {
>
> static struct sev_config sev_cfg __read_mostly;
>
> +/*
> + * Page State Change structure for use when accepting memory or when changing
> + * page state. Use is protected by a spinlock with interrupts disabled, but an
> + * NMI could still be raised, so check if running in an NMI an use the MSR
> + * protocol in these cases.
> + */
> +static struct snp_psc_desc psc_desc;
> +static DEFINE_SPINLOCK(psc_desc_lock);
> +
> static __always_inline bool on_vc_stack(struct pt_regs *regs)
> {
> unsigned long sp = regs->sp;
> @@ -660,7 +672,7 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
> }
> }
>
> -static void __init early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op)
> +static void early_set_pages_state(unsigned long paddr, unsigned int npages, enum psc_op op)
> {
> unsigned long paddr_end;
> u64 val;
> @@ -742,26 +754,17 @@ void __init snp_prep_memory(unsigned long paddr, unsigned int sz, enum psc_op op
> WARN(1, "invalid memory op %d\n", op);
> }
>
> -static int vmgexit_psc(struct snp_psc_desc *desc)
> +static int __vmgexit_psc(struct snp_psc_desc *desc)
> {
> int cur_entry, end_entry, ret = 0;
> struct snp_psc_desc *data;
> struct ghcb_state state;
> struct es_em_ctxt ctxt;
> - unsigned long flags;
> struct ghcb *ghcb;
>
> - /*
> - * __sev_get_ghcb() needs to run with IRQs disabled because it is using
> - * a per-CPU GHCB.
> - */
> - local_irq_save(flags);
> -
> ghcb = __sev_get_ghcb(&state);
> - if (!ghcb) {
> - ret = 1;
> - goto out_unlock;
> - }
> + if (!ghcb)
> + return 1;
>
> /* Copy the input desc into GHCB shared buffer */
> data = (struct snp_psc_desc *)ghcb->shared_buffer;
> @@ -820,9 +823,6 @@ static int vmgexit_psc(struct snp_psc_desc *desc)
> out:
> __sev_put_ghcb(&state);
>
> -out_unlock:
> - local_irq_restore(flags);
> -
> return ret;
> }
>
> @@ -861,18 +861,25 @@ static void __set_pages_state(struct snp_psc_desc *data, unsigned long vaddr,
> i++;
> }
>
> - if (vmgexit_psc(data))
> + if (__vmgexit_psc(data))
> sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC);
> }
>
> static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
> {
> unsigned long vaddr_end, next_vaddr;
> - struct snp_psc_desc *desc;
> + unsigned long flags;
>
> - desc = kmalloc(sizeof(*desc), GFP_KERNEL_ACCOUNT);
> - if (!desc)
> - panic("SNP: failed to allocate memory for PSC descriptor\n");
> + /*
> + * Use the MSR protocol when either:
> + * - executing in an NMI to avoid any possibility of a deadlock
> + * - per-CPU GHCBs are not yet registered, since __vmgexit_psc()
> + * uses the per-CPU GHCB.
> + */
> + if (in_nmi() || !ghcb_percpu_ready)
> + return early_set_pages_state(__pa(vaddr), npages, op);
> +
> + spin_lock_irqsave(&psc_desc_lock, flags);
>
> vaddr = vaddr & PAGE_MASK;
> vaddr_end = vaddr + (npages << PAGE_SHIFT);
> @@ -882,12 +889,12 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
> next_vaddr = min_t(unsigned long, vaddr_end,
> (VMGEXIT_PSC_MAX_ENTRY * PAGE_SIZE) + vaddr);
>
> - __set_pages_state(desc, vaddr, next_vaddr, op);
> + __set_pages_state(&psc_desc, vaddr, next_vaddr, op);
>
> vaddr = next_vaddr;
> }
>
> - kfree(desc);
> + spin_unlock_irqrestore(&psc_desc_lock, flags);
> }
>
> void snp_set_memory_shared(unsigned long vaddr, unsigned int npages)
> @@ -1254,6 +1261,8 @@ void setup_ghcb(void)
> if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
> snp_register_per_cpu_ghcb();
>
> + ghcb_percpu_ready = true;
> +
> return;
> }
>
Powered by blists - more mailing lists