[PATCH 0/2] arm{,64}/kvm: survive unknown traps from guests

[PATCH 0/2] arm{,64}/kvm: survive unknown traps from guests

[Mark Rutland]

As future versions of the architecture add trappable functionality, it is
possible that KVM guests may be able to trigger exceptions that KVM doesn't
currently understand. Currently, we will BUG() if we take such an exception.

While we can't handle such traps very gracefully, we should limit any fallout
to the guest, rather than allowing guests to potentially bring down the host.

These patches try to make arm/arm64 KVM robust in these cases.

Thanks,
Mark.

Mark Rutland (2):
  arm/kvm: survive unknown traps from guests
  arm64/kvm: survive unknown traps from guests

 arch/arm/include/asm/kvm_arm.h |  1 +
 arch/arm/kvm/handle_exit.c     | 19 ++++++++++++-------
 arch/arm64/kvm/handle_exit.c   | 19 ++++++++++++-------
 3 files changed, 25 insertions(+), 14 deletions(-)

-- 
1.9.1

[PATCH 1/2] arm/kvm: survive unknown traps from guests

[Mark Rutland]

Currently we BUG() if we see a HSR.EC value we don't recognise. As
configurable disables/enables are added to the architecture (controlled
by RES1/RES0 bits respectively), with associated synchronous exceptions,
it may be possible for a guest to trigger exceptions with classes that
we don't recognise.

While we can't service these exceptions in a manner useful to the guest,
we can avoid bringing down the host. Per ARM DDI 0406C.c, all currently
unallocated HSR EC encodings are reserved, and per ARM DDI
0487A.k_iss10775, page G6-4395, EC values within the range 0x00 - 0x2c
are reserved for future use with synchronous exceptions, and EC values
within the range 0x2d - 0x3f may be used for either synchronous or
asynchronous exceptions.

The patch makes KVM handle any unknown EC by injecting an UNDEFINED
exception into the guest, with a corresponding (ratelimited) warning in
the host dmesg. We could later improve on this with with a new (opt-in)
exit to the host userspace.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Dave Martin <dave.martin@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm/include/asm/kvm_arm.h |  1 +
 arch/arm/kvm/handle_exit.c     | 19 ++++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index e22089f..a3f0b3d 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -209,6 +209,7 @@
 #define HSR_EC_IABT_HYP	(0x21)
 #define HSR_EC_DABT	(0x24)
 #define HSR_EC_DABT_HYP	(0x25)
+#define HSR_EC_MAX	(0x3f)
 
 #define HSR_WFI_IS_WFE		(_AC(1, UL) << 0)
 
diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
index 4e40d19..96af65a 100644
--- a/arch/arm/kvm/handle_exit.c
+++ b/arch/arm/kvm/handle_exit.c
@@ -79,7 +79,19 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+
+	kvm_pr_unimpl("Unknown exception class: hsr: %#08x\n",
+		      hsr);
+
+	kvm_inject_undefined(vcpu);
+	return 1;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
+	[0 ... HSR_EC_MAX]	= kvm_handle_unknown_ec,
 	[HSR_EC_WFI]		= kvm_handle_wfx,
 	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
 	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
@@ -98,13 +110,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 {
 	u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
 
-	if (hsr_ec >= ARRAY_SIZE(arm_exit_handlers) ||
-	    !arm_exit_handlers[hsr_ec]) {
-		kvm_err("Unknown exception class: hsr: %#08x\n",
-			(unsigned int)kvm_vcpu_get_hsr(vcpu));
-		BUG();
-	}
-
 	return arm_exit_handlers[hsr_ec];
 }
 
-- 
1.9.1

[PATCH 2/2] arm64/kvm: survive unknown traps from guests

[Mark Rutland]

Currently we BUG() if we see an ESR_EL2.EC value we don't recognise. As
configurable disables/enables are added to the architecture (controlled
by RES1/RES0 bits respectively), with associated synchronous exceptions,
it may be possible for a guest to trigger exceptions with classes that
we don't recognise.

While we can't service these exceptions in a manner useful to the guest,
we can avoid bringing down the host. Per ARM DDI 0487A.k_iss10775, page
D7-1937, EC values within the range 0x00 - 0x2c are reserved for future
use with synchronous exceptions, and EC values within the range 0x2d -
0x3f may be used for either synchronous or asynchronous exceptions.

The patch makes KVM handle any unknown EC by injecting an UNDEFINED
exception into the guest, with a corresponding (ratelimited) warning in
the host dmesg. We could later improve on this with with a new (opt-in)
exit to the host userspace.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Dave Martin <dave.martin@arm.com>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
---
 arch/arm64/kvm/handle_exit.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 1bfe30d..fa1b18e 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -135,7 +135,19 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return ret;
 }
 
+static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+
+	kvm_pr_unimpl("Unknown exception class: hsr: %#08x -- %s\n",
+		      hsr, esr_get_class_string(hsr));
+
+	kvm_inject_undefined(vcpu);
+	return 1;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
+	[0 ... ESR_ELx_EC_MAX]	= kvm_handle_unknown_ec,
 	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
 	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
 	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
@@ -162,13 +174,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
 	u32 hsr = kvm_vcpu_get_hsr(vcpu);
 	u8 hsr_ec = ESR_ELx_EC(hsr);
 
-	if (hsr_ec >= ARRAY_SIZE(arm_exit_handlers) ||
-	    !arm_exit_handlers[hsr_ec]) {
-		kvm_err("Unknown exception class: hsr: %#08x -- %s\n",
-			hsr, esr_get_class_string(hsr));
-		BUG();
-	}
-
 	return arm_exit_handlers[hsr_ec];
 }
 
-- 
1.9.1

[PATCH 2/2] arm64/kvm: survive unknown traps from guests

[Suzuki K Poulose]

On 20/02/17 12:30, Mark Rutland wrote:
> Currently we BUG() if we see an ESR_EL2.EC value we don't recognise. As
> configurable disables/enables are added to the architecture (controlled
> by RES1/RES0 bits respectively), with associated synchronous exceptions,
> it may be possible for a guest to trigger exceptions with classes that
> we don't recognise.
>
> While we can't service these exceptions in a manner useful to the guest,
> we can avoid bringing down the host. Per ARM DDI 0487A.k_iss10775, page
> D7-1937, EC values within the range 0x00 - 0x2c are reserved for future
> use with synchronous exceptions, and EC values within the range 0x2d -
> 0x3f may be used for either synchronous or asynchronous exceptions.
>
> The patch makes KVM handle any unknown EC by injecting an UNDEFINED
> exception into the guest, with a corresponding (ratelimited) warning in
> the host dmesg. We could later improve on this with with a new (opt-in)
> exit to the host userspace.

If the unknown EC happens to be an asynchronous exception (allocated from the second range),
we are not sure if that was triggered by this guest, or a different guest or even
the hypervisor if we cannot really isolate the exception(with/without the RAS
extensions, i.e ESB). And I accept there may not be a perfect solution to the problem
either. May be we could explicitly mention about "unsure" exceptions, so that
it might help, people who may not really have the deep knowledge about the
exception code schemes.

Suzuki

[PATCH 2/2] arm64/kvm: survive unknown traps from guests

[Mark Rutland]

On Mon, Feb 20, 2017 at 01:48:32PM +0000, Suzuki K Poulose wrote:
> On 20/02/17 12:30, Mark Rutland wrote:

> >The patch makes KVM handle any unknown EC by injecting an UNDEFINED
> >exception into the guest, with a corresponding (ratelimited) warning in
> >the host dmesg. We could later improve on this with with a new (opt-in)
> >exit to the host userspace.
> 
> If the unknown EC happens to be an asynchronous exception (allocated
> from the second range), we are not sure if that was triggered by this
> guest, or a different guest or even the hypervisor if we cannot really
> isolate the exception(with/without the RAS extensions, i.e ESB).

Sure; that is certainly a worry. 

> And I accept there may not be a perfect solution to the problem
> either. May be we could explicitly mention about "unsure" exceptions,
> so that it might help, people who may not really have the deep
> knowledge about the exception code schemes.

I guess we could, though I'm not sure how this is going to be helpful
for those without an understanding of the EC values. We can only say it
*may* be an asynchronous exception, so to understand the issue you need
to understand the EC value.

Thanks,
Mark.

[PATCH 1/2] arm/kvm: survive unknown traps from guests

[Christoffer Dall]

On Mon, Feb 20, 2017 at 12:30:11PM +0000, Mark Rutland wrote:
> Currently we BUG() if we see a HSR.EC value we don't recognise. As
> configurable disables/enables are added to the architecture (controlled
> by RES1/RES0 bits respectively), with associated synchronous exceptions,
> it may be possible for a guest to trigger exceptions with classes that
> we don't recognise.
> 
> While we can't service these exceptions in a manner useful to the guest,
> we can avoid bringing down the host. Per ARM DDI 0406C.c, all currently
> unallocated HSR EC encodings are reserved, and per ARM DDI
> 0487A.k_iss10775, page G6-4395, EC values within the range 0x00 - 0x2c
> are reserved for future use with synchronous exceptions, and EC values
> within the range 0x2d - 0x3f may be used for either synchronous or
> asynchronous exceptions.
> 
> The patch makes KVM handle any unknown EC by injecting an UNDEFINED
> exception into the guest, with a corresponding (ratelimited) warning in
> the host dmesg. We could later improve on this with with a new (opt-in)
> exit to the host userspace.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Dave Martin <dave.martin@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm/include/asm/kvm_arm.h |  1 +
>  arch/arm/kvm/handle_exit.c     | 19 ++++++++++++-------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index e22089f..a3f0b3d 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -209,6 +209,7 @@
>  #define HSR_EC_IABT_HYP	(0x21)
>  #define HSR_EC_DABT	(0x24)
>  #define HSR_EC_DABT_HYP	(0x25)
> +#define HSR_EC_MAX	(0x3f)
>  
>  #define HSR_WFI_IS_WFE		(_AC(1, UL) << 0)
>  
> diff --git a/arch/arm/kvm/handle_exit.c b/arch/arm/kvm/handle_exit.c
> index 4e40d19..96af65a 100644
> --- a/arch/arm/kvm/handle_exit.c
> +++ b/arch/arm/kvm/handle_exit.c
> @@ -79,7 +79,19 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +
> +	kvm_pr_unimpl("Unknown exception class: hsr: %#08x\n",
> +		      hsr);
> +
> +	kvm_inject_undefined(vcpu);
> +	return 1;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
> +	[0 ... HSR_EC_MAX]	= kvm_handle_unknown_ec,

Cool stuff, didn't know you could do this in C.

>  	[HSR_EC_WFI]		= kvm_handle_wfx,
>  	[HSR_EC_CP15_32]	= kvm_handle_cp15_32,
>  	[HSR_EC_CP15_64]	= kvm_handle_cp15_64,
> @@ -98,13 +110,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>  {
>  	u8 hsr_ec = kvm_vcpu_trap_get_class(vcpu);
>  
> -	if (hsr_ec >= ARRAY_SIZE(arm_exit_handlers) ||
> -	    !arm_exit_handlers[hsr_ec]) {
> -		kvm_err("Unknown exception class: hsr: %#08x\n",
> -			(unsigned int)kvm_vcpu_get_hsr(vcpu));
> -		BUG();
> -	}
> -
>  	return arm_exit_handlers[hsr_ec];
>  }
>  
> -- 
> 1.9.1
> 

Reviewed-by: Christoffer Dall <cdall@linaro.org>

[PATCH 2/2] arm64/kvm: survive unknown traps from guests

[Christoffer Dall]

On Mon, Feb 20, 2017 at 12:30:12PM +0000, Mark Rutland wrote:
> Currently we BUG() if we see an ESR_EL2.EC value we don't recognise. As
> configurable disables/enables are added to the architecture (controlled
> by RES1/RES0 bits respectively), with associated synchronous exceptions,
> it may be possible for a guest to trigger exceptions with classes that
> we don't recognise.
> 
> While we can't service these exceptions in a manner useful to the guest,
> we can avoid bringing down the host. Per ARM DDI 0487A.k_iss10775, page
> D7-1937, EC values within the range 0x00 - 0x2c are reserved for future
> use with synchronous exceptions, and EC values within the range 0x2d -
> 0x3f may be used for either synchronous or asynchronous exceptions.
> 
> The patch makes KVM handle any unknown EC by injecting an UNDEFINED
> exception into the guest, with a corresponding (ratelimited) warning in
> the host dmesg. We could later improve on this with with a new (opt-in)
> exit to the host userspace.
> 
> Signed-off-by: Mark Rutland <mark.rutland@arm.com>
> Cc: Christoffer Dall <christoffer.dall@linaro.org>
> Cc: Dave Martin <dave.martin@arm.com>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> ---
>  arch/arm64/kvm/handle_exit.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 1bfe30d..fa1b18e 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -135,7 +135,19 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return ret;
>  }
>  
> +static int kvm_handle_unknown_ec(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +
> +	kvm_pr_unimpl("Unknown exception class: hsr: %#08x -- %s\n",
> +		      hsr, esr_get_class_string(hsr));
> +
> +	kvm_inject_undefined(vcpu);
> +	return 1;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
> +	[0 ... ESR_ELx_EC_MAX]	= kvm_handle_unknown_ec,
>  	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
>  	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
>  	[ESR_ELx_EC_CP15_64]	= kvm_handle_cp15_64,
> @@ -162,13 +174,6 @@ static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>  	u32 hsr = kvm_vcpu_get_hsr(vcpu);
>  	u8 hsr_ec = ESR_ELx_EC(hsr);
>  
> -	if (hsr_ec >= ARRAY_SIZE(arm_exit_handlers) ||
> -	    !arm_exit_handlers[hsr_ec]) {
> -		kvm_err("Unknown exception class: hsr: %#08x -- %s\n",
> -			hsr, esr_get_class_string(hsr));
> -		BUG();
> -	}
> -
>  	return arm_exit_handlers[hsr_ec];
>  }
>  
> -- 

Reviewed-by: Christoffer Dall <cdall@linaro.org>

[PATCH 1/2] arm/kvm: survive unknown traps from guests

[Mark Rutland]

On Mon, Mar 06, 2017 at 07:08:03AM -0800, Christoffer Dall wrote:
> On Mon, Feb 20, 2017 at 12:30:11PM +0000, Mark Rutland wrote:
> >  static exit_handle_fn arm_exit_handlers[] = {
> > +	[0 ... HSR_EC_MAX]	= kvm_handle_unknown_ec,
> 
> Cool stuff, didn't know you could do this in C.

Strictly speaking, it's a GCC-ism, though we use it all over the place:

[mark at leverpostej:~/src/linux]% git grep '\[.*\.\.\..*\]\s=' | wc -l
394

It may also make checkpatch a bit grumpy, though I think that's being
worked on.

> Reviewed-by: Christoffer Dall <cdall@linaro.org>

Cheers!

I assume you or Marc will pick this up?

Thanks,
Mark.

[PATCH 1/2] arm/kvm: survive unknown traps from guests

[Marc Zyngier]

On Mon, Mar 06 2017 at  5:33:30 pm GMT, Mark Rutland <mark.rutland@arm.com> wrote:
> On Mon, Mar 06, 2017 at 07:08:03AM -0800, Christoffer Dall wrote:
>> On Mon, Feb 20, 2017 at 12:30:11PM +0000, Mark Rutland wrote:
>> >  static exit_handle_fn arm_exit_handlers[] = {
>> > +	[0 ... HSR_EC_MAX]	= kvm_handle_unknown_ec,
>> 
>> Cool stuff, didn't know you could do this in C.
>
> Strictly speaking, it's a GCC-ism, though we use it all over the place:
>
> [mark at leverpostej:~/src/linux]% git grep '\[.*\.\.\..*\]\s=' | wc -l
> 394
>
> It may also make checkpatch a bit grumpy, though I think that's being
> worked on.
>
>> Reviewed-by: Christoffer Dall <cdall@linaro.org>
>
> Cheers!
>
> I assume you or Marc will pick this up?

Yes, I'll queue that.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.