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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/05/2026 8:00 am, Jan Beulich wrote:
> 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").

I don't understand this comment.

The FRED work added extra fields into %cs/%ss with unions, but the
fields named cs and ss are still uint16_t.  That aspect didn't change.

~Andrew