qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: John Snow
Subject: Re: [Qemu-devel] [PATCH] Fix bug in implementation of SYSRET instruction for x86-64
Date: Thu, 05 Mar 2015 12:21:43 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

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,

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]