On Wed, Oct 09, 2024 at 07:36:03PM +0100, Marc Zyngier wrote:
> As there is very little ordering in the KVM API, userspace can
> instanciate a half-baked GIC (missing its memory map, for example)
> at almost any time.
>
> This means that, with the right timing, a thread running vcpu-0
> can enter the kernel without a GIC configured and get a GIC created
> behind its back by another thread. Amusingly, it will pick up
> that GIC and start messing with the data structures without the
> GIC having been fully initialised.
Huh, I'm definitely missing something. Could you remind me where we open
up this race between KVM_RUN && kvm_vgic_create()?
I'd thought the fact that the latter takes all the vCPU mutexes and
checks if any vCPU in the VM has run would be enough to guard against
such a race, but clearly not...
> Similarly, a thread running vcpu-1 can enter the kernel, and try
> to init the GIC that was previously created. Since this GIC isn't
> properly configured (no memory map), it fails to correctly initialise.
>
> And that's the point where we decide to teardown the GIC, freeing all
> its resources. Behind vcpu-0's back. Things stop pretty abruptly,
> with a variety of symptoms. Clearly, this isn't good, we should be
> a bit more careful about this.
>
> It is obvious that this guest is not viable, as it is missing some
> important part of its configuration. So instead of trying to tear
> bits of it down, let's just mark it as *dead*. It means that any
> further interaction from userspace will result in -EIO. The memory
> will be released on the "normal" path, when userspace gives up.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Alexander Potapenko <glider@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
Anyway, regarless of *how* we got here, it is pretty clear that tearing
things down on the error path is a bad idea. So:
Reviewed-by: Oliver Upton <oliver.upton@xxxxxxxxx>
> ---
> arch/arm64/kvm/arm.c | 3 +++
> arch/arm64/kvm/vgic/vgic-init.c | 6 +++---
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index a0d01c46e4084..b97ada19f06a7 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -997,6 +997,9 @@ static int kvm_vcpu_suspend(struct kvm_vcpu *vcpu)
> static int check_vcpu_requests(struct kvm_vcpu *vcpu)
> {
> if (kvm_request_pending(vcpu)) {
> + if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu))
> + return -EIO;
> +
> if (kvm_check_request(KVM_REQ_SLEEP, vcpu))
> kvm_vcpu_sleep(vcpu);
>
> diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c
> index e7c53e8af3d16..c4cbf798e71a4 100644
> --- a/arch/arm64/kvm/vgic/vgic-init.c
> +++ b/arch/arm64/kvm/vgic/vgic-init.c
> @@ -536,10 +536,10 @@ int kvm_vgic_map_resources(struct kvm *kvm)
> out:
> mutex_unlock(&kvm->arch.config_lock);
> out_slots:
> - mutex_unlock(&kvm->slots_lock);
> -
> if (ret)
> - kvm_vgic_destroy(kvm);
> + kvm_vm_dead(kvm);
> +
> + mutex_unlock(&kvm->slots_lock);
>
> return ret;
> }
> --
> 2.39.2
>
--
Thanks,
Oliver
