Re: [bug report] KVM: arm64: nv: Handle ERETA[AB] instructions

[Marc Zyngier <maz@kernel.org>]

Hi Dan,

On Wed, 24 Apr 2024 07:39:53 +0100,
Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> Hello Marc Zyngier,
> 
> Commit 213b3d1ea161 ("KVM: arm64: nv: Handle ERETA[AB] instructions")
> from Apr 19, 2024 (linux-next), leads to the following Smatch static
> checker warning:
> 
> 	arch/arm64/kvm/emulate-nested.c:2209 kvm_emulate_nested_eret()
> 	error: uninitialized symbol 'elr'.
> 
> arch/arm64/kvm/emulate-nested.c
>     2173 void kvm_emulate_nested_eret(struct kvm_vcpu *vcpu)
>     2174 {
>     2175         u64 spsr, elr, esr;
>     2176 
>     2177         /*
>     2178          * Forward this trap to the virtual EL2 if the virtual
>     2179          * HCR_EL2.NV bit is set and this is coming from !EL2.
>     2180          */
>     2181         if (forward_traps(vcpu, HCR_NV))
>     2182                 return;
>     2183 
>     2184         /* Check for an ERETAx */
>     2185         esr = kvm_vcpu_get_esr(vcpu);
>     2186         if (esr_iss_is_eretax(esr) && !kvm_auth_eretax(vcpu, &elr)) {
>     2187                 /*
>     2188                  * Oh no, ERETAx failed to authenticate.  If we have
>     2189                  * FPACCOMBINE, deliver an exception right away.  If we
>     2190                  * don't, then let the mangled ELR value trickle down the
>     2191                  * ERET handling, and the guest will have a little surprise.
>     2192                  */
>     2193                 if (kvm_has_pauth(vcpu->kvm, FPACCOMBINE)) {
>     2194                         esr &= ESR_ELx_ERET_ISS_ERETA;
>     2195                         esr |= FIELD_PREP(ESR_ELx_EC_MASK, ESR_ELx_EC_FPAC);
>     2196                         kvm_inject_nested_sync(vcpu, esr);
>     2197                         return;
>     2198                 }
>     2199         }
>     2200 
>     2201         preempt_disable();
>     2202         kvm_arch_vcpu_put(vcpu);
>     2203 
>     2204         spsr = __vcpu_sys_reg(vcpu, SPSR_EL2);
>     2205         spsr = kvm_check_illegal_exception_return(vcpu, spsr);
>     2206         if (!esr_iss_is_eretax(esr))
>     2207                 elr = __vcpu_sys_reg(vcpu, ELR_EL2);
> 
> elr is unitialized on else path

Not quite.

Look at line 2186, and realise that kvm_auth_eretax() does write to
elr by being passed a pointer to it.

I guess Smatch isn't capable of seeing through something being
assigned in another compilation unit.

The code *could* be written as:

	if (!esr_iss_is_eretax(esr)) {
		[...]
		elr = vcpu_sys_reg(vcpu, ELR_EL2);
		[...]
	} else if (!kvm_auth_eretax(vcpu, &elr))
		[...]
	}

but that would pointlessly duplicate some of the logic, and I'm not
sure Smatch would grok that either.

	M.

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