| Message ID | 20140425171718.GA1591@morn.localdomain |
|---|---|
| State | New |
| Headers | show |
Il 25/04/2014 19:17, Kevin O'Connor ha scritto: > The current SMI interrupt handler is being run with the same CPL as > the code it interrupts. If the existing code is running with CPL=3, > then the SMI handler can cause spurious exceptions. The System > Management Mode (SMM) should always run at the highest protection > level. KVM computes the CPL as follows: if (CR0.PE == 0) return 0; if (!EFER.LMA && EFLAGS.VM) return 3; return CS.selector & 3; Perhaps this can be used instead of save/restore of the CPL field. Paolo > Signed-off-by: Kevin O'Connor <kevin@koconnor.net> > --- > > I'm not very familiar with the QEMU TCG code, so it is possible there > is a better way to accomplish this. I can confirm that without this > patch an extended SeaBIOS SMI handler can cause spurious faults, but > it works correctly with this patch. > > --- > target-i386/smm_helper.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c > index 35901c9..ad5abf2 100644 > --- a/target-i386/smm_helper.c > +++ b/target-i386/smm_helper.c > @@ -66,6 +66,7 @@ void do_smm_enter(X86CPU *cpu) > stq_phys(cs->as, sm_state + offset + 8, dt->base); > } > > + stw_phys(cs->as, sm_state + 0x7e62, env->hflags & HF_CPL_MASK); > stq_phys(cs->as, sm_state + 0x7e68, env->gdt.base); > stl_phys(cs->as, sm_state + 0x7e64, env->gdt.limit); > > @@ -134,6 +135,7 @@ void do_smm_enter(X86CPU *cpu) > > stl_phys(cs->as, sm_state + 0x7f74, env->gdt.base); > stl_phys(cs->as, sm_state + 0x7f70, env->gdt.limit); > + stw_phys(cs->as, sm_state + 0x7f6e, env->hflags & HF_CPL_MASK); > > stl_phys(cs->as, sm_state + 0x7f58, env->idt.base); > stl_phys(cs->as, sm_state + 0x7f54, env->idt.limit); > @@ -163,6 +165,7 @@ void do_smm_enter(X86CPU *cpu) > cpu_load_eflags(env, 0, ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | > DF_MASK)); > env->eip = 0x00008000; > + cpu_x86_set_cpl(env, 0); > cpu_x86_load_seg_cache(env, R_CS, (env->smbase >> 4) & 0xffff, env->smbase, > 0xffffffff, 0); > cpu_x86_load_seg_cache(env, R_DS, 0, 0, 0xffffffff, 0); > @@ -201,6 +204,7 @@ void helper_rsm(CPUX86State *env) > 0xf0ff) << 8); > } > > + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7e62) & HF_CPL_MASK); > env->gdt.base = ldq_phys(cs->as, sm_state + 0x7e68); > env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7e64); > > @@ -271,6 +275,7 @@ void helper_rsm(CPUX86State *env) > > env->gdt.base = ldl_phys(cs->as, sm_state + 0x7f74); > env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7f70); > + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7f6e) & HF_CPL_MASK); > > env->idt.base = ldl_phys(cs->as, sm_state + 0x7f58); > env->idt.limit = ldl_phys(cs->as, sm_state + 0x7f54); >
On Sat, 2014-04-26 at 11:06 +0200, Paolo Bonzini wrote: > Il 25/04/2014 19:17, Kevin O'Connor ha scritto: > > The current SMI interrupt handler is being run with the same CPL as > > the code it interrupts. If the existing code is running with CPL=3, > > then the SMI handler can cause spurious exceptions. The System > > Management Mode (SMM) should always run at the highest protection > > level. > > KVM computes the CPL as follows: > > if (CR0.PE == 0) > return 0; > > if (!EFER.LMA && EFLAGS.VM) > return 3; > > return CS.selector & 3; Hi Paolo, The above algorithm is correct only for the protected mode, right? For real-address mode is not correct (taken from the Intel Dev Manual and not from my limited knowledge). Why don't we use the value of the DPL field from SS which is always equal to the logical processor’s CPL? Of course, there is only a short period of time the processor is not on protected mode, but in this time is is possible that the CS segment selector is changed and the CPL with it... Any thoughts? Makes sense to change the way the KVM computes the CPL? Thanks, Marcel > > Perhaps this can be used instead of save/restore of the CPL field. > > Paolo > > > Signed-off-by: Kevin O'Connor <kevin@koconnor.net> > > --- > > > > I'm not very familiar with the QEMU TCG code, so it is possible there > > is a better way to accomplish this. I can confirm that without this > > patch an extended SeaBIOS SMI handler can cause spurious faults, but > > it works correctly with this patch. > > > > --- > > target-i386/smm_helper.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c > > index 35901c9..ad5abf2 100644 > > --- a/target-i386/smm_helper.c > > +++ b/target-i386/smm_helper.c > > @@ -66,6 +66,7 @@ void do_smm_enter(X86CPU *cpu) > > stq_phys(cs->as, sm_state + offset + 8, dt->base); > > } > > > > + stw_phys(cs->as, sm_state + 0x7e62, env->hflags & HF_CPL_MASK); > > stq_phys(cs->as, sm_state + 0x7e68, env->gdt.base); > > stl_phys(cs->as, sm_state + 0x7e64, env->gdt.limit); > > > > @@ -134,6 +135,7 @@ void do_smm_enter(X86CPU *cpu) > > > > stl_phys(cs->as, sm_state + 0x7f74, env->gdt.base); > > stl_phys(cs->as, sm_state + 0x7f70, env->gdt.limit); > > + stw_phys(cs->as, sm_state + 0x7f6e, env->hflags & HF_CPL_MASK); > > > > stl_phys(cs->as, sm_state + 0x7f58, env->idt.base); > > stl_phys(cs->as, sm_state + 0x7f54, env->idt.limit); > > @@ -163,6 +165,7 @@ void do_smm_enter(X86CPU *cpu) > > cpu_load_eflags(env, 0, ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | > > DF_MASK)); > > env->eip = 0x00008000; > > + cpu_x86_set_cpl(env, 0); > > cpu_x86_load_seg_cache(env, R_CS, (env->smbase >> 4) & 0xffff, env->smbase, > > 0xffffffff, 0); > > cpu_x86_load_seg_cache(env, R_DS, 0, 0, 0xffffffff, 0); > > @@ -201,6 +204,7 @@ void helper_rsm(CPUX86State *env) > > 0xf0ff) << 8); > > } > > > > + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7e62) & HF_CPL_MASK); > > env->gdt.base = ldq_phys(cs->as, sm_state + 0x7e68); > > env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7e64); > > > > @@ -271,6 +275,7 @@ void helper_rsm(CPUX86State *env) > > > > env->gdt.base = ldl_phys(cs->as, sm_state + 0x7f74); > > env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7f70); > > + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7f6e) & HF_CPL_MASK); > > > > env->idt.base = ldl_phys(cs->as, sm_state + 0x7f58); > > env->idt.limit = ldl_phys(cs->as, sm_state + 0x7f54); > > > >
Il 27/04/2014 14:22, Marcel Apfelbaum ha scritto: > On Sat, 2014-04-26 at 11:06 +0200, Paolo Bonzini wrote: >> Il 25/04/2014 19:17, Kevin O'Connor ha scritto: >>> The current SMI interrupt handler is being run with the same CPL as >>> the code it interrupts. If the existing code is running with CPL=3, >>> then the SMI handler can cause spurious exceptions. The System >>> Management Mode (SMM) should always run at the highest protection >>> level. >> >> KVM computes the CPL as follows: >> >> if (CR0.PE == 0) >> return 0; >> >> if (!EFER.LMA && EFLAGS.VM) >> return 3; >> >> return CS.selector & 3; > > Hi Paolo, > > The above algorithm is correct only for the protected mode, right? The CR0.PE == 0 case is for real mode. You're right that for the real->protected transition time CS.selector's low 3 bits can be anything, while CR0.PE is already 1 *and* CPL is still zero. Kevin's patch should handle this right for TCG, but there may be indeed a KVM bug looming. > For real-address mode is not correct (taken from the Intel Dev Manual > and not from my limited knowledge). > Why don't we use the value of the DPL field from SS which is always > equal to the logical processor’s CPL? The Intel manual says the CPL is "the protection level of the currently executing code segment". CS.DPL is indeed != CPL for conforming code segments. > Of course, there is only a short period of time the processor is not on protected > mode, but in this time is is possible that the CS segment selector is changed > and the CPL with it... > > Any thoughts? Makes sense to change the way the KVM computes the CPL? If it ain't broken... :) but perhaps it is broken. Paolo
On Sun, Apr 27, 2014 at 04:29:25PM +0200, Paolo Bonzini wrote: > Il 27/04/2014 14:22, Marcel Apfelbaum ha scritto: > >On Sat, 2014-04-26 at 11:06 +0200, Paolo Bonzini wrote: > >>KVM computes the CPL as follows: > >> > >>if (CR0.PE == 0) > >> return 0; > >> > >>if (!EFER.LMA && EFLAGS.VM) > >> return 3; > >> > >>return CS.selector & 3; > > > >The above algorithm is correct only for the protected mode, right? > > The CR0.PE == 0 case is for real mode. > > You're right that for the real->protected transition time > CS.selector's low 3 bits can be anything, while CR0.PE is already 1 > *and* CPL is still zero. Kevin's patch should handle this right for > TCG, but there may be indeed a KVM bug looming. I was wondering about that as well. The Intel docs state that the CPL is bits 0-1 of the CS.selector register, and that protected mode starts immediately after setting the PE bit. The CS.selector field should be the value of %cs in real mode, which is the value added to eip (after shifting right by 4). I guess that means that the real mode code that enables the PE bit must run with a code segment aligned to a value of 4. (Which effectively means code alignment of 64 bytes because of the segment shift.) -Kevin
diff --git a/target-i386/smm_helper.c b/target-i386/smm_helper.c index 35901c9..ad5abf2 100644 --- a/target-i386/smm_helper.c +++ b/target-i386/smm_helper.c @@ -66,6 +66,7 @@ void do_smm_enter(X86CPU *cpu) stq_phys(cs->as, sm_state + offset + 8, dt->base); } + stw_phys(cs->as, sm_state + 0x7e62, env->hflags & HF_CPL_MASK); stq_phys(cs->as, sm_state + 0x7e68, env->gdt.base); stl_phys(cs->as, sm_state + 0x7e64, env->gdt.limit); @@ -134,6 +135,7 @@ void do_smm_enter(X86CPU *cpu) stl_phys(cs->as, sm_state + 0x7f74, env->gdt.base); stl_phys(cs->as, sm_state + 0x7f70, env->gdt.limit); + stw_phys(cs->as, sm_state + 0x7f6e, env->hflags & HF_CPL_MASK); stl_phys(cs->as, sm_state + 0x7f58, env->idt.base); stl_phys(cs->as, sm_state + 0x7f54, env->idt.limit); @@ -163,6 +165,7 @@ void do_smm_enter(X86CPU *cpu) cpu_load_eflags(env, 0, ~(CC_O | CC_S | CC_Z | CC_A | CC_P | CC_C | DF_MASK)); env->eip = 0x00008000; + cpu_x86_set_cpl(env, 0); cpu_x86_load_seg_cache(env, R_CS, (env->smbase >> 4) & 0xffff, env->smbase, 0xffffffff, 0); cpu_x86_load_seg_cache(env, R_DS, 0, 0, 0xffffffff, 0); @@ -201,6 +204,7 @@ void helper_rsm(CPUX86State *env) 0xf0ff) << 8); } + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7e62) & HF_CPL_MASK); env->gdt.base = ldq_phys(cs->as, sm_state + 0x7e68); env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7e64); @@ -271,6 +275,7 @@ void helper_rsm(CPUX86State *env) env->gdt.base = ldl_phys(cs->as, sm_state + 0x7f74); env->gdt.limit = ldl_phys(cs->as, sm_state + 0x7f70); + cpu_x86_set_cpl(env, lduw_phys(cs->as, sm_state + 0x7f6e) & HF_CPL_MASK); env->idt.base = ldl_phys(cs->as, sm_state + 0x7f58); env->idt.limit = ldl_phys(cs->as, sm_state + 0x7f54);
The current SMI interrupt handler is being run with the same CPL as the code it interrupts. If the existing code is running with CPL=3, then the SMI handler can cause spurious exceptions. The System Management Mode (SMM) should always run at the highest protection level. Signed-off-by: Kevin O'Connor <kevin@koconnor.net> --- I'm not very familiar with the QEMU TCG code, so it is possible there is a better way to accomplish this. I can confirm that without this patch an extended SeaBIOS SMI handler can cause spurious faults, but it works correctly with this patch. --- target-i386/smm_helper.c | 5 +++++ 1 file changed, 5 insertions(+)