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

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

Posted by Teddy Astie 1 day, 7 hours ago

__{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@vates.tech>
---
v2:
 * Don't add variable in push() macro
 * Use uint32_t type for temporary value.

 xen/arch/x86/pv/emul-gate-op.c | 6 ++++--
 1 file changed, 4 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..9c229c46c4 100644
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -286,12 +286,14 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
     if ( !jump )
     {
         unsigned int ss, esp, *stkp;
+        uint32_t value;
         int rc;
 #define push(item) do \
         { \
+            value = item; \
             --stkp; \
             esp -= 4; \
-            rc = __put_guest(item, stkp); \
+            rc = copy_to_guest_pv(stkp, &value, sizeof(value)); \
             if ( rc ) \
             { \
                 pv_inject_page_fault(PFEC_write_access, \
@@ -359,7 +361,7 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
                     unsigned int parm;
 
                     --ustkp;
-                    rc = __get_guest(parm, ustkp);
+                    rc = copy_from_guest_pv(&parm, ustkp, sizeof(parm));
                     if ( rc )
                     {
                         pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc);
-- 
2.52.0



-- 
 | Vates 

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

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

Posted by Andrew Cooper 1 day, 6 hours ago

On 22/05/2026 1:33 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@vates.tech>
> ---
> v2:
>  * Don't add variable in push() macro
>  * Use uint32_t type for temporary value.
>
>  xen/arch/x86/pv/emul-gate-op.c | 6 ++++--
>  1 file changed, 4 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..9c229c46c4 100644
> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -286,12 +286,14 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>      if ( !jump )
>      {
>          unsigned int ss, esp, *stkp;
> +        uint32_t value;
>          int rc;
>  #define push(item) do \
>          { \
> +            value = item; \
>              --stkp; \
>              esp -= 4; \
> -            rc = __put_guest(item, stkp); \
> +            rc = copy_to_guest_pv(stkp, &value, sizeof(value)); \
>              if ( rc ) \
>              { \
>                  pv_inject_page_fault(PFEC_write_access, \
> @@ -359,7 +361,7 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>                      unsigned int parm;
>  
>                      --ustkp;
> -                    rc = __get_guest(parm, ustkp);
> +                    rc = copy_from_guest_pv(&parm, ustkp, sizeof(parm));
>                      if ( rc )
>                      {
>                          pv_inject_page_fault(0, (unsigned long)(ustkp + 1) - rc);

As with v1, __copy_*() rather than copy_*().

I've fixed up and committed (including Jan's suggestion), along with
some improvements to the commit message.

x86/pv32.  "gate emulation" rather than "emulation gate", brackets on
function names.

~Andrew

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

Posted by Jan Beulich 1 day, 7 hours ago

On 22.05.2026 14:33, 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@vates.tech>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
preferably with ...

> --- a/xen/arch/x86/pv/emul-gate-op.c
> +++ b/xen/arch/x86/pv/emul-gate-op.c
> @@ -286,12 +286,14 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>      if ( !jump )
>      {
>          unsigned int ss, esp, *stkp;
> +        uint32_t value;
>          int rc;
>  #define push(item) do \
>          { \
> +            value = item; \

... (item) here. I think we're okay without, but let's play safe.

Likely can be adjusted by the committer.

Jan

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

Posted by Teddy Astie 1 day, 7 hours ago

Le 22/05/2026 à 14:44, Jan Beulich a écrit :
> On 22.05.2026 14:33, 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@vates.tech>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> preferably with ...
> 
>> --- a/xen/arch/x86/pv/emul-gate-op.c
>> +++ b/xen/arch/x86/pv/emul-gate-op.c
>> @@ -286,12 +286,14 @@ void pv_emulate_gate_op(struct cpu_user_regs *regs)
>>       if ( !jump )
>>       {
>>           unsigned int ss, esp, *stkp;
>> +        uint32_t value;
>>           int rc;
>>   #define push(item) do \
>>           { \
>> +            value = item; \
> 
> ... (item) here. I think we're okay without, but let's play safe.
> 
> Likely can be adjusted by the committer.
> 

Looks good to me.

> Jan
> 

Teddy