| 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
| ||
|
Date: Fri, 31 Oct 2014 14:11:42 +0800
From: Wanpeng Li <wanpeng.li@...ux.intel.com>
To: "Chen, Tiejun" <tiejun.chen@...el.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Gleb Natapov <gleb@...nel.org>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Takashi Iwai <tiwai@...e.de>, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86: fix access memslots w/o hold srcu read lock
Hi Tiejun,
On Fri, Oct 31, 2014 at 01:30:35PM +0800, Chen, Tiejun wrote:
>On 2014/10/31 12:33, Wanpeng Li wrote:
>>The srcu read lock must be held while accessing memslots (e.g.
>>when using gfn_to_* functions), however, commit c24ae0dcd3e8
>>("kvm: x86: Unpin and remove kvm_arch->apic_access_page") call
>>gfn_to_page() in kvm_vcpu_reload_apic_access_page() w/o hold it
>>which leads to suspicious rcu_dereference_check() usage warning.
>>This patch fix it by holding srcu read lock when call gfn_to_page()
>>in kvm_vcpu_reload_apic_access_page() function.
>>
>>
>>[ INFO: suspicious RCU usage. ]
>>3.18.0-rc2-test2+ #70 Not tainted
>>-------------------------------
>>include/linux/kvm_host.h:474 suspicious rcu_dereference_check() usage!
>>
>>other info that might help us debug this:
>>
>>rcu_scheduler_active = 1, debug_locks = 0
>>1 lock held by qemu-system-x86/2371:
>> #0: (&vcpu->mutex){+.+...}, at: [<ffffffffa037d800>] vcpu_load+0x20/0xd0 [kvm]
>>
>>stack backtrace:
>>CPU: 4 PID: 2371 Comm: qemu-system-x86 Not tainted 3.18.0-rc2-test2+ #70
>>Hardware name: Dell Inc. OptiPlex 9010/0M9KCM, BIOS A12 01/10/2013
>> 0000000000000001 ffff880209983ca8 ffffffff816f514f 0000000000000000
>> ffff8802099b8990 ffff880209983cd8 ffffffff810bd687 00000000000fee00
>> ffff880208a2c000 ffff880208a10000 ffff88020ef50040 ffff880209983d08
>>Call Trace:
>> [<ffffffff816f514f>] dump_stack+0x4e/0x71
>> [<ffffffff810bd687>] lockdep_rcu_suspicious+0xe7/0x120
>> [<ffffffffa037d055>] gfn_to_memslot+0xd5/0xe0 [kvm]
>> [<ffffffffa03807d3>] __gfn_to_pfn+0x33/0x60 [kvm]
>> [<ffffffffa0380885>] gfn_to_page+0x25/0x90 [kvm]
>> [<ffffffffa038aeec>] kvm_vcpu_reload_apic_access_page+0x3c/0x80 [kvm]
>> [<ffffffffa08f0a9c>] vmx_vcpu_reset+0x20c/0x460 [kvm_intel]
>> [<ffffffffa039ab8e>] kvm_vcpu_reset+0x15e/0x1b0 [kvm]
>> [<ffffffffa039ac0c>] kvm_arch_vcpu_setup+0x2c/0x50 [kvm]
>> [<ffffffffa037f7e0>] kvm_vm_ioctl+0x1d0/0x780 [kvm]
>> [<ffffffff810bc664>] ? __lock_is_held+0x54/0x80
>> [<ffffffff812231f0>] do_vfs_ioctl+0x300/0x520
>> [<ffffffff8122ee45>] ? __fget+0x5/0x250
>> [<ffffffff8122f0fa>] ? __fget_light+0x2a/0xe0
>> [<ffffffff81223491>] SyS_ioctl+0x81/0xa0
>> [<ffffffff816fed6d>] system_call_fastpath+0x16/0x1b
>>
>>Reported-by: Takashi Iwai <tiwai@...e.de>
>>Reported-by: Alexei Starovoitov <alexei.starovoitov@...il.com>
>>Signed-off-by: Wanpeng Li <wanpeng.li@...ux.intel.com>
>>---
>> arch/x86/kvm/x86.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>>index 0033df3..2d97329 100644
>>--- a/arch/x86/kvm/x86.c
>>+++ b/arch/x86/kvm/x86.c
>>@@ -6059,6 +6059,7 @@ static void kvm_vcpu_flush_tlb(struct kvm_vcpu *vcpu)
>> void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
>> {
>> struct page *page = NULL;
>>+ int idx;
>>
>> if (!irqchip_in_kernel(vcpu->kvm))
>> return;
>>@@ -6066,7 +6067,9 @@ void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
>> if (!kvm_x86_ops->set_apic_access_page_addr)
>> return;
>>
>>+ idx = srcu_read_lock(&vcpu->kvm->srcu);
>
>There's another scenario that we already hold srcu before call
>kvm_vcpu_reload_apic_access_page(),
>
>__vcpu_run()
> |
> + vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
> + r = vcpu_enter_guest(vcpu);
> |
> + kvm_vcpu_reload_apic_access_page(vcpu);
>
You are right. Great thanks for your pointing out. After recheck all the
callsites of kvm_vcpu_reload_apic_access_page(), just vmx_vcpu_reset()
path need to be fixed.
Regards,
Wanpeng Li
>So according to backtrace I think we should fix as follows:
>
>kvm: x86: vmx: hold kvm->srcu while reload apic access page
>
>kvm_vcpu_reload_apic_access_page() needs to access memslots via
>gfn_to_page(), so its necessary to hold kvm->srcu.
>
>Signed-off-by: Tiejun Chen <tiejun.chen@...el.com>
>---
> arch/x86/kvm/vmx.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>index b25a588..9fa1f46 100644
>--- a/arch/x86/kvm/vmx.c
>+++ b/arch/x86/kvm/vmx.c
>@@ -4442,6 +4442,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
> {
> struct vcpu_vmx *vmx = to_vmx(vcpu);
> struct msr_data apic_base_msr;
>+ int idx;
>
> vmx->rmode.vm86_active = 0;
>
>@@ -4509,7 +4510,9 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu)
> vmcs_write32(TPR_THRESHOLD, 0);
> }
>
>+ idx = srcu_read_lock(&vcpu->kvm->srcu);
> kvm_vcpu_reload_apic_access_page(vcpu);
>+ srcu_read_unlock(&vcpu->kvm->srcu, idx);
>
> if (vmx_vm_has_apicv(vcpu->kvm))
> memset(&vmx->pi_desc, 0, sizeof(struct pi_desc));
>--
>1.9.1
>
>Thanks
>Tiejun
>> page = gfn_to_page(vcpu->kvm, APIC_DEFAULT_PHYS_BASE >> PAGE_SHIFT);
>>+ srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> kvm_x86_ops->set_apic_access_page_addr(vcpu, page_to_phys(page));
>>
>> /*
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists