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@xxxxxxxx>
Reviewed-by: Cédric Le Goater <clg@xxxxxxxx>
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;
>
>
