Received: by 2002:a25:7ec1:0:0:0:0:0 with SMTP id z184csp4527558ybc; Tue, 26 Nov 2019 10:16:42 -0800 (PST) X-Google-Smtp-Source: APXvYqwYPYRM8KjY5w65BYxNQ8Nl+IFmj1WSBz+OldThPGwq4Y2c4TG2FeadXdtBjLgsQUdvoIJ1 X-Received: by 2002:a17:906:5fd9:: with SMTP id k25mr43578513ejv.142.1574792202708; Tue, 26 Nov 2019 10:16:42 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1574792202; cv=none; d=google.com; s=arc-20160816; b=byBm3J5/UcxsfOdKIcD5sFq3fgxtxp/NO5GABYpCfn7liSaHyWNC17O69bnVYnqOqq iEDxsmuprsCcncAqBeSSC9zYqRS5u2b+89CP68XxS0r1LSOqnPxUYgQoIWp0vAl94PoJ 66Pnp516fkv2G6TJiq1ZrPEixxJg2QQy5H+YCtRgv5VZ/aWKx3yqlBeeNwCNZxR9WKsr TI8eS+03jbLq9kaVHzR1f+U9usppu7EOHs0JOxXCNzuPLdrAcr61mP28Q4LVmd3QEOT7 p3BfBCK9yH3Izke+uvsYYaC2p2qcHQb1hMXVvP6RFfoe6Ha8EziV9e9OCmUXsTcW4F1G Ou2w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=Omr8N4IWQtPOd2Fg8caij/HFntbpuJG/PLvm2MiGjf0=; b=QrGx3UYipgMRBrbdrkUdvn9JAbhhfYTH177yXhyBboE/+GNZ5gyCvjz6Hjl2b/ZzZa amCJRn7jHP7ain3s6cadKfrUOE7fapidDWz5SlVPMSHTFncrXSTqqimEVl0whh5VGFpF fyoeiF6DPGTN3swFfsa8qweuqjtkLdyBLEhMf5vz5D/Rv2+ACWf5WfVxuF5huwfbPVNj TGg6/QHd09q4svPHrJyvLsYcOXkfVnCPTpf/EaaOPEMe9LkBxEZhUy/8buZdxY0pTqJM FZs9TSKlCqeRoiqLi4AAGvGkMIanZzVJm7M8miPSpt2WcfCQ/6UDMLJjXHpLq9Y7e5eR Rtsw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id c12si8712190eda.402.2019.11.26.10.16.17; Tue, 26 Nov 2019 10:16:42 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727166AbfKZSOH (ORCPT + 99 others); Tue, 26 Nov 2019 13:14:07 -0500 Received: from mga01.intel.com ([192.55.52.88]:35839 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725870AbfKZSOH (ORCPT ); Tue, 26 Nov 2019 13:14:07 -0500 X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Nov 2019 10:14:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,246,1571727600"; d="scan'208";a="211493575" Received: from sjchrist-coffee.jf.intel.com (HELO linux.intel.com) ([10.54.74.41]) by orsmga006.jf.intel.com with ESMTP; 26 Nov 2019 10:14:06 -0800 Date: Tue, 26 Nov 2019 10:14:06 -0800 From: Sean Christopherson To: Leonardo Bras Cc: kvm-ppc@vger.kernel.org, linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Paul Mackerras , Benjamin Herrenschmidt , Michael Ellerman , Paolo Bonzini , Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 1/1] powerpc/kvm/book3s: Fixes possible 'use after release' of kvm Message-ID: <20191126181406.GC22233@linux.intel.com> References: <20191126175212.377171-1-leonardo@linux.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20191126175212.377171-1-leonardo@linux.ibm.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 26, 2019 at 02:52:12PM -0300, Leonardo Bras wrote: > Fixes a possible 'use after free' of kvm variable. > It does use mutex_unlock(&kvm->lock) after possible freeing a variable > with kvm_put_kvm(kvm). Moving the calls to kvm_put_kvm() to the end of the functions doesn't actually fix a use-after-free. In these flows, the reference being released is a borrowed reference that KVM takes on behalf of the entity it is creating, e.g. device, vcpu, or spapr tce. The caller of these create helpers must also hold its own reference to @kvm on top of the borrowed reference, i.e. these kvm_put_kvm() calls will never free @kvm (assuming there are no refcounting bugs elsewhere in KVM). If one these kvm_put_kvm() calls did unexpectedly free @kvm (due to a bug somewhere else), KVM would still hit a use-after-free scenario as the caller still thinks @kvm is valid. Currently, this would only happen on a subsequent ioctl() on the caller's file descriptor (which holds a pointer to @kvm), as the callers of these functions don't directly dereference @kvm after the functions return. But, not deferencing @kvm isn't deliberate or functionally required, it's just how the code happens to be written. The intent of adding kvm_put_kvm_no_destroy() was primarily to document that under no circumstance should the to-be-put reference be the *last* reference to @kvm. Moving the call to kvm_put_kvm{_no_destroy}() doesn't change that > Signed-off-by: Leonardo Bras > --- > arch/powerpc/kvm/book3s_64_vio.c | 3 +-- > virt/kvm/kvm_main.c | 8 ++++---- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c > index 5834db0a54c6..a402ead833b6 100644 > --- a/arch/powerpc/kvm/book3s_64_vio.c > +++ b/arch/powerpc/kvm/book3s_64_vio.c > @@ -316,14 +316,13 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm, > > if (ret >= 0) > list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables); > - else > - kvm_put_kvm(kvm); > > mutex_unlock(&kvm->lock); > > if (ret >= 0) > return ret; > > + kvm_put_kvm(kvm); > kfree(stt); > fail_acct: > account_locked_vm(current->mm, kvmppc_stt_pages(npages), false); > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 13efc291b1c7..f37089b60d09 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -2744,10 +2744,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > /* Now it's all set up, let userspace reach it */ > kvm_get_kvm(kvm); > r = create_vcpu_fd(vcpu); > - if (r < 0) { > - kvm_put_kvm(kvm); > + if (r < 0) > goto unlock_vcpu_destroy; > - } > > kvm->vcpus[atomic_read(&kvm->online_vcpus)] = vcpu; > > @@ -2771,6 +2769,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > mutex_lock(&kvm->lock); > kvm->created_vcpus--; > mutex_unlock(&kvm->lock); > + if (r < 0) > + kvm_put_kvm(kvm); > return r; > } > > @@ -3183,10 +3183,10 @@ static int kvm_ioctl_create_device(struct kvm *kvm, > kvm_get_kvm(kvm); > ret = anon_inode_getfd(ops->name, &kvm_device_fops, dev, O_RDWR | O_CLOEXEC); > if (ret < 0) { > - kvm_put_kvm(kvm); > mutex_lock(&kvm->lock); > list_del(&dev->vm_node); > mutex_unlock(&kvm->lock); > + kvm_put_kvm(kvm); > ops->destroy(dev); > return ret; > } > -- > 2.23.0 >