Re: [PATCH v20 06/14] KVM: arm64: Validate GCS exception lock when emulating ERET

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]

On Thu, Sep 03, 2026 at 04:37:37PM +0100, Leonardo Bras wrote:
> On Tue, Sep 01, 2026 at 10:47:04PM +0100, Mark Brown wrote:

> > +	return vcpu_read_sys_reg(vcpu, GCSCR_EL2) & GCSCR_ELx_EXLOCKEN;
> > +}

> The above perfectly translates the GCS part of IllegalExceptionReturn().

> It's a nit, as I suppose there should be no compiler warning on that, but
> the function should return a bool, and the last return line returns an u64.

> Maybe adding a "return !!()" would be better?

There's no need to manually do translations like that in C, the
conversion of 0 to false and any non-zero value to true when an
integer is used in a boolean context is in the standard.

> >  	 * - trying to return to EL1 with HCR_EL2.TGE set
> > +	 * - GCSCR_ELx.EXLOCKEN is 1 and PSTATE.EXLOCK is 0 when attempting
> > +	 *   to return from ELx the same EL.
> >  	 */
> >  	if (mode == PSR_MODE_EL3t   || mode == PSR_MODE_EL3h ||
> >  	    mode == 0b00001         || (mode & BIT(1))       ||
> >  	    (spsr & PSR_MODE32_BIT) ||
> > +	    kvm_check_illegal_exlock_return(vcpu, spsr) ||
> >  	    (vcpu_el2_tge_is_set(vcpu) && (mode == PSR_MODE_EL1t ||
> >  					   mode == PSR_MODE_EL1h))) {
> >  		u64 mask;
> 
> In IllegalExceptionReturn(), the GCS-related clause happens at the end, and 
> here it happens before the TGE one. Could this cause any weird behavior in 
> the future?
> 
> I get that by doing like this you don't change the last line of the "if", 
> but I wonder if that could change anything.

Given that we take the same action regardless of which or clause
triggers I can't see how it would matter.

> > --- a/arch/arm64/kvm/hyp/vhe/switch.c
> > +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> > @@ -383,6 +383,10 @@ static bool kvm_hyp_handle_eret(struct kvm_vcpu *vcpu, u64 *exit_code)
> >  		return false;
> >  	}

> > +	/* Push GCS exception lock failures into the slow path */
> > +	if (kvm_check_illegal_exlock_return(vcpu, spsr))
> > +		return false;

> >  	/* If ERETAx fails, take the slow path */
> >  	if (esr_iss_is_eretax(esr)) {
> >  		if (!(vcpu_has_ptrauth(vcpu) && kvm_auth_eretax(vcpu, &elr)))

> IIUC, this function will be called on the __kvm_vcpu_run_vhe() inner loop, 
> in the cases where the guest exited due to a eret. 

> What you change here is that in case of an illegal exlock return, it goes 
> out of the loop and return to host kernel, probably to deal with it in the 
> mentioned slowpath, the same way the ERETAx entry does.

> I don't question on this being needed. 
> I would just like to understand why this is needed here. 

> This does not seem to be related to nested, as this is called in 
> __fixup_guest_exit() and not in fixup_nv_guest_exit(). But would not 
> hardware be responsible for cheking this, then?

The code is here because it's part of the ERET handling, this should
only happen for NV as we're not trapping ERET instructions otherwise.
We need this because ERETs from vEL2 are handled in software, modulo the
NV3 fast path mentioned at the top of the function.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]