Re: [PATCH 2/8] KVM: x86: pop sreg accesses only 2 bytes

[Nadav Amit <nadav.amit@gmail.com>]

Feng <feng.wu@intel.com> wrote:

> 
> 
>> -----Original Message-----
>> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
>> Behalf Of Nadav Amit
>> Sent: Thursday, December 25, 2014 5:55 PM
>> To: Chen, Tiejun
>> Cc: Paolo Bonzini; kvm list
>> Subject: Re: [PATCH 2/8] KVM: x86: pop sreg accesses only 2 bytes
>> 
>> Tiejun <tiejun.chen@intel.com> 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@cs.technion.ac.il>
>>>> ---
>>>> 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
>> different when opsize is 8, since it is not like the push case, where the
>> segment register value was zero extended.
> 
> Let's take 64-bits operand size as an example. When pushing a segment register, it
> uses zero-extended value, so 8 bytes will be pushed on the stack. When popping it,
> the current code return the top 8 bytes in the stack, and it only uses the lowest 2
> bytes for load_segment_descriptor(). what is the issue here?
The issue I try to solve is that during the emulated write operation of the
pop the read is perform using the wrong size (operand size instead of
segment selector size). As you indicated, the destination register/memory of
the pop instruction will be identical before the fix and after the fix.

However, the emulated read may cause #PF if the operand-size that does not
occur on read hardware. Consider for instance a case in which the operand
size is 8, RSP=0xFFC and the page of [0x1000] is non-present. In this case
POP-SREG should not cause a #PF, yet on KVM it does.

Nadav