[v4,2/6] x86/sev-es: Disable IRQs while GHCB is active

Commit Message

Joerg Roedel June 10, 2021, 9:11 a.m. UTC

From: Joerg Roedel <[e-mail]>

The #VC handler only cares about IRQs being disabled while the GHCB is
active, as it must not be interrupted by something which could cause
another #VC while it holds the GHCB (NMI is the exception for which the
backup GHCB is there).

Make sure nothing interrupts the code path while the GHCB is active by
disabling IRQs in sev_es_get_ghcb() and restoring the previous irq state
in sev_es_put_ghcb().

Signed-off-by: Joerg Roedel <[e-mail]>
---
 arch/x86/kernel/sev.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

Comments

Borislav Petkov June 11, 2021, 2:05 p.m. UTC | #1

On Thu, Jun 10, 2021 at 11:11:37AM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[e-mail]>
> 
> The #VC handler only cares about IRQs being disabled while the GHCB is
> active, as it must not be interrupted by something which could cause
> another #VC while it holds the GHCB (NMI is the exception for which the
> backup GHCB is there).
> 
> Make sure nothing interrupts the code path while the GHCB is active by
> disabling IRQs in sev_es_get_ghcb() and restoring the previous irq state
> in sev_es_put_ghcb().

Why this unnecessarily complicated passing of flags back and forth?

Why not simply "sandwich" them:

	local_irq_save()
	sev_es_get_ghcb()

	...blablabla

	sev_es_put_ghcb()
	local_irq_restore();

in every call site?

What's the difference in passing *flags in and have the
get_ghcb/put_ghcb save/restore flags instead of the callers?

> -static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
> +static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state,
> +						    unsigned long *flags)
>  {
>  	struct sev_es_runtime_data *data;
>  	struct ghcb *ghcb;
>  
> +	/*
> +	 * Nothing shall interrupt this code path while holding the per-cpu
> +	 * GHCB. The backup GHCB is only for NMIs interrupting this path.

Hmm, so why aren't you accessing/setting data->ghcb_active and
data->backup_ghcb_active safely using cmpxchg() if this path can be
interrupted by an NMI?

Joerg Roedel June 11, 2021, 2:20 p.m. UTC | #2

On Fri, Jun 11, 2021 at 04:05:15PM +0200, Borislav Petkov wrote:
> On Thu, Jun 10, 2021 at 11:11:37AM +0200, Joerg Roedel wrote:
> Why not simply "sandwich" them:
> 
> 	local_irq_save()
> 	sev_es_get_ghcb()
> 
> 	...blablabla
> 
> 	sev_es_put_ghcb()
> 	local_irq_restore();
> 
> in every call site?

I am not a fan of this, because its easily forgotten to add
local_irq_save()/local_irq_restore() calls around those. Yes, we can add
irqs_disabled() assertions to the functions, but we can as well just
disable/enable IRQs in them. Only the previous value of EFLAGS.IF needs
to be carried from one function to the other.

> Hmm, so why aren't you accessing/setting data->ghcb_active and
> data->backup_ghcb_active safely using cmpxchg() if this path can be
> interrupted by an NMI?

Using cmpxchg is not necessary here. It is all per-cpu data, so local to
the current cpu. If an NMI happens anywhere in sev_es_get_ghcb() it can
still use the GHCB, because the interrupted #VC handler will not start
writing to it before sev_es_get_ghcb() returned.

Problems only come up when one path starts writing to the GHCB, but that
happens long after it is marked active.

Regards,

	Joerg

Borislav Petkov June 11, 2021, 2:34 p.m. UTC | #3

On Fri, Jun 11, 2021 at 04:20:36PM +0200, Joerg Roedel wrote:
> I am not a fan of this, because its easily forgotten to add
> local_irq_save()/local_irq_restore() calls around those. Yes, we can add
> irqs_disabled() assertions to the functions, but we can as well just
> disable/enable IRQs in them. Only the previous value of EFLAGS.IF needs
> to be carried from one function to the other.

Wrappers:

	sev_es_get_ghcb():

		local_irq_save()
		__sev_es_get_ghcb()

and the reverse.

Patch

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 4fd997bbf059..2a922d1b03c8 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -192,14 +192,23 @@  void noinstr __sev_es_ist_exit(void)
 	this_cpu_write(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC], *(unsigned long *)ist);
 }
 
-static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)
+static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state,
+						    unsigned long *flags)
 {
 	struct sev_es_runtime_data *data;
 	struct ghcb *ghcb;
 
+	/*
+	 * Nothing shall interrupt this code path while holding the per-cpu
+	 * GHCB. The backup GHCB is only for NMIs interrupting this path.
+	 */
+	local_irq_save(*flags);
+
 	data = this_cpu_read(runtime_data);
 	ghcb = &data->ghcb_page;
 
+
+
 	if (unlikely(data->ghcb_active)) {
 		/* GHCB is already in use - save its contents */
 
@@ -479,7 +488,8 @@  static enum es_result vc_slow_virt_to_phys(struct ghcb *ghcb, struct es_em_ctxt
 /* Include code shared with pre-decompression boot stage */
 #include "sev-shared.c"
 
-static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
+static __always_inline void sev_es_put_ghcb(struct ghcb_state *state,
+					    unsigned long flags)
 {
 	struct sev_es_runtime_data *data;
 	struct ghcb *ghcb;
@@ -500,14 +510,17 @@  static __always_inline void sev_es_put_ghcb(struct ghcb_state *state)
 		vc_ghcb_invalidate(ghcb);
 		data->ghcb_active = false;
 	}
+
+	local_irq_restore(flags);
 }
 
 void noinstr __sev_es_nmi_complete(void)
 {
 	struct ghcb_state state;
+	unsigned long flags;
 	struct ghcb *ghcb;
 
-	ghcb = sev_es_get_ghcb(&state);
+	ghcb = sev_es_get_ghcb(&state, &flags);
 
 	vc_ghcb_invalidate(ghcb);
 	ghcb_set_sw_exit_code(ghcb, SVM_VMGEXIT_NMI_COMPLETE);
@@ -517,7 +530,7 @@  void noinstr __sev_es_nmi_complete(void)
 	sev_es_wr_ghcb_msr(__pa_nodebug(ghcb));
 	VMGEXIT();
 
-	sev_es_put_ghcb(&state);
+	sev_es_put_ghcb(&state, flags);
 }
 
 static u64 get_jump_table_addr(void)
@@ -527,9 +540,7 @@  static u64 get_jump_table_addr(void)
 	struct ghcb *ghcb;
 	u64 ret = 0;
 
-	local_irq_save(flags);
-
-	ghcb = sev_es_get_ghcb(&state);
+	ghcb = sev_es_get_ghcb(&state, &flags);
 
 	vc_ghcb_invalidate(ghcb);
 	ghcb_set_sw_exit_code(ghcb, SVM_VMGEXIT_AP_JUMP_TABLE);
@@ -543,9 +554,7 @@  static u64 get_jump_table_addr(void)
 	    ghcb_sw_exit_info_2_is_valid(ghcb))
 		ret = ghcb->save.sw_exit_info_2;
 
-	sev_es_put_ghcb(&state);
-
-	local_irq_restore(flags);
+	sev_es_put_ghcb(&state, flags);
 
 	return ret;
 }
@@ -666,9 +675,10 @@  static bool __init sev_es_setup_ghcb(void)
 static void sev_es_ap_hlt_loop(void)
 {
 	struct ghcb_state state;
+	unsigned long flags;
 	struct ghcb *ghcb;
 
-	ghcb = sev_es_get_ghcb(&state);
+	ghcb = sev_es_get_ghcb(&state, &flags);
 
 	while (true) {
 		vc_ghcb_invalidate(ghcb);
@@ -685,7 +695,7 @@  static void sev_es_ap_hlt_loop(void)
 			break;
 	}
 
-	sev_es_put_ghcb(&state);
+	sev_es_put_ghcb(&state, flags);
 }
 
 /*
@@ -1333,6 +1343,7 @@  DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
 	struct ghcb_state state;
 	struct es_em_ctxt ctxt;
 	enum es_result result;
+	unsigned long flags;
 	struct ghcb *ghcb;
 
 	/*
@@ -1353,7 +1364,7 @@  DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
 	 * keep the IRQs disabled to protect us against concurrent TLB flushes.
 	 */
 
-	ghcb = sev_es_get_ghcb(&state);
+	ghcb = sev_es_get_ghcb(&state, &flags);
 
 	vc_ghcb_invalidate(ghcb);
 	result = vc_init_em_ctxt(&ctxt, regs, error_code);
@@ -1361,7 +1372,7 @@  DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
 	if (result == ES_OK)
 		result = vc_handle_exitcode(&ctxt, ghcb, error_code);
 
-	sev_es_put_ghcb(&state);
+	sev_es_put_ghcb(&state, flags);
 
 	/* Done - now check the result */
 	switch (result) {