[PATCH v2 0/4] Fix some KVM/HYP interactions with kprobes

[PATCH v2 0/4] Fix some KVM/HYP interactions with kprobes

[James Morse]

Hi guys,

When looking at Masami Hiramatsu's kprobe cleanup series, it occurred
to me the 'no KVM' isn't just about the EL1/EL2  split on non-VHE systems,
but whether KVM is prepared to handle stepping on a breakpoint. It's not.

This labels all the VHE-only KVM functions that run during world switch
with NOKPROBE_SYMBOL(), and always blacklists __hyp_text for kprobes
to cover the common guest entry/exit code.

I anticipate patch 1 going via the KVM tree to avoid conflicts.
Patch 2 will conflict with [0].


I'm not sure what the best thing to do with the hyp-stub is.
Patch 3 moves it to __hyp_text, and patch 4 covers the hibernate fallout
from doing that. We don't have any other mmu-off but not idmap'd text.
Probing the hyp-stub has to be done by address as the symbol names
alias those in the __entry_text, which is blacklisted. (although this
might depend on link order). I think anyone doing this is trying to
shoot themselves in the foot.


Know issues:
* Other regions we should blacklist are the kexec and hibernate 'copy
  everything' code, as the vectors may have been overwritten by the time
  we step on the probe. cpu-suspend needs investigating...


Thanks,

James

[0] lore.kernel.org/r/154753341900.31541.8135985235882849464.stgit@devbox

James Morse (4):
  KVM: arm64: Forbid kprobing of the VHE world-switch code
  arm64: kprobe: Always blacklist the KVM world-switch code
  arm64: hyp-stub: Forbid kprobing of the hyp-stub
  arm64: hibernate: Clean the __hyp_text to PoC after resume

 arch/arm64/kernel/hibernate.c      | 4 +++-
 arch/arm64/kernel/hyp-stub.S       | 2 ++
 arch/arm64/kernel/probes/kprobes.c | 6 +++---
 arch/arm64/kvm/hyp/switch.c        | 5 +++++
 arch/arm64/kvm/hyp/sysreg-sr.c     | 5 +++++
 5 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[James Morse]

On systems with VHE the kernel and KVM's world-switch code run at the
same exception level. Code that is only used on a VHE system does not
need to be annotated as __hyp_text as it can reside anywhere in the
kernel text.

__hyp_text was also used to prevent kprobes from patching breakpoint
instructions into this region, as this code runs at a different
exception level. While this is no longer true with VHE, KVM still
switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
world-switch code will cause a hyp-panic.

echo "p:weasel sysreg_save_guest_state_vhe" > /sys/kernel/debug/tracing/kprobe_events
echo 1 > /sys/kernel/debug/tracing/events/kprobes/weasel/enable
lkvm run -k /boot/Image --console serial -p "console=ttyS0 earlycon=uart,mmio,0x3f8"

  # lkvm run -k /boot/Image -m 384 -c 3 --name guest-1474
  Info: Placing fdt at 0x8fe00000 - 0x8fffffff
  Info: virtio-mmio.devices=0x200@0x10000:36

  Info: virtio-mmio.devices=0x200@0x10200:37

  Info: virtio-mmio.devices=0x200@0x10400:38

[  614.178186] Kernel panic - not syncing: HYP panic:
[  614.178186] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
[  614.178186] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
[  614.178186] VCPU:00000000f8de32f1
[  614.178383] CPU: 2 PID: 1482 Comm: kvm-vcpu-0 Not tainted 5.0.0-rc2 #10799
[  614.178446] Call trace:
[  614.178480]  dump_backtrace+0x0/0x148
[  614.178567]  show_stack+0x24/0x30
[  614.178658]  dump_stack+0x90/0xb4
[  614.178710]  panic+0x13c/0x2d8
[  614.178793]  hyp_panic+0xac/0xd8
[  614.178880]  kvm_vcpu_run_vhe+0x9c/0xe0
[  614.178958]  kvm_arch_vcpu_ioctl_run+0x454/0x798
[  614.179038]  kvm_vcpu_ioctl+0x360/0x898
[  614.179087]  do_vfs_ioctl+0xc4/0x858
[  614.179174]  ksys_ioctl+0x84/0xb8
[  614.179261]  __arm64_sys_ioctl+0x28/0x38
[  614.179348]  el0_svc_common+0x94/0x108
[  614.179401]  el0_svc_handler+0x38/0x78
[  614.179487]  el0_svc+0x8/0xc
[  614.179558] SMP: stopping secondary CPUs
[  614.179661] Kernel Offset: disabled
[  614.179695] CPU features: 0x003,2a80aa38
[  614.179758] Memory Limit: none
[  614.179858] ---[ end Kernel panic - not syncing: HYP panic:
[  614.179858] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
[  614.179858] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
[  614.179858] VCPU:00000000f8de32f1 ]---

Annotate the VHE world-switch functions that aren't marked
__hyp_text using NOKPROBE_SYMBOL().

Signed-off-by: James Morse <james.morse@arm.com>
Fixes: 3f5c90b890ac ("KVM: arm64: Introduce VHE-specific kvm_vcpu_run")
---

This has been an issue since the VHE/non-VHE world-switch paths were
split.


Changes since v1:
 * Switched to NOKPROBE_SYMBOL() as this doesn't move code between
   sections.

---
 arch/arm64/kvm/hyp/switch.c    | 5 +++++
 arch/arm64/kvm/hyp/sysreg-sr.c | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
index b0b1478094b4..421ebf6f7086 100644
--- a/arch/arm64/kvm/hyp/switch.c
+++ b/arch/arm64/kvm/hyp/switch.c
@@ -23,6 +23,7 @@
 #include <kvm/arm_psci.h>
 
 #include <asm/cpufeature.h>
+#include <asm/kprobes.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_host.h>
@@ -107,6 +108,7 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 
 	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
 }
+NOKPROBE_SYMBOL(activate_traps_vhe);
 
 static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 {
@@ -154,6 +156,7 @@ static void deactivate_traps_vhe(void)
 	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
 	write_sysreg(vectors, vbar_el1);
 }
+NOKPROBE_SYMBOL(deactivate_traps_vhe);
 
 static void __hyp_text __deactivate_traps_nvhe(void)
 {
@@ -513,6 +516,7 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	return exit_code;
 }
+NOKPROBE_SYMBOL(kvm_vcpu_run_vhe);
 
 /* Switch to the guest for legacy non-VHE systems */
 int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
@@ -620,6 +624,7 @@ static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
 	      read_sysreg_el2(esr),   read_sysreg_el2(far),
 	      read_sysreg(hpfar_el2), par, vcpu);
 }
+NOKPROBE_SYMBOL(__hyp_call_panic_vhe);
 
 void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
 {
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
index 68d6f7c3b237..b426e2cf973c 100644
--- a/arch/arm64/kvm/hyp/sysreg-sr.c
+++ b/arch/arm64/kvm/hyp/sysreg-sr.c
@@ -18,6 +18,7 @@
 #include <linux/compiler.h>
 #include <linux/kvm_host.h>
 
+#include <asm/kprobes.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
@@ -98,12 +99,14 @@ void sysreg_save_host_state_vhe(struct kvm_cpu_context *ctxt)
 {
 	__sysreg_save_common_state(ctxt);
 }
+NOKPROBE_SYMBOL(sysreg_save_host_state_vhe);
 
 void sysreg_save_guest_state_vhe(struct kvm_cpu_context *ctxt)
 {
 	__sysreg_save_common_state(ctxt);
 	__sysreg_save_el2_return_state(ctxt);
 }
+NOKPROBE_SYMBOL(sysreg_save_guest_state_vhe);
 
 static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
 {
@@ -188,12 +191,14 @@ void sysreg_restore_host_state_vhe(struct kvm_cpu_context *ctxt)
 {
 	__sysreg_restore_common_state(ctxt);
 }
+NOKPROBE_SYMBOL(sysreg_restore_host_state_vhe);
 
 void sysreg_restore_guest_state_vhe(struct kvm_cpu_context *ctxt)
 {
 	__sysreg_restore_common_state(ctxt);
 	__sysreg_restore_el2_return_state(ctxt);
 }
+NOKPROBE_SYMBOL(sysreg_restore_guest_state_vhe);
 
 void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
 {
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] arm64: kprobe: Always blacklist the KVM world-switch code

[James Morse]

On systems with VHE the kernel and KVM's world-switch code run at the
same exception level. Code that is only used on a VHE system does not
need to be annotated as __hyp_text as it can reside anywhere in the
 kernel text.

__hyp_text was also used to prevent kprobes from patching breakpoint
instructions into this region, as this code runs at a different
exception level. While this is no longer true with VHE, KVM still
switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
world-switch code will cause a hyp-panic.

Move the __hyp_text check in the kprobes blacklist so it applies on
VHE systems too, to cover the common code and guest enter/exit
assembly.

Fixes: 888b3c8720e0 ("arm64: Treat all entry code as non-kprobe-able")
Signed-off-by: James Morse <james.morse@arm.com>
Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 2a5b338b2542..f17afb99890c 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -478,13 +478,13 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 	    addr < (unsigned long)__entry_text_end) ||
 	    (addr >= (unsigned long)__idmap_text_start &&
 	    addr < (unsigned long)__idmap_text_end) ||
+	    (addr >= (unsigned long)__hyp_text_start &&
+	    addr < (unsigned long)__hyp_text_end) ||
 	    !!search_exception_tables(addr))
 		return true;
 
 	if (!is_kernel_in_hyp_mode()) {
-		if ((addr >= (unsigned long)__hyp_text_start &&
-		    addr < (unsigned long)__hyp_text_end) ||
-		    (addr >= (unsigned long)__hyp_idmap_text_start &&
+		if ((addr >= (unsigned long)__hyp_idmap_text_start &&
 		    addr < (unsigned long)__hyp_idmap_text_end))
 			return true;
 	}
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] arm64: hyp-stub: Forbid kprobing of the hyp-stub

[James Morse]

The hyp-stub is loaded by the kernel's early startup code at EL2
during boot, before KVM takes ownership later. The hyp-stub's
text is part of the regular kernel text, meaning it can be kprobed.

A breakpoint in the hyp-stub causes the CPU to spin in el2_sync_invalid.

Add it to the __hyp_text.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: stable@vger.kernel.org
---

This has been a problem since kprobes was merged, it should
probably have been covered in 888b3c8720e0.

I'm not sure __hyp_text is the right place. Its not idmaped,
and as it contains a set of vectors, adding it to the host/hyp
idmap sections could grow them beyond a page... but it does
run with the MMU off, so does need to be cleaned to PoC when
anything wacky, like hibernate happens. With this patch,
hibernate should clean the __hyp_text to PoC too.
---
 arch/arm64/kernel/hyp-stub.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
index e1261fbaa374..17f325ba831e 100644
--- a/arch/arm64/kernel/hyp-stub.S
+++ b/arch/arm64/kernel/hyp-stub.S
@@ -28,6 +28,8 @@
 #include <asm/virt.h>
 
 	.text
+	.pushsection	.hyp.text, "ax"
+
 	.align 11
 
 ENTRY(__hyp_stub_vectors)
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] arm64: hibernate: Clean the __hyp_text to PoC after resume

[James Morse]

During resume hibernate restores all physical memory. Any memory
that is accessed with the MMU disabled needs to be cleaned to the
PoC.

KVMs __hyp_text was previously ommitted as it runs with the MMU
enabled, but now that the hyp-stub is located in this section,
we must clean __hyp_text too.

This ensures secondary CPUs that come online after hibernate
has finished resuming, and load KVM via the freshly written
hyp-stub see the correct instructions.

Signed-off-by: James Morse <james.morse@arm.com>
Cc: stable@vger.kernel.org
---
 arch/arm64/kernel/hibernate.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/hibernate.c b/arch/arm64/kernel/hibernate.c
index 29cdc99688f3..9859e1178e6b 100644
--- a/arch/arm64/kernel/hibernate.c
+++ b/arch/arm64/kernel/hibernate.c
@@ -299,8 +299,10 @@ int swsusp_arch_suspend(void)
 		dcache_clean_range(__idmap_text_start, __idmap_text_end);
 
 		/* Clean kvm setup code to PoC? */
-		if (el2_reset_needed())
+		if (el2_reset_needed()) {
 			dcache_clean_range(__hyp_idmap_text_start, __hyp_idmap_text_end);
+			dcache_clean_range(__hyp_text_start, __hyp_text_end);
+		}
 
 		/* make the crash dump kernel image protected again */
 		crash_post_resume();
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[Masami Hiramatsu]

On Thu, 24 Jan 2019 16:32:54 +0000
James Morse <james.morse@arm.com> wrote:

> On systems with VHE the kernel and KVM's world-switch code run at the
> same exception level. Code that is only used on a VHE system does not
> need to be annotated as __hyp_text as it can reside anywhere in the
> kernel text.
> 
> __hyp_text was also used to prevent kprobes from patching breakpoint
> instructions into this region, as this code runs at a different
> exception level. While this is no longer true with VHE, KVM still
> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
> world-switch code will cause a hyp-panic.
> 
> echo "p:weasel sysreg_save_guest_state_vhe" > /sys/kernel/debug/tracing/kprobe_events
> echo 1 > /sys/kernel/debug/tracing/events/kprobes/weasel/enable
> lkvm run -k /boot/Image --console serial -p "console=ttyS0 earlycon=uart,mmio,0x3f8"
> 
>   # lkvm run -k /boot/Image -m 384 -c 3 --name guest-1474
>   Info: Placing fdt at 0x8fe00000 - 0x8fffffff
>   Info: virtio-mmio.devices=0x200@0x10000:36
> 
>   Info: virtio-mmio.devices=0x200@0x10200:37
> 
>   Info: virtio-mmio.devices=0x200@0x10400:38
> 
> [  614.178186] Kernel panic - not syncing: HYP panic:
> [  614.178186] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
> [  614.178186] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
> [  614.178186] VCPU:00000000f8de32f1
> [  614.178383] CPU: 2 PID: 1482 Comm: kvm-vcpu-0 Not tainted 5.0.0-rc2 #10799
> [  614.178446] Call trace:
> [  614.178480]  dump_backtrace+0x0/0x148
> [  614.178567]  show_stack+0x24/0x30
> [  614.178658]  dump_stack+0x90/0xb4
> [  614.178710]  panic+0x13c/0x2d8
> [  614.178793]  hyp_panic+0xac/0xd8
> [  614.178880]  kvm_vcpu_run_vhe+0x9c/0xe0
> [  614.178958]  kvm_arch_vcpu_ioctl_run+0x454/0x798
> [  614.179038]  kvm_vcpu_ioctl+0x360/0x898
> [  614.179087]  do_vfs_ioctl+0xc4/0x858
> [  614.179174]  ksys_ioctl+0x84/0xb8
> [  614.179261]  __arm64_sys_ioctl+0x28/0x38
> [  614.179348]  el0_svc_common+0x94/0x108
> [  614.179401]  el0_svc_handler+0x38/0x78
> [  614.179487]  el0_svc+0x8/0xc
> [  614.179558] SMP: stopping secondary CPUs
> [  614.179661] Kernel Offset: disabled
> [  614.179695] CPU features: 0x003,2a80aa38
> [  614.179758] Memory Limit: none
> [  614.179858] ---[ end Kernel panic - not syncing: HYP panic:
> [  614.179858] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
> [  614.179858] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
> [  614.179858] VCPU:00000000f8de32f1 ]---
> 
> Annotate the VHE world-switch functions that aren't marked
> __hyp_text using NOKPROBE_SYMBOL().

This looks good to me!

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Fixes: 3f5c90b890ac ("KVM: arm64: Introduce VHE-specific kvm_vcpu_run")
> ---
> 
> This has been an issue since the VHE/non-VHE world-switch paths were
> split.
> 
> 
> Changes since v1:
>  * Switched to NOKPROBE_SYMBOL() as this doesn't move code between
>    sections.
> 
> ---
>  arch/arm64/kvm/hyp/switch.c    | 5 +++++
>  arch/arm64/kvm/hyp/sysreg-sr.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index b0b1478094b4..421ebf6f7086 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -23,6 +23,7 @@
>  #include <kvm/arm_psci.h>
>  
>  #include <asm/cpufeature.h>
> +#include <asm/kprobes.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_host.h>
> @@ -107,6 +108,7 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
>  }
> +NOKPROBE_SYMBOL(activate_traps_vhe);
>  
>  static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  {
> @@ -154,6 +156,7 @@ static void deactivate_traps_vhe(void)
>  	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
>  	write_sysreg(vectors, vbar_el1);
>  }
> +NOKPROBE_SYMBOL(deactivate_traps_vhe);
>  
>  static void __hyp_text __deactivate_traps_nvhe(void)
>  {
> @@ -513,6 +516,7 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	return exit_code;
>  }
> +NOKPROBE_SYMBOL(kvm_vcpu_run_vhe);
>  
>  /* Switch to the guest for legacy non-VHE systems */
>  int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> @@ -620,6 +624,7 @@ static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
>  	      read_sysreg_el2(esr),   read_sysreg_el2(far),
>  	      read_sysreg(hpfar_el2), par, vcpu);
>  }
> +NOKPROBE_SYMBOL(__hyp_call_panic_vhe);
>  
>  void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
>  {
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 68d6f7c3b237..b426e2cf973c 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -18,6 +18,7 @@
>  #include <linux/compiler.h>
>  #include <linux/kvm_host.h>
>  
> +#include <asm/kprobes.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_hyp.h>
> @@ -98,12 +99,14 @@ void sysreg_save_host_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_common_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_save_host_state_vhe);
>  
>  void sysreg_save_guest_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_common_state(ctxt);
>  	__sysreg_save_el2_return_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_save_guest_state_vhe);
>  
>  static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
>  {
> @@ -188,12 +191,14 @@ void sysreg_restore_host_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_common_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_restore_host_state_vhe);
>  
>  void sysreg_restore_guest_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_common_state(ctxt);
>  	__sysreg_restore_el2_return_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_restore_guest_state_vhe);
>  
>  void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  {
> -- 
> 2.20.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] arm64: hyp-stub: Forbid kprobing of the hyp-stub

[Christoffer Dall]

On Thu, Jan 24, 2019 at 04:32:56PM +0000, James Morse wrote:
> The hyp-stub is loaded by the kernel's early startup code at EL2
> during boot, before KVM takes ownership later. The hyp-stub's
> text is part of the regular kernel text, meaning it can be kprobed.
> 
> A breakpoint in the hyp-stub causes the CPU to spin in el2_sync_invalid.
> 
> Add it to the __hyp_text.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Cc: stable@vger.kernel.org
> ---
> 
> This has been a problem since kprobes was merged, it should
> probably have been covered in 888b3c8720e0.
> 
> I'm not sure __hyp_text is the right place. Its not idmaped,
> and as it contains a set of vectors, adding it to the host/hyp
> idmap sections could grow them beyond a page... but it does
> run with the MMU off, so does need to be cleaned to PoC when
> anything wacky, like hibernate happens. With this patch,
> hibernate should clean the __hyp_text to PoC too.

How did this code get cleaned before?

Is there a problem you can identify with putting it in __hyp_text?
Seems to me we should just stick it there if it has no negative
side-effects and otherwise we have to make up a separate section with a
specialized meaning.


Thanks,

    Christoffer

> ---
>  arch/arm64/kernel/hyp-stub.S | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kernel/hyp-stub.S b/arch/arm64/kernel/hyp-stub.S
> index e1261fbaa374..17f325ba831e 100644
> --- a/arch/arm64/kernel/hyp-stub.S
> +++ b/arch/arm64/kernel/hyp-stub.S
> @@ -28,6 +28,8 @@
>  #include <asm/virt.h>
>  
>  	.text
> +	.pushsection	.hyp.text, "ax"
> +
>  	.align 11
>  
>  ENTRY(__hyp_stub_vectors)
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[Christoffer Dall]

On Thu, Jan 24, 2019 at 04:32:54PM +0000, James Morse wrote:
> On systems with VHE the kernel and KVM's world-switch code run at the
> same exception level. Code that is only used on a VHE system does not
> need to be annotated as __hyp_text as it can reside anywhere in the
> kernel text.
> 
> __hyp_text was also used to prevent kprobes from patching breakpoint
> instructions into this region, as this code runs at a different
> exception level. While this is no longer true with VHE, KVM still
> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
> world-switch code will cause a hyp-panic.

Forgive potentially very stupid questions here, but:

 (1) Would it make sense to move the save/restore VBAR_EL1 to the last
     possible moment, and would that actually allow kprobes to work for
     the world-switch code, or does that just result in other weird
     problems?

 (2) Are we sure that this catches every call path of every non-inlined
     function called after switchign VBAR_EL1?  Can kprobes only be
     called on exported symbols, or can you (if you know the address
     somehow) put a kprobe on a static function as well.  If there are
     any concerns in this area, we might want to consider (1) more
     closely.


Thanks,

    Christoffer

> 
> echo "p:weasel sysreg_save_guest_state_vhe" > /sys/kernel/debug/tracing/kprobe_events
> echo 1 > /sys/kernel/debug/tracing/events/kprobes/weasel/enable
> lkvm run -k /boot/Image --console serial -p "console=ttyS0 earlycon=uart,mmio,0x3f8"
> 
>   # lkvm run -k /boot/Image -m 384 -c 3 --name guest-1474
>   Info: Placing fdt at 0x8fe00000 - 0x8fffffff
>   Info: virtio-mmio.devices=0x200@0x10000:36
> 
>   Info: virtio-mmio.devices=0x200@0x10200:37
> 
>   Info: virtio-mmio.devices=0x200@0x10400:38
> 
> [  614.178186] Kernel panic - not syncing: HYP panic:
> [  614.178186] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
> [  614.178186] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
> [  614.178186] VCPU:00000000f8de32f1
> [  614.178383] CPU: 2 PID: 1482 Comm: kvm-vcpu-0 Not tainted 5.0.0-rc2 #10799
> [  614.178446] Call trace:
> [  614.178480]  dump_backtrace+0x0/0x148
> [  614.178567]  show_stack+0x24/0x30
> [  614.178658]  dump_stack+0x90/0xb4
> [  614.178710]  panic+0x13c/0x2d8
> [  614.178793]  hyp_panic+0xac/0xd8
> [  614.178880]  kvm_vcpu_run_vhe+0x9c/0xe0
> [  614.178958]  kvm_arch_vcpu_ioctl_run+0x454/0x798
> [  614.179038]  kvm_vcpu_ioctl+0x360/0x898
> [  614.179087]  do_vfs_ioctl+0xc4/0x858
> [  614.179174]  ksys_ioctl+0x84/0xb8
> [  614.179261]  __arm64_sys_ioctl+0x28/0x38
> [  614.179348]  el0_svc_common+0x94/0x108
> [  614.179401]  el0_svc_handler+0x38/0x78
> [  614.179487]  el0_svc+0x8/0xc
> [  614.179558] SMP: stopping secondary CPUs
> [  614.179661] Kernel Offset: disabled
> [  614.179695] CPU features: 0x003,2a80aa38
> [  614.179758] Memory Limit: none
> [  614.179858] ---[ end Kernel panic - not syncing: HYP panic:
> [  614.179858] PS:404003c9 PC:ffff0000100d70e0 ESR:f2000004
> [  614.179858] FAR:0000000080080000 HPFAR:0000000000800800 PAR:1d00007edbadc0de
> [  614.179858] VCPU:00000000f8de32f1 ]---
> 
> Annotate the VHE world-switch functions that aren't marked
> __hyp_text using NOKPROBE_SYMBOL().
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> Fixes: 3f5c90b890ac ("KVM: arm64: Introduce VHE-specific kvm_vcpu_run")
> ---
> 
> This has been an issue since the VHE/non-VHE world-switch paths were
> split.
> 
> 
> Changes since v1:
>  * Switched to NOKPROBE_SYMBOL() as this doesn't move code between
>    sections.
> 
> ---
>  arch/arm64/kvm/hyp/switch.c    | 5 +++++
>  arch/arm64/kvm/hyp/sysreg-sr.c | 5 +++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index b0b1478094b4..421ebf6f7086 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -23,6 +23,7 @@
>  #include <kvm/arm_psci.h>
>  
>  #include <asm/cpufeature.h>
> +#include <asm/kprobes.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_host.h>
> @@ -107,6 +108,7 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
>  
>  	write_sysreg(kvm_get_hyp_vector(), vbar_el1);
>  }
> +NOKPROBE_SYMBOL(activate_traps_vhe);
>  
>  static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
>  {
> @@ -154,6 +156,7 @@ static void deactivate_traps_vhe(void)
>  	write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1);
>  	write_sysreg(vectors, vbar_el1);
>  }
> +NOKPROBE_SYMBOL(deactivate_traps_vhe);
>  
>  static void __hyp_text __deactivate_traps_nvhe(void)
>  {
> @@ -513,6 +516,7 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
>  
>  	return exit_code;
>  }
> +NOKPROBE_SYMBOL(kvm_vcpu_run_vhe);
>  
>  /* Switch to the guest for legacy non-VHE systems */
>  int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> @@ -620,6 +624,7 @@ static void __hyp_call_panic_vhe(u64 spsr, u64 elr, u64 par,
>  	      read_sysreg_el2(esr),   read_sysreg_el2(far),
>  	      read_sysreg(hpfar_el2), par, vcpu);
>  }
> +NOKPROBE_SYMBOL(__hyp_call_panic_vhe);
>  
>  void __hyp_text __noreturn hyp_panic(struct kvm_cpu_context *host_ctxt)
>  {
> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> index 68d6f7c3b237..b426e2cf973c 100644
> --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> @@ -18,6 +18,7 @@
>  #include <linux/compiler.h>
>  #include <linux/kvm_host.h>
>  
> +#include <asm/kprobes.h>
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/kvm_hyp.h>
> @@ -98,12 +99,14 @@ void sysreg_save_host_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_common_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_save_host_state_vhe);
>  
>  void sysreg_save_guest_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_save_common_state(ctxt);
>  	__sysreg_save_el2_return_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_save_guest_state_vhe);
>  
>  static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
>  {
> @@ -188,12 +191,14 @@ void sysreg_restore_host_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_common_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_restore_host_state_vhe);
>  
>  void sysreg_restore_guest_state_vhe(struct kvm_cpu_context *ctxt)
>  {
>  	__sysreg_restore_common_state(ctxt);
>  	__sysreg_restore_el2_return_state(ctxt);
>  }
> +NOKPROBE_SYMBOL(sysreg_restore_guest_state_vhe);
>  
>  void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
>  {
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64: kprobe: Always blacklist the KVM world-switch code

[Christoffer Dall]

On Thu, Jan 24, 2019 at 04:32:55PM +0000, James Morse wrote:
> On systems with VHE the kernel and KVM's world-switch code run at the
> same exception level. Code that is only used on a VHE system does not
> need to be annotated as __hyp_text as it can reside anywhere in the
>  kernel text.
> 
> __hyp_text was also used to prevent kprobes from patching breakpoint
> instructions into this region, as this code runs at a different
> exception level. While this is no longer true with VHE, KVM still
> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
> world-switch code will cause a hyp-panic.
> 
> Move the __hyp_text check in the kprobes blacklist so it applies on
> VHE systems too, to cover the common code and guest enter/exit
> assembly.
> 
> Fixes: 888b3c8720e0 ("arm64: Treat all entry code as non-kprobe-able")
> Signed-off-by: James Morse <james.morse@arm.com>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Reviewed-by: Christoffer Dall <christoffer.dall@arm.com>

> ---
>  arch/arm64/kernel/probes/kprobes.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 2a5b338b2542..f17afb99890c 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -478,13 +478,13 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
>  	    addr < (unsigned long)__entry_text_end) ||
>  	    (addr >= (unsigned long)__idmap_text_start &&
>  	    addr < (unsigned long)__idmap_text_end) ||
> +	    (addr >= (unsigned long)__hyp_text_start &&
> +	    addr < (unsigned long)__hyp_text_end) ||
>  	    !!search_exception_tables(addr))
>  		return true;
>  
>  	if (!is_kernel_in_hyp_mode()) {
> -		if ((addr >= (unsigned long)__hyp_text_start &&
> -		    addr < (unsigned long)__hyp_text_end) ||
> -		    (addr >= (unsigned long)__hyp_idmap_text_start &&
> +		if ((addr >= (unsigned long)__hyp_idmap_text_start &&
>  		    addr < (unsigned long)__hyp_idmap_text_end))
>  			return true;
>  	}
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[James Morse]

Hey Christoffer,

On 31/01/2019 08:08, Christoffer Dall wrote:
> On Thu, Jan 24, 2019 at 04:32:54PM +0000, James Morse wrote:
>> On systems with VHE the kernel and KVM's world-switch code run at the
>> same exception level. Code that is only used on a VHE system does not
>> need to be annotated as __hyp_text as it can reside anywhere in the
>> kernel text.
>>
>> __hyp_text was also used to prevent kprobes from patching breakpoint
>> instructions into this region, as this code runs at a different
>> exception level. While this is no longer true with VHE, KVM still
>> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
>> world-switch code will cause a hyp-panic.
> 
> Forgive potentially very stupid questions here, but:
> 
>  (1) Would it make sense to move the save/restore VBAR_EL1 to the last
>      possible moment, and would that actually allow kprobes to work for
>      the world-switch code, or does that just result in other weird
>      problems?

This would work for taking the debug exception. But next kprobes wants to
single-step the probed instruction in an out-of-line slot. I don't think we can
do this if we've already configured the debug hardware for the guest.
(If could at least turn single-step off when we return to guest-EL0, which
guest-EL1 was single-stepping)


>  (2) Are we sure that this catches every call path of every non-inlined
>      function called after switchign VBAR_EL1?  Can kprobes only be
>      called on exported symbols, or can you (if you know the address
>      somehow) put a kprobe on a static function as well.  If there are
>      any concerns in this area, we might want to consider (1) more
>      closely.

Hmmm, good question. The blacklisting applies to whole symbols as seen by
kallsyms, the compiler has no idea what is going on.

If it chose not to inline something, it would be kprobe'able yes.

__kprobes uses a section function-attribute instead. The gcc manual[0] doesn't
say what happens when inline and the section attributes are used together. (or
at least I couldn't find it)

A quick experiment with gcc 8.2.0 shows adding __kprobes on the inlines gets
discarded when they are inlined. I'm not sure how to trick the compiler into
not-inlining it to see what happens, but adding 'noinline' to the header file
causes it to duplicate the function everywhere, but puts it in the __kprobes
section.

(For KVM we could use the 'flatten' attribute, but that does say 'if possible'.
Alternatively we can decorate all the inline helpers we know we use with
__kprobes as a safety net.)

I think this is a wider problem with kprobes.


Thanks,

James

[0]
https://gcc.gnu.org/onlinedocs/gcc-8.2.0/gcc/Common-Function-Attributes.html#Common-Function-Attributes

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[Christoffer Dall]

On Thu, Jan 31, 2019 at 06:53:06PM +0000, James Morse wrote:
> Hey Christoffer,
> 
> On 31/01/2019 08:08, Christoffer Dall wrote:
> > On Thu, Jan 24, 2019 at 04:32:54PM +0000, James Morse wrote:
> >> On systems with VHE the kernel and KVM's world-switch code run at the
> >> same exception level. Code that is only used on a VHE system does not
> >> need to be annotated as __hyp_text as it can reside anywhere in the
> >> kernel text.
> >>
> >> __hyp_text was also used to prevent kprobes from patching breakpoint
> >> instructions into this region, as this code runs at a different
> >> exception level. While this is no longer true with VHE, KVM still
> >> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
> >> world-switch code will cause a hyp-panic.
> > 
> > Forgive potentially very stupid questions here, but:
> > 
> >  (1) Would it make sense to move the save/restore VBAR_EL1 to the last
> >      possible moment, and would that actually allow kprobes to work for
> >      the world-switch code, or does that just result in other weird
> >      problems?
> 
> This would work for taking the debug exception. But next kprobes wants to
> single-step the probed instruction in an out-of-line slot. I don't think we can
> do this if we've already configured the debug hardware for the guest.
> (If could at least turn single-step off when we return to guest-EL0, which
> guest-EL1 was single-stepping)
> 
> 

I suspected something like that, let's not go there.

> >  (2) Are we sure that this catches every call path of every non-inlined
> >      function called after switchign VBAR_EL1?  Can kprobes only be
> >      called on exported symbols, or can you (if you know the address
> >      somehow) put a kprobe on a static function as well.  If there are
> >      any concerns in this area, we might want to consider (1) more
> >      closely.
> 
> Hmmm, good question. The blacklisting applies to whole symbols as seen by
> kallsyms, the compiler has no idea what is going on.
> 
> If it chose not to inline something, it would be kprobe'able yes.
> 
> __kprobes uses a section function-attribute instead. The gcc manual[0] doesn't
> say what happens when inline and the section attributes are used together. (or
> at least I couldn't find it)
> 
> A quick experiment with gcc 8.2.0 shows adding __kprobes on the inlines gets
> discarded when they are inlined. I'm not sure how to trick the compiler into
> not-inlining it to see what happens, but adding 'noinline' to the header file
> causes it to duplicate the function everywhere, but puts it in the __kprobes
> section.
> 
> (For KVM we could use the 'flatten' attribute, but that does say 'if possible'.
> Alternatively we can decorate all the inline helpers we know we use with
> __kprobes as a safety net.)
> 
> I think this is a wider problem with kprobes.
> 

Sounds like it.  Probably in the "you did something crazy, and your
kernel is going to suffer from it" category.

Let's stick to your approach.

Thanks for the explanation.

    Christoffer

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] arm64: hyp-stub: Forbid kprobing of the hyp-stub

[James Morse]

Hi Christoffer,

On 31/01/2019 08:04, Christoffer Dall wrote:
> On Thu, Jan 24, 2019 at 04:32:56PM +0000, James Morse wrote:
>> The hyp-stub is loaded by the kernel's early startup code at EL2
>> during boot, before KVM takes ownership later. The hyp-stub's
>> text is part of the regular kernel text, meaning it can be kprobed.
>>
>> A breakpoint in the hyp-stub causes the CPU to spin in el2_sync_invalid.
>>
>> Add it to the __hyp_text.

>> This has been a problem since kprobes was merged, it should
>> probably have been covered in 888b3c8720e0.
>>
>> I'm not sure __hyp_text is the right place. Its not idmaped,
>> and as it contains a set of vectors, adding it to the host/hyp
>> idmap sections could grow them beyond a page... but it does
>> run with the MMU off, so does need to be cleaned to PoC when
>> anything wacky, like hibernate happens. With this patch,
>> hibernate should clean the __hyp_text to PoC too.
> 
> How did this code get cleaned before?

It didn't need to be cleaned as KVM executes it with the MMU on.
KVM's MMU-off code lives in the hyp_idmap, which is cleaned. (as is the kernel's
idmap).

The hibernate-cache-cleaning was trying to do the absolute minimum, but the
hyp-stub got forgotten.


> Is there a problem you can identify with putting it in __hyp_text?

> Seems to me we should just stick it there if it has no negative
> side-effects and otherwise we have to make up a separate section with a
> specialized meaning.

Yup, there is no problem with the extra cache-maintenance.
The hyp-stub is the odd one out, its runtime code that runs with the MMU off,
but isn't idmaped. I wasn't sure if we wanted to create some special
section.(having to name it is a good enough reason not to!)


Thanks,

James

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] KVM: arm64: Forbid kprobing of the VHE world-switch code

[Marc Zyngier]

On 24/01/2019 16:32, James Morse wrote:
> On systems with VHE the kernel and KVM's world-switch code run at the
> same exception level. Code that is only used on a VHE system does not
> need to be annotated as __hyp_text as it can reside anywhere in the
> kernel text.
> 
> __hyp_text was also used to prevent kprobes from patching breakpoint
> instructions into this region, as this code runs at a different
> exception level. While this is no longer true with VHE, KVM still
> switches VBAR_EL1, meaning a kprobe's breakpoint executed in the
> world-switch code will cause a hyp-panic.

[...]

For what it is worth, I've now queued this patch as a fix for 5.0.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel