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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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