linux-kernel - Re: [PATCH] KVM: nVMX: do not warn when MSR bitmap address is not backed

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] [day] [month] [year] [list]

Date:   Tue, 7 Mar 2017 20:22:21 +0100
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Jim Mattson <jmattson@...gle.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        kvm list <kvm@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] KVM: nVMX: do not warn when MSR bitmap address is not
 backed

2017-03-07 09:12-0800, Jim Mattson:
> I believe this behavior would be documented in the chipset data sheet
> rather than the SDM, since the chipset returns all 1s for an unclaimed
> read.

Good point, thanks.  I'll add it as commit comment when applying.

I looked at some data sheets, but couldn't find where they say that :(
Emulating a (preferable userspace-configurable) fallback would need
deeper changes, though.

> Reviewed-by: Jim Mattson <jmattson@...gle.com>
> 
> On Tue, Mar 7, 2017 at 8:51 AM, Radim Krčmář <rkrcmar@...hat.com> wrote:
>> Before trying to do nested_get_page() in nested_vmx_merge_msr_bitmap(),
>> we have already checked that the MSR bitmap address is valid (4k aligned
>> and within physical limits).  SDM doesn't specify what happens if the
>> there is no memory mapped at the valid address, but Intel CPUs treat the
>> situation as if the bitmap was configured to trap all MSRs.
>>
>> KVM already does that by returning false and a correct handling doesn't
>> need the guest-trigerrable warning that was reported by syzkaller:
>> (The warning was originally there to catch some possible bugs in nVMX.)
>>
>>   ------------[ cut here ]------------
>>   WARNING: CPU: 0 PID: 7832 at arch/x86/kvm/vmx.c:9709
>>   nested_vmx_merge_msr_bitmap arch/x86/kvm/vmx.c:9709 [inline]
>>   WARNING: CPU: 0 PID: 7832 at arch/x86/kvm/vmx.c:9709
>>   nested_get_vmcs12_pages+0xfb6/0x15c0 arch/x86/kvm/vmx.c:9640
>>   Kernel panic - not syncing: panic_on_warn set ...
>>   CPU: 0 PID: 7832 Comm: syz-executor1 Not tainted 4.10.0+ #229
>>   Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>   Call Trace:
>>    __dump_stack lib/dump_stack.c:15 [inline]
>>    dump_stack+0x2ee/0x3ef lib/dump_stack.c:51
>>    panic+0x1fb/0x412 kernel/panic.c:179
>>    __warn+0x1c4/0x1e0 kernel/panic.c:540
>>    warn_slowpath_null+0x2c/0x40 kernel/panic.c:583
>>    nested_vmx_merge_msr_bitmap arch/x86/kvm/vmx.c:9709 [inline]
>>    nested_get_vmcs12_pages+0xfb6/0x15c0 arch/x86/kvm/vmx.c:9640
>>    enter_vmx_non_root_mode arch/x86/kvm/vmx.c:10471 [inline]
>>    nested_vmx_run+0x6186/0xaab0 arch/x86/kvm/vmx.c:10561
>>    handle_vmlaunch+0x1a/0x20 arch/x86/kvm/vmx.c:7312
>>    vmx_handle_exit+0xfc0/0x3f00 arch/x86/kvm/vmx.c:8526
>>    vcpu_enter_guest arch/x86/kvm/x86.c:6982 [inline]
>>    vcpu_run arch/x86/kvm/x86.c:7044 [inline]
>>    kvm_arch_vcpu_ioctl_run+0x1418/0x4840 arch/x86/kvm/x86.c:7205
>>    kvm_vcpu_ioctl+0x673/0x1120 arch/x86/kvm/../../../virt/kvm/kvm_main.c:2570
>>
>> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>> Signed-off-by: Radim Krčmář <rkrcmar@...hat.com>
>> ---
>>  arch/x86/kvm/vmx.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index ab338581b3ec..98e82ee1e699 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -9680,10 +9680,8 @@ static inline bool nested_vmx_merge_msr_bitmap(struct kvm_vcpu *vcpu,
>>                 return false;
>>
>>         page = nested_get_page(vcpu, vmcs12->msr_bitmap);
>> -       if (!page) {
>> -               WARN_ON(1);
>> +       if (!page)
>>                 return false;
>> -       }
>>         msr_bitmap_l1 = (unsigned long *)kmap(page);
>>
>>         memset(msr_bitmap_l0, 0xff, PAGE_SIZE);
>> --
>> 2.12.0
>>