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

[Oliver Upton <oliver.upton@linux.dev>]

Hey,

On Wed, Apr 02, 2025 at 01:19:28PM +0100, Marc Zyngier wrote:
> On Tue, 01 Apr 2025 23:42:34 +0100,
> Oliver Upton <oliver.upton@linux.dev> wrote:
> >  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.

Agreed, this reads better.

> > 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?

I had this:

  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.

But I'm happy to expand a bit to make that more clear.

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

Good point, I'll respin in a moment.

Thanks for having a look!

Best,
Oliver