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

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

[Dan Carpenter]

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

    2208 
--> 2209         trace_kvm_nested_eret(vcpu, elr, spsr);
    2210 
    2211         /*
    2212          * Note that the current exception level is always the virtual EL2,
    2213          * since we set HCR_EL2.NV bit only when entering the virtual EL2.
    2214          */
    2215         *vcpu_pc(vcpu) = elr;
                                  ^^^

    2216         *vcpu_cpsr(vcpu) = spsr;
    2217 
    2218         kvm_arch_vcpu_load(vcpu, smp_processor_id());
    2219         preempt_enable();
    2220 }

regards,
dan carpenter

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

[Marc Zyngier]

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.

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

[Dan Carpenter]

Crud.  I should have seen that.  That's my bad.  Certainly don't change
the code to silence the warning.

The issue for Smatch is that it doesn't parse bitwise tests or pure
functions properly...  I know *how* to fix it and it's not *complicated*
but it's quite a bit of code and I haven't gotten around to doing it
yet.

regards,
dan carpenter

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

[Marc Zyngier]

On Wed, 24 Apr 2024 13:47:11 +0100,
Dan Carpenter <dan.carpenter@linaro.org> wrote:
> 
> Crud.  I should have seen that.  That's my bad.  Certainly don't change
> the code to silence the warning.
> 
> The issue for Smatch is that it doesn't parse bitwise tests or pure
> functions properly...  I know *how* to fix it and it's not *complicated*
> but it's quite a bit of code and I haven't gotten around to doing it
> yet.

No worries. I prefer getting the odd report for a false positive and
look into it rather than turn a blind eye. At least you have a path
for the tool to be improved, which is what matters.

Thanks,

	M.

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