diff mbox series

[07/10] target/i386/tcg: Use DPL-level accesses for interrupts and call gates

Message ID 20240710062920.73063-8-pbonzini@redhat.com
State New
Headers show
Series [01/10] target/i386/tcg: Remove SEG_ADDL | expand

Commit Message

Paolo Bonzini July 10, 2024, 6:29 a.m. UTC 
This fixes a bug wherein i386/tcg assumed an interrupt return using
the CALL or JMP instructions were always going from kernel or user mode to
kernel mode, when using a call gate. This assumption is violated if
the call gate has a DPL that is greater than 0.

In addition, the stack accesses should count as explicit, not implicit
("kernel" in QEMU code), so that SMAP is not applied if DPL=3.

Analyzed-by: Robert R. Henry <rrh.henry@gmail.com>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/249
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target/i386/tcg/seg_helper.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Richard Henderson July 10, 2024, 3:57 p.m. UTC | #1 
On 7/9/24 23:29, Paolo Bonzini wrote:
> This fixes a bug wherein i386/tcg assumed an interrupt return using
> the CALL or JMP instructions were always going from kernel or user mode to
> kernel mode, when using a call gate. This assumption is violated if
> the call gate has a DPL that is greater than 0.
> 
> In addition, the stack accesses should count as explicit, not implicit
> ("kernel" in QEMU code), so that SMAP is not applied if DPL=3.
> 
> Analyzed-by: Robert R. Henry<rrh.henry@gmail.com>
> Resolves:https://gitlab.com/qemu-project/qemu/-/issues/249
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
> ---
>   target/i386/tcg/seg_helper.c | 13 ++++++-------
>   1 file changed, 6 insertions(+), 7 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
Michael Tokarev Oct. 18, 2024, 4:02 p.m. UTC | #2 
10.07.2024 09:29, Paolo Bonzini wrote:
> This fixes a bug wherein i386/tcg assumed an interrupt return using
> the CALL or JMP instructions were always going from kernel or user mode to
> kernel mode, when using a call gate. This assumption is violated if
> the call gate has a DPL that is greater than 0.
> 
> In addition, the stack accesses should count as explicit, not implicit
> ("kernel" in QEMU code), so that SMAP is not applied if DPL=3.
> 
> Analyzed-by: Robert R. Henry <rrh.henry@gmail.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/249
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This sounds like qemu-stable material, is it not?

It can be picked up for 9.1.x, but for 9.0 and before it needs a few
other changes in this area, like v9.0.0-2238-g8053862af9 "target/i386/tcg:
Compute MMU index once" and v9.0.0-2236-g059368bcf5 "target/i386/tcg:
Reorg push/pop within seg_helper.c", or it needs a proper backport.

What do you think?

Thanks,

/mjt
Michael Tokarev Oct. 25, 2024, 3:26 p.m. UTC | #3 
18.10.2024 19:02, Michael Tokarev wrote:
> 10.07.2024 09:29, Paolo Bonzini wrote:
>> This fixes a bug wherein i386/tcg assumed an interrupt return using
>> the CALL or JMP instructions were always going from kernel or user mode to
>> kernel mode, when using a call gate. This assumption is violated if
>> the call gate has a DPL that is greater than 0.
>>
>> In addition, the stack accesses should count as explicit, not implicit
>> ("kernel" in QEMU code), so that SMAP is not applied if DPL=3.
>>
>> Analyzed-by: Robert R. Henry <rrh.henry@gmail.com>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/249
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This sounds like qemu-stable material, is it not?
> 
> It can be picked up for 9.1.x, but for 9.0 and before it needs a few
> other changes in this area, like v9.0.0-2238-g8053862af9 "target/i386/tcg:
> Compute MMU index once" and v9.0.0-2236-g059368bcf5 "target/i386/tcg:
> Reorg push/pop within seg_helper.c", or it needs a proper backport.
> 
> What do you think?

A friendly ping/help? :)

Or should I drop this from 9.1.x too?

Thanks,

/mjt
Paolo Bonzini Oct. 25, 2024, 3:28 p.m. UTC | #4 
On Fri, Oct 25, 2024 at 5:26 PM Michael Tokarev <mjt@tls.msk.ru> wrote:
> > It can be picked up for 9.1.x, but for 9.0 and before it needs a few
> > other changes in this area, like v9.0.0-2238-g8053862af9 "target/i386/tcg:
> > Compute MMU index once" and v9.0.0-2236-g059368bcf5 "target/i386/tcg:
> > Reorg push/pop within seg_helper.c", or it needs a proper backport.
> >
> > What do you think?
>
> A friendly ping/help? :)

Hi! No, this is totally ok for 9.1.x; it missed 9.1 but it was already
submitted back then and it's okay to apply it there.

On the other hand, Richard wrote some large cleanup patches to enable
this relatively small patch. The bug has been there for many years, we
can keep it in 9.0.x and earlier.

Thanks,

Paolo
Michael Tokarev Oct. 25, 2024, 3:31 p.m. UTC | #5 
25.10.2024 18:28, Paolo Bonzini wrote:

> Hi! No, this is totally ok for 9.1.x; it missed 9.1 but it was already
> submitted back then and it's okay to apply it there.
> 
> On the other hand, Richard wrote some large cleanup patches to enable
> this relatively small patch. The bug has been there for many years, we
> can keep it in 9.0.x and earlier.

Aha. That makes good sense.

Thank you for the follow-up and for clearing my confusion :)

/mjt
diff mbox series

Patch

diff --git a/target/i386/tcg/seg_helper.c b/target/i386/tcg/seg_helper.c
index 07e3667639a..1430f477c43 100644
--- a/target/i386/tcg/seg_helper.c
+++ b/target/i386/tcg/seg_helper.c
@@ -678,7 +678,7 @@  static void do_interrupt_protected(CPUX86State *env, int intno, int is_int,
 
     sa.env = env;
     sa.ra = 0;
-    sa.mmu_index = cpu_mmu_index_kernel(env);
+    sa.mmu_index = x86_mmu_index_pl(env, dpl);
 
     if (type == 5) {
         /* task gate */
@@ -984,7 +984,7 @@  static void do_interrupt64(CPUX86State *env, int intno, int is_int,
 
     sa.env = env;
     sa.ra = 0;
-    sa.mmu_index = cpu_mmu_index_kernel(env);
+    sa.mmu_index = x86_mmu_index_pl(env, dpl);
     sa.sp_mask = -1;
     sa.ss_base = 0;
     if (dpl < cpl || ist != 0) {
@@ -1119,7 +1119,7 @@  static void do_interrupt_real(CPUX86State *env, int intno, int is_int,
     sa.sp = env->regs[R_ESP];
     sa.sp_mask = 0xffff;
     sa.ss_base = env->segs[R_SS].base;
-    sa.mmu_index = cpu_mmu_index_kernel(env);
+    sa.mmu_index = x86_mmu_index_pl(env, 0);
 
     if (is_int) {
         old_eip = next_eip;
@@ -1583,7 +1583,7 @@  void helper_lcall_real(CPUX86State *env, uint32_t new_cs, uint32_t new_eip,
     sa.sp = env->regs[R_ESP];
     sa.sp_mask = get_sp_mask(env->segs[R_SS].flags);
     sa.ss_base = env->segs[R_SS].base;
-    sa.mmu_index = cpu_mmu_index_kernel(env);
+    sa.mmu_index = x86_mmu_index_pl(env, 0);
 
     if (shift) {
         pushl(&sa, env->segs[R_CS].selector);
@@ -1619,17 +1619,17 @@  void helper_lcall_protected(CPUX86State *env, int new_cs, target_ulong new_eip,
         raise_exception_err_ra(env, EXCP0D_GPF, new_cs & 0xfffc, GETPC());
     }
     cpl = env->hflags & HF_CPL_MASK;
+    dpl = (e2 >> DESC_DPL_SHIFT) & 3;
     LOG_PCALL("desc=%08x:%08x\n", e1, e2);
 
     sa.env = env;
     sa.ra = GETPC();
-    sa.mmu_index = cpu_mmu_index_kernel(env);
+    sa.mmu_index = x86_mmu_index_pl(env, dpl);
 
     if (e2 & DESC_S_MASK) {
         if (!(e2 & DESC_CS_MASK)) {
             raise_exception_err_ra(env, EXCP0D_GPF, new_cs & 0xfffc, GETPC());
         }
-        dpl = (e2 >> DESC_DPL_SHIFT) & 3;
         if (e2 & DESC_C_MASK) {
             /* conforming code segment */
             if (dpl > cpl) {
@@ -1691,7 +1691,6 @@  void helper_lcall_protected(CPUX86State *env, int new_cs, target_ulong new_eip,
     } else {
         /* check gate type */
         type = (e2 >> DESC_TYPE_SHIFT) & 0x1f;
-        dpl = (e2 >> DESC_DPL_SHIFT) & 3;
         rpl = new_cs & 3;
 
 #ifdef TARGET_X86_64