[PATCH 0/3] KVM/arm64 fixes for AArch32 handling

[PATCH 0/3] KVM/arm64 fixes for AArch32 handling

[Marc Zyngier]

The (very much unloved) AArch32 handling has recently been found
lacking in a number of ways:

- Nina spotted a brown paper-bag quality bug in the register narrowing
  code when writing one of the core registers (GPRs, PSTATE) from
  userspace

- We never allowed System mode to be restored. Nobody ever complained,
  but this is wrong nonetheless

- The handling of traps failing their condition check went from dodgy
  to outright broken when the handling of ESR_EL2 was upgraded from 32
  to 64 bit (patch already posted).

All these are stable material, and I plan to merge them after -rc1
is released.

        M.

Marc Zyngier (3):
  KVM: arm64: Fix AArch32 register narrowing on userspace write
  KVM: arm64: Allow AArch32 PSTATE.M to be restored as System mode
  KVM: arm64: AArch32: Fix spurious trapping of conditional instructions

 arch/arm64/kvm/guest.c       |  3 ++-
 arch/arm64/kvm/hyp/aarch32.c | 18 ++++++++++++++++--
 2 files changed, 18 insertions(+), 3 deletions(-)

-- 
2.39.2


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

[PATCH 1/3] KVM: arm64: Fix AArch32 register narrowing on userspace write

[Marc Zyngier]

When userspace writes to once of the core registers, we make
sure to narrow the corresponding GPRs if PSTATE indicates
an AArch32 context.

The code tries to check whether the context is EL0 or EL1 so
that it narrows the correct registers. But it does so by checking
the full PSTATE instead of PSTATE.M.

As a consequence, and if we are restoring an AArch32 EL0 context
in a 64bit guest, and that PSTATE has *any* bit set outside of
PSTATE.M, we narrow *all* registers instead of only the first 15,
destroying the 64bit state.

Obviously, this is not something the guest is likely to enjoy.

Correctly masking PSTATE to only evaluate PSTATE.M fixes it.

Fixes: 90c1f934ed71 ("KVM: arm64: Get rid of the AArch32 register mapping code")
Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/guest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index e2f762d959bb..d9617b11f7a8 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -276,7 +276,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 	if (*vcpu_cpsr(vcpu) & PSR_MODE32_BIT) {
 		int i, nr_reg;
 
-		switch (*vcpu_cpsr(vcpu)) {
+		switch (*vcpu_cpsr(vcpu) & PSR_AA32_MODE_MASK) {
 		/*
 		 * Either we are dealing with user mode, and only the
 		 * first 15 registers (+ PC) must be narrowed to 32bit.
-- 
2.39.2


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

Re: [PATCH 1/3] KVM: arm64: Fix AArch32 register narrowing on userspace write

[Nina Schoetterl-Glausch]

On Fri, 2024-05-24 at 15:19 +0100, Marc Zyngier wrote:
> When userspace writes to once of the core registers, we make
> sure to narrow the corresponding GPRs if PSTATE indicates
> an AArch32 context.
> 
> The code tries to check whether the context is EL0 or EL1 so
> that it narrows the correct registers. But it does so by checking
> the full PSTATE instead of PSTATE.M.
> 
> As a consequence, and if we are restoring an AArch32 EL0 context
> in a 64bit guest, and that PSTATE has *any* bit set outside of
> PSTATE.M, we narrow *all* registers instead of only the first 15,
> destroying the 64bit state.
> 
> Obviously, this is not something the guest is likely to enjoy.
> 
> Correctly masking PSTATE to only evaluate PSTATE.M fixes it.
> 
> Fixes: 90c1f934ed71 ("KVM: arm64: Get rid of the AArch32 register mapping code")
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/guest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index e2f762d959bb..d9617b11f7a8 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -276,7 +276,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if (*vcpu_cpsr(vcpu) & PSR_MODE32_BIT) {
>  		int i, nr_reg;
>  
> -		switch (*vcpu_cpsr(vcpu)) {
> +		switch (*vcpu_cpsr(vcpu) & PSR_AA32_MODE_MASK) {
>  		/*
>  		 * Either we are dealing with user mode, and only the
>  		 * first 15 registers (+ PC) must be narrowed to 32bit.

Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>

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

Re: [PATCH 1/3] KVM: arm64: Fix AArch32 register narrowing on userspace write

[Oliver Upton]

On Fri, May 24, 2024 at 03:19:54PM +0100, Marc Zyngier wrote:
> When userspace writes to once of the core registers, we make

s/once/one/

> sure to narrow the corresponding GPRs if PSTATE indicates
> an AArch32 context.
> 
> The code tries to check whether the context is EL0 or EL1 so
> that it narrows the correct registers. But it does so by checking
> the full PSTATE instead of PSTATE.M.
> 
> As a consequence, and if we are restoring an AArch32 EL0 context
> in a 64bit guest, and that PSTATE has *any* bit set outside of
> PSTATE.M, we narrow *all* registers instead of only the first 15,
> destroying the 64bit state.
> 
> Obviously, this is not something the guest is likely to enjoy.
> 
> Correctly masking PSTATE to only evaluate PSTATE.M fixes it.
> 
> Fixes: 90c1f934ed71 ("KVM: arm64: Get rid of the AArch32 register mapping code")
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/guest.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index e2f762d959bb..d9617b11f7a8 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -276,7 +276,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  	if (*vcpu_cpsr(vcpu) & PSR_MODE32_BIT) {
>  		int i, nr_reg;
>  
> -		switch (*vcpu_cpsr(vcpu)) {
> +		switch (*vcpu_cpsr(vcpu) & PSR_AA32_MODE_MASK) {
>  		/*
>  		 * Either we are dealing with user mode, and only the
>  		 * first 15 registers (+ PC) must be narrowed to 32bit.
> -- 
> 2.39.2
> 

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

[PATCH 2/3] KVM: arm64: Allow AArch32 PSTATE.M to be restored as System mode

[Marc Zyngier]

It appears that we don't allowed a vcpu to be restored in AArch32
System mode, as we *never* included it in the list of valid modes.

Just add it to the list of allowed modes.

Fixes: 0d854a60b1d7 ("arm64: KVM: enable initialization of a 32bit vcpu")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/guest.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index d9617b11f7a8..11098eb7eb44 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -251,6 +251,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
 		case PSR_AA32_MODE_SVC:
 		case PSR_AA32_MODE_ABT:
 		case PSR_AA32_MODE_UND:
+		case PSR_AA32_MODE_SYS:
 			if (!vcpu_el1_is_32bit(vcpu))
 				return -EINVAL;
 			break;
-- 
2.39.2


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

Re: [PATCH 2/3] KVM: arm64: Allow AArch32 PSTATE.M to be restored as System mode

[Oliver Upton]

On Fri, May 24, 2024 at 03:19:55PM +0100, Marc Zyngier wrote:
> It appears that we don't allowed a vcpu to be restored in AArch32

s/allowed/allow/

> System mode, as we *never* included it in the list of valid modes.
> 
> Just add it to the list of allowed modes.
> 
> Fixes: 0d854a60b1d7 ("arm64: KVM: enable initialization of a 32bit vcpu")
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/guest.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
> index d9617b11f7a8..11098eb7eb44 100644
> --- a/arch/arm64/kvm/guest.c
> +++ b/arch/arm64/kvm/guest.c
> @@ -251,6 +251,7 @@ static int set_core_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  		case PSR_AA32_MODE_SVC:
>  		case PSR_AA32_MODE_ABT:
>  		case PSR_AA32_MODE_UND:
> +		case PSR_AA32_MODE_SYS:
>  			if (!vcpu_el1_is_32bit(vcpu))
>  				return -EINVAL;
>  			break;
> -- 
> 2.39.2
> 

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

[PATCH 3/3] KVM: arm64: AArch32: Fix spurious trapping of conditional instructions

[Marc Zyngier]

We recently upgraded the view of ESR_EL2 to 64bit, in keeping with
the requirements of the architecture.

However, the AArch32 emulation code was left unaudited, and the
(already dodgy) code that triages whether a trap is spurious or not
(because the condition code failed) broke in a subtle way:

If ESR_EL2.ISS2 is ever non-zero (unlikely, but hey, this is the ARM
architecture we're talking about), the hack that tests the top bits
of ESR_EL2.EC will break in an interesting way.

Instead, use kvm_vcpu_trap_get_class() to obtain the EC, and list
all the possible ECs that can fail a condition code check.

While we're at it, add SMC32 to the list, as it is explicitly listed
as being allowed to trap despite failing a condition code check (as
described in the HCR_EL2.TSC documentation).

Fixes: 0b12620fddb8 ("KVM: arm64: Treat ESR_EL2 as a 64-bit register")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/hyp/aarch32.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/aarch32.c b/arch/arm64/kvm/hyp/aarch32.c
index 8d9670e6615d..449fa58cf3b6 100644
--- a/arch/arm64/kvm/hyp/aarch32.c
+++ b/arch/arm64/kvm/hyp/aarch32.c
@@ -50,9 +50,23 @@ bool kvm_condition_valid32(const struct kvm_vcpu *vcpu)
 	u32 cpsr_cond;
 	int cond;
 
-	/* Top two bits non-zero?  Unconditional. */
-	if (kvm_vcpu_get_esr(vcpu) >> 30)
+	/*
+	 * These are the exception classes that could fire with a
+	 * conditional instruction.
+	 */
+	switch (kvm_vcpu_trap_get_class(vcpu)) {
+	case ESR_ELx_EC_CP15_32:
+	case ESR_ELx_EC_CP15_64:
+	case ESR_ELx_EC_CP14_MR:
+	case ESR_ELx_EC_CP14_LS:
+	case ESR_ELx_EC_FP_ASIMD:
+	case ESR_ELx_EC_CP10_ID:
+	case ESR_ELx_EC_CP14_64:
+	case ESR_ELx_EC_SVC32:
+		break;
+	default:
 		return true;
+	}
 
 	/* Is condition field valid? */
 	cond = kvm_vcpu_get_condition(vcpu);
-- 
2.39.2


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

Re: [PATCH 0/3] KVM/arm64 fixes for AArch32 handling

[Oliver Upton]

On Fri, May 24, 2024 at 03:19:53PM +0100, Marc Zyngier wrote:
> The (very much unloved) AArch32 handling has recently been found
> lacking in a number of ways:
> 
> - Nina spotted a brown paper-bag quality bug in the register narrowing
>   code when writing one of the core registers (GPRs, PSTATE) from
>   userspace

Yuck!

> - We never allowed System mode to be restored. Nobody ever complained,
>   but this is wrong nonetheless
> 
> - The handling of traps failing their condition check went from dodgy
>   to outright broken when the handling of ESR_EL2 was upgraded from 32
>   to 64 bit (patch already posted).
> 
> All these are stable material, and I plan to merge them after -rc1
> is released.

Please do!

Acked-by: Oliver Upton <oliver.upton@linux.dev>

-- 
Thanks,
Oliver

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

Re: [PATCH 0/3] KVM/arm64 fixes for AArch32 handling

[Marc Zyngier]

On Fri, 24 May 2024 15:19:53 +0100, Marc Zyngier wrote:
> The (very much unloved) AArch32 handling has recently been found
> lacking in a number of ways:
> 
> - Nina spotted a brown paper-bag quality bug in the register narrowing
>   code when writing one of the core registers (GPRs, PSTATE) from
>   userspace
> 
> [...]

Applied to fixes, thanks!

[1/3] KVM: arm64: Fix AArch32 register narrowing on userspace write
      commit: 947051e361d551e0590777080ffc4926190f62f2
[2/3] KVM: arm64: Allow AArch32 PSTATE.M to be restored as System mode
      commit: dfe6d190f38fc5df5ff2614b463a5195a399c885
[3/3] KVM: arm64: AArch32: Fix spurious trapping of conditional instructions
      commit: c92e8b9eacebb4060634ebd9395bba1b29aadc68

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.



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