x86/sev-es: Fixes for SEV-ES Guest Support
Subsystems (4)
v2 cover x86/sev-es: Fixes for SEV-ES Guest Support
From: Joerg Roedel <[email protected]>
Hi,
here is the second version of my pending SEV-ES fixes. The most important patches are patch 1 to 5, as they fix warnings and splats that trigger with various debugging options are enabled.
Patches 6 to 8 fix a correctness issue in the instruction emulation part of the #VC exception handler.
Please review.
Thanks,
Joerg
Link to v1: https://lore.kernel.org/lkml/[email protected]/
Changes since v1:
- Documented why __get_user()/__put_user() are safe to use in the #VC handlers memory access path.
- Merged the revert into patch 3
- Refactored code in the instruction decoder and added #GP reporting when getting the instructions linear address fails.
Joerg Roedel (8):
x86/sev-es: Don't return NULL from sev_es_get_ghcb() x86/sev-es: Forward page-faults which happen during emulation x86/sev-es: Use __put_user()/__get_user() for data accesses x86/sev-es: Fix error message in runtime #VC handler x86/sev-es: Leave NMI-mode before sending signals 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/include/asm/insn-eval.h | 6 +- arch/x86/kernel/sev.c | 127 +++++++++++++++++++++---------- arch/x86/kernel/umip.c | 10 +-- arch/x86/lib/insn-eval.c | 52 ++++++++----- 4 files changed, 129 insertions(+), 66 deletions(-)
base-commit: a50c5bebc99c525e7fbc059988c6a5ab8680cb76
Signature · 1 line
2.31.1
PATCH v2 1/8 x86/sev-es: Don't return NULL from sev_es_get_ghcb()
From: Joerg Roedel <[email protected]>
The sev_es_get_ghcb() is called from several places, but only one of them checks the return value. The reaction to returning NULL is always the same: Calling panic() and kill the machine.
Instead of adding checks to all call-places, move the panic() into the function itself so that it will no longer return NULL.
--- arch/x86/kernel/sev.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
arch/x86/kernel/sev.c +12 -13
@@ -203,8 +203,18 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)if (unlikely (data->ghcb_active)) {/* GHCB is already in use - save its contents */ -if (unlikely (data->backup_ghcb_active)) -return NULL ; +if (unlikely (data->backup_ghcb_active)) { +/* +* Backup-GHCB is also already in use. There is no way +* to continue here so just kill the machine. To make +* panic() work, mark GHCBs inactive so that messages +* can be printed out. +*/ + data->ghcb_active = false; + data->backup_ghcb_active = false; + +panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); + }/* Mark backup_ghcb active before writing to it */ data->backup_ghcb_active = true;
@@ -1289,7 +1299,6 @@ static __always_inline bool on_vc_fallback_stack(struct pt_regs *regs)*/ DEFINE_IDTENTRY_VC_SAFE_STACK (exc_vmm_communication) { -struct sev_es_runtime_data *data =this_cpu_read (runtime_data);irqentry_state_t irq_state;struct ghcb_state state;struct es_em_ctxt ctxt;
@@ -1315,16 +1324,6 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)*/ ghcb =sev_es_get_ghcb (&state); -if (!ghcb) { -/* -* Mark GHCBs inactive so that panic() is able to print the -* message. -*/ - data->ghcb_active = false; - data->backup_ghcb_active = false; - -panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); - }vc_ghcb_invalidate (ghcb); result =vc_init_em_ctxt (&ctxt, regs, error_code);
Signature · 1 line
2.31.1
[tip: x86/urgent] x86/sev-es: Don't return NULL from sev_es_get_ghcb()
The following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: b250f2f7792d15bcde98e0456781e2835556d5fa Gitweb: https://git.kernel.org/tip/b250f2f7792d15bcde98e0456781e2835556d5fa Author: Joerg Roedel <[email protected]> AuthorDate: Wed, 19 May 2021 15:52:44 +02:00 Committer: Borislav Petkov <[email protected]> CommitterDate: Wed, 19 May 2021 17:05:13 +02:00
x86/sev-es: Don't return NULL from sev_es_get_ghcb()
sev_es_get_ghcb() is called from several places but only one of them checks the return value. The reaction to returning NULL is always the
same: calling panic() and kill the machine.
Instead of adding checks to all call sites, move the panic() into the function itself so that it will no longer return NULL.
--- arch/x86/kernel/sev.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)
arch/x86/kernel/sev.c +12 -13
@@ -203,8 +203,18 @@ static __always_inline struct ghcb *sev_es_get_ghcb(struct ghcb_state *state)if (unlikely (data->ghcb_active)) {/* GHCB is already in use - save its contents */ -if (unlikely (data->backup_ghcb_active)) -return NULL ; +if (unlikely (data->backup_ghcb_active)) { +/* +* Backup-GHCB is also already in use. There is no way +* to continue here so just kill the machine. To make +* panic() work, mark GHCBs inactive so that messages +* can be printed out. +*/ + data->ghcb_active = false; + data->backup_ghcb_active = false; + +panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); + }/* Mark backup_ghcb active before writing to it */ data->backup_ghcb_active = true;
@@ -1289,7 +1299,6 @@ static __always_inline bool on_vc_fallback_stack(struct pt_regs *regs)*/ DEFINE_IDTENTRY_VC_SAFE_STACK (exc_vmm_communication) { -struct sev_es_runtime_data *data =this_cpu_read (runtime_data);irqentry_state_t irq_state;struct ghcb_state state;struct es_em_ctxt ctxt;
@@ -1315,16 +1324,6 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)*/ ghcb =sev_es_get_ghcb (&state); -if (!ghcb) { -/* -* Mark GHCBs inactive so that panic() is able to print the -* message. -*/ - data->ghcb_active = false; - data->backup_ghcb_active = false; - -panic ("Unable to handle #VC exception! GHCB and Backup GHCB are already in use" ); - }vc_ghcb_invalidate (ghcb); result =vc_init_em_ctxt (&ctxt, regs, error_code);
PATCH v2 2/8 x86/sev-es: Forward page-faults which happen during emulation
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
When emulating guest instructions for MMIO or IOIO accesses the #VC handler might get a page-fault and will not be able to complete. Forward the page-fault in this case to the correct handler instead of killing the machine.
--- arch/x86/kernel/sev.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/x86/kernel/sev.c +4 -0
@@ -1270,6 +1270,10 @@ static __always_inline void vc_forward_exception(struct es_em_ctxt *ctxt)case X86_TRAP_UD :exc_invalid_op (ctxt->regs);break ; +case X86_TRAP_PF : +write_cr2 (ctxt->fi.cr2); +exc_page_fault (ctxt->regs, error_code); +break ;case X86_TRAP_AC :exc_alignment_check (ctxt->regs, error_code);break ;
Signature · 1 line
2.31.1
[tip: x86/urgent] x86/sev-es: Forward page-faults which happen during emulation
The following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: c25bbdb564060adaad5c3a8a10765c13487ba6a3 Gitweb: https://git.kernel.org/tip/c25bbdb564060adaad5c3a8a10765c13487ba6a3 Author: Joerg Roedel <[email protected]> AuthorDate: Wed, 19 May 2021 15:52:45 +02:00 Committer: Borislav Petkov <[email protected]> CommitterDate: Wed, 19 May 2021 17:13:04 +02:00
x86/sev-es: Forward page-faults which happen during emulation
When emulating guest instructions for MMIO or IOIO accesses, the #VC handler might get a page-fault and will not be able to complete. Forward the page-fault in this case to the correct handler instead of killing the machine.
--- arch/x86/kernel/sev.c | 4 ++++ 1 file changed, 4 insertions(+)
arch/x86/kernel/sev.c +4 -0
@@ -1270,6 +1270,10 @@ static __always_inline void vc_forward_exception(struct es_em_ctxt *ctxt)case X86_TRAP_UD :exc_invalid_op (ctxt->regs);break ; +case X86_TRAP_PF : +write_cr2 (ctxt->fi.cr2); +exc_page_fault (ctxt->regs, error_code); +break ;case X86_TRAP_AC :exc_alignment_check (ctxt->regs, error_code);break ;
PATCH v2 3/8 x86/sev-es: Use __put_user()/__get_user() for data accesses
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
The put_user() and get_user() functions do checks on the address which is passed to them. They check whether the address is actually a user-space address and whether its fine to access it. They also call might_fault() to indicate that they could fault and possibly sleep.
All of these checks are neither wanted nor needed in the #VC exception handler, which can be invoked from almost any context and also for MMIO instructions from kernel space on kernel memory. All the #VC handler wants to know is whether a fault happened when the access was tried.
This is provided by __put_user()/__get_user(), which just do the access no matter what. Also add comments explaining why __get_user() and __put_user() are the best choice here and why it is safe to use them in this context. Also explain why copy_to/from_user can't be used.
In addition, also revert commit
024f60d6552 x86/sev-es: ("Handle string port IO to kernel memory properly")
because using __get_user()/__put_user() fixes the same problem while the above commit introduced several problems:
1) It uses access_ok() which is only allowed in task context.
2) It uses memcpy() which has no fault handling at all and is thus unsafe to use here.
--- arch/x86/kernel/sev.c | 66 ++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 20 deletions(-)
arch/x86/kernel/sev.c +46 -20
@@ -315,31 +315,44 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,u16 d2;u8 d1; -/* If instruction ran in kernel mode and the I/O buffer is in kernel space */ -if (!user_mode (ctxt->regs) && !access_ok (target, size)) { -memcpy (dst, buf, size); -return ES_OK ; - } - +/* +* This function uses __put_user() independent of whether kernel or user +* memory is accessed. This works fine because __put_user() does no +* sanity checks of the pointer being accessed. All that it does is +* to report when the access failed. +* +* Also, this function runs in atomic context, so __put_user() is not +* allowed to sleep. The page-fault handler detects that it is running +* in atomic context and will not try to take mmap_sem and handle the +* fault, so additional pagefault_enable()/disable() calls are not +* needed. +* +* The access can't be done via copy_to_user() here because +* vc_write_mem() must not use string instructions to access unsafe +* memory. The reason is that MOVS is emulated by the #VC handler by +* splitting the move up into a read and a write and taking a nested #VC +* exception on whatever of them is the MMIO access. Using string +* instructions here would cause infinite nesting. +*/ switch (size) {case 1 :memcpy (&d1, buf,1 ); -if (put_user (d1, target)) +if (__put_user (d1, target)) goto fault;break ;case 2 :memcpy (&d2, buf,2 ); -if (put_user (d2, target)) +if (__put_user (d2, target)) goto fault;break ;case 4 :memcpy (&d4, buf,4 ); -if (put_user (d4, target)) +if (__put_user (d4, target)) goto fault;break ;case 8 :memcpy (&d8, buf,8 ); -if (put_user (d8, target)) +if (__put_user (d8, target)) goto fault;break ;default :
@@ -370,30 +383,43 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,u16 d2;u8 d1; -/* If instruction ran in kernel mode and the I/O buffer is in kernel space */ -if (!user_mode (ctxt->regs) && !access_ok (s, size)) { -memcpy (buf, src, size); -return ES_OK ; - } - +/* +* This function uses __get_user() independent of whether kernel or user +* memory is accessed. This works fine because __get_user() does no +* sanity checks of the pointer being accessed. All that it does is +* to report when the access failed. +* +* Also, this function runs in atomic context, so __get_user() is not +* allowed to sleep. The page-fault handler detects that it is running +* in atomic context and will not try to take mmap_sem and handle the +* fault, so additional pagefault_enable()/disable() calls are not +* needed. +* +* The access can't be done via copy_from_user() here because +* vc_read_mem() must not use string instructions to access unsafe +* memory. The reason is that MOVS is emulated by the #VC handler by +* splitting the move up into a read and a write and taking a nested #VC +* exception on whatever of them is the MMIO access. Using string +* instructions here would cause infinite nesting. +*/ switch (size) {case 1 : -if (get_user (d1, s)) +if (__get_user (d1, s)) goto fault;memcpy (buf, &d1,1 );break ;case 2 : -if (get_user (d2, s)) +if (__get_user (d2, s)) goto fault;memcpy (buf, &d2,2 );break ;case 4 : -if (get_user (d4, s)) +if (__get_user (d4, s)) goto fault;memcpy (buf, &d4,4 );break ;case 8 : -if (get_user (d8, s)) +if (__get_user (d8, s)) goto fault;memcpy (buf, &d8,8 );break ;
Signature · 1 line
2.31.1
[tip: x86/urgent] x86/sev-es: Use __put_user()/__get_user() for data accesses
The following commit has been merged into the x86/urgent branch of tip:
The following commit has been merged into the x86/urgent branch of tip:
Commit-ID: 4954f5b8ef0baf70fe978d1a99a5f70e4dd5c877 Gitweb: https://git.kernel.org/tip/4954f5b8ef0baf70fe978d1a99a5f70e4dd5c877 Author: Joerg Roedel <[email protected]> AuthorDate: Wed, 19 May 2021 15:52:46 +02:00 Committer: Borislav Petkov <[email protected]> CommitterDate: Wed, 19 May 2021 18:45:37 +02:00
x86/sev-es: Use __put_user()/__get_user() for data accesses
The put_user() and get_user() functions do checks on the address which is passed to them. They check whether the address is actually a user-space address and whether its fine to access it. They also call might_fault() to indicate that they could fault and possibly sleep.
All of these checks are neither wanted nor needed in the #VC exception handler, which can be invoked from almost any context and also for MMIO instructions from kernel space on kernel memory. All the #VC handler wants to know is whether a fault happened when the access was tried.
This is provided by __put_user()/__get_user(), which just do the access no matter what. Also add comments explaining why __get_user() and __put_user() are the best choice here and why it is safe to use them in this context. Also explain why copy_to/from_user can't be used.
In addition, also revert commit
7024f60d6552 ("x86/sev-es: Handle string port IO to kernel memory properly")
because using __get_user()/__put_user() fixes the same problem while the above commit introduced several problems:
1) It uses access_ok() which is only allowed in task context.
2) It uses memcpy() which has no fault handling at all and is thus unsafe to use here.
[ bp: Fix up commit ID of the reverted commit above. ]
--- arch/x86/kernel/sev.c | 66 +++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 20 deletions(-)
arch/x86/kernel/sev.c +46 -20
@@ -315,31 +315,44 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,u16 d2;u8 d1; -/* If instruction ran in kernel mode and the I/O buffer is in kernel space */ -if (!user_mode (ctxt->regs) && !access_ok (target, size)) { -memcpy (dst, buf, size); -return ES_OK ; - } - +/* +* This function uses __put_user() independent of whether kernel or user +* memory is accessed. This works fine because __put_user() does no +* sanity checks of the pointer being accessed. All that it does is +* to report when the access failed. +* +* Also, this function runs in atomic context, so __put_user() is not +* allowed to sleep. The page-fault handler detects that it is running +* in atomic context and will not try to take mmap_sem and handle the +* fault, so additional pagefault_enable()/disable() calls are not +* needed. +* +* The access can't be done via copy_to_user() here because +* vc_write_mem() must not use string instructions to access unsafe +* memory. The reason is that MOVS is emulated by the #VC handler by +* splitting the move up into a read and a write and taking a nested #VC +* exception on whatever of them is the MMIO access. Using string +* instructions here would cause infinite nesting. +*/ switch (size) {case 1 :memcpy (&d1, buf,1 ); -if (put_user (d1, target)) +if (__put_user (d1, target)) goto fault;break ;case 2 :memcpy (&d2, buf,2 ); -if (put_user (d2, target)) +if (__put_user (d2, target)) goto fault;break ;case 4 :memcpy (&d4, buf,4 ); -if (put_user (d4, target)) +if (__put_user (d4, target)) goto fault;break ;case 8 :memcpy (&d8, buf,8 ); -if (put_user (d8, target)) +if (__put_user (d8, target)) goto fault;break ;default :
@@ -370,30 +383,43 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,u16 d2;u8 d1; -/* If instruction ran in kernel mode and the I/O buffer is in kernel space */ -if (!user_mode (ctxt->regs) && !access_ok (s, size)) { -memcpy (buf, src, size); -return ES_OK ; - } - +/* +* This function uses __get_user() independent of whether kernel or user +* memory is accessed. This works fine because __get_user() does no +* sanity checks of the pointer being accessed. All that it does is +* to report when the access failed. +* +* Also, this function runs in atomic context, so __get_user() is not +* allowed to sleep. The page-fault handler detects that it is running +* in atomic context and will not try to take mmap_sem and handle the +* fault, so additional pagefault_enable()/disable() calls are not +* needed. +* +* The access can't be done via copy_from_user() here because +* vc_read_mem() must not use string instructions to access unsafe +* memory. The reason is that MOVS is emulated by the #VC handler by +* splitting the move up into a read and a write and taking a nested #VC +* exception on whatever of them is the MMIO access. Using string +* instructions here would cause infinite nesting. +*/ switch (size) {case 1 : -if (get_user (d1, s)) +if (__get_user (d1, s)) goto fault;memcpy (buf, &d1,1 );break ;case 2 : -if (get_user (d2, s)) +if (__get_user (d2, s)) goto fault;memcpy (buf, &d2,2 );break ;case 4 : -if (get_user (d4, s)) +if (__get_user (d4, s)) goto fault;memcpy (buf, &d4,4 );break ;case 8 : -if (get_user (d8, s)) +if (__get_user (d8, s)) goto fault;memcpy (buf, &d8,8 );break ;
PATCH v2 4/8 x86/sev-es: Fix error message in runtime #VC handler
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
The runtime #VC handler is not "early" anymore. Fix the copy&paste error and remove that word from the error message.
--- arch/x86/kernel/sev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kernel/sev.c +1 -1
@@ -1369,7 +1369,7 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)vc_finish_insn (&ctxt);break ;case ES_UNSUPPORTED : -pr_err_ratelimited ("Unsupported exit-code 0x%02lx in early #VC exception (IP: 0x%lx)\n" , +pr_err_ratelimited ("Unsupported exit-code 0x%02lx in #VC exception (IP: 0x%lx)\n" , error_code, regs->ip); goto fail;case ES_VMM_ERROR :
Signature · 1 line
2.31.1
PATCH v2 5/8 x86/sev-es: Leave NMI-mode before sending signals
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
The error path in the runtime #VC handler sends a signal to kill the current task if the exception was raised from user-space. Some parts of the #VC handler run in NMI mode, because it is critical that it is not interrupted (except from an NMI) while the GHCB is in use.
But sending signals in NMI-mode is actually broken and triggers lockdep warnings. On the other side, when the signal is sent, there is no reason for the handler to still be in NMI-mode, as the GHCB is not used anymore.
Leave NMI-mode before entering the error path to get rid of the lockdep warnings.
--- arch/x86/kernel/sev.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
arch/x86/kernel/sev.c +13 -4
@@ -1343,9 +1343,10 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)return ; } +instrumentation_begin (); + irq_state =irqentry_nmi_enter (regs);lockdep_assert_irqs_disabled (); -instrumentation_begin ();/* * This is invoked through an interrupt gate, so IRQs are disabled. The
@@ -1395,13 +1396,19 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)BUG (); } -out: -instrumentation_end ();irqentry_nmi_exit (regs, irq_state); +instrumentation_end ();return ; fail: +/* +* Leave NMI mode - the GHCB is not busy anymore and depending on where +* the #VC came from this code is about to either kill the task (when in +* task context) or kill the machine. +*/ +irqentry_nmi_exit (regs, irq_state); +if (user_mode (regs)) {/* * Do not kill the machine if user-space triggered the
@@ -1423,7 +1430,9 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)panic ("Returned from Terminate-Request to Hypervisor\n" ); } - goto out; +instrumentation_end (); + +return ; }/* This handler runs on the #VC fall-back stack. It can cause further #VC exceptions */
Signature · 1 line
2.31.1
Re: [PATCH v2 5/8] x86/sev-es: Leave NMI-mode before sending signals
Quoted 4 lines · Joerg Roedel
arch/x86/kernel/sev.c +0
-0@@ -1343,9 +1343,10 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)return ;
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1352
Quoted 10 lines
arch/x86/kernel/sev.c +2
-1} +instrumentation_begin (); + irq_state =irqentry_nmi_enter (regs);lockdep_assert_irqs_disabled (); -instrumentation_begin ();/* * This is invoked through an interrupt gate, so IRQs are disabled. The
That's just plain wrong. No instrumentation is allowed before you enter the exception context.
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1400
Quoted 8 lines
arch/x86/kernel/sev.c +1
-2@@ -1395,13 +1396,19 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)BUG (); } -out: -instrumentation_end ();irqentry_nmi_exit (regs, irq_state); +instrumentation_end ();
And this can't be right either, same issue, no instrumentation is allowed after you leave the exception context.
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1411
Quoted 11 lines
arch/x86/kernel/sev.c +7
-0return ; fail: +/* +* Leave NMI mode - the GHCB is not busy anymore and depending on where +* the #VC came from this code is about to either kill the task (when in +* task context) or kill the machine. +*/ +irqentry_nmi_exit (regs, irq_state); +
And this is wrong too; because at this point the handler doesn't run in _any_ context anymore, certainly not one you can call regular C code from.
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1436
Quoted 12 lines
arch/x86/kernel/sev.c +3
-1if (user_mode (regs)) {/* * Do not kill the machine if user-space triggered the @@ -1423,7 +1430,9 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)panic ("Returned from Terminate-Request to Hypervisor\n" ); } - goto out; +instrumentation_end (); + +return ; }
You either get to do what MCE does, or what MCE does. That is, either use task_work or MCE_USER and have the _user() handler use irqentry_enter_from_user_mode().
The above is an absolute no-go.
Re: [PATCH v2 5/8] x86/sev-es: Leave NMI-mode before sending signals
Hi Peter,
thanks for your review.
Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · Joerg Roedel
arch/x86/kernel/sev.c +2
-1@@ -1343,9 +1343,10 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)return ; } +instrumentation_begin (); + irq_state =irqentry_nmi_enter (regs);lockdep_assert_irqs_disabled (); -instrumentation_begin ();
Quoted 5 lines
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1352
Quoted 2 lines
arch/x86/kernel/sev.c +0
-0/* * This is invoked through an interrupt gate, so IRQs are disabled. The That's just plain wrong. No instrumentation is allowed before you enter the exception context.
Okay.
Quoted 6 lines
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1411
Quoted 2 lines
arch/x86/kernel/sev.c +2
-0+irqentry_nmi_exit (regs, irq_state); +And this is wrong too; because at this point the handler doesn't run in _any_ context anymore, certainly not one you can call regular C code from.
The #VC handler is at this point not running on the IST stack anymore, but on the stack it came from or on the task stack. So my believe was that at this point it inherits the context it came from (just like the page-fault handler). But I also don't fully understand the context tracking, so is my assumption wrong?
Regards,
Joerg
Re: [PATCH v2 5/8] x86/sev-es: Leave NMI-mode before sending signals
Quoted 33 lines · Joerg Roedel
Hi Peter,
thanks for your review.
Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · Joerg Roedel
arch/x86/kernel/sev.c +2
-1@@ -1343,9 +1343,10 @@ DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication)return ; } +instrumentation_begin (); + irq_state =irqentry_nmi_enter (regs);lockdep_assert_irqs_disabled (); -instrumentation_begin ();Quoted 5 lines
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1352
Quoted 2 lines
arch/x86/kernel/sev.c +0
-0/* * This is invoked through an interrupt gate, so IRQs are disabled. The That's just plain wrong. No instrumentation is allowed before you enter the exception context.
Okay.
Quoted 6 lines
arch/x86/kernel/sev.c DEFINE_IDTENTRY_VC_SAFE_STACK(exc_vmm_communication) · line 1411
Quoted 2 lines
arch/x86/kernel/sev.c +2
-0+irqentry_nmi_exit (regs, irq_state); +And this is wrong too; because at this point the handler doesn't run in _any_ context anymore, certainly not one you can call regular C code from.
Quoted 5 lines
The #VC handler is at this point not running on the IST stack anymore, but on the stack it came from or on the task stack. So my believe was that at this point it inherits the context it came from (just like the page-fault handler). But I also don't fully understand the context tracking, so is my assumption wrong?
Being on the right stack is only part of the issue; you also need to make sure your runtime environment is set up.
Regular kernel C expects a whole lot of things to be present; esp. so with all the debug options on. The irqentry_*_enter() family of functions very carefully sets up this environment and the irqentry_*_exit() undoes it again. Before and after you really cannot run normal code.
Just an example, RCU might not be watching, it might think the CPU is in userspace and advance the GP while you're relying on it not doing so.
Similarly lockdep is in some undefined state and any lock used can trigger random 'funny' things.
Just because this is 'C', doesn't immediately mean you can go call any random function. Up until recently most of this was in ASM. There's a reason for the noinstr annotations.
PATCH v2 6/8 x86/insn-eval: Make 0 a valid RIP for insn_get_effective_ip()
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
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().
--- arch/x86/lib/insn-eval.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
arch/x86/lib/insn-eval.c +7 -7
@@ -1417,7 +1417,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs) } } -static unsigned long insn_get_effective_ip (struct pt_regs *regs) +static int insn_get_effective_ip (struct pt_regs *regs,unsigned long *ip) {unsigned long seg_base =0 ;
@@ -1430,10 +1430,12 @@ static unsigned long insn_get_effective_ip(struct pt_regs *regs)if (!user_64bit_mode (regs)) { seg_base =insn_get_seg_base (regs,INAT_SEG_REG_CS );if (seg_base ==-1L ) -return 0 ; +return -EINVAL ; } -return seg_base + regs->ip; + *ip = seg_base + regs->ip; + +return 0 ; }/**
@@ -1455,8 +1457,7 @@ int insn_fetch_from_user(struct pt_regs *regs, unsigned char buf[MAX_INSN_SIZE])unsigned long ip;int not_copied; - ip =insn_get_effective_ip (regs); -if (!ip) +if (insn_get_effective_ip (regs, &ip))return 0 ; not_copied =copy_from_user (buf, (void __user *)ip,MAX_INSN_SIZE );
@@ -1484,8 +1485,7 @@ int insn_fetch_from_user_inatomic(struct pt_regs *regs, unsigned char buf[MAX_INunsigned long ip;int not_copied; - ip =insn_get_effective_ip (regs); -if (!ip) +if (insn_get_effective_ip (regs, &ip))return 0 ; not_copied =__copy_from_user_inatomic (buf, (void __user *)ip,MAX_INSN_SIZE );
Signature · 1 line
2.31.1
PATCH v2 7/8 x86/insn: Extend error reporting from insn_fetch_from_user[_inatomic]()
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
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.
--- arch/x86/include/asm/insn-eval.h | 6 +++-- arch/x86/kernel/sev.c | 8 +++---- arch/x86/kernel/umip.c | 10 ++++----- arch/x86/lib/insn-eval.c | 38 +++++++++++++++++++++++--------- 4 files changed, 41 insertions(+), 21 deletions(-)
arch/x86/include/asm/insn-eval.h +4 -2
@@ -22,9 +22,11 @@ int insn_get_modrm_reg_off(struct insn *insn, struct pt_regs *regs);unsigned long insn_get_seg_base (struct pt_regs *regs,int seg_reg_idx);int insn_get_code_seg_params (struct pt_regs *regs);int insn_fetch_from_user (struct pt_regs *regs, -unsigned char buf[MAX_INSN_SIZE ]); +unsigned char buf[MAX_INSN_SIZE ], +int *copied);int insn_fetch_from_user_inatomic (struct pt_regs *regs, -unsigned char buf[MAX_INSN_SIZE ]); +unsigned char buf[MAX_INSN_SIZE ], +int *copied);bool insn_decode_from_regs (struct insn *insn,struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ],int buf_size);
arch/x86/kernel/sev.c +4 -4
@@ -258,17 +258,17 @@ static int vc_fetch_insn_kernel(struct es_em_ctxt *ctxt,static enum es_result __vc_decode_user_insn (struct es_em_ctxt *ctxt) {char buffer[MAX_INSN_SIZE ]; -int res; +int insn_bytes =0 , res; - res =insn_fetch_from_user_inatomic (ctxt->regs, buffer); -if (!res) { + res =insn_fetch_from_user_inatomic (ctxt->regs, buffer, &insn_bytes); +if (res) { ctxt->fi.vector =X86_TRAP_PF ; ctxt->fi.error_code =X86_PF_INSTR |X86_PF_USER ; ctxt->fi.cr2 = ctxt->regs->ip;return ES_EXCEPTION ; } -if (!insn_decode_from_regs (&ctxt->insn, ctxt->regs, buffer, res)) +if (!insn_decode_from_regs (&ctxt->insn, ctxt->regs, buffer, insn_bytes))return ES_DECODE_FAILED ;if (ctxt->insn.immediate.got)
arch/x86/kernel/umip.c +5 -5
@@ -346,13 +346,13 @@ bool fixup_umip_exception(struct pt_regs *regs)if (!regs)return false; - nr_copied =insn_fetch_from_user (regs, buf); -/* -* The insn_fetch_from_user above could have failed if user code -* is protected by a memory protection key. Give up on emulation -* in such a case. Should we issue a page fault? +* Give up on emulation if fetching the instruction failed. Should we +* issue a page fault or a #GP? */ +if (!insn_fetch_from_user (regs, buf,NULL )) +return false; +if (!nr_copied)return false;
arch/x86/lib/insn-eval.c +28 -10
@@ -1442,27 +1442,36 @@ static int insn_get_effective_ip(struct pt_regs *regs, unsigned long *ip)* insn_fetch_from_user() - Copy instruction bytes from user-space memory * @regs: Structure with register values as seen when entering kernel mode * @buf: Array to store the fetched instruction +* @copied: Pointer to an int where the number of copied instruction bytes +* is stored. Can be NULL. * * Gets the linear address of the instruction and copies the instruction bytes * to the buf. * * Returns: * -* Number of instruction bytes copied. +* -EINVAL if the linear address of the instruction could not be calculated +* -EFAULT if nothing was copied +* 0 on success * -* 0 if nothing was copied. */ -int insn_fetch_from_user (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ]) +int insn_fetch_from_user (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ], +int *copied) {unsigned long ip;int not_copied; +int bytes;if (insn_get_effective_ip (regs, &ip)) -return 0 ; +return -EINVAL ; not_copied =copy_from_user (buf, (void __user *)ip,MAX_INSN_SIZE ); -return MAX_INSN_SIZE - not_copied; + bytes =MAX_INSN_SIZE - not_copied; +if (copied) + *copied = bytes; + +return bytes ?0 : -EFAULT ; }/**
@@ -1470,27 +1479,36 @@ int insn_fetch_from_user(struct pt_regs *regs, unsigned char buf[MAX_INSN_SIZE])* while in atomic code * @regs: Structure with register values as seen when entering kernel mode * @buf: Array to store the fetched instruction +* @copied: Pointer to an int where the number of copied instruction bytes +* is stored. Can be NULL. * * Gets the linear address of the instruction and copies the instruction bytes * to the buf. This function must be used in atomic context. * * Returns: * -* Number of instruction bytes copied. +* -EINVAL if the linear address of the instruction could not be calculated +* -EFAULT if nothing was copied +* 0 on success * -* 0 if nothing was copied. */ -int insn_fetch_from_user_inatomic (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ]) +int insn_fetch_from_user_inatomic (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ], +int *copied) {unsigned long ip;int not_copied; +int bytes;if (insn_get_effective_ip (regs, &ip)) -return 0 ; +return -EINVAL ; not_copied =__copy_from_user_inatomic (buf, (void __user *)ip,MAX_INSN_SIZE ); -return MAX_INSN_SIZE - not_copied; + bytes =MAX_INSN_SIZE - not_copied; +if (copied) + *copied = bytes; + +return bytes ?0 : -EFAULT ; }/**
Signature · 1 line
2.31.1
Borislav Petkov
maintainerRe: [PATCH v2 7/8] x86/insn: Extend error reporting from insn_fetch_from_user[_inatomic]()
Quoted 33 lines · Joerg Roedel
arch/x86/lib/insn-eval.c +8
-4@@ -1442,27 +1442,36 @@ static int insn_get_effective_ip(struct pt_regs *regs, unsigned long *ip)* insn_fetch_from_user() - Copy instruction bytes from user-space memory * @regs: Structure with register values as seen when entering kernel mode * @buf: Array to store the fetched instruction +* @copied: Pointer to an int where the number of copied instruction bytes +* is stored. Can be NULL. * * Gets the linear address of the instruction and copies the instruction bytes * to the buf. * * Returns: * -* Number of instruction bytes copied. +* -EINVAL if the linear address of the instruction could not be calculated +* -EFAULT if nothing was copied +* 0 on success * -* 0 if nothing was copied. */ -int insn_fetch_from_user (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ]) +int insn_fetch_from_user (struct pt_regs *regs,unsigned char buf[MAX_INSN_SIZE ], +int *copied) {unsigned long ip;int not_copied; +int bytes;if (insn_get_effective_ip (regs, &ip)) -return 0 ;
arch/x86/lib/insn-eval.c static int insn_get_effective_ip(struct pt_regs *regs, unsigned long *ip) · line 1474
Quoted 10 lines
arch/x86/lib/insn-eval.c +6
-1+return -EINVAL ; not_copied =copy_from_user (buf, (void __user *)ip,MAX_INSN_SIZE ); -return MAX_INSN_SIZE - not_copied; + bytes =MAX_INSN_SIZE - not_copied; +if (copied) + *copied = bytes; + +return bytes ?0 : -EFAULT ;
Why not simpler?
return value >= 0 says how many bytes were copied
return value < 0 means some kind of error
And then you don't need @copied...
Ditto for the other one.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquettePATCH v2 8/8 x86/sev-es: Propagate #GP if getting linear instruction address failed
In reply to Joerg RoedelFrom: Joerg Roedel <[email protected]>
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.
--- arch/x86/kernel/sev.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
arch/x86/kernel/sev.c +6 -1
@@ -261,11 +261,16 @@ static enum es_result __vc_decode_user_insn(struct es_em_ctxt *ctxt)int insn_bytes =0 , res; res =insn_fetch_from_user_inatomic (ctxt->regs, buffer, &insn_bytes); -if (res) { +if (res == -EFAULT ) { ctxt->fi.vector =X86_TRAP_PF ; ctxt->fi.error_code =X86_PF_INSTR |X86_PF_USER ; ctxt->fi.cr2 = ctxt->regs->ip;return ES_EXCEPTION ; + }else if (res == -EINVAL ) { + ctxt->fi.vector =X86_TRAP_GP ; + ctxt->fi.error_code =0 ; + ctxt->fi.cr2 =0 ; +return ES_EXCEPTION ; }if (!insn_decode_from_regs (&ctxt->insn, ctxt->regs, buffer, insn_bytes))
Signature · 1 line
2.31.1