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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.05.2026 12:56, Andrew Cooper wrote:
> 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.

__{get,put}_guest() determine the amount of data to copy from the pointer
they're passed. That being unsigned int *, 32 bits will be copied
regardless of field type. Actually: __put_guest() casts the incoming
value to the type the pointer argument points to. So FRED work didn't
break anything, and I was wrong to ask for a 2nd Fixes: tag. Zero-
extension was and is there.

For __get_guest() et al there looks to be potential of data corruption, if
variable type is less wide than pointed-to type.

Jan