{"id":806364,"url":"http://patchwork.ozlabs.org/api/1.0/patches/806364/?format=json","project":{"id":23,"url":"http://patchwork.ozlabs.org/api/1.0/projects/23/?format=json","name":"KVM PowerPC development","link_name":"kvm-ppc","list_id":"kvm-ppc.vger.kernel.org","list_email":"kvm-ppc@vger.kernel.org","web_url":null,"scm_url":null,"webscm_url":null},"msgid":"<20170828044228.GB12629@fergus.ozlabs.ibm.com>","date":"2017-08-28T04:42:29","name":"KVM: PPC: Book3S HV: Protect updates to spapr_tce_tables list","commit_ref":null,"pull_url":null,"state":"accepted","archived":false,"hash":"5724e7c23e933f12e8c8489c2119479f3e305432","submitter":{"id":67079,"url":"http://patchwork.ozlabs.org/api/1.0/people/67079/?format=json","name":"Paul Mackerras","email":"paulus@ozlabs.org"},"delegate":null,"mbox":"http://patchwork.ozlabs.org/project/kvm-ppc/patch/20170828044228.GB12629@fergus.ozlabs.ibm.com/mbox/","series":[{"id":70,"url":"http://patchwork.ozlabs.org/api/1.0/series/70/?format=json","date":"2017-08-28T04:42:29","name":"KVM: PPC: Book3S HV: Protect updates to spapr_tce_tables list","version":1,"mbox":"http://patchwork.ozlabs.org/series/70/mbox/"}],"check":"pending","checks":"http://patchwork.ozlabs.org/api/patches/806364/checks/","tags":{},"headers":{"Return-Path":"","X-Original-To":"incoming@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=kvm-ppc-owner@vger.kernel.org;\n\treceiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tsecure) header.d=ozlabs.org header.i=@ozlabs.org header.b=\"KN0uFsws\";\n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xgfJW39wjz9sPk\n\tfor ;\n\tMon, 28 Aug 2017 14:42:35 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751186AbdH1Eme (ORCPT );\n\tMon, 28 Aug 2017 00:42:34 -0400","from ozlabs.org ([103.22.144.67]:48009 \"EHLO ozlabs.org\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751177AbdH1Emd (ORCPT );\n\tMon, 28 Aug 2017 00:42:33 -0400","by ozlabs.org (Postfix, from userid 1003)\n\tid 3xgfJR5fNNz9s9Y; Mon, 28 Aug 2017 14:42:31 +1000 (AEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ozlabs.org; s=201707; \n\tt=1503895351; bh=9xLWyDRG3Wc7dvWeZgiUXrBcfjL8HhlSwnm4XWw43i8=;\n\th=Date:From:To:Cc:Subject:From;\n\tb=KN0uFswsSiVGvmfRqi9cv3Vy3d+uud+W0N9eb1ig/ahdTu0ZltQO7c/8rVyV+9GQg\n\t9pPnkFkSjmYLxc206dZ1dJux5+Zz0woQh501W3rB7lsT/8suQHQ6xfbtXcsolrjryN\n\tJX3u1lDg+RinF5eRIQOalPnL3rbiKtX14F81u2bkRYlYyZG576NjigLuQoi27Jng+B\n\tEjrJutFf5aoChQL5wximirtYAEsN/oypWjSXI0SNepTra1GaXPgfGRF9eEuOgj6gCg\n\tc0KXXVe8HJup1bikRw2TJss+CBkJLs5Mw7cvSSygLJMB3SZpbcyMdbHqZcjbaQv0HN\n\tSKdnSWtjUkhBw==","Date":"Mon, 28 Aug 2017 14:42:29 +1000","From":"Paul Mackerras ","To":"kvm-ppc@vger.kernel.org, kvm@vger.kernel.org,\n\tlinuxppc-dev@lists.ozlabs.org","Cc":"nixiaoming , David Hildenbrand ,\n\tDavid Gibson ,\n\tAl Viro ","Subject":"[PATCH] KVM: PPC: Book3S HV: Protect updates to spapr_tce_tables\n\tlist","Message-ID":"<20170828044228.GB12629@fergus.ozlabs.ibm.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","User-Agent":"Mutt/1.5.24 (2015-08-30)","Sender":"kvm-ppc-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"kvm-ppc@vger.kernel.org"},"content":"Al Viro pointed out that while one thread of a process is executing\nin kvm_vm_ioctl_create_spapr_tce(), another thread could guess the\nfile descriptor returned by anon_inode_getfd() and close() it before\nthe first thread has added it to the kvm->arch.spapr_tce_tables list.\nThat highlights a more general problem: there is no mutual exclusion\nbetween writers to the spapr_tce_tables list, leading to the\npossibility of the list becoming corrupted, which could cause a\nhost kernel crash.\n\nTo fix the mutual exclusion problem, we add a mutex_lock/unlock\npair around the list_del_rce in kvm_spapr_tce_release(). Also,\nthis moves the call to anon_inode_getfd() inside the region\nprotected by the kvm->lock mutex, after we have done the check for\na duplicate LIOBN. This means that if another thread does guess the\nfile descriptor and closes it, its call to kvm_spapr_tce_release()\nwill not do any harm because it will have to wait until the first\nthread has released kvm->lock.\n\nThe other things that the second thread could do with the guessed\nfile descriptor are to mmap it or to pass it as a parameter to a\nKVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM device fd. An mmap\ncall won't cause any harm because kvm_spapr_tce_mmap() and\nkvm_spapr_tce_fault() don't access the spapr_tce_tables list or\nthe kvmppc_spapr_tce_table.list field, and the fields that they do use\nhave been properly initialized by the time of the anon_inode_getfd()\ncall.\n\nThe KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl calls\nkvm_spapr_tce_attach_iommu_group(), which scans the spapr_tce_tables\nlist looking for the kvmppc_spapr_tce_table struct corresponding to\nthe fd given as the parameter. Either it will find the new entry\nor it won't; if it doesn't, it just returns an error, and if it\ndoes, it will function normally. So, in each case there is no\nharmful effect.\n\nSigned-off-by: Paul Mackerras \n---\n arch/powerpc/kvm/book3s_64_vio.c | 21 ++++++++++-----------\n 1 file changed, 10 insertions(+), 11 deletions(-)","diff":"diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c\nindex 53766e2..8f2da8b 100644\n--- a/arch/powerpc/kvm/book3s_64_vio.c\n+++ b/arch/powerpc/kvm/book3s_64_vio.c\n@@ -265,8 +265,11 @@ static int kvm_spapr_tce_release(struct inode *inode, struct file *filp)\n {\n \tstruct kvmppc_spapr_tce_table *stt = filp->private_data;\n \tstruct kvmppc_spapr_tce_iommu_table *stit, *tmp;\n+\tstruct kvm *kvm = stt->kvm;\n \n+\tmutex_lock(&kvm->lock);\n \tlist_del_rcu(&stt->list);\n+\tmutex_unlock(&kvm->lock);\n \n \tlist_for_each_entry_safe(stit, tmp, &stt->iommu_tables, next) {\n \t\tWARN_ON(!kref_read(&stit->kref));\n@@ -298,7 +301,6 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,\n \tunsigned long npages, size;\n \tint ret = -ENOMEM;\n \tint i;\n-\tint fd = -1;\n \n \tif (!args->size)\n \t\treturn -EINVAL;\n@@ -328,11 +330,6 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,\n \t\t\tgoto fail;\n \t}\n \n-\tret = fd = anon_inode_getfd(\"kvm-spapr-tce\", &kvm_spapr_tce_fops,\n-\t\t\t\t stt, O_RDWR | O_CLOEXEC);\n-\tif (ret < 0)\n-\t\tgoto fail;\n-\n \tmutex_lock(&kvm->lock);\n \n \t/* Check this LIOBN hasn't been previously allocated */\n@@ -344,17 +341,19 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,\n \t\t}\n \t}\n \n-\tif (!ret) {\n+\tif (!ret)\n+\t\tret = anon_inode_getfd(\"kvm-spapr-tce\", &kvm_spapr_tce_fops,\n+\t\t\t\t stt, O_RDWR | O_CLOEXEC);\n+\n+\tif (ret >= 0) {\n \t\tlist_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);\n \t\tkvm_get_kvm(kvm);\n \t}\n \n \tmutex_unlock(&kvm->lock);\n \n-\tif (!ret)\n-\t\treturn fd;\n-\n-\tput_unused_fd(fd);\n+\tif (ret >= 0)\n+\t\treturn ret;\n \n fail:\n \tfor (i = 0; i < npages; i++)\n","prefixes":[]}