linux-kernel - Re: kvm: use-after-free in process

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACT4Y+ZquKsPz6hO=4RNPs0Wpa1Ayns2ZNAx90EbBbooUFEJJg@mail.gmail.com>
Date:   Sun, 15 Jan 2017 18:11:08 +0100
From:   Dmitry Vyukov <dvyukov@...gle.com>
To:     Steve Rutherford <srutherford@...gle.com>
Cc:     syzkaller <syzkaller@...glegroups.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        KVM list <kvm@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: kvm: use-after-free in process_srcu

On Fri, Jan 13, 2017 at 10:19 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> On Fri, Jan 13, 2017 at 4:30 AM, Steve Rutherford
> <srutherford@...gle.com> wrote:
>> I'm not that familiar with the kernel's workqueues, but this seems
>> like the classic "callback outlives the memory it references"
>> use-after-free, where the process_srcu callback is outliving struct
>> kvm (which contains the srcu_struct). If that's right, then calling
>> srcu_barrier (which should wait for all of the call_srcu callbacks to
>> complete, which are what enqueue the process_srcu callbacks) before
>> cleanup_srcu_struct in kvm_destroy_vm probably fixes this.
>>
>> The corresponding patch to virt/kvm/kvm_main.c looks something like:
>> static void kvm_destroy_vm(struct kvm *kvm)
>> {
>> ...
>>         for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++)
>>                 kvm_free_memslots(kvm, kvm->memslots[i]);
>> +      srcu_barrier(&kvm->irq_srcu);
>>         cleanup_srcu_struct(&kvm->irq_srcu);
>> +      srcu_barrier(&kvm->srcu);
>>         cleanup_srcu_struct(&kvm->srcu);
>> ...
>>
>>
>> Since we don't have a repro, this obviously won't be readily testable.
>> I find srcu subtle enough that I don't trust my reasoning fully (in
>> particular, I don't trust that waiting for all of the call_srcu
>> callbacks to complete also waits for all of the process_srcu
>> callbacks). Someone else know if that's the case?
>
>
> From the function description it looks like it should do the trick:
>
> 514 /**
> 515  * srcu_barrier - Wait until all in-flight call_srcu() callbacks complete.
> 516  * @sp: srcu_struct on which to wait for in-flight callbacks.
> 517  */
> 518 void srcu_barrier(struct srcu_struct *sp)
>
> I see this failure happening several times per day. I've applied your
> patch locally and will check if I see these failures happening.


I have not seen the crash in 3 days, when usually I see several
crashes per night. So I think we can consider that the patch fixes the
crash:

Tested-by: Dmitry Vyukov <dvyukov@...gle.com>