[PATCH] kvm/svm: fixed a potential register value inconsistent with variable ldr_reg

[PATCH] kvm/svm: fixed a potential register value inconsistent with variable ldr_reg

[彭浩(Richard)]

If the ldr value is read out to zero, it does not call avic_ldr_write to update
the virtual register, but the variable ldr_reg is updated.

Fixes: 98d90582be2e ("SVM: Fix AVIC DFR and LDR handling")
Signed-off-by: Peng Hao <richard.peng@oppo.com>
---
 arch/x86/kvm/svm/avic.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 8c550999ace0..318735e0f2d0 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -417,7 +417,6 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)

 static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
 {
-       int ret = 0;
        struct vcpu_svm *svm = to_svm(vcpu);
        u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
        u32 id = kvm_xapic_id(vcpu->arch.apic);
@@ -427,13 +426,16 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)

        avic_invalidate_logical_id_entry(vcpu);

-       if (ldr)
+       if (ldr) {
+               int ret;
                ret = avic_ldr_write(vcpu, id, ldr);

-       if (!ret)
-               svm->ldr_reg = ldr;
-
-       return ret;
+               if (!ret)
+                       svm->ldr_reg = ldr;
+               else
+                       return ret;
+       }
+       return 0;
}

 static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
--
2.18.4

Re: [PATCH] kvm/svm: fixed a potential register value inconsistent with variable ldr_reg

[Sean Christopherson]

On Wed, Nov 25, 2020, 彭浩(Richard) wrote:
> If the ldr value is read out to zero, it does not call avic_ldr_write to update
> the virtual register, but the variable ldr_reg is updated.

Is there a failure associated with this?  And/or can you elaborate on why
skipping the svm->ldr_reg is correct?

I'm not familiar with the AVIC spec, and it's not at all clear to me what the
correct behavior should be for the LDR updates.  E.g. skipping the svm->ldr_reg
update appears to break avic_handle_apic_id_update(), which will see a stale
svm->ldr_reg and call avic_invalidate_logical_id_entry() when it presumably
should not.

> Fixes: 98d90582be2e ("SVM: Fix AVIC DFR and LDR handling")
> Signed-off-by: Peng Hao <richard.peng@oppo.com>
> ---
>  arch/x86/kvm/svm/avic.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 8c550999ace0..318735e0f2d0 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -417,7 +417,6 @@ static void avic_invalidate_logical_id_entry(struct kvm_vcpu *vcpu)
> 
>  static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
>  {
> -       int ret = 0;
>         struct vcpu_svm *svm = to_svm(vcpu);
>         u32 ldr = kvm_lapic_get_reg(vcpu->arch.apic, APIC_LDR);
>         u32 id = kvm_xapic_id(vcpu->arch.apic);
> @@ -427,13 +426,16 @@ static int avic_handle_ldr_update(struct kvm_vcpu *vcpu)
> 
>         avic_invalidate_logical_id_entry(vcpu);
> 
> -       if (ldr)
> +       if (ldr) {
> +               int ret;
>                 ret = avic_ldr_write(vcpu, id, ldr);
> 
> -       if (!ret)
> -               svm->ldr_reg = ldr;
> -
> -       return ret;
> +               if (!ret)
> +                       svm->ldr_reg = ldr;
> +               else
> +                       return ret;
> +       }
> +       return 0;
> }
> 
>  static int avic_handle_apic_id_update(struct kvm_vcpu *vcpu)
> --
> 2.18.4