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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxx>

> 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