lkml.org 
[lkml]   [2017]   [Oct]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 4.9 08/64] KVM: PPC: Book3S: Fix race and leak in kvm_vm_ioctl_create_spapr_tce()
    Date
    4.9-stable review patch.  If anyone has any objections, please let me know.

    ------------------

    From: Paul Mackerras <paulus@ozlabs.org>

    commit 47c5310a8dbe7c2cb9f0083daa43ceed76c257fa upstream, with part
    of commit edd03602d97236e8fea13cd76886c576186aa307 folded in.

    Nixiaoming pointed out that there is a memory leak in
    kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd()
    fails; the memory allocated for the kvmppc_spapr_tce_table struct
    is not freed, and nor are the pages allocated for the iommu
    tables. In addition, we have already incremented the process's
    count of locked memory pages, and this doesn't get restored on
    error.

    David Hildenbrand pointed out that there is a race in that the
    function checks early on that there is not already an entry in the
    stt->iommu_tables list with the same LIOBN, but an entry with the
    same LIOBN could get added between then and when the new entry is
    added to the list.

    This fixes all three problems. To simplify things, we now call
    anon_inode_getfd() before placing the new entry in the list. The
    check for an existing entry is done while holding the kvm->lock
    mutex, immediately before adding the new entry to the list.
    Finally, on failure we now call kvmppc_account_memlimit to
    decrement the process's count of locked memory pages.

    [paulus@ozlabs.org - folded in that part of edd03602d972 ("KVM:
    PPC: Book3S HV: Protect updates to spapr_tce_tables list", 2017-08-28)
    which restructured the code that 47c5310a8dbe modified, to avoid
    a build failure caused by the absence of put_unused_fd().]

    Fixes: 54738c097163 ("KVM: PPC: Accelerate H_PUT_TCE by implementing it in real mode")
    Fixes: f8626985c7c2 ("KVM: PPC: Account TCE-containing pages in locked_vm")
    Reported-by: Nixiaoming <nixiaoming@huawei.com>
    Reported-by: David Hildenbrand <david@redhat.com>
    Signed-off-by: Paul Mackerras <paulus@ozlabs.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

    ---
    arch/powerpc/kvm/book3s_64_vio.c | 56 ++++++++++++++++++++++-----------------
    1 file changed, 32 insertions(+), 24 deletions(-)

    --- a/arch/powerpc/kvm/book3s_64_vio.c
    +++ b/arch/powerpc/kvm/book3s_64_vio.c
    @@ -150,6 +150,7 @@ long kvm_vm_ioctl_create_spapr_tce(struc
    struct kvm_create_spapr_tce_64 *args)
    {
    struct kvmppc_spapr_tce_table *stt = NULL;
    + struct kvmppc_spapr_tce_table *siter;
    unsigned long npages, size;
    int ret = -ENOMEM;
    int i;
    @@ -157,24 +158,16 @@ long kvm_vm_ioctl_create_spapr_tce(struc
    if (!args->size)
    return -EINVAL;

    - /* Check this LIOBN hasn't been previously allocated */
    - list_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {
    - if (stt->liobn == args->liobn)
    - return -EBUSY;
    - }
    -
    size = args->size;
    npages = kvmppc_tce_pages(size);
    ret = kvmppc_account_memlimit(kvmppc_stt_pages(npages), true);
    - if (ret) {
    - stt = NULL;
    - goto fail;
    - }
    + if (ret)
    + return ret;

    stt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),
    GFP_KERNEL);
    if (!stt)
    - goto fail;
    + goto fail_acct;

    stt->liobn = args->liobn;
    stt->page_shift = args->page_shift;
    @@ -188,24 +181,39 @@ long kvm_vm_ioctl_create_spapr_tce(struc
    goto fail;
    }

    - kvm_get_kvm(kvm);
    -
    mutex_lock(&kvm->lock);
    - list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);

    - mutex_unlock(&kvm->lock);
    + /* Check this LIOBN hasn't been previously allocated */
    + ret = 0;
    + list_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) {
    + if (siter->liobn == args->liobn) {
    + ret = -EBUSY;
    + break;
    + }
    + }
    +
    + if (!ret)
    + ret = anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
    + stt, O_RDWR | O_CLOEXEC);
    +
    + if (ret >= 0) {
    + list_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);
    + kvm_get_kvm(kvm);
    + }

    - return anon_inode_getfd("kvm-spapr-tce", &kvm_spapr_tce_fops,
    - stt, O_RDWR | O_CLOEXEC);
    + mutex_unlock(&kvm->lock);

    -fail:
    - if (stt) {
    - for (i = 0; i < npages; i++)
    - if (stt->pages[i])
    - __free_page(stt->pages[i]);
    + if (ret >= 0)
    + return ret;

    - kfree(stt);
    - }
    + fail:
    + for (i = 0; i < npages; i++)
    + if (stt->pages[i])
    + __free_page(stt->pages[i]);
    +
    + kfree(stt);
    + fail_acct:
    + kvmppc_account_memlimit(kvmppc_stt_pages(npages), false);
    return ret;
    }


    \
     
     \ /
      Last update: 2017-10-03 15:03    [W:4.225 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site