Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1437621 > unrolled thread

[PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference

Started byWanpeng Li <kernellwp@gmail.com>
First post2016-07-06 12:40 +0200
Last post2016-07-06 14:00 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference Wanpeng Li <kernellwp@gmail.com> - 2016-07-06 12:40 +0200
    Re: [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer  dereference Paolo Bonzini <pbonzini@redhat.com> - 2016-07-06 13:10 +0200
      Re: [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference Wanpeng Li <kernellwp@gmail.com> - 2016-07-06 13:40 +0200
        Re: [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference Wanpeng Li <kernellwp@gmail.com> - 2016-07-06 14:00 +0200

#1437621 — [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-06 12:40 +0200
Subject[PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference
Message-ID<rRSpj-81D-7@gated-at.bofh.it>
From: Wanpeng Li <wanpeng.li@hotmail.com>

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<          (null)>]           (null)
PGD 0 
Oops: 0010 [#1] SMP
Call Trace:
 ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
 handle_preemption_timer+0xe/0x20 [kvm_intel]
 vmx_handle_exit+0x169/0x15a0 [kvm_intel]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
 ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
 ? vcpu_load+0x1c/0x60 [kvm]
 ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
 kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
 do_vfs_ioctl+0x96/0x6a0
 ? __fget_light+0x2a/0x90
 SyS_ioctl+0x79/0x90
 do_syscall_64+0x68/0x180
 entry_SYSCALL64_slow_path+0x25/0x25
Code:  Bad RIP value.
RIP  [<          (null)>]           (null)
 RSP <ffff8800b5263c48>
CR2: 0000000000000000
---[ end trace 9c70c48b1a2bc66e ]---

This can be reproduced readily by preemption timer enabled on L0 and disabled 
on L1.

Preemption timer for nested VMX is emulated by hrtimer which is started on L2
entry, stopped on L2 exit and evaluated via the check_nested_events hook. However,
nested_vmx_exit_handled is always return true for preemption timer vmexit, then 
the L1 preemption timer vmexit is captured and be treated as a L2 preemption 
timer vmexit, incurr a nested vmexit dereference NULL pointer.

This patch fix it by depending on check_nested_events to capture L2 preemption 
timer(emulated hrtimer) expire and nested vmexit.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krčmář <rkrcmar@redhat.com>
Cc: Yunhong Jiang <yunhong.jiang@intel.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Haozhong Zhang <haozhong.zhang@intel.com>
Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
---
v1 -> v2:
 * fix typo in patch description

 arch/x86/kvm/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 85e2f0a..29c16a8 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
 	case EXIT_REASON_PCOMMIT:
 		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
+	case EXIT_REASON_PREEMPTION_TIMER:
+		return false;
 	default:
 		return true;
 	}
-- 
1.9.1

[toc] | [next] | [standalone]

#1437640 — Re: [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference

FromPaolo Bonzini <pbonzini@redhat.com>
Date2016-07-06 13:10 +0200
SubjectRe: [PATCH v2] KVM: nVMX: Fix preemption timer kernel NULL pointer dereference
Message-ID<rRSSl-8rf-13@gated-at.bofh.it>
In reply to#1437621

On 06/07/2016 12:29, Wanpeng Li wrote:
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<          (null)>]           (null)
> PGD 0 
> Oops: 0010 [#1] SMP
> Call Trace:
>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>  ? vcpu_load+0x1c/0x60 [kvm]
>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>  do_vfs_ioctl+0x96/0x6a0
>  ? __fget_light+0x2a/0x90
>  SyS_ioctl+0x79/0x90
>  do_syscall_64+0x68/0x180
>  entry_SYSCALL64_slow_path+0x25/0x25
> Code:  Bad RIP value.
> RIP  [<          (null)>]           (null)
>  RSP <ffff8800b5263c48>
> CR2: 0000000000000000
> ---[ end trace 9c70c48b1a2bc66e ]---

This is happening in L2, while the patch is for L1, right?  So the commit
title should be "KVM: nVMX: fix incorrect preemption timer vmexit in nested guest".

The patch looks correct, but I'm not sure how you get a preemption 
timer vmexit while vmcs02 is active:

 	exec_control = vmcs12->pin_based_vm_exec_control;
 	exec_control |= vmcs_config.pin_based_exec_ctrl;
	exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;

In other words, don't you need something like

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f6e5cc679898..edf925a2da9d 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1121,9 +1121,6 @@ static inline bool cpu_has_broken_vmx_preemption_timer(void)
 
 static inline bool cpu_has_vmx_preemption_timer(void)
 {
-	if (cpu_has_broken_vmx_preemption_timer())
-		return false;
-
 	return vmcs_config.pin_based_exec_ctrl &
 		PIN_BASED_VMX_PREEMPTION_TIMER;
 }
@@ -3406,6 +3403,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf)
 	if (adjust_vmx_controls(min, opt, MSR_IA32_VMX_PINBASED_CTLS,
 				&_pin_based_exec_control) < 0)
 		return -EIO;
+
+	if (cpu_has_broken_vmx_preemption_timer())
+		_pin_based_exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
 
 	if (!(_cpu_based_2nd_exec_control &
 		SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY))
@@ -9797,8 +9796,10 @@ static void prepare_vmcs02(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
 	vmcs_write64(VMCS_LINK_POINTER, -1ull);
 
 	exec_control = vmcs12->pin_based_vm_exec_control;
+ 	exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
	exec_control |= vmcs_config.pin_based_exec_ctrl;
- 	exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
+	if (vmx->hv_deadline_tsc == -1)
+		exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
 
 	if (nested_cpu_has_posted_intr(vmcs12)) {
 		/*


for the L1 TSC deadline timer to trigger while L2 is running?  It's untested,
but this would replace the patch that Haozhong submitted earlier.

Paolo

> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Cc: Yunhong Jiang <yunhong.jiang@intel.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Cc: Haozhong Zhang <haozhong.zhang@intel.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@hotmail.com>
> ---
> v1 -> v2:
>  * fix typo in patch description
> 
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 85e2f0a..29c16a8 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -8041,6 +8041,8 @@ static bool nested_vmx_exit_handled(struct kvm_vcpu *vcpu)
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_XSAVES);
>  	case EXIT_REASON_PCOMMIT:
>  		return nested_cpu_has2(vmcs12, SECONDARY_EXEC_PCOMMIT);
> +	case EXIT_REASON_PREEMPTION_TIMER:
> +		return false;
>  	default:
>  		return true;
>  	}
>

[toc] | [prev] | [next] | [standalone]

#1437651

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-06 13:40 +0200
Message-ID<rRTlo-bj-37@gated-at.bofh.it>
In reply to#1437640
2016-07-06 19:02 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>
>
> On 06/07/2016 12:29, Wanpeng Li wrote:
>> BUG: unable to handle kernel NULL pointer dereference at           (null)
>> IP: [<          (null)>]           (null)
>> PGD 0
>> Oops: 0010 [#1] SMP
>> Call Trace:
>>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>  ? vcpu_load+0x1c/0x60 [kvm]
>>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>>  do_vfs_ioctl+0x96/0x6a0
>>  ? __fget_light+0x2a/0x90
>>  SyS_ioctl+0x79/0x90
>>  do_syscall_64+0x68/0x180
>>  entry_SYSCALL64_slow_path+0x25/0x25
>> Code:  Bad RIP value.
>> RIP  [<          (null)>]           (null)
>>  RSP <ffff8800b5263c48>
>> CR2: 0000000000000000
>> ---[ end trace 9c70c48b1a2bc66e ]---
>
> This is happening in L2, while the patch is for L1, right?  So the commit
> title should be "KVM: nVMX: fix incorrect preemption timer vmexit in nested guest".

Thanks. I will send out another version. :)

>
> The patch looks correct, but I'm not sure how you get a preemption
> timer vmexit while vmcs02 is active:
>
>         exec_control = vmcs12->pin_based_vm_exec_control;
>         exec_control |= vmcs_config.pin_based_exec_ctrl;
>         exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>
> In other words, don't you need something like

After apply your patch, L0 calltrace.

[  355.840045] ------------[ cut here ]------------
[  355.840072] WARNING: CPU: 3 PID: 4176 at
/home/kernel/kvm/arch/x86/kvm/lapic.c:1362
kvm_lapic_expired_hv_timer+0x6c/0x90 [kvm]
[  355.840073] Modules linked in: kvm_intel(OE) kvm(OE) openvswitch
nf_nat_ipv6 nf_nat_ipv4 libcrc32c ip6t_REJECT nf_reject_ipv6
nf_log_ipv6 xt_hl ip6t_rt nf_conntrack_ipv6 nf_defrag_ipv6 ipt_REJECT
snd_hda_codec_hdmi nf_reject_ipv4 nf_log_ipv4 nf_log_common xt_LOG
xt_limit xt_tcpudp intel_rapl xt_addrtype x86_pkg_temp_thermal i915
intel_powerclamp nf_conntrack_ipv4 coretemp nf_defrag_ipv4
xt_conntrack snd_hda_codec_realtek snd_hda_codec_generic
ip6table_filter snd_hda_intel drm_kms_helper ip6_tables snd_hda_codec
nf_conntrack_netbios_ns nf_conntrack_broadcast nf_nat_ftp nf_nat
snd_hda_core nf_conntrack_ftp snd_hwdep nf_conntrack snd_pcm
iptable_filter snd_seq_midi ip_tables snd_seq_midi_event x_tables
snd_rawmidi irqbypass snd_seq crct10dif_pclmul crc32_pclmul
ghash_clmulni_intel drm aesni_intel snd_seq_device aes_x86_64
snd_timer lrw dcdbas gf128mul snd glue_helper ablk_helper cryptd
mei_me serio_raw mei i2c_algo_bit soundcore shpchp lpc_ich video
mac_hid bnep rfcomm bluetooth nfsd auth_rpcgss nfs_acl nfs parport_pc
ppdev lockd grace sunrpc lp fscache parport hid_generic usbhid ahci
psmouse hid e1000e libahci ptp pps_core [last unloaded: kvm]
[  355.840135] CPU: 3 PID: 4176 Comm: qemu-system-x86 Tainted: G
 W  OE   4.7.0-rc1+ #19
[  355.840136] Hardware name: Dell Inc. OptiPlex 7020/0F5C5X, BIOS A03
01/08/2015
[  355.840137]  0000000000000000 ffff880203bf7be8 ffffffff8143a669
0000000000000000
[  355.840140]  0000000000000000 ffff880203bf7c28 ffffffff8108b5a1
00000552117eb580
[  355.840142]  ffff8800d9ea3c00 ffff880204ed0000 0000000000000001
ffff8802028c4280
[  355.840145] Call Trace:
[  355.840149]  [<ffffffff8143a669>] dump_stack+0x99/0xd0
[  355.840152]  [<ffffffff8108b5a1>] __warn+0xd1/0xf0
[  355.840154]  [<ffffffff8108b68d>] warn_slowpath_null+0x1d/0x20
[  355.840166]  [<ffffffffc08976fc>] kvm_lapic_expired_hv_timer+0x6c/0x90 [kvm]
[  355.840169]  [<ffffffffc057061e>] handle_preemption_timer+0xe/0x20
[kvm_intel]
[  355.840172]  [<ffffffffc057c329>] vmx_handle_exit+0x169/0x15a0 [kvm_intel]
[  355.840182]  [<ffffffffc087c5cf>] ?
kvm_arch_vcpu_ioctl_run+0xdef/0x1aa0 [kvm]
[  355.840190]  [<ffffffffc087c661>] kvm_arch_vcpu_ioctl_run+0xe81/0x1aa0 [kvm]
[  355.840197]  [<ffffffffc087c5cf>] ?
kvm_arch_vcpu_ioctl_run+0xdef/0x1aa0 [kvm]
[  355.840206]  [<ffffffffc08751a7>] ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
[  355.840213]  [<ffffffffc0860c13>] kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
[  355.840216]  [<ffffffff810e6794>] ? __lock_is_held+0x54/0x70
[  355.840218]  [<ffffffff8126a646>] do_vfs_ioctl+0x96/0x6a0
[  355.840220]  [<ffffffff8127734a>] ? __fget_light+0x2a/0x90
[  355.840221]  [<ffffffff8126acc9>] SyS_ioctl+0x79/0x90
[  355.840223]  [<ffffffff81003c5c>] do_syscall_64+0x7c/0x1e0
[  355.840226]  [<ffffffff818b9483>] entry_SYSCALL64_slow_path+0x25/0x25
[  355.840227] ---[ end trace 4efbadf14355f403 ]---

[toc] | [prev] | [next] | [standalone]

#1437660

FromWanpeng Li <kernellwp@gmail.com>
Date2016-07-06 14:00 +0200
Message-ID<rRTEJ-iL-1@gated-at.bofh.it>
In reply to#1437651
2016-07-06 19:38 GMT+08:00 Wanpeng Li <kernellwp@gmail.com>:
> 2016-07-06 19:02 GMT+08:00 Paolo Bonzini <pbonzini@redhat.com>:
>>
>>
>> On 06/07/2016 12:29, Wanpeng Li wrote:
>>> BUG: unable to handle kernel NULL pointer dereference at           (null)
>>> IP: [<          (null)>]           (null)
>>> PGD 0
>>> Oops: 0010 [#1] SMP
>>> Call Trace:
>>>  ? kvm_lapic_expired_hv_timer+0x47/0x90 [kvm]
>>>  handle_preemption_timer+0xe/0x20 [kvm_intel]
>>>  vmx_handle_exit+0x169/0x15a0 [kvm_intel]
>>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>>  kvm_arch_vcpu_ioctl_run+0xdee/0x19d0 [kvm]
>>>  ? kvm_arch_vcpu_ioctl_run+0xd5d/0x19d0 [kvm]
>>>  ? vcpu_load+0x1c/0x60 [kvm]
>>>  ? kvm_arch_vcpu_load+0x57/0x260 [kvm]
>>>  kvm_vcpu_ioctl+0x2d3/0x7c0 [kvm]
>>>  do_vfs_ioctl+0x96/0x6a0
>>>  ? __fget_light+0x2a/0x90
>>>  SyS_ioctl+0x79/0x90
>>>  do_syscall_64+0x68/0x180
>>>  entry_SYSCALL64_slow_path+0x25/0x25
>>> Code:  Bad RIP value.
>>> RIP  [<          (null)>]           (null)
>>>  RSP <ffff8800b5263c48>
>>> CR2: 0000000000000000
>>> ---[ end trace 9c70c48b1a2bc66e ]---
>>
>> This is happening in L2, while the patch is for L1, right?  So the commit
>> title should be "KVM: nVMX: fix incorrect preemption timer vmexit in nested guest".
>
> Thanks. I will send out another version. :)
>
>>
>> The patch looks correct, but I'm not sure how you get a preemption
>> timer vmexit while vmcs02 is active:
>>
>>         exec_control = vmcs12->pin_based_vm_exec_control;
>>         exec_control |= vmcs_config.pin_based_exec_ctrl;
>>         exec_control &= ~PIN_BASED_VMX_PREEMPTION_TIMER;
>>
>> In other words, don't you need something like
>
> After apply your patch, L0 calltrace.

my patch + your patch, L0 calltrace.
w/o my patch + your patch, L1 the same calltrace.

Regards,
Wanpeng Li

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web