[PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Jan Beulich]

When invoked by compat mode, mode_64bit() will be false at the start of
emulation. The logic after complete_insn, however, needs to consider the
mode switched into, in particular to avoid truncating RIP.

Inspired by / paralleling and extending Linux commit 943dea8af21b ("KVM:
x86: Update emulator context mode if SYSENTER xfers to 64-bit mode").

While there, tighten a related assertion in x86_emulate_wrapper() - we
want to be sure to not switch into an impossible mode when the code gets
built for 32-bit only (as is possible for the test harness).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
In principle we could drop SYSENTER's ctxt->lma dependency when setting
_regs.r(ip). I wasn't certain whether leaving it as is serves as kind of
documentation ...

--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -6127,6 +6127,10 @@ x86_emulate(
              (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
             goto done;
 
+        if ( ctxt->lma )
+            /* In particular mode_64bit() needs to return true from here on. */
+            ctxt->addr_size = ctxt->sp_size = 64;
+
         /*
          * SYSCALL (unlike most instructions) evaluates its singlestep action
          * based on the resulting EFLAGS.TF, not the starting EFLAGS.TF.
@@ -6927,6 +6931,10 @@ x86_emulate(
                                       ctxt)) != X86EMUL_OKAY )
             goto done;
 
+        if ( ctxt->lma )
+            /* In particular mode_64bit() needs to return true from here on. */
+            ctxt->addr_size = ctxt->sp_size = 64;
+
         singlestep = _regs.eflags & X86_EFLAGS_TF;
         break;
 
@@ -12113,8 +12121,12 @@ int x86_emulate_wrapper(
     unsigned long orig_ip = ctxt->regs->r(ip);
     int rc;
 
+#ifdef __x86_64__
     if ( mode_64bit() )
         ASSERT(ctxt->lma);
+#else
+    ASSERT(!ctxt->lma && !mode_64bit());
+#endif
 
     rc = x86_emulate(ctxt, ops);
 

Re: [PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Andrew Cooper]

On 10/02/2021 09:57, Jan Beulich wrote:
> When invoked by compat mode, mode_64bit() will be false at the start of
> emulation. The logic after complete_insn, however, needs to consider the
> mode switched into, in particular to avoid truncating RIP.
>
> Inspired by / paralleling and extending Linux commit 943dea8af21b ("KVM:
> x86: Update emulator context mode if SYSENTER xfers to 64-bit mode").
>
> While there, tighten a related assertion in x86_emulate_wrapper() - we
> want to be sure to not switch into an impossible mode when the code gets
> built for 32-bit only (as is possible for the test harness).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> In principle we could drop SYSENTER's ctxt->lma dependency when setting
> _regs.r(ip). I wasn't certain whether leaving it as is serves as kind of
> documentation ...
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -6127,6 +6127,10 @@ x86_emulate(
>               (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
>              goto done;
>  
> +        if ( ctxt->lma )
> +            /* In particular mode_64bit() needs to return true from here on. */
> +            ctxt->addr_size = ctxt->sp_size = 64;

I think this is fine as presented, but don't we want the logical
opposite for SYSRET/SYSEXIT ?

We truncate rip suitably already, but don't know what other checks may
appear in the future.

~Andrew

Re: [PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Jan Beulich]

On 10.02.2021 13:28, Andrew Cooper wrote:
> On 10/02/2021 09:57, Jan Beulich wrote:
>> When invoked by compat mode, mode_64bit() will be false at the start of
>> emulation. The logic after complete_insn, however, needs to consider the
>> mode switched into, in particular to avoid truncating RIP.
>>
>> Inspired by / paralleling and extending Linux commit 943dea8af21b ("KVM:
>> x86: Update emulator context mode if SYSENTER xfers to 64-bit mode").
>>
>> While there, tighten a related assertion in x86_emulate_wrapper() - we
>> want to be sure to not switch into an impossible mode when the code gets
>> built for 32-bit only (as is possible for the test harness).
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> In principle we could drop SYSENTER's ctxt->lma dependency when setting
>> _regs.r(ip). I wasn't certain whether leaving it as is serves as kind of
>> documentation ...
>>
>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>> @@ -6127,6 +6127,10 @@ x86_emulate(
>>               (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
>>              goto done;
>>  
>> +        if ( ctxt->lma )
>> +            /* In particular mode_64bit() needs to return true from here on. */
>> +            ctxt->addr_size = ctxt->sp_size = 64;
> 
> I think this is fine as presented, but don't we want the logical
> opposite for SYSRET/SYSEXIT ?
> 
> We truncate rip suitably already,

This is why I left them alone, i.e. ...

> but don't know what other checks may appear in the future.

... I thought we would deal with this if and when such checks
would appear. Just like considered in the post-description
remark, we could drop the conditional part from sysexit's
setting of _regs.r(ip), and _then_ we would indeed need a
respective change there, for the truncation to happen at
complete_insn:.

Jan

Re: [PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Andrew Cooper]

On 10/02/2021 13:54, Jan Beulich wrote:
> On 10.02.2021 13:28, Andrew Cooper wrote:
>> On 10/02/2021 09:57, Jan Beulich wrote:
>>> When invoked by compat mode, mode_64bit() will be false at the start of
>>> emulation. The logic after complete_insn, however, needs to consider the
>>> mode switched into, in particular to avoid truncating RIP.
>>>
>>> Inspired by / paralleling and extending Linux commit 943dea8af21b ("KVM:
>>> x86: Update emulator context mode if SYSENTER xfers to 64-bit mode").
>>>
>>> While there, tighten a related assertion in x86_emulate_wrapper() - we
>>> want to be sure to not switch into an impossible mode when the code gets
>>> built for 32-bit only (as is possible for the test harness).
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> In principle we could drop SYSENTER's ctxt->lma dependency when setting
>>> _regs.r(ip). I wasn't certain whether leaving it as is serves as kind of
>>> documentation ...
>>>
>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>> @@ -6127,6 +6127,10 @@ x86_emulate(
>>>               (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
>>>              goto done;
>>>  
>>> +        if ( ctxt->lma )
>>> +            /* In particular mode_64bit() needs to return true from here on. */
>>> +            ctxt->addr_size = ctxt->sp_size = 64;
>> I think this is fine as presented, but don't we want the logical
>> opposite for SYSRET/SYSEXIT ?
>>
>> We truncate rip suitably already,
> This is why I left them alone, i.e. ...
>
>> but don't know what other checks may appear in the future.
> ... I thought we would deal with this if and when such checks
> would appear.

Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citirix.com>

> Just like considered in the post-description
> remark, we could drop the conditional part from sysexit's
> setting of _regs.r(ip), and _then_ we would indeed need a
> respective change there, for the truncation to happen at
> complete_insn:.

I think it would look odd changing just rip and not rsp truncation.

~Andrew

Re: [PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Jan Beulich]

On 10.02.2021 15:02, Andrew Cooper wrote:
> On 10/02/2021 13:54, Jan Beulich wrote:
>> On 10.02.2021 13:28, Andrew Cooper wrote:
>>> On 10/02/2021 09:57, Jan Beulich wrote:
>>>> When invoked by compat mode, mode_64bit() will be false at the start of
>>>> emulation. The logic after complete_insn, however, needs to consider the
>>>> mode switched into, in particular to avoid truncating RIP.
>>>>
>>>> Inspired by / paralleling and extending Linux commit 943dea8af21b ("KVM:
>>>> x86: Update emulator context mode if SYSENTER xfers to 64-bit mode").
>>>>
>>>> While there, tighten a related assertion in x86_emulate_wrapper() - we
>>>> want to be sure to not switch into an impossible mode when the code gets
>>>> built for 32-bit only (as is possible for the test harness).
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> In principle we could drop SYSENTER's ctxt->lma dependency when setting
>>>> _regs.r(ip). I wasn't certain whether leaving it as is serves as kind of
>>>> documentation ...
>>>>
>>>> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
>>>> @@ -6127,6 +6127,10 @@ x86_emulate(
>>>>               (rc = ops->write_segment(x86_seg_ss, &sreg, ctxt)) )
>>>>              goto done;
>>>>  
>>>> +        if ( ctxt->lma )
>>>> +            /* In particular mode_64bit() needs to return true from here on. */
>>>> +            ctxt->addr_size = ctxt->sp_size = 64;
>>> I think this is fine as presented, but don't we want the logical
>>> opposite for SYSRET/SYSEXIT ?
>>>
>>> We truncate rip suitably already,
>> This is why I left them alone, i.e. ...
>>
>>> but don't know what other checks may appear in the future.
>> ... I thought we would deal with this if and when such checks
>> would appear.
> 
> Ok.  Reviewed-by: Andrew Cooper <andrew.cooper3@citirix.com>

Thanks.

>> Just like considered in the post-description
>> remark, we could drop the conditional part from sysexit's
>> setting of _regs.r(ip), and _then_ we would indeed need a
>> respective change there, for the truncation to happen at
>> complete_insn:.
> 
> I think it would look odd changing just rip and not rsp truncation.

Yes, this was another consideration of mine as well. But it
is a fact that we treat rip and rsp differently in this
regard. Perhaps generated code overall could benefit from
treating rsp more like rip, but this would need careful
looking at all the involved pieces - especially in cases
where the updated stack pointer gets further used we may
not be able to defer the truncation to complete_insn:.

Jan

Re: [PATCH] x86emul: fix SYSENTER/SYSCALL switching into 64-bit mode

[Andrew Cooper]

On 10/02/2021 14:18, Jan Beulich wrote:
> On 10.02.2021 15:02, Andrew Cooper wrote:
>> On 10/02/2021 13:54, Jan Beulich wrote:
>>> Just like considered in the post-description
>>> remark, we could drop the conditional part from sysexit's
>>> setting of _regs.r(ip), and _then_ we would indeed need a
>>> respective change there, for the truncation to happen at
>>> complete_insn:.
>> I think it would look odd changing just rip and not rsp truncation.
> Yes, this was another consideration of mine as well. But it
> is a fact that we treat rip and rsp differently in this
> regard. Perhaps generated code overall could benefit from
> treating rsp more like rip, but this would need careful
> looking at all the involved pieces - especially in cases
> where the updated stack pointer gets further used we may
> not be able to defer the truncation to complete_insn:.

There are other differences.  rip gets updated on every instruction,
while rsp does not.  We also have instructions with (possibly multiple)
rsp-relative memory references which need truncating individually to get
proper behaviour.

~Andrew