[PATCH 3/6] KVM: PPC: Book3S HV: XIVE: Ensure VP isn't already in use

[Cédric Le Goater]

On 23/09/2019 17:43, Greg Kurz wrote:
> We currently prevent userspace to connect a new vCPU if we already have
> one with the same vCPU id. This is good but unfortunately not enough,
> because VP ids derive from the packed vCPU ids, and kvmppc_pack_vcpu_id()
> can return colliding values. For examples, 348 stays unchanged since it
> is < KVM_MAX_VCPUS, but it is also the packed value of 2392 when the
> guest's core stride is 8. Nothing currently prevents userspace to connect
> vCPUs with forged ids, that end up being associated to the same VP. This
> confuses the irq layer and likely crashes the kernel:
> 
> [96631.670454] genirq: Flags mismatch irq 4161. 00010000 (kvm-1-2392) vs. 00010000 (kvm-1-348)
> 
> Check the VP id instead of the vCPU id when a new vCPU is connected.
> The allocation of the XIVE CPU structure in kvmppc_xive_connect_vcpu()
> is moved after the check to avoid the need for rollback.
> 
> Signed-off-by: Greg Kurz <groug at kaod.org>


Reviewed-by: Cédric Le Goater <clg at kaod.org>

C.


> ---
>  arch/powerpc/kvm/book3s_xive.c        |   24 ++++++++++++++++--------
>  arch/powerpc/kvm/book3s_xive.h        |   12 ++++++++++++
>  arch/powerpc/kvm/book3s_xive_native.c |    6 ++++--
>  3 files changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_xive.c b/arch/powerpc/kvm/book3s_xive.c
> index 2ef43d037a4f..01bff7befc9f 100644
> --- a/arch/powerpc/kvm/book3s_xive.c
> +++ b/arch/powerpc/kvm/book3s_xive.c
> @@ -1217,6 +1217,7 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>  	struct kvmppc_xive *xive = dev->private;
>  	struct kvmppc_xive_vcpu *xc;
>  	int i, r = -EBUSY;
> +	u32 vp_id;
>  
>  	pr_devel("connect_vcpu(cpu=%d)\n", cpu);
>  
> @@ -1228,25 +1229,32 @@ int kvmppc_xive_connect_vcpu(struct kvm_device *dev,
>  		return -EPERM;
>  	if (vcpu->arch.irq_type != KVMPPC_IRQ_DEFAULT)
>  		return -EBUSY;
> -	if (kvmppc_xive_find_server(vcpu->kvm, cpu)) {
> -		pr_devel("Duplicate !\n");
> -		return -EEXIST;
> -	}
>  	if (cpu >= (KVM_MAX_VCPUS * vcpu->kvm->arch.emul_smt_mode)) {
>  		pr_devel("Out of bounds !\n");
>  		return -EINVAL;
>  	}
> -	xc = kzalloc(sizeof(*xc), GFP_KERNEL);
> -	if (!xc)
> -		return -ENOMEM;
>  
>  	/* We need to synchronize with queue provisioning */
>  	mutex_lock(&xive->lock);
> +
> +	vp_id = kvmppc_xive_vp(xive, cpu);
> +	if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) {
> +		pr_devel("Duplicate !\n");
> +		r = -EEXIST;
> +		goto bail;
> +	}
> +
> +	xc = kzalloc(sizeof(*xc), GFP_KERNEL);
> +	if (!xc) {
> +		r = -ENOMEM;
> +		goto bail;
> +	}
> +
>  	vcpu->arch.xive_vcpu = xc;
>  	xc->xive = xive;
>  	xc->vcpu = vcpu;
>  	xc->server_num = cpu;
> -	xc->vp_id = kvmppc_xive_vp(xive, cpu);
> +	xc->vp_id = vp_id;
>  	xc->mfrr = 0xff;
>  	xc->valid = true;
>  
> diff --git a/arch/powerpc/kvm/book3s_xive.h b/arch/powerpc/kvm/book3s_xive.h
> index 955b820ffd6d..fe3ed50e0818 100644
> --- a/arch/powerpc/kvm/book3s_xive.h
> +++ b/arch/powerpc/kvm/book3s_xive.h
> @@ -220,6 +220,18 @@ static inline u32 kvmppc_xive_vp(struct kvmppc_xive *xive, u32 server)
>  	return xive->vp_base + kvmppc_pack_vcpu_id(xive->kvm, server);
>  }
>  
> +static inline bool kvmppc_xive_vp_in_use(struct kvm *kvm, u32 vp_id)
> +{
> +	struct kvm_vcpu *vcpu = NULL;
> +	int i;
> +
> +	kvm_for_each_vcpu(i, vcpu, kvm) {
> +		if (vcpu->arch.xive_vcpu && vp_id == vcpu->arch.xive_vcpu->vp_id)
> +			return true;
> +	}
> +	return false;
> +}
> +
>  /*
>   * Mapping between guest priorities and host priorities
>   * is as follow.
> diff --git a/arch/powerpc/kvm/book3s_xive_native.c b/arch/powerpc/kvm/book3s_xive_native.c
> index 84a354b90f60..53a22771908c 100644
> --- a/arch/powerpc/kvm/book3s_xive_native.c
> +++ b/arch/powerpc/kvm/book3s_xive_native.c
> @@ -106,6 +106,7 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>  	struct kvmppc_xive *xive = dev->private;
>  	struct kvmppc_xive_vcpu *xc = NULL;
>  	int rc;
> +	u32 vp_id;
>  
>  	pr_devel("native_connect_vcpu(server=%d)\n", server_num);
>  
> @@ -124,7 +125,8 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>  
>  	mutex_lock(&xive->lock);
>  
> -	if (kvmppc_xive_find_server(vcpu->kvm, server_num)) {
> +	vp_id = kvmppc_xive_vp(xive, server_num);
> +	if (kvmppc_xive_vp_in_use(xive->kvm, vp_id)) {
>  		pr_devel("Duplicate !\n");
>  		rc = -EEXIST;
>  		goto bail;
> @@ -141,7 +143,7 @@ int kvmppc_xive_native_connect_vcpu(struct kvm_device *dev,
>  	xc->vcpu = vcpu;
>  	xc->server_num = server_num;
>  
> -	xc->vp_id = kvmppc_xive_vp(xive, server_num);
> +	xc->vp_id = vp_id;
>  	xc->valid = true;
>  	vcpu->arch.irq_type = KVMPPC_IRQ_XIVE;
>  
>