kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support
@ 2021-05-12  7:54 Joerg Roedel
  2021-05-12  7:54 ` [PATCH 1/6] x86/sev-es: Don't return NULL from sev_es_get_ghcb() Joerg Roedel
                   ` (5 more replies)
  0 siblings, 6 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen,
	Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

From: Joerg Roedel <jroedel@suse.de>

Hi,

here is a collection of fixes for the SEV-ES guest support. They fix
simple and more severe issues and are all targeted for v5.13. The most
important fixes are the revert of 7024f60d6552 which just doesn't work
in any context the #VC handler could run and the fix to forward #PF
exceptions caused during emulation. The issue that 7024f60d6552
intended to fix should be fixed correctly with these patches too.

Please review and test.

Regards,

	Joerg

Joerg Roedel (6):
  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
  Revert "x86/sev-es: Handle string port IO to kernel memory properly"
  x86/sev-es: Fix error message in runtime #VC handler
  x86/sev-es: Leave NMI-mode before sending signals

 arch/x86/kernel/sev.c | 76 +++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 38 deletions(-)


base-commit: 059e5c321a65657877924256ea8ad9c0df257b45
-- 
2.31.1


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH 1/6] x86/sev-es: Don't return NULL from sev_es_get_ghcb()
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  2021-05-12  7:54 ` [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation Joerg Roedel
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

From: Joerg Roedel <jroedel@suse.de>

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.

Fixes: 0786138c78e7 ("x86/sev-es: Add a Runtime #VC Exception Handler")
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 9578c82832aa..c49270c7669e 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -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;
@@ -1284,7 +1294,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;
@@ -1310,16 +1319,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);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
  2021-05-12  7:54 ` [PATCH 1/6] x86/sev-es: Don't return NULL from sev_es_get_ghcb() Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  2021-05-12 17:31   ` Sean Christopherson
  2021-05-12  7:54 ` [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user Joerg Roedel
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

From: Joerg Roedel <jroedel@suse.de>

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.

Fixes: 0786138c78e7 ("x86/sev-es: Add a Runtime #VC Exception Handler")
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index c49270c7669e..6530a844eb61 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1265,6 +1265,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;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
  2021-05-12  7:54 ` [PATCH 1/6] x86/sev-es: Don't return NULL from sev_es_get_ghcb() Joerg Roedel
  2021-05-12  7:54 ` [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  2021-05-12  8:04   ` David Laight
  2021-05-12 15:57   ` Dave Hansen
  2021-05-12  7:54 ` [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly" Joerg Roedel
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

From: Joerg Roedel <jroedel@suse.de>

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 required 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.

Fixes: f980f9c31a92 ("x86/sev-es: Compile early handler code into kernel image")
Cc: stable@vger.kernel.org # v5.10+
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 6530a844eb61..110b39345b40 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -342,22 +342,22 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,
 	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:
@@ -396,22 +396,22 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,
 
 	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;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly"
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
                   ` (2 preceding siblings ...)
  2021-05-12  7:54 ` [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  2021-05-12 17:38   ` Sean Christopherson
  2021-05-12  7:54 ` [PATCH 5/6] x86/sev-es: Fix error message in runtime #VC handler Joerg Roedel
  2021-05-12  7:54 ` [PATCH 6/6] x86/sev-es: Leave NMI-mode before sending signals Joerg Roedel
  5 siblings, 1 reply; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

From: Joerg Roedel <jroedel@suse.de>

This reverts commit 7024f60d655272bd2ca1d3a4c9e0a63319b1eea1.

The commit reverted here introduces a short-cut into the #VC handlers
memory access code which only works reliably in task context. But the
kernels #VC handler can be invoked from any context, making the
access_ok() call trigger a warning with CONFIG_DEBUG_ATOMIC_SLEEP
enabled.

Also the memcpy() used in the reverted patch is wrong, as it has no
page-fault handling. Access to kernel memory can also fault due to
kernel bugs, and those should not be reported as faults from the #VC
handler but as bugs of their real call-site, which is correctly later
done from vc_forward_exception().

Fixes: 7024f60d6552 ("x86/sev-es: Handle string port IO to kernel memory properly")
Cc: stable@vger.kernel.org # v5.11
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 110b39345b40..f4f319004713 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -333,12 +333,6 @@ 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;
-	}
-
 	switch (size) {
 	case 1:
 		memcpy(&d1, buf, 1);
@@ -388,12 +382,6 @@ 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;
-	}
-
 	switch (size) {
 	case 1:
 		if (__get_user(d1, s))
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 5/6] x86/sev-es: Fix error message in runtime #VC handler
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
                   ` (3 preceding siblings ...)
  2021-05-12  7:54 ` [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly" Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  2021-05-12  7:54 ` [PATCH 6/6] x86/sev-es: Leave NMI-mode before sending signals Joerg Roedel
  5 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen,
	Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

From: Joerg Roedel <jroedel@suse.de>

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 <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index f4f319004713..77155c4f07f5 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1326,7 +1326,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:
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH 6/6] x86/sev-es: Leave NMI-mode before sending signals
  2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
                   ` (4 preceding siblings ...)
  2021-05-12  7:54 ` [PATCH 5/6] x86/sev-es: Fix error message in runtime #VC handler Joerg Roedel
@ 2021-05-12  7:54 ` Joerg Roedel
  5 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  7:54 UTC (permalink / raw)
  To: x86, Hyunwook Baek
  Cc: Joerg Roedel, Joerg Roedel, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen,
	Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

From: Joerg Roedel <jroedel@suse.de>

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.

Fixes: 62441a1fb532 ("x86/sev-es: Correctly track IRQ states in runtime #VC handler")
Signed-off-by: Joerg Roedel <jroedel@suse.de>
---
 arch/x86/kernel/sev.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 77155c4f07f5..79cbed2f28de 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -1300,9 +1300,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
@@ -1352,13 +1353,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
@@ -1380,7 +1387,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 */
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* RE: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  7:54 ` [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user Joerg Roedel
@ 2021-05-12  8:04   ` David Laight
  2021-05-12  8:16     ` Juergen Gross
  2021-05-12  8:37     ` 'Joerg Roedel'
  2021-05-12 15:57   ` Dave Hansen
  1 sibling, 2 replies; 22+ messages in thread
From: David Laight @ 2021-05-12  8:04 UTC (permalink / raw)
  To: 'Joerg Roedel', x86, Hyunwook Baek
  Cc: Joerg Roedel, stable, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen,
	Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

From: Joerg
> Sent: 12 May 2021 08:55
> 
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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 required 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.

That can't be right at all.
__put/get_user() are only valid on user addresses and will try to
fault in a missing page - so can sleep.

At best this is abused the calls.

	David

> Fixes: f980f9c31a92 ("x86/sev-es: Compile early handler code into kernel image")
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kernel/sev.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 6530a844eb61..110b39345b40 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -342,22 +342,22 @@ static enum es_result vc_write_mem(struct es_em_ctxt *ctxt,
>  	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:
> @@ -396,22 +396,22 @@ static enum es_result vc_read_mem(struct es_em_ctxt *ctxt,
> 
>  	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;
> --
> 2.31.1

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:04   ` David Laight
@ 2021-05-12  8:16     ` Juergen Gross
  2021-05-12  8:50       ` 'Joerg Roedel'
  2021-05-12  8:37     ` 'Joerg Roedel'
  1 sibling, 1 reply; 22+ messages in thread
From: Juergen Gross @ 2021-05-12  8:16 UTC (permalink / raw)
  To: David Laight, 'Joerg Roedel', x86, Hyunwook Baek
  Cc: Joerg Roedel, stable, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas,
	Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev,
	Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization


[-- Attachment #1.1.1: Type: text/plain, Size: 1082 bytes --]

On 12.05.21 10:04, David Laight wrote:
> From: Joerg
>> Sent: 12 May 2021 08:55
>>
>> From: Joerg Roedel <jroedel@suse.de>
>>
>> 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 required 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.
> 
> That can't be right at all.
> __put/get_user() are only valid on user addresses and will try to
> fault in a missing page - so can sleep.
> 
> At best this is abused the calls.

You want something like xen_safe_[read|write]_ulong().


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:04   ` David Laight
  2021-05-12  8:16     ` Juergen Gross
@ 2021-05-12  8:37     ` 'Joerg Roedel'
  2021-05-12 15:59       ` Dave Hansen
  1 sibling, 1 reply; 22+ messages in thread
From: 'Joerg Roedel' @ 2021-05-12  8:37 UTC (permalink / raw)
  To: David Laight
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

On Wed, May 12, 2021 at 08:04:33AM +0000, David Laight wrote:
> That can't be right at all.
> __put/get_user() are only valid on user addresses and will try to
> fault in a missing page - so can sleep.

Yes, in general these functions can sleep, but not in this context. They
are called in atomic context and the page-fault handler will notice that
and goes down the __bad_area_nosemaphore() path and only do the fixup.

I also thought about adding page_fault_disable()/page_fault_enable()
calls, but being in atomic context is enough according to the
faulthandler_disabled() implementation.

This is exactly what is needed here. All I want to know is whether a
fault happened or not, the page-fault handler must not try to fix the
fault in any way. If a fault happens it is later fixed up in
vc_forward_exception().

> At best this is abused the calls.

Yes, but that is only due to the naming of these functions. In this case
they do exactly what is needed.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:16     ` Juergen Gross
@ 2021-05-12  8:50       ` 'Joerg Roedel'
  2021-05-12  8:58         ` Juergen Gross
  0 siblings, 1 reply; 22+ messages in thread
From: 'Joerg Roedel' @ 2021-05-12  8:50 UTC (permalink / raw)
  To: Juergen Gross
  Cc: David Laight, x86, Hyunwook Baek, Joerg Roedel, stable, hpa,
	Andy Lutomirski, Dave Hansen, Peter Zijlstra, Jiri Slaby,
	Dan Williams, Tom Lendacky, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote:
> You want something like xen_safe_[read|write]_ulong().

From a first glance I can't see it, what is the difference between the
xen_safe_*_ulong() functions and __get_user()/__put_user()? The only
difference I can see is that __get/__put_user() support different access
sizes, but neither of those disables page-faults by itself, for example.

Couldn't these xen-specific functions not also be replaces by
__get_user()/__put_user()?

Regards,

	Joerg


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:50       ` 'Joerg Roedel'
@ 2021-05-12  8:58         ` Juergen Gross
  2021-05-12  9:31           ` David Laight
  2021-05-12  9:32           ` Joerg Roedel
  0 siblings, 2 replies; 22+ messages in thread
From: Juergen Gross @ 2021-05-12  8:58 UTC (permalink / raw)
  To: 'Joerg Roedel'
  Cc: David Laight, x86, Hyunwook Baek, Joerg Roedel, stable, hpa,
	Andy Lutomirski, Dave Hansen, Peter Zijlstra, Jiri Slaby,
	Dan Williams, Tom Lendacky, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization


[-- Attachment #1.1.1: Type: text/plain, Size: 702 bytes --]

On 12.05.21 10:50, 'Joerg Roedel' wrote:
> On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote:
>> You want something like xen_safe_[read|write]_ulong().
> 
>  From a first glance I can't see it, what is the difference between the
> xen_safe_*_ulong() functions and __get_user()/__put_user()? The only
> difference I can see is that __get/__put_user() support different access
> sizes, but neither of those disables page-faults by itself, for example.
> 
> Couldn't these xen-specific functions not also be replaces by
> __get_user()/__put_user()?

No, those were used before, but commit 9da3f2b7405440 broke Xen's use
case. That is why I did commit 1457d8cf7664f.


Juergen

[-- Attachment #1.1.2: OpenPGP_0xB0DE9DD628BF132F.asc --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* RE: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:58         ` Juergen Gross
@ 2021-05-12  9:31           ` David Laight
  2021-05-12  9:32           ` Joerg Roedel
  1 sibling, 0 replies; 22+ messages in thread
From: David Laight @ 2021-05-12  9:31 UTC (permalink / raw)
  To: 'Juergen Gross', 'Joerg Roedel'
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas,
	Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev,
	Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization

From: Juergen Gross
> Sent: 12 May 2021 09:58
> 
> On 12.05.21 10:50, 'Joerg Roedel' wrote:
> > On Wed, May 12, 2021 at 10:16:12AM +0200, Juergen Gross wrote:
> >> You want something like xen_safe_[read|write]_ulong().
> >
> >  From a first glance I can't see it, what is the difference between the
> > xen_safe_*_ulong() functions and __get_user()/__put_user()? The only
> > difference I can see is that __get/__put_user() support different access
> > sizes, but neither of those disables page-faults by itself, for example.
> >
> > Couldn't these xen-specific functions not also be replaces by
> > __get_user()/__put_user()?
> 
> No, those were used before, but commit 9da3f2b7405440 broke Xen's use
> case. That is why I did commit 1457d8cf7664f.

I've just looked at 9da3f2b7405440.

It doesn't look right to me - wrong return value for a lot of cases.
OTOH it isn't in my newest tree at all.

If bogus_uaccess() is now elsewhere I can't see how (without changes)
it would allow through these faults.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:58         ` Juergen Gross
  2021-05-12  9:31           ` David Laight
@ 2021-05-12  9:32           ` Joerg Roedel
  2021-05-19 11:33             ` 'Joerg Roedel'
  1 sibling, 1 reply; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12  9:32 UTC (permalink / raw)
  To: Juergen Gross
  Cc: 'Joerg Roedel',
	David Laight, x86, Hyunwook Baek, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Kees Cook, David Rientjes, Cfir Cohen, Erdem Aktas,
	Masami Hiramatsu, Mike Stunes, Sean Christopherson, Martin Radev,
	Arvind Sankar, linux-coco, linux-kernel, kvm, virtualization

On Wed, May 12, 2021 at 10:58:20AM +0200, Juergen Gross wrote:
> No, those were used before, but commit 9da3f2b7405440 broke Xen's use
> case. That is why I did commit 1457d8cf7664f.

I see, thanks for the heads-up. So here this is not a big issue, because
when an access to kernel space faults under SEV-ES, its a kernel bug
anyway. The issue is that it is not reported correctly.

I think I need to re-work the helper and use probe_kernel_read/write()
when the address is in kernel space. This distinction is already made
when fetching instruction bytes in the #VC handler, but I thought I
could get around it for data accesses.

Having the distinction between user and kernel memory accesses
explicitly in the code seems to be the most robust solution.

Regards,

	Joerg


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  7:54 ` [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user Joerg Roedel
  2021-05-12  8:04   ` David Laight
@ 2021-05-12 15:57   ` Dave Hansen
  2021-05-12 16:00     ` Joerg Roedel
  1 sibling, 1 reply; 22+ messages in thread
From: Dave Hansen @ 2021-05-12 15:57 UTC (permalink / raw)
  To: Joerg Roedel, x86, Hyunwook Baek
  Cc: Joerg Roedel, stable, hpa, Andy Lutomirski, Dave Hansen,
	Peter Zijlstra, Jiri Slaby, Dan Williams, Tom Lendacky,
	Juergen Gross, Kees Cook, David Rientjes, Cfir Cohen,
	Erdem Aktas, Masami Hiramatsu, Mike Stunes, Sean Christopherson,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

On 5/12/21 12:54 AM, Joerg Roedel wrote:
> 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 required 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.

The changelog _helps_, but using a "user" function to handle kernel MMIO
for its error handling properties seems like it's begging for a comment.

__put_user() also seems to have fun stuff like __chk_user_ptr().  It all
seems sketchy to me.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  8:37     ` 'Joerg Roedel'
@ 2021-05-12 15:59       ` Dave Hansen
  0 siblings, 0 replies; 22+ messages in thread
From: Dave Hansen @ 2021-05-12 15:59 UTC (permalink / raw)
  To: 'Joerg Roedel', David Laight
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

On 5/12/21 1:37 AM, 'Joerg Roedel' wrote:
> I also thought about adding page_fault_disable()/page_fault_enable()
> calls, but being in atomic context is enough according to the
> faulthandler_disabled() implementation.

That would be nice to add to a comment:
page_fault_disable()/page_fault_enable() are not needed because of the
context this must be called in.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12 15:57   ` Dave Hansen
@ 2021-05-12 16:00     ` Joerg Roedel
  0 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-12 16:00 UTC (permalink / raw)
  To: Dave Hansen
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

On Wed, May 12, 2021 at 08:57:53AM -0700, Dave Hansen wrote:
> The changelog _helps_, but using a "user" function to handle kernel MMIO
> for its error handling properties seems like it's begging for a comment.
> 
> __put_user() also seems to have fun stuff like __chk_user_ptr().  It all
> seems sketchy to me.

Yeah, as Juergen already pointed out, there are certain problems with
that too. But I don't want to write my own accessors, so I will
introduce a separate user and kernel path to these functions.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation
  2021-05-12  7:54 ` [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation Joerg Roedel
@ 2021-05-12 17:31   ` Sean Christopherson
  2021-05-19 13:16     ` Joerg Roedel
  0 siblings, 1 reply; 22+ messages in thread
From: Sean Christopherson @ 2021-05-12 17:31 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

On Wed, May 12, 2021, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> 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.
> 
> Fixes: 0786138c78e7 ("x86/sev-es: Add a Runtime #VC Exception Handler")
> Cc: stable@vger.kernel.org # v5.10+
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kernel/sev.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index c49270c7669e..6530a844eb61 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -1265,6 +1265,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;

This got me looking at the flows that "inject" #PF, and I'm pretty sure there
are bugs in __vc_decode_user_insn() + insn_get_effective_ip().

Problem #1: __vc_decode_user_insn() assumes a #PF if insn_fetch_from_user_inatomic()
fails, but the majority of failure cases in insn_get_seg_base() are #GPs, not #PF.

	res = insn_fetch_from_user_inatomic(ctxt->regs, buffer);
	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;
	}

Problem #2: Using '0' as an error code means a legitimate effective IP of '0'
will be misinterpreted as a failure.  Practically speaking, I highly doubt anyone
will ever actually run code at address 0, but it's technically possible.  The
most robust approach would be to pass a pointer to @ip and return an actual error
code.  Using a non-canonical magic value might also work, but that could run afoul
of future shenanigans like LAM.

	ip = insn_get_effective_ip(regs);
	if (!ip)
		return 0;

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly"
  2021-05-12  7:54 ` [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly" Joerg Roedel
@ 2021-05-12 17:38   ` Sean Christopherson
  2021-05-19 12:22     ` Joerg Roedel
  0 siblings, 1 reply; 22+ messages in thread
From: Sean Christopherson @ 2021-05-12 17:38 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

On Wed, May 12, 2021, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel@suse.de>
> 
> This reverts commit 7024f60d655272bd2ca1d3a4c9e0a63319b1eea1.
> 
> The commit reverted here introduces a short-cut into the #VC handlers
> memory access code which only works reliably in task context. But the
> kernels #VC handler can be invoked from any context, making the
> access_ok() call trigger a warning with CONFIG_DEBUG_ATOMIC_SLEEP
> enabled.
> 
> Also the memcpy() used in the reverted patch is wrong, as it has no
> page-fault handling. Access to kernel memory can also fault due to
> kernel bugs, and those should not be reported as faults from the #VC
> handler but as bugs of their real call-site, which is correctly later
> done from vc_forward_exception().

The changelog should call out that a previous patch fixed the original bug by
switching to unchecked versions of get/put.  Without that, this reads like we're
reverting to even worse behavior.

Alternatively, and probably even better, fold this revert into the switch to
the unchecked version (sounds like those will use kernel-specific flavors?).

> Fixes: 7024f60d6552 ("x86/sev-es: Handle string port IO to kernel memory properly")
> Cc: stable@vger.kernel.org # v5.11
> Signed-off-by: Joerg Roedel <jroedel@suse.de>
> ---
>  arch/x86/kernel/sev.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> index 110b39345b40..f4f319004713 100644
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -333,12 +333,6 @@ 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;
> -	}
> -
>  	switch (size) {
>  	case 1:
>  		memcpy(&d1, buf, 1);
> @@ -388,12 +382,6 @@ 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;
> -	}
> -
>  	switch (size) {
>  	case 1:
>  		if (__get_user(d1, s))
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user
  2021-05-12  9:32           ` Joerg Roedel
@ 2021-05-19 11:33             ` 'Joerg Roedel'
  0 siblings, 0 replies; 22+ messages in thread
From: 'Joerg Roedel' @ 2021-05-19 11:33 UTC (permalink / raw)
  To: Joerg Roedel
  Cc: Juergen Gross, David Laight, x86, Hyunwook Baek, stable, hpa,
	Andy Lutomirski, Dave Hansen, Peter Zijlstra, Jiri Slaby,
	Dan Williams, Tom Lendacky, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Sean Christopherson, Martin Radev, Arvind Sankar, linux-coco,
	linux-kernel, kvm, virtualization

On Wed, May 12, 2021 at 11:32:35AM +0200, Joerg Roedel wrote:
> On Wed, May 12, 2021 at 10:58:20AM +0200, Juergen Gross wrote:
> > No, those were used before, but commit 9da3f2b7405440 broke Xen's use
> > case. That is why I did commit 1457d8cf7664f.
>
> [...]
>
> Having the distinction between user and kernel memory accesses
> explicitly in the code seems to be the most robust solution.

On the other hand, as I found out today, 9da3f2b7405440 had a short
life-time and got reverted upstream. So using __get_user()/__put_user()
should be fine in this code path. It just deserves a comment explaining
its use here and why pagefault_enable()/disable() is not needed.
Even the get_kernel* helpers use __get_user_size() internally.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly"
  2021-05-12 17:38   ` Sean Christopherson
@ 2021-05-19 12:22     ` Joerg Roedel
  0 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-19 12:22 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

On Wed, May 12, 2021 at 05:38:29PM +0000, Sean Christopherson wrote:
> Alternatively, and probably even better, fold this revert into the switch to
> the unchecked version (sounds like those will use kernel-specific flavors?).

I folded this revert into the previous commit. But I kept the
__get_user()/__put_user() calls and just added a comment explaining why
they are used and why it is safe to use them.

After all, even the get_kernel*() functions call __get_user_size() under
the hood.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation
  2021-05-12 17:31   ` Sean Christopherson
@ 2021-05-19 13:16     ` Joerg Roedel
  0 siblings, 0 replies; 22+ messages in thread
From: Joerg Roedel @ 2021-05-19 13:16 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: x86, Hyunwook Baek, Joerg Roedel, stable, hpa, Andy Lutomirski,
	Dave Hansen, Peter Zijlstra, Jiri Slaby, Dan Williams,
	Tom Lendacky, Juergen Gross, Kees Cook, David Rientjes,
	Cfir Cohen, Erdem Aktas, Masami Hiramatsu, Mike Stunes,
	Martin Radev, Arvind Sankar, linux-coco, linux-kernel, kvm,
	virtualization

Hi Sean,

On Wed, May 12, 2021 at 05:31:03PM +0000, Sean Christopherson wrote:
> This got me looking at the flows that "inject" #PF, and I'm pretty sure there
> are bugs in __vc_decode_user_insn() + insn_get_effective_ip().
> 
> Problem #1: __vc_decode_user_insn() assumes a #PF if insn_fetch_from_user_inatomic()
> fails, but the majority of failure cases in insn_get_seg_base() are #GPs, not #PF.
> 
> 	res = insn_fetch_from_user_inatomic(ctxt->regs, buffer);
> 	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;
> 	}
> 
> Problem #2: Using '0' as an error code means a legitimate effective IP of '0'
> will be misinterpreted as a failure.  Practically speaking, I highly doubt anyone
> will ever actually run code at address 0, but it's technically possible.  The
> most robust approach would be to pass a pointer to @ip and return an actual error
> code.  Using a non-canonical magic value might also work, but that could run afoul
> of future shenanigans like LAM.
> 
> 	ip = insn_get_effective_ip(regs);
> 	if (!ip)
> 		return 0;

Your observations are all correct. I put some changes onto this
patch-set to fix these problems.

Regards,

	Joerg

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2021-05-19 13:16 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-12  7:54 [PATCH 0/6] x86/sev-es: Fixes for SEV-ES guest support Joerg Roedel
2021-05-12  7:54 ` [PATCH 1/6] x86/sev-es: Don't return NULL from sev_es_get_ghcb() Joerg Roedel
2021-05-12  7:54 ` [PATCH 2/6] x86/sev-es: Forward page-faults which happen during emulation Joerg Roedel
2021-05-12 17:31   ` Sean Christopherson
2021-05-19 13:16     ` Joerg Roedel
2021-05-12  7:54 ` [PATCH 3/6] x86/sev-es: Use __put_user()/__get_user Joerg Roedel
2021-05-12  8:04   ` David Laight
2021-05-12  8:16     ` Juergen Gross
2021-05-12  8:50       ` 'Joerg Roedel'
2021-05-12  8:58         ` Juergen Gross
2021-05-12  9:31           ` David Laight
2021-05-12  9:32           ` Joerg Roedel
2021-05-19 11:33             ` 'Joerg Roedel'
2021-05-12  8:37     ` 'Joerg Roedel'
2021-05-12 15:59       ` Dave Hansen
2021-05-12 15:57   ` Dave Hansen
2021-05-12 16:00     ` Joerg Roedel
2021-05-12  7:54 ` [PATCH 4/6] Revert "x86/sev-es: Handle string port IO to kernel memory properly" Joerg Roedel
2021-05-12 17:38   ` Sean Christopherson
2021-05-19 12:22     ` Joerg Roedel
2021-05-12  7:54 ` [PATCH 5/6] x86/sev-es: Fix error message in runtime #VC handler Joerg Roedel
2021-05-12  7:54 ` [PATCH 6/6] x86/sev-es: Leave NMI-mode before sending signals Joerg Roedel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).