Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 25.01.2012 17:00, schrieb Joerg Roedel: On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote: On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote: However, task_switch_interception() itself does some more based on the value of reason, for example it decides whether or not to call skip_emulated_instruction(). Joerg need to help us here. If intercept of task switch happens before rip is advanced past instruction that cause it we have to know somehow that task switch was caused by instruction. It is not enough that HW checks permission, we still lack essential info. Hmm, the RIP in the VMCB points to the instruction causing the task switch. This is also true for lcall and ljmp. But in my experiments I have seen exit_int_info.valid = 1 for task-switches that went through the IDT. But I havn't tested the VM86 case, though. Kevin, can you please re-verify that exit_int_info.valid is always 0 in your experiment? On what hardware have you tested this? I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. This is the /proc/cpuinfo snippet for the first core: processor : 0 vendor_id : AuthenticAMD cpu family : 15 model : 107 model name : AMD Athlon(tm) 64 X2 Dual Core Processor 5200+ stepping: 2 cpu MHz : 1800.000 cache size : 512 KB physical id : 0 siblings: 2 core id : 0 cpu cores : 2 apicid : 0 initial apicid : 0 fpu : yes fpu_exception : yes cpuid level : 1 wp : yes flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ht syscall nx mmxext fxsr_opt rdtscp lm 3dnowext 3dnow rep_good nopl extd_apicid pni cx16 lahf_lm cmp_legacy svm extapic cr8_legacy 3dnowprefetch lbrv bogomips: 3592.64 TLB size: 1024 4K pages clflush size: 64 cache_alignment : 64 address sizes : 40 bits physical, 48 bits virtual power management: ts fid vid ttp tm stc 100mhzsteps Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote: Am 25.01.2012 17:00, schrieb Joerg Roedel: I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. Okay, you are testing on a K8 which has exactly this bug. As I just found out it is documented as erratum 701. The good news is that this only happens on K8 and Fam11h, any later AMD processor doesn't have this bug. Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 27.01.2012 14:34, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote: Am 25.01.2012 17:00, schrieb Joerg Roedel: I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. Okay, you are testing on a K8 which has exactly this bug. As I just found out it is documented as erratum 701. The good news is that this only happens on K8 and Fam11h, any later AMD processor doesn't have this bug. Meh. Unless you give me a newer processor, this doesn't really help me... Doesn't look like there's any way to get a workaround, is there? I guess I'll have to hack it locally and possibly break other guests with the hacked module. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote: Am 27.01.2012 14:34, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote: Am 25.01.2012 17:00, schrieb Joerg Roedel: I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. Okay, you are testing on a K8 which has exactly this bug. As I just found out it is documented as erratum 701. The good news is that this only happens on K8 and Fam11h, any later AMD processor doesn't have this bug. Meh. Unless you give me a newer processor, this doesn't really help me... Doesn't look like there's any way to get a workaround, is there? I guess I'll have to hack it locally and possibly break other guests with the hacked module. No, unfortunatly there is no workaround for this problem. How do you plan to hack around it? Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 27.01.2012 15:17, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote: Am 27.01.2012 14:34, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote: Am 25.01.2012 17:00, schrieb Joerg Roedel: I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. Okay, you are testing on a K8 which has exactly this bug. As I just found out it is documented as erratum 701. The good news is that this only happens on K8 and Fam11h, any later AMD processor doesn't have this bug. Meh. Unless you give me a newer processor, this doesn't really help me... Doesn't look like there's any way to get a workaround, is there? I guess I'll have to hack it locally and possibly break other guests with the hacked module. No, unfortunatly there is no workaround for this problem. How do you plan to hack around it? I know that my guest only uses iret and exceptions for task switches, so I think in my case I can assume that any TASK_SWITCH_CALL is really a TASK_SWITCH_GATE and I don't have to skip an instruction. Not quite upstreamable, obviously. Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Fri, Jan 27, 2012 at 04:02:30PM +0100, Kevin Wolf wrote: Am 27.01.2012 15:17, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 02:55:12PM +0100, Kevin Wolf wrote: Am 27.01.2012 14:34, schrieb Joerg Roedel: On Fri, Jan 27, 2012 at 01:58:38PM +0100, Kevin Wolf wrote: Am 25.01.2012 17:00, schrieb Joerg Roedel: I just retried. I use kvm-kmod and kvm.git with HEAD at ff92e9b5 plus the tree patches of this series plus a printk to output exit_int_info in task_switch_intercept(). I ran taskswitch2 from kvm-unittests and got two failures and my VM86 unit test which hung when trying to return from VM86. I also ran the kernel that made me aware of the issue initially. All debug messages show exit_int_info = 0. Okay, you are testing on a K8 which has exactly this bug. As I just found out it is documented as erratum 701. The good news is that this only happens on K8 and Fam11h, any later AMD processor doesn't have this bug. Meh. Unless you give me a newer processor, this doesn't really help me... Doesn't look like there's any way to get a workaround, is there? I guess I'll have to hack it locally and possibly break other guests with the hacked module. No, unfortunatly there is no workaround for this problem. How do you plan to hack around it? I know that my guest only uses iret and exceptions for task switches, so I think in my case I can assume that any TASK_SWITCH_CALL is really a TASK_SWITCH_GATE and I don't have to skip an instruction. You still need to know what exception caused task switch. Some of them require you to skip an instruction. Not quite upstreamable, obviously. Kevin -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote: On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote: However, task_switch_interception() itself does some more based on the value of reason, for example it decides whether or not to call skip_emulated_instruction(). Joerg need to help us here. If intercept of task switch happens before rip is advanced past instruction that cause it we have to know somehow that task switch was caused by instruction. It is not enough that HW checks permission, we still lack essential info. Hmm, the RIP in the VMCB points to the instruction causing the task switch. This is also true for lcall and ljmp. But in my experiments I have seen exit_int_info.valid = 1 for task-switches that went through the IDT. But I havn't tested the VM86 case, though. Kevin, can you please re-verify that exit_int_info.valid is always 0 in your experiment? On what hardware have you tested this? Thanks, Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Wed, Jan 25, 2012 at 05:00:58PM +0100, Joerg Roedel wrote: On Tue, Jan 24, 2012 at 06:23:50PM +0200, Gleb Natapov wrote: On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote: However, task_switch_interception() itself does some more based on the value of reason, for example it decides whether or not to call skip_emulated_instruction(). Joerg need to help us here. If intercept of task switch happens before rip is advanced past instruction that cause it we have to know somehow that task switch was caused by instruction. It is not enough that HW checks permission, we still lack essential info. Hmm, the RIP in the VMCB points to the instruction causing the task switch. This is also true for lcall and ljmp. But in my experiments I have seen exit_int_info.valid = 1 for task-switches that went through the IDT. But I havn't tested the VM86 case, though. I can confirm that I get exit_int_info.valid = 1 for all scenarios when task switch is caused by idt event. Just checked it here. Kevin, can you please re-verify that exit_int_info.valid is always 0 in your experiment? On what hardware have you tested this? Thanks, Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
Currently, all task switches check privileges against the DPL of the
TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
the DPL of this take gate is used for the check instead. Exceptions,
external interrupts and iret shouldn't perform any check.
This patch fixes the problem for VMX. For SVM, the logic used to
determine the source of the task switch is buggy, so we can't pass
useful information to the emulator there and just disable the check in
all cases.
Is this accurate description? Since on SVM you never get
TASK_SWITCH_GATE reason the check is always done using TSS dpl, not
disabled in all cases, correct?
Signed-off-by: Kevin Wolf kw...@redhat.com
---
arch/x86/include/asm/kvm_emulate.h |2 +-
arch/x86/include/asm/kvm_host.h|4 +-
arch/x86/kvm/emulate.c | 51
+++-
arch/x86/kvm/svm.c |3 +-
arch/x86/kvm/vmx.c |5 ++-
arch/x86/kvm/x86.c |6 ++--
6 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index ab4092e..c8a9cf3 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt
*ctxt);
#define EMULATION_INTERCEPTED 2
int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
- u16 tss_selector, int reason,
+ u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code);
int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
#endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..0533fc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int
seg);
int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int
seg);
-int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
- bool has_error_code, u32 error_code);
+int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
+ int reason, bool has_error_code, u32 error_code);
int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 05a562b..1b98a2c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt
*ctxt,
return 1;
}
+static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
+ u16 index, struct kvm_desc_struct *desc)
+{
+ struct kvm_desc_ptr dt;
+ ulong addr;
+
+ ctxt-ops-get_idt(ctxt, dt);
+
+ if (dt.size index * 8 + 7)
+ return emulate_gp(ctxt, index 3 | 0x2);
+
+ addr = dt.address + index * 8;
+ return ctxt-ops-read_std(ctxt, addr, desc, sizeof *desc,
+ctxt-exception);
+}
+
static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
u16 selector, struct desc_ptr *dt)
{
@@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
}
static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
-u16 tss_selector, int reason,
+u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code)
{
struct x86_emulate_ops *ops = ctxt-ops;
@@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
ulong old_tss_base =
ops-get_cached_segment_base(ctxt, VCPU_SREG_TR);
u32 desc_limit;
+ int dpl;
/* FIXME: old_tss_base == ~0 ? */
@@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
/* FIXME: check that next_tss_desc is tss */
- if (reason != TASK_SWITCH_IRET) {
- if ((tss_selector 3) next_tss_desc.dpl ||
- ops-cpl(ctxt) next_tss_desc.dpl)
- return emulate_gp(ctxt, 0);
+ /*
+ * Check privileges. The three cases are task switch caused by...
+ *
+ * 1. Software interrupt: Check against DPL of the task gate
+ * 2. Exception/IRQ/iret: No check is performed
+ * 3. jmp/call: Check agains DPL of the TSS
+ */
+ dpl = -1;
+ if (reason == TASK_SWITCH_GATE) {
+ if
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 24.01.2012 10:52, schrieb Gleb Natapov:
On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote:
Currently, all task switches check privileges against the DPL of the
TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
the DPL of this take gate is used for the check instead. Exceptions,
external interrupts and iret shouldn't perform any check.
This patch fixes the problem for VMX. For SVM, the logic used to
determine the source of the task switch is buggy, so we can't pass
useful information to the emulator there and just disable the check in
all cases.
Is this accurate description? Since on SVM you never get
TASK_SWITCH_GATE reason the check is always done using TSS dpl, not
disabled in all cases, correct?
Hm, right, they turn out as TASK_SWITCH_CALL, so we check against the
TSS. I'll fix the commit message if I need to do a v2.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
arch/x86/include/asm/kvm_emulate.h |2 +-
arch/x86/include/asm/kvm_host.h|4 +-
arch/x86/kvm/emulate.c | 51
+++-
arch/x86/kvm/svm.c |3 +-
arch/x86/kvm/vmx.c |5 ++-
arch/x86/kvm/x86.c |6 ++--
6 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index ab4092e..c8a9cf3 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt
*ctxt);
#define EMULATION_INTERCEPTED 2
int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
- u16 tss_selector, int reason,
+ u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code);
int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
#endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/include/asm/kvm_host.h
b/arch/x86/include/asm/kvm_host.h
index 52d6640..0533fc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int
seg);
int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int
seg);
-int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
-bool has_error_code, u32 error_code);
+int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
+int reason, bool has_error_code, u32 error_code);
int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 05a562b..1b98a2c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt
*ctxt,
return 1;
}
+static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
+ u16 index, struct kvm_desc_struct *desc)
+{
+struct kvm_desc_ptr dt;
+ulong addr;
+
+ctxt-ops-get_idt(ctxt, dt);
+
+if (dt.size index * 8 + 7)
+return emulate_gp(ctxt, index 3 | 0x2);
+
+addr = dt.address + index * 8;
+return ctxt-ops-read_std(ctxt, addr, desc, sizeof *desc,
+ ctxt-exception);
+}
+
static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
u16 selector, struct desc_ptr *dt)
{
@@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt
*ctxt,
}
static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
- u16 tss_selector, int reason,
+ u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code)
{
struct x86_emulate_ops *ops = ctxt-ops;
@@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
ulong old_tss_base =
ops-get_cached_segment_base(ctxt, VCPU_SREG_TR);
u32 desc_limit;
+int dpl;
/* FIXME: old_tss_base == ~0 ? */
@@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
/* FIXME: check that next_tss_desc is tss */
-if (reason != TASK_SWITCH_IRET) {
-if ((tss_selector 3) next_tss_desc.dpl ||
-ops-cpl(ctxt) next_tss_desc.dpl)
-return emulate_gp(ctxt, 0);
+/*
+ * Check privileges. The three cases are task switch caused by...
+ *
+ * 1. Software interrupt: Check against DPL of the task gate
+ * 2. Exception/IRQ/iret: No check is performed
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
+ } else if (reason != TASK_SWITCH_IRET) {
+ dpl = next_tss_desc.dpl;
}
No need parentheses around one statement.
Documentation/CodingStyle says:
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:
Then you need to put parentheses around if (reason != TASK_SWITCH_IRET)
if you want to follow the letter of the CodingStyle :)
But I do not see this coding stile part widely used in core kernel code:
$ git grep } else$ kernel | wc -l
122
Can't think of re to check when the rule is followed :(
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 24.01.2012 11:17, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
+ } else if (reason != TASK_SWITCH_IRET) {
+ dpl = next_tss_desc.dpl;
}
No need parentheses around one statement.
Documentation/CodingStyle says:
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:
Then you need to put parentheses around if (reason != TASK_SWITCH_IRET)
if you want to follow the letter of the CodingStyle :)
Not sure what you mean. If it is 'else { if (...) { ... then no, the
document isn't crazy like that.
But I do not see this coding stile part widely used in core kernel code:
$ git grep } else$ kernel | wc -l
122
Can't think of re to check when the rule is followed :(
Seem to be at least 77 occurences (git grep -A 2 } else { into a file
as git grep doesn't seem to do multi-line expressions and then on that
file } else {\n.*\n.*}$)
But anyway, I don't really want to discuss the right coding style here
and I'll apply whatever is considered right. Though if you think that
checkpatch.pl and Documentation/CodingStyle are both wrong, please get
them fixed. People might take them serious.
Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
Am 24.01.2012 11:17, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
+} else if (reason != TASK_SWITCH_IRET) {
+dpl = next_tss_desc.dpl;
}
No need parentheses around one statement.
Documentation/CodingStyle says:
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:
Then you need to put parentheses around if (reason != TASK_SWITCH_IRET)
if you want to follow the letter of the CodingStyle :)
Not sure what you mean. If it is 'else { if (...) { ... then no, the
document isn't crazy like that.
The document says:
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
So I do not see how you can interpret it otherwise.
But I do not see this coding stile part widely used in core kernel code:
$ git grep } else$ kernel | wc -l
122
Can't think of re to check when the rule is followed :(
Seem to be at least 77 occurences (git grep -A 2 } else { into a file
as git grep doesn't seem to do multi-line expressions and then on that
file } else {\n.*\n.*}$)
But anyway, I don't really want to discuss the right coding style here
and I'll apply whatever is considered right. Though if you think that
checkpatch.pl and Documentation/CodingStyle are both wrong, please get
them fixed. People might take them serious.
The code looked strange for kernel code. If checkpatch.pl complains about
missing parentheses then they should of course stay. Our interpretation
of CodingStyle is different.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 24.01.2012 11:52, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
Am 24.01.2012 11:17, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
+} else if (reason != TASK_SWITCH_IRET) {
+dpl = next_tss_desc.dpl;
}
No need parentheses around one statement.
Documentation/CodingStyle says:
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:
Then you need to put parentheses around if (reason != TASK_SWITCH_IRET)
if you want to follow the letter of the CodingStyle :)
Not sure what you mean. If it is 'else { if (...) { ... then no, the
document isn't crazy like that.
The document says:
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
So I do not see how you can interpret it otherwise.
Well, I just read more than only one paragraph. It further says:
Note that the closing brace is empty on a line of its own, _except_ in
the cases where it is followed by a continuation of the same statement,
ie a while in a do-statement or an else in an if-statement, like
this:
if (x == y) {
..
} else if (x y) {
...
} else {
}
But I do not see this coding stile part widely used in core kernel code:
$ git grep } else$ kernel | wc -l
122
Can't think of re to check when the rule is followed :(
Seem to be at least 77 occurences (git grep -A 2 } else { into a file
as git grep doesn't seem to do multi-line expressions and then on that
file } else {\n.*\n.*}$)
But anyway, I don't really want to discuss the right coding style here
and I'll apply whatever is considered right. Though if you think that
checkpatch.pl and Documentation/CodingStyle are both wrong, please get
them fixed. People might take them serious.
The code looked strange for kernel code. If checkpatch.pl complains about
missing parentheses then they should of course stay. Our interpretation
of CodingStyle is different.
checkpatch.pl accepts both ways, and as the counts above show they are
both used in core kernel code. If Avi or Marcelo want to have it changed
anyway, I'll resend.
Kevin
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 12:23:07PM +0100, Kevin Wolf wrote:
Am 24.01.2012 11:52, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:38:24AM +0100, Kevin Wolf wrote:
Am 24.01.2012 11:17, schrieb Gleb Natapov:
On Tue, Jan 24, 2012 at 11:09:09AM +0100, Kevin Wolf wrote:
+ } else if (reason != TASK_SWITCH_IRET) {
+ dpl = next_tss_desc.dpl;
}
No need parentheses around one statement.
Documentation/CodingStyle says:
This does not apply if only one branch of a conditional statement is a
single statement; in the latter case use braces in both branches:
Then you need to put parentheses around if (reason != TASK_SWITCH_IRET)
if you want to follow the letter of the CodingStyle :)
Not sure what you mean. If it is 'else { if (...) { ... then no, the
document isn't crazy like that.
The document says:
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
So I do not see how you can interpret it otherwise.
Well, I just read more than only one paragraph. It further says:
Note that the closing brace is empty on a line of its own, _except_ in
the cases where it is followed by a continuation of the same statement,
ie a while in a do-statement or an else in an if-statement, like
this:
if (x == y) {
..
} else if (x y) {
...
} else {
}
I saw that, but here it talks about something different: if closing
brace should be the only thing on a line or not. It does not specify how
much statements ... represents. Example is chosen to show the point
paragraph before it tries to make and for that you need braces.
But I do not see this coding stile part widely used in core kernel code:
$ git grep } else$ kernel | wc -l
122
Can't think of re to check when the rule is followed :(
Seem to be at least 77 occurences (git grep -A 2 } else { into a file
as git grep doesn't seem to do multi-line expressions and then on that
file } else {\n.*\n.*}$)
But anyway, I don't really want to discuss the right coding style here
and I'll apply whatever is considered right. Though if you think that
checkpatch.pl and Documentation/CodingStyle are both wrong, please get
them fixed. People might take them serious.
The code looked strange for kernel code. If checkpatch.pl complains about
missing parentheses then they should of course stay. Our interpretation
of CodingStyle is different.
checkpatch.pl accepts both ways, and as the counts above show they are
both used in core kernel code. If Avi or Marcelo want to have it changed
anyway, I'll resend.
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote: This patch fixes the problem for VMX. For SVM, the logic used to determine the source of the task switch is buggy, so we can't pass useful information to the emulator there and just disable the check in all cases. Actually, SVM isn't buggy :) For SVM you do not need to do any priviledge checks in software because the hardware already takes care of that. In other words, KVM only gets a task-switch intercept if the priviledges are all checked and correct. Joerg -- AMD Operating System Research Center Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach General Managers: Alberto Bozzo Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 24.01.2012 15:03, schrieb Joerg Roedel: On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote: This patch fixes the problem for VMX. For SVM, the logic used to determine the source of the task switch is buggy, so we can't pass useful information to the emulator there and just disable the check in all cases. Actually, SVM isn't buggy :) For SVM you do not need to do any priviledge checks in software because the hardware already takes care of that. In other words, KVM only gets a task-switch intercept if the priviledges are all checked and correct. Okay, that's good to hear. The current code is still buggy because as Gleb noted it checks against the TSS DPL. We need to disable that check for SVM then. Also all checks for TASK_SWITCH_GATE indicate that something is wrong because it will never happen. Are you going to rewrite task_switch_interception() on top of this series? Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote: Am 24.01.2012 15:03, schrieb Joerg Roedel: On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote: This patch fixes the problem for VMX. For SVM, the logic used to determine the source of the task switch is buggy, so we can't pass useful information to the emulator there and just disable the check in all cases. Actually, SVM isn't buggy :) For SVM you do not need to do any priviledge checks in software because the hardware already takes care of that. In other words, KVM only gets a task-switch intercept if the priviledges are all checked and correct. Okay, that's good to hear. The current code is still buggy because as Gleb noted it checks against the TSS DPL. We need to disable that check for SVM then. Also all checks for TASK_SWITCH_GATE indicate that something is wrong because it will never happen. Not necessary. Currently all checks for TASK_SWITCH_GATE also check for TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to kvm_task_switch(). Are you going to rewrite task_switch_interception() on top of this series? Kevin -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Am 24.01.2012 15:16, schrieb Gleb Natapov: On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote: Am 24.01.2012 15:03, schrieb Joerg Roedel: On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote: This patch fixes the problem for VMX. For SVM, the logic used to determine the source of the task switch is buggy, so we can't pass useful information to the emulator there and just disable the check in all cases. Actually, SVM isn't buggy :) For SVM you do not need to do any priviledge checks in software because the hardware already takes care of that. In other words, KVM only gets a task-switch intercept if the priviledges are all checked and correct. Okay, that's good to hear. The current code is still buggy because as Gleb noted it checks against the TSS DPL. We need to disable that check for SVM then. Also all checks for TASK_SWITCH_GATE indicate that something is wrong because it will never happen. Not necessary. Currently all checks for TASK_SWITCH_GATE also check for TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to kvm_task_switch(). Yes, the emulator itself would be fixed by passing TASK_SWITCH_GATE and idt_index = -1 (although it looks a bit brittle). However, task_switch_interception() itself does some more based on the value of reason, for example it decides whether or not to call skip_emulated_instruction(). Kevin -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
On Tue, Jan 24, 2012 at 03:24:50PM +0100, Kevin Wolf wrote: Am 24.01.2012 15:16, schrieb Gleb Natapov: On Tue, Jan 24, 2012 at 03:15:13PM +0100, Kevin Wolf wrote: Am 24.01.2012 15:03, schrieb Joerg Roedel: On Mon, Jan 23, 2012 at 05:10:46PM +0100, Kevin Wolf wrote: This patch fixes the problem for VMX. For SVM, the logic used to determine the source of the task switch is buggy, so we can't pass useful information to the emulator there and just disable the check in all cases. Actually, SVM isn't buggy :) For SVM you do not need to do any priviledge checks in software because the hardware already takes care of that. In other words, KVM only gets a task-switch intercept if the priviledges are all checked and correct. Okay, that's good to hear. The current code is still buggy because as Gleb noted it checks against the TSS DPL. We need to disable that check for SVM then. Also all checks for TASK_SWITCH_GATE indicate that something is wrong because it will never happen. Not necessary. Currently all checks for TASK_SWITCH_GATE also check for TASK_SWITCH_CALL, so I think you can fix SVM case in your patch by passing TASK_SWITCH_GATE instead of TASK_SWITCH_CALL to kvm_task_switch(). Yes, the emulator itself would be fixed by passing TASK_SWITCH_GATE and idt_index = -1 (although it looks a bit brittle). I think it is OK. Emulator should emulate the spec and if workaround is needed it should be in platform code. By passing TASK_SWITCH_GATE and idt_index = -1 we do exactly that. However, task_switch_interception() itself does some more based on the value of reason, for example it decides whether or not to call skip_emulated_instruction(). Joerg need to help us here. If intercept of task switch happens before rip is advanced past instruction that cause it we have to know somehow that task switch was caused by instruction. It is not enough that HW checks permission, we still lack essential info. -- Gleb. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] KVM: x86 emulator: Fix task switch privilege checks
Currently, all task switches check privileges against the DPL of the
TSS. This is only correct for jmp/call to a TSS. If a task gate is used,
the DPL of this take gate is used for the check instead. Exceptions,
external interrupts and iret shouldn't perform any check.
This patch fixes the problem for VMX. For SVM, the logic used to
determine the source of the task switch is buggy, so we can't pass
useful information to the emulator there and just disable the check in
all cases.
Signed-off-by: Kevin Wolf kw...@redhat.com
---
arch/x86/include/asm/kvm_emulate.h |2 +-
arch/x86/include/asm/kvm_host.h|4 +-
arch/x86/kvm/emulate.c | 51 +++-
arch/x86/kvm/svm.c |3 +-
arch/x86/kvm/vmx.c |5 ++-
arch/x86/kvm/x86.c |6 ++--
6 files changed, 55 insertions(+), 16 deletions(-)
diff --git a/arch/x86/include/asm/kvm_emulate.h
b/arch/x86/include/asm/kvm_emulate.h
index ab4092e..c8a9cf3 100644
--- a/arch/x86/include/asm/kvm_emulate.h
+++ b/arch/x86/include/asm/kvm_emulate.h
@@ -372,7 +372,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt
*ctxt);
#define EMULATION_INTERCEPTED 2
int x86_emulate_insn(struct x86_emulate_ctxt *ctxt);
int emulator_task_switch(struct x86_emulate_ctxt *ctxt,
-u16 tss_selector, int reason,
+u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code);
int emulate_int_real(struct x86_emulate_ctxt *ctxt, int irq);
#endif /* _ASM_X86_KVM_X86_EMULATE_H */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 52d6640..0533fc4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -741,8 +741,8 @@ int kvm_emulate_wbinvd(struct kvm_vcpu *vcpu);
void kvm_get_segment(struct kvm_vcpu *vcpu, struct kvm_segment *var, int seg);
int kvm_load_segment_descriptor(struct kvm_vcpu *vcpu, u16 selector, int seg);
-int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int reason,
- bool has_error_code, u32 error_code);
+int kvm_task_switch(struct kvm_vcpu *vcpu, u16 tss_selector, int idt_index,
+ int reason, bool has_error_code, u32 error_code);
int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0);
int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3);
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index 05a562b..1b98a2c 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1151,6 +1151,22 @@ static int pio_in_emulated(struct x86_emulate_ctxt *ctxt,
return 1;
}
+static int read_interrupt_descriptor(struct x86_emulate_ctxt *ctxt,
+u16 index, struct kvm_desc_struct *desc)
+{
+ struct kvm_desc_ptr dt;
+ ulong addr;
+
+ ctxt-ops-get_idt(ctxt, dt);
+
+ if (dt.size index * 8 + 7)
+ return emulate_gp(ctxt, index 3 | 0x2);
+
+ addr = dt.address + index * 8;
+ return ctxt-ops-read_std(ctxt, addr, desc, sizeof *desc,
+ ctxt-exception);
+}
+
static void get_descriptor_table_ptr(struct x86_emulate_ctxt *ctxt,
u16 selector, struct desc_ptr *dt)
{
@@ -2350,7 +2366,7 @@ static int task_switch_32(struct x86_emulate_ctxt *ctxt,
}
static int emulator_do_task_switch(struct x86_emulate_ctxt *ctxt,
- u16 tss_selector, int reason,
+ u16 tss_selector, int idt_index, int reason,
bool has_error_code, u32 error_code)
{
struct x86_emulate_ops *ops = ctxt-ops;
@@ -2360,6 +2376,7 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
ulong old_tss_base =
ops-get_cached_segment_base(ctxt, VCPU_SREG_TR);
u32 desc_limit;
+ int dpl;
/* FIXME: old_tss_base == ~0 ? */
@@ -2372,12 +2389,32 @@ static int emulator_do_task_switch(struct
x86_emulate_ctxt *ctxt,
/* FIXME: check that next_tss_desc is tss */
- if (reason != TASK_SWITCH_IRET) {
- if ((tss_selector 3) next_tss_desc.dpl ||
- ops-cpl(ctxt) next_tss_desc.dpl)
- return emulate_gp(ctxt, 0);
+ /*
+* Check privileges. The three cases are task switch caused by...
+*
+* 1. Software interrupt: Check against DPL of the task gate
+* 2. Exception/IRQ/iret: No check is performed
+* 3. jmp/call: Check agains DPL of the TSS
+*/
+ dpl = -1;
+ if (reason == TASK_SWITCH_GATE) {
+ if (idt_index != -1) {
+ struct kvm_desc_struct task_gate_desc;
+
+ ret = read_interrupt_descriptor(ctxt, idt_index,
+ task_gate_desc);
+ if
