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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

> 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.
> 
> to the commit message.
> 
> ~Andrew