linux-kernel - Re: [PATCH AUTOSEL for 4.14 006/110] KVM/x86: Check input paging mode when cs.l is set

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: <20180204021730.kiv7n6ag6t4srrql@sasha-lappy>
Date:   Sun, 4 Feb 2018 02:17:37 +0000
From:   Sasha Levin <Alexander.Levin@...rosoft.com>
To:     Eric Biggers <ebiggers3@...il.com>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        Lan Tianyu <tianyu.lan@...el.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH AUTOSEL for 4.14 006/110] KVM/x86: Check input paging mode
 when cs.l is set

On Sat, Feb 03, 2018 at 05:24:29PM -0800, Eric Biggers wrote:
>On Sat, Feb 03, 2018 at 06:00:29PM +0000, Sasha Levin wrote:
>> From: Lan Tianyu <tianyu.lan@...el.com>
>>
>> [ Upstream commit f29810335965ac1f7bcb501ee2af5f039f792416 ]
>>
>> Reported by syzkaller:
>>     WARNING: CPU: 0 PID: 27962 at arch/x86/kvm/emulate.c:5631 x86_emulate_insn+0x557/0x15f0 [kvm]
>>     Modules linked in: kvm_intel kvm [last unloaded: kvm]
>>     CPU: 0 PID: 27962 Comm: syz-executor Tainted: G    B   W        4.15.0-rc2-next-20171208+ #32
>>     Hardware name: Intel Corporation S1200SP/S1200SP, BIOS S1200SP.86B.01.03.0006.040720161253 04/07/2016
>>     RIP: 0010:x86_emulate_insn+0x557/0x15f0 [kvm]
>>     RSP: 0018:ffff8807234476d0 EFLAGS: 00010282
>>     RAX: 0000000000000000 RBX: ffff88072d0237a0 RCX: ffffffffa0065c4d
>>     RDX: 1ffff100e5a046f9 RSI: 0000000000000003 RDI: ffff88072d0237c8
>>     RBP: ffff880723447728 R08: ffff88072d020000 R09: ffffffffa008d240
>>     R10: 0000000000000002 R11: ffffed00e7d87db3 R12: ffff88072d0237c8
>>     R13: ffff88072d023870 R14: ffff88072d0238c2 R15: ffffffffa008d080
>>     FS:  00007f8a68666700(0000) GS:ffff880802200000(0000) knlGS:0000000000000000
>>     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>     CR2: 000000002009506c CR3: 000000071fec4005 CR4: 00000000003626f0
>>     Call Trace:
>>      x86_emulate_instruction+0x3bc/0xb70 [kvm]
>>      ? reexecute_instruction.part.162+0x130/0x130 [kvm]
>>      vmx_handle_exit+0x46d/0x14f0 [kvm_intel]
>>      ? trace_event_raw_event_kvm_entry+0xe7/0x150 [kvm]
>>      ? handle_vmfunc+0x2f0/0x2f0 [kvm_intel]
>>      ? wait_lapic_expire+0x25/0x270 [kvm]
>>      vcpu_enter_guest+0x720/0x1ef0 [kvm]
>>      ...
>>
>> When CS.L is set, vcpu should run in the 64 bit paging mode.
>> Current kvm set_sregs function doesn't have such check when
>> userspace inputs sreg values. This will lead unexpected behavior.
>> This patch is to add checks for CS.L, EFER.LME, EFER.LMA and
>> CR4.PAE when get SREG inputs from userspace in order to avoid
>> unexpected behavior.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@...hat.com>
>> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
>> Cc: Paolo Bonzini <pbonzini@...hat.com>
>> Cc: Radim Krčmář <rkrcmar@...hat.com>
>> Cc: Dmitry Vyukov <dvyukov@...gle.com>
>> Cc: Jim Mattson <jmattson@...gle.com>
>> Signed-off-by: Tianyu Lan <tianyu.lan@...el.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@...hat.com>
>> Signed-off-by: Sasha Levin <alexander.levin@...rosoft.com>
>> ---
>>  arch/x86/kvm/x86.c | 26 ++++++++++++++++++++++++++
>>  1 file changed, 26 insertions(+)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 8c28023a43b1..ad0f18107c74 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -7473,6 +7473,29 @@ int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
>>  }
>>  EXPORT_SYMBOL_GPL(kvm_task_switch);
>>
>> +int kvm_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
>> +{
>> +	if ((sregs->efer & EFER_LME) && (sregs->cr0 & X86_CR0_PG_BIT)) {
>> +		/*
>> +		 * When EFER.LME and CR0.PG are set, the processor is in
>> +		 * 64-bit mode (though maybe in a 32-bit code segment).
>> +		 * CR4.PAE and EFER.LMA must be set.
>> +		 */
>> +		if (!(sregs->cr4 & X86_CR4_PAE_BIT)
>> +		    || !(sregs->efer & EFER_LMA))
>> +			return -EINVAL;
>> +	} else {
>> +		/*
>> +		 * Not in 64-bit mode: EFER.LMA is clear and the code
>> +		 * segment cannot be 64-bit.
>> +		 */
>> +		if (sregs->efer & EFER_LMA || sregs->cs.l)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>
>This commit is broken and there was a fix for it merged: 37b95951c58 ("KVM/x86:
>Fix wrong macro references of X86_CR0_PG_BIT and X86_CR4_PAE_BIT in
>kvm_valid_sregs()").  Shouldn't your script have picked that up too?

It should have, yes. I tried to figure out why it didn't and it looks
like the "Fixes:" line is a bit messed up in that commit, and my script
didn't parse it right:

	Fixes: f29810335965a(KVM/x86: Check input paging mode when cs.l
is set)

-- 

Thanks,
Sasha