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

[John Snow <jsnow@redhat.com>]

CC'ing X86 maintainers.

On 03/04/2015 12:48 PM, Bill Paul wrote:
> 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,
>
> wpaul@windriver.com | Master of Unix-Fu - Wind River Systems
>
> =============================================================================
>
> "I put a dollar in a change machine. Nothing changed." - George Carlin
>
> =============================================================================
>
> Signed-off-by: Bill Paul <wpaul@windriver.com>
>
> ---
>
> 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
>