Re: [PATCH 3/3] KVM: arm64: Don't translate FAR if invalid/unsafe

[Marc Zyngier <maz@kernel.org>]

On Tue, 01 Apr 2025 23:42:34 +0100,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Don't re-walk the page tables if an SEA occurred during the faulting
> page table walk to avoid taking a fatal exception in the hyp.
> Additionally, check that FAR_EL2 is valid for SEAs not taken on PTW
> as the architecture doesn't guarantee it contains the fault VA.
> 
> Finally, fix up the rest of the abort path by checking for SEAs early
> and bugging the VM if we get further along with an UNKNOWN fault IPA.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/esr.h           | 22 ++++++++++++++++++
>  arch/arm64/include/asm/kvm_emulate.h   |  7 +++++-
>  arch/arm64/include/asm/kvm_ras.h       |  2 +-
>  arch/arm64/kvm/hyp/include/hyp/fault.h | 26 ++++++++++++++++-----
>  arch/arm64/kvm/mmu.c                   | 31 ++++++++++++++++----------
>  5 files changed, 69 insertions(+), 19 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 7b096ed87360..4b874ad95f2e 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -465,6 +465,28 @@ static inline bool esr_fsc_is_access_flag_fault(unsigned long esr)
>  	       (esr == ESR_ELx_FSC_ACCESS_L(0));
>  }
>  
> +static inline bool esr_fsc_is_sea_ttw(unsigned long esr)
> +{
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_SEA_TTW(3)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(2)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(1)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(0)) ||
> +	       (esr == ESR_ELx_FSC_SEA_TTW(-1));
> +}
> +
> +static inline bool esr_fsc_is_secc_ttw(unsigned long esr)
> +{
> +	esr = esr & ESR_ELx_FSC;
> +
> +	return (esr == ESR_ELx_FSC_SECC_TTW(3)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(2)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(1)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(0)) ||
> +	       (esr == ESR_ELx_FSC_SECC_TTW(-1));
> +}
> +
>  /* Indicate whether ESR.EC==0x1A is for an ERETAx instruction */
>  static inline bool esr_iss_is_eretax(unsigned long esr)
>  {
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 5d8c0832acbe..775e200a9be4 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -305,7 +305,12 @@ static __always_inline unsigned long kvm_vcpu_get_hfar(const struct kvm_vcpu *vc
>  
>  static __always_inline phys_addr_t kvm_vcpu_get_fault_ipa(const struct kvm_vcpu *vcpu)
>  {
> -	return ((phys_addr_t)vcpu->arch.fault.hpfar_el2 & HPFAR_EL2_FIPA) << 8;
> +	u64 hpfar = vcpu->arch.fault.hpfar_el2;
> +
> +	if (unlikely(!(hpfar & HPFAR_EL2_NS)))
> +		return INVALID_GPA;

Eh, why not ;-)

> +
> +	return ((phys_addr_t)hpfar & HPFAR_EL2_FIPA) << 8;

nit: as you are reworking this area, I'd rather see

	return FIELD_GET(HPFAR_EL2_FIPA, hpfar) << 12;

as it nicely shows what the FIPA field represents, instead of the
odd-looking 8 here. The compiler will nicely sort it out anyway.

Same thing in handle_host_mem_abort(), both of which could be fixed in
the previous patch.

>  }
>  
>  static inline u64 kvm_vcpu_get_disr(const struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/include/asm/kvm_ras.h b/arch/arm64/include/asm/kvm_ras.h
> index 87e10d9a635b..9398ade632aa 100644
> --- a/arch/arm64/include/asm/kvm_ras.h
> +++ b/arch/arm64/include/asm/kvm_ras.h
> @@ -14,7 +14,7 @@
>   * Was this synchronous external abort a RAS notification?
>   * Returns '0' for errors handled by some RAS subsystem, or -ENOENT.
>   */
> -static inline int kvm_handle_guest_sea(phys_addr_t addr, u64 esr)
> +static inline int kvm_handle_guest_sea(void)
>  {
>  	/* apei_claim_sea(NULL) expects to mask interrupts itself */
>  	lockdep_assert_irqs_enabled();
> diff --git a/arch/arm64/kvm/hyp/include/hyp/fault.h b/arch/arm64/kvm/hyp/include/hyp/fault.h
> index 84d165e17bd0..1ac9b23a382a 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/fault.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/fault.h
> @@ -12,6 +12,16 @@
>  #include <asm/kvm_hyp.h>
>  #include <asm/kvm_mmu.h>
>  
> +static inline bool __fault_safe_to_translate(u64 esr)
> +{
> +	u64 fsc = esr & ESR_ELx_FSC;
> +
> +	if (esr_fsc_is_sea_ttw(esr) || esr_fsc_is_secc_ttw(esr))
> +		return false;
> +
> +	return !(fsc == ESR_ELx_FSC_EXTABT && (esr & ESR_ELx_FnV));
> +}

Ah, neat.

> +
>  static inline bool __translate_far_to_hpfar(u64 far, u64 *hpfar)
>  {
>  	int ret;
> @@ -71,17 +81,23 @@ static inline bool __hpfar_valid(u64 esr)
>  
>  static inline bool __get_fault_info(u64 esr, struct kvm_vcpu_fault_info *fault)
>  {
> -	u64 hpfar, far;
> +	u64 hpfar;
>  
> -	far = read_sysreg_el2(SYS_FAR);
> +	fault->far_el2		= read_sysreg_el2(SYS_FAR);
> +	fault->hpfar_el2	= 0;
>  
>  	if (__hpfar_valid(esr))
>  		hpfar = read_sysreg(hpfar_el2);
> -	else if (!__translate_far_to_hpfar(far, &hpfar))
> +	else if (unlikely(!__fault_safe_to_translate(esr)))
> +		return true;

This may cause some unexpected damage, see below.

> +	else if (!__translate_far_to_hpfar(fault->far_el2, &hpfar))
>  		return false;
>  
> -	fault->far_el2 = far;
> -	fault->hpfar_el2 = hpfar;
> +	/*
> +	 * Hijack HPFAR_EL2.NS (RES0 in Non-secure) to indicate a valid
> +	 * HPFAR value.
> +	 */
> +	fault->hpfar_el2 = hpfar | HPFAR_EL2_NS;
>  	return true;
>  }
>  
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 2feb6c6b63af..754f2fe0cc67 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -1794,9 +1794,28 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu)
>  	gfn_t gfn;
>  	int ret, idx;
>  
> +	/* Synchronous External Abort? */
> +	if (kvm_vcpu_abt_issea(vcpu)) {
> +		/*
> +		 * For RAS the host kernel may handle this abort.
> +		 * There is no need to pass the error into the guest.
> +		 */
> +		if (kvm_handle_guest_sea())
> +			kvm_inject_vabt(vcpu);
> +
> +		return 1;
> +	}
> +

Maybe add a line to the commit message to indicate that you're moving
this around?

>  	esr = kvm_vcpu_get_esr(vcpu);
>  
> +	/*
> +	 * The fault IPA should be reliable at this point as we're not dealing
> +	 * with an SEA.
> +	 */
>  	ipa = fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +	if (KVM_BUG_ON(ipa == INVALID_GPA, vcpu->kvm))
> +		return -EFAULT;
> +

I think you're missing something similar in handle_host_mem_abort(),
which will take fault.hpfar_el2 at face value even when it is unsafe
to translate. At least testing for HPFAR_EL2_NS seems necessary.

Thanks,

	M.

-- 
Jazz isn't dead. It just smells funny.