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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2026 18:48, 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)); \

But a cast expression isn't an lvalue, so & cannot be applied to it (much
like it can't be applied to (x + y) as you mentioned above).

Jan

>               if ( rc ) \
>               { \
>                   pv_inject_page_fault(PFEC_write_access, \
> 
> 
>> Can also be fixed on commit.
>>
>> ~Andrew
> 
> Teddy