{"id":806365,"url":"http://patchwork.ozlabs.org/api/1.0/patches/806365/?format=json","project":{"id":2,"url":"http://patchwork.ozlabs.org/api/1.0/projects/2/?format=json","name":"Linux PPC development","link_name":"linuxppc-dev","list_id":"linuxppc-dev.lists.ozlabs.org","list_email":"linuxppc-dev@lists.ozlabs.org","web_url":"https://github.com/linuxppc/wiki/wiki","scm_url":"https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git","webscm_url":"https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/"},"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":{"id":13,"url":"http://patchwork.ozlabs.org/api/1.0/users/13/?format=json","username":"paulus","first_name":"Paul","last_name":"Mackerras","email":"paulus@samba.org"},"mbox":"http://patchwork.ozlabs.org/project/linuxppc-dev/patch/20170828044228.GB12629@fergus.ozlabs.ibm.com/mbox/","series":[{"id":71,"url":"http://patchwork.ozlabs.org/api/1.0/series/71/?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/71/mbox/"}],"check":"pending","checks":"http://patchwork.ozlabs.org/api/patches/806365/checks/","tags":{},"headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xgfLC6JWMz9s9Y\n\tfor ;\n\tMon, 28 Aug 2017 14:44:03 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xgfLC546BzDqYR\n\tfor ;\n\tMon, 28 Aug 2017 14:44:03 +1000 (AEST)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xgfJR6x6MzDq8f\n\tfor ;\n\tMon, 28 Aug 2017 14:42:31 +1000 (AEST)","by ozlabs.org (Postfix, from userid 1003)\n\tid 3xgfJR5fNNz9s9Y; Mon, 28 Aug 2017 14:42:31 +1000 (AEST)"],"Authentication-Results":["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","lists.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","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tsecure) header.d=ozlabs.org header.i=@ozlabs.org header.b=\"KN0uFsws\"; \n\tdkim-atps=neutral"],"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","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)","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"David Gibson ,\n\tnixiaoming , Al Viro ,\n\tDavid Hildenbrand ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"},"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":[]}