[Qemu-devel] [PATCH] Fix bug in implementation of SYSRET instruction for x86-64

[Bill Paul]

Hi guys. I seem to have found a bug in the helper_systet() function in

target-i386/seg_helper.c. I downloaded the Intel architecture manual from here:

http://www.intel.com/content/www/us/en/processors/architectures-software-developer-manuals.html

And it describes the behavior of SYSRET with regards to the stack selector (SS) as follows:

SS.Selector <-- (IA32_STAR[63:48]+8) OR 3; (* RPL forced to 3 *)

In other words, the value of the SS register is supposed to be loaded from bits 63:48 of the IA32_STAR model-specific register (MSR), incremented by 8, _AND_ ORed with 3. ORing in the 3 sets the privilege level to 3 (user). This is done since SYSRET returns to user mode after a system call.

The code in helper_sysret() performs the "increment by 8" step, but _NOT_ the "OR with 3" step.

Here's another description of the SYSRET instruction which says basically the same thing, though in slightly different format:

http://tptp.cc/mirrors/siyobik.info/instruction/SYSRET.html

[...]

SS(SEL) = IA32_STAR[63:48] + 8;

SS(PL) = 0x3;

[...]

The effect of this bug is that when x86-64 code uses the SYSCALL instruction to enter kernel mode, the SYSRET instruction will return the CPU to user mode with the SS register incorrectly set (the privilege level bits will be 0). In my case, the original SS value for user mode was 0x2B, but after the return from SYSRET, it was changed to 0x28. It took me quite some time to realize this was due to a bug in QEMU rather than my own code.

This caused a problem later when the user-mode code was preempted by an interrupt. At the end of the interrupt handling, an IRET instruction was used to exit the interrupt context, and the IRET instruction would generate a general protection fault because it would attempt to validate the stack selector value and notice that it was set for PL 0 (supervisor) instead of PL 3 (user).

From my reading, the code is quite clearly in error with respect to the Intel documentation, and fixing the bug in my local sources results in my test code (which runs correctly on real hardware) working correctly in QEMU. It seems this bug has been there for a long time -- I happened to have the sources for QEMU 0.10.5 and the bug is there too (in target-i386/op_helper.c). I'm puzzled as to how it's gone unnoticed for so long, which has me thinking that maybe I'm missing something. I'm pretty sure this is wrong though.

I'm including a patch to fix this below. It seems to fix the problem quite nicely on my QEMU 2.2.0 installation. I'm also attaching a separate copy in case my mail client mangles the formatting on the inline copy.

-Bill

--

=============================================================================

-Bill Paul (510) 749-2329 | Senior Member of Technical Staff,

address@hidden | Master of Unix-Fu - Wind River Systems

=============================================================================

"I put a dollar in a change machine. Nothing changed." - George Carlin

=============================================================================

Signed-off-by: Bill Paul <address@hidden>

---

target-i386/seg_helper.c | 4 ++--

1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c

index fa374d0..2bc757a 100644

--- a/target-i386/seg_helper.c

+++ b/target-i386/seg_helper.c

@@ -1043,7 +1043,7 @@ void helper_sysret(CPUX86State *env, int dflag)

DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);

env->eip = (uint32_t)env->regs[R_ECX];

}

- cpu_x86_load_seg_cache(env, R_SS, selector + 8,

+ cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,

0, 0xffffffff,

DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

@@ -1056,7 +1056,7 @@ void helper_sysret(CPUX86State *env, int dflag)

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

DESC_CS_MASK | DESC_R_MASK | DESC_A_MASK);

env->eip = (uint32_t)env->regs[R_ECX];

- cpu_x86_load_seg_cache(env, R_SS, selector + 8,

+ cpu_x86_load_seg_cache(env, R_SS, (selector + 8) | 3,

0, 0xffffffff,

DESC_G_MASK | DESC_B_MASK | DESC_P_MASK |

DESC_S_MASK | (3 << DESC_DPL_SHIFT) |

--

1.8.0

0001-This-checkin-corrects-a-bug-in-the-implementation-of.patch
Description: Text Data