Re: [PATCH] pv32: Fix bogus cr2 on fault in emulation gate

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2026 08:33, Jan Beulich wrote:
> On 20.05.2026 19:21, Andrew Cooper wrote:
>> On 20/05/2026 5:48 pm, Teddy Astie wrote:
>>> Le 20/05/2026 à 18:34, Andrew Cooper a écrit :
>>>> On 20/05/2026 4:51 pm, Teddy Astie wrote:
>>>>> __{put,get}_guest returns -EFAULT on access faults which causes
>>>>> the injected cr2 to be off by 14 bytes (as EFAULT is 14) which is
>>>>> incorrect.
>>>>>
>>>>> Fix the computation by relying on copy_{from,to}_guest_pv which
>>>>> reports the number of remaining bytes instead of a negative errno,
>>>>> such that we can compute the offset properly.
>>>>>
>>>>> Fixes: 70ad570b2799 ("x86/64: paravirt 32-on-64 call gate support")
>>>>> Signed-off-by: Teddy Astie <teddy.astie@xxxxxxxxxx>
>>>>> ---
>>>>>   xen/arch/x86/pv/emul-gate-op.c | 5 +++--
>>>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>>>> b/xen/arch/x86/pv/emul-gate-op.c
>>>>> index c2c699fbff..cacc171115 100644
>>>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>>>> @@ -289,9 +289,10 @@ void pv_emulate_gate_op(struct cpu_user_regs
>>>>> *regs)
>>>>>           int rc;
>>>>>   #define push(item) do \
>>>>>           { \
>>>>> +            unsigned int __value = item; \
>>>>>               --stkp; \
>>>>>               esp -= 4; \
>>>>> -            rc = __put_guest(item, stkp); \
>>>>> +            rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>>>
>>>> Oh, this probably violates MISRA, but you don't need to use a separate
>>>> variable because sizeof() has no side effects.
>>>>
>>>> Given that the expression is now &item, I think it needs to be &(item).
>>>>
>>>
>>> I tried something like that, but it looked a bit weird and clang
>>> wasn't happy (at least in language server) because of the &(x + y).
>>>
>>> We also need to ensure that we're actually copying 32-bits scalars
>>> (and not 16-bits or 64-bits ones) like the previous behavior.
>>>
>>> That diff seems to work though
>>>
>>> diff --git a/xen/arch/x86/pv/emul-gate-op.c
>>> b/xen/arch/x86/pv/emul-gate-op.c
>>> index cacc171115..b72a3058dd 100644
>>> --- a/xen/arch/x86/pv/emul-gate-op.c
>>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>>> @@ -289,10 +289,9 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>>          int rc;
>>>  #define push(item) do \
>>>          { \
>>> -            unsigned int __value = item; \
>>>              --stkp; \
>>>              esp -= 4; \
>>> -            rc = copy_to_guest_pv(stkp, &__value, sizeof(__value)); \
>>> +            rc = copy_to_guest_pv(stkp, &(uint32_t)(item),
>>> sizeof(uint32_t)); \
>>>              if ( rc ) \
>>>              { \
>>>                  pv_inject_page_fault(PFEC_write_access, \ 
>>
>> Oh, that's a second bug you're fixing then.
>>
>> Pushes of ss/cs need to be done with 4-byte writes and zero extended.
> 
> And they are: Access size is derived from the pointer passed, not from the
> item.

Oh, while access size has always been correct, ....

>> I've added:
>>
>> The use of a local variable in push() also fixes a second bug.  On all
>> but the earliest 32bit CPUs, segment selectors pushes are
>> zero-extended 32bit stores.  Xen was not doing this for %ss and %cs.

... zero-extension was lost with the FRED work, so a 2nd Fixes: tag is
going to be necessary: cb29eed2dae7 ("x86/traps: Extend struct
cpu_user_regs/cpu_info with FRED fields").

Jan

>> to the commit message.
>>
>> ~Andrew
>