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: Thu, 15 Sep 2016 08:58:55 +0800 From: Wanpeng Li <kernellwp@...il.com> To: Paolo Bonzini <pbonzini@...hat.com> Cc: Radim Krcmar <rkrcmar@...hat.com>, kvm <kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org> Subject: Re: kvm-unit-test fail for split irqchip 2016-09-14 17:42 GMT+08:00 Paolo Bonzini <pbonzini@...hat.com>: > > > On 14/09/2016 05:57, Wanpeng Li wrote: >> 2016-09-14 4:43 GMT+08:00 Paolo Bonzini <pbonzini@...hat.com>: >>> >>> >>> On 13/09/2016 21:01, Radim Krcmar wrote: >>>> kvm_handle_interrupt() does >>>> >>>> interrupt_request |= CPU_INTERRUPT_HARD >>>> >>>> which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), but >>>> that function uses stale information from APIC and injects 62 again. >>>> If we synchronized the APIC, then the test would #GP, because there >>>> would be no injectable interrupt in LAPIC or PIC, so pic_read_irq() >>>> would return 15, thinking it was spurious. >>>> >>>> I think the bug starts in pic_irq_request(), which should not touch >>>> LAPIC. The patch below makes it work (just the second hunk is >>>> sufficient), but it's still far from sane code ... >>> >>> This makes sense. Most of the functions exported by hw/intc/apic.c >>> should only be used with a userspace APIC: >>> >>> 0000000000000b70 T apic_accept_pic_intr >>> 00000000000010f0 T apic_deliver_irq >>> 00000000000011e0 T apic_deliver_pic_intr >>> 0000000000001310 T apic_get_interrupt >>> 0000000000001270 T apic_poll_irq >>> 0000000000000a40 T apic_sipi >>> >>> The patch is okay, though for consistency with other code I'd use >>> !kvm_irqchip_in_kernel() rather than !kvm_irqchip_is_split(). >>> >>> Wanpeng, can you do that, >> >> Yeah, I just sent out a patch to fix the bug. Thanks for the long >> discussion with me and thanks Radim's proposal. :) >> >>> and change hw/intc/apic.c to use a new casting >>> macro >>> >>> APICCommonState *s = APIC(dev); >>> >>> instead of APIC_COMMON? >>> >> >> I'm not familiar with QOM too much, what APIC macro do you like? > > The macro would be defined like > > #define TYPE_APIC "apic" > #define APIC(obj) \ > OBJECT_CHECK(APICCommonState, (obj), TYPE_APIC) > > With this change and without your other patch, you should get an > assertion failure in eventinj.flat (which would have pointed immediately > at the bug!). Good point, I just sent out the patch, it catches the bug as you expected. :) Regards, Wanpeng Li
Powered by blists - more mailing lists