From: Joerg Roedel <redacted>
Hi,
here is the next revision of my pending fixes for Linux' SEV-ES
support. Changes to the previous version are:
- Removed first patch which is now in tip/x86/urgent already
- Removed patch "x86/sev-es: Run #VC handler in plain IRQ state"
and replaced it with
"x86/sev-es: Split up runtime #VC handler for correct state tracking"
as per suggestion from PeterZ
Changes are based on tip/x86/urgent. Please review.
Thanks,
Joerg
Joerg Roedel (6):
x86/sev-es: Fix error message in runtime #VC handler
x86/sev-es: Disable IRQs while GHCB is active
x86/sev-es: Split up runtime #VC handler for correct state tracking
x86/insn-eval: Make 0 a valid RIP for insn_get_effective_ip()
x86/insn: Extend error reporting from
insn_fetch_from_user[_inatomic]()
x86/sev-es: Propagate #GP if getting linear instruction address failed
arch/x86/kernel/sev.c | 174 +++++++++++++++++++++++----------------
arch/x86/kernel/umip.c | 10 +--
arch/x86/lib/insn-eval.c | 22 +++--
3 files changed, 122 insertions(+), 84 deletions(-)
base-commit: efa165504943f2128d50f63de0c02faf6dcceb0d
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Joerg Roedel <redacted>
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 <redacted>
---
arch/x86/kernel/sev.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
@@ -192,14 +192,23 @@ void noinstr __sev_es_ist_exit(void)this_cpu_write(cpu_tss_rw.x86_tss.ist[IST_INDEX_VC],*(unsignedlong*)ist);}-static__always_inlinestructghcb*sev_es_get_ghcb(structghcb_state*state)+static__always_inlinestructghcb*sev_es_get_ghcb(structghcb_state*state,+unsignedlong*flags){structsev_es_runtime_data*data;structghcb*ghcb;+/*+*Nothingshallinterruptthiscodepathwhileholdingtheper-cpu+*GHCB.ThebackupGHCBisonlyforNMIsinterruptingthispath.+*/+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_inlinevoidsev_es_put_ghcb(structghcb_state*state)+static__always_inlinevoidsev_es_put_ghcb(structghcb_state*state,+unsignedlongflags){structsev_es_runtime_data*data;structghcb*ghcb;
@@ -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){
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Joerg Roedel <redacted>
Split up the #VC handler code into a from-user and a from-kernel part.
This allows clean and correct state tracking, as the #VC handler needs
to enter NMI-state when raised from kernel mode and plain IRQ state when
raised from user-mode.
Fixes: 62441a1fb532 ("x86/sev-es: Correctly track IRQ states in runtime #VC handler")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Joerg Roedel <redacted>
---
arch/x86/kernel/sev.c | 118 ++++++++++++++++++++++++------------------
1 file changed, 68 insertions(+), 50 deletions(-)
@@ -1382,15 +1353,18 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)caseES_UNSUPPORTED:pr_err_ratelimited("Unsupported exit-code 0x%02lx in #VC exception (IP: 0x%lx)\n",error_code,regs->ip);-gotofail;+ret=false;+break;caseES_VMM_ERROR:pr_err_ratelimited("Failure in communication with VMM (exit-code 0x%02lx IP: 0x%lx)\n",error_code,regs->ip);-gotofail;+ret=false;+break;caseES_DECODE_FAILED:pr_err_ratelimited("Failed to decode instruction (exit-code 0x%02lx IP: 0x%lx)\n",error_code,regs->ip);-gotofail;+ret=false;+break;caseES_EXCEPTION:vc_forward_exception(&ctxt);break;
@@ -1406,24 +1380,16 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)BUG();}-out:-instrumentation_end();-irqentry_nmi_exit(regs,irq_state);+returnret;+}-return;+staticvoidvc_handle_from_kernel(structpt_regs*regs,unsignedlongerror_code)+{+irqentry_state_tirq_state=irqentry_nmi_enter(regs);-fail:-if(user_mode(regs)){-/*-*Donotkillthemachineifuser-spacetriggeredthe-*exception.SendSIGBUSinsteadandletuser-spacedealwith-*it.-*/-force_sig_fault(SIGBUS,BUS_OBJERR,(void__user*)0);-}else{-pr_emerg("PANIC: Unhandled #VC exception in kernel space (result=%d)\n",-result);+instrumentation_begin();+if(!vc_raw_handle_exception(regs,error_code)){/* Show some debug info */show_regs(regs);
@@ -1434,7 +1400,59 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)panic("Returned from Terminate-Request to Hypervisor\n");}-gotoout;+instrumentation_end();+irqentry_nmi_exit(regs,irq_state);+}++staticvoidvc_handle_from_user(structpt_regs*regs,unsignedlongerror_code)+{+irqentry_state_tirq_state=irqentry_enter(regs);++instrumentation_begin();++if(!vc_raw_handle_exception(regs,error_code)){+/*+*Donotkillthemachineifuser-spacetriggeredthe+*exception.SendSIGBUSinsteadandletuser-spacedealwith+*it.+*/+force_sig_fault(SIGBUS,BUS_OBJERR,(void__user*)0);+}++instrumentation_end();+irqentry_exit(regs,irq_state);+}+/*+*Main#VCexceptionhandler.Itiscalledwhentheentrycodewasableto+*switchofftheISTtoasafekernelstack.+*+*Withthecurrentimplementationitisalwayspossibletoswitchtoasafe+*stackbecause#VCexceptionsonlyhappenatknownplaces,likeintercepted+*instructionsoraccessestoMMIOareas/IOports.Theycanalsohappenwith+*codeinstrumentationwhenthehypervisorintercepts#DB,butthecritical+*pathsareforbiddentobeinstrumented,so#DBexceptionscurrentlyalso+*onlyhappeninsafeplaces.+*/+DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)+{+/*+*Handle#DBbeforecallinginto!noinstrcodetoavoidrecursive#DB.+*/+if(error_code==SVM_EXIT_EXCP_BASE+X86_TRAP_DB){+vc_handle_trap_db(regs);+return;+}++/*+*Thisisinvokedthroughaninterruptgate,soIRQsaredisabled.The+*codebelowmightwalkpage-tablesforuserorkerneladdresses,so+*keeptheIRQsdisabledtoprotectusagainstconcurrentTLBflushes.+*/++if(user_mode(regs))+vc_handle_from_user(regs,error_code);+else+vc_handle_from_kernel(regs,error_code);}/* This handler runs on the #VC fall-back stack. It can cause further #VC exceptions */
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Joerg Roedel <redacted>
The runtime #VC handler is not "early" anymore. Fix the copy&paste error
and remove that word from the error message.
Signed-off-by: Joerg Roedel <redacted>
---
arch/x86/kernel/sev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Joerg Roedel <redacted>
In theory 0 is a valid value for the instruction pointer, so don't use
it as the error return value from insn_get_effective_ip().
Signed-off-by: Joerg Roedel <redacted>
---
arch/x86/lib/insn-eval.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
From: Joerg Roedel <redacted>
When an instruction is fetched from user-space, segmentation needs to
be taken into account. This means that getting the linear address of
an instruction can fail. Hardware would raise a #GP
exception in that case, but the #VC exception handler would emulate it
as a page-fault.
The insn_fetch_from_user*() functions now provide the relevant
information in case of an failure. Use that and propagate a #GP when
the linear address of an instruction to fetch could not be calculated.
Signed-off-by: Joerg Roedel <redacted>
---
arch/x86/kernel/sev.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
@@ -270,11 +270,18 @@ static enum es_result __vc_decode_user_insn(struct es_em_ctxt *ctxt)intinsn_bytes;insn_bytes=insn_fetch_from_user_inatomic(ctxt->regs,buffer);-if(insn_bytes<=0){+if(insn_bytes==0){+/* Nothing could be copied */ctxt->fi.vector=X86_TRAP_PF;ctxt->fi.error_code=X86_PF_INSTR|X86_PF_USER;ctxt->fi.cr2=ctxt->regs->ip;returnES_EXCEPTION;+}elseif(insn_bytes==-EINVAL){+/* Effective RIP could not be calculated */+ctxt->fi.vector=X86_TRAP_GP;+ctxt->fi.error_code=0;+ctxt->fi.cr2=0;+returnES_EXCEPTION;}if(!insn_decode_from_regs(&ctxt->insn,ctxt->regs,buffer,insn_bytes))
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From: Joerg Roedel <redacted>
The error reporting from the insn_fetch_from_user*() functions is not
very verbose. Extend it to include information on whether the linear
RIP could not be calculated or whether the memory access faulted.
This will be used in the SEV-ES code to propagate the correct
exception depending on what went wrong during instruction fetch.
Signed-off-by: Joerg Roedel <redacted>
---
arch/x86/kernel/sev.c | 8 ++++----
arch/x86/kernel/umip.c | 10 ++++------
arch/x86/lib/insn-eval.c | 8 ++++++--
3 files changed, 14 insertions(+), 12 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2021-06-10 10:21:15
Bah, I suppose the trouble is that this SEV crap requires PARAVIRT?
I should really get around to fixing noinstr validation with PARAVIRT on
:-(
On Thu, Jun 10, 2021 at 11:11:38AM +0200, Joerg Roedel wrote:
+static void vc_handle_from_kernel(struct pt_regs *regs, unsigned long error_code)
static noinstr ...
quoted hunk
+{
+ irqentry_state_t irq_state = irqentry_nmi_enter(regs);
+ instrumentation_begin();
+ if (!vc_raw_handle_exception(regs, error_code)) {
/* Show some debug info */
show_regs(regs);
@@ -1434,7 +1400,59 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) panic("Returned from Terminate-Request to Hypervisor\n"); }+ instrumentation_end();+ irqentry_nmi_exit(regs, irq_state);+}++static void vc_handle_from_user(struct pt_regs *regs, unsigned long error_code)
static noinstr ...
+{
+ irqentry_state_t irq_state = irqentry_enter(regs);
+
+ instrumentation_begin();
+
+ if (!vc_raw_handle_exception(regs, error_code)) {
+ /*
+ * Do not kill the machine if user-space triggered the
+ * exception. Send SIGBUS instead and let user-space deal with
+ * it.
+ */
+ force_sig_fault(SIGBUS, BUS_OBJERR, (void __user *)0);
+ }
+
+ instrumentation_end();
+ irqentry_exit(regs, irq_state);
+}
+ linebreak
+/*
+ * Main #VC exception handler. It is called when the entry code was able to
+ * switch off the IST to a safe kernel stack.
+ *
+ * With the current implementation it is always possible to switch to a safe
+ * stack because #VC exceptions only happen at known places, like intercepted
+ * instructions or accesses to MMIO areas/IO ports. They can also happen with
+ * code instrumentation when the hypervisor intercepts #DB, but the critical
+ * paths are forbidden to be instrumented, so #DB exceptions currently also
+ * only happen in safe places.
+ */
+DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)
+{
+ /*
+ * Handle #DB before calling into !noinstr code to avoid recursive #DB.
+ */
+ if (error_code == SVM_EXIT_EXCP_BASE + X86_TRAP_DB) {
+ vc_handle_trap_db(regs);
+ return;
+ }
+
+ /*
+ * This is invoked through an interrupt gate, so IRQs are disabled. The
+ * code below might walk page-tables for user or kernel addresses, so
+ * keep the IRQs disabled to protect us against concurrent TLB flushes.
+ */
+
+ if (user_mode(regs))
+ vc_handle_from_user(regs, error_code);
+ else
+ vc_handle_from_kernel(regs, error_code);
}
#DB and MCE use idtentry_mce_db and split out in asm. When I look at
idtentry_vc, it appears to me that VC_SAFE_STACK already implies
from-user, or am I reading that wrong?
Ah, it appears you're muddling things up again by then also calling
safe_stack_ from exc_.
How about you don't do that and have exc_ call your new from_kernel
function, then we know that safe_stack_ is always from-user. Then also
maybe do:
s/VS_SAFE_STACK/VC_USER/
s/safe_stack_/noist_/
to match all the others (#DB/MCE).
Also, AFAICT, you don't actually need DEFINE_IDTENTRY_VC_IST, it doesn't
have an ASM counterpart.
So then you end up with something like:
DEFINE_IDTENTRY_VC(exc_vc)
{
if (unlikely(on_vc_fallback_stack(regs))) {
instrumentation_begin();
panic("boohooo\n");
instrumentation_end();
}
vc_from_kernel();
}
DEFINE_IDTENTRY_VC_USER(exc_vc)
{
vc_from_user();
}
Which is, I'm thinking, much simpler, no?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
#DB and MCE use idtentry_mce_db and split out in asm. When I look at
idtentry_vc, it appears to me that VC_SAFE_STACK already implies
from-user, or am I reading that wrong?
VC_SAFE_STACK does not imply from-user. It means that the #VC handler
asm code was able to switch away from the IST stack to either the
task-stack (if from-user or syscall gap) or to the previous kernel
stack. There is a check in vc_switch_off_ist() that shows which stacks
are considered safe.
If it can not switch to a safe stack the VC entry code switches to the
fall-back stack and a special handler function is called, which for now
just panics the system.
How about you don't do that and have exc_ call your new from_kernel
function, then we know that safe_stack_ is always from-user. Then also
maybe do:
s/VS_SAFE_STACK/VC_USER/
s/safe_stack_/noist_/
to match all the others (#DB/MCE).
So #VC is different from #DB and #MCE in that it switches stacks even
when coming from kernel mode, so that the #VC handler can be nested.
What I can do is to call the from_user function directly from asm in
the .Lfrom_user_mode_switch_stack path. That will avoid having another
from_user check in C code.
DEFINE_IDTENTRY_VC(exc_vc)
{
if (unlikely(on_vc_fallback_stack(regs))) {
instrumentation_begin();
panic("boohooo\n");
instrumentation_end();
The on_vc_fallback_stack() path is for now only calling panic(), because
it can't be hit when the hypervisor is behaving correctly. In the future
it is not clear yet if that path needs to be extended for SNP page
validation exceptions, which can basically happen anywhere.
The implementation of SNP should make sure that all memory touched
during entry (while on unsafe stacks) is always validated, but not sure
yet if that holds when live-migration of SNP guests is added to the
picture.
There is the possibility that this doesn't fit in the above branch, but
it can also be moved to a separate function if needed.
}
vc_from_kernel();
}
DEFINE_IDTENTRY_VC_USER(exc_vc)
{
vc_from_user();
}
Which is, I'm thinking, much simpler, no?
On Thu, Jun 10, 2021 at 11:11:37AM +0200, Joerg Roedel wrote:
From: Joerg Roedel <redacted>
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.
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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.