On 2014/12/25 17:55, Nadav Amit wrote:
Tiejun <tiejun.chen@xxxxxxxxx> wrote:
On 2014/12/25 8:52, Nadav Amit wrote:
Although pop sreg updates RSP according to the operand size, only 2 bytes are
read. The current behavior may result in incorrect #GP or #PF exceptions.
Signed-off-by: Nadav Amit <namit@xxxxxxxxxxxxxxxxx>
---
arch/x86/kvm/emulate.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index e5a84be..702da5e 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -1830,12 +1830,14 @@ static int em_pop_sreg(struct x86_emulate_ctxt *ctxt)
unsigned long selector;
int rc;
Looks we just should do similar thing to em_push_sreg(),
unsigned long selector;
int rc;
+ if (ctxt->op_bytes == 4) {
+ rsp_increment(ctxt, -2);
+ ctxt->op_bytes = 2;
+ }
rc = emulate_pop(ctxt, &selector, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
return rc;
Right?
I don’t think so. It seems the behaviour of push and pop is a bit different.
For push: “If the source operand is a segment register (16 bits) and the
operand size is 64-bits, a zero-extended value is pushed on the stack; if
the operand size is 32-bits ... all recent Core and Atom processors perform
a 16-bit move, leaving the upper portion of the stack location unmodified.”
Therefore, for push in the case of op_bytes==8 we push zero-extended value.
For pop the behaviour is not well-documented, but experimentally it appears
only the first two bytes are accessed. I cannot see why it would be
Maybe we can comment something here, like "/* Just force 2 byte
destination to already work well in most cases. */".
different when opsize is 8, since it is not like the push case, where the
segment register value was zero extended.
Thanks for your explanation.
If you feel strongly about it, I’ll create a unit test.
Based on your description I think I can stand with you at this point.
Tiejun
Nadav
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
