Re: [Xen-devel] [PATCH] don't pass r12/x16 as reference

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi Stefano,

On 01/13/2018 12:07 AM, Stefano Stabellini wrote:
> r12 and x16 are of different sizes; when passing r12 as a reference to
> do_trap_hypercall on arm64, we end up dereferencing it as a pointer to a
> 64bit value, but actually it isn't.
>
> Instead, pass r12/x16 as values and explicitly overwrite them when
> necessary, using the pointer name.
>
> CID: 1457708
> Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 013c160..3b31917 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1453,6 +1453,7 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>   
>   #ifdef CONFIG_ARM_64
>   #define HYPERCALL_RESULT_REG(r) (r)->x0
> +#define HYPERCALL_NR(r) (r)->x16
>   #define HYPERCALL_ARG1(r) (r)->x0
>   #define HYPERCALL_ARG2(r) (r)->x1
>   #define HYPERCALL_ARG3(r) (r)->x2
> @@ -1461,6 +1462,7 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>   #define HYPERCALL_ARGS(r) (r)->x0, (r)->x1, (r)->x2, (r)->x3, (r)->x4
>   #else
>   #define HYPERCALL_RESULT_REG(r) (r)->r0
> +#define HYPERCALL_NR(r) (r)->r12
>   #define HYPERCALL_ARG1(r) (r)->r0
>   #define HYPERCALL_ARG2(r) (r)->r1
>   #define HYPERCALL_ARG3(r) (r)->r2
> @@ -1469,7 +1471,7 @@ static void do_debug_trap(struct cpu_user_regs *regs, 
> unsigned int code)
>   #define HYPERCALL_ARGS(r) (r)->r0, (r)->r1, (r)->r2, (r)->r3, (r)->r4
>   #endif
>   
> -static void do_trap_hypercall(struct cpu_user_regs *regs, register_t *nr,
> +static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned int nr,
>                                 unsigned long iss)
>   {
>       arm_hypercall_fn_t call = NULL;
> @@ -1479,7 +1481,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, 
> register_t *nr,
>       if ( iss != XEN_HYPERCALL_TAG )
>           domain_crash_synchronous();
>   
> -    if ( *nr >= ARRAY_SIZE(arm_hypercall_table) )
> +    if ( nr >= ARRAY_SIZE(arm_hypercall_table) )
>       {
>           perfc_incr(invalid_hypercalls);
>           HYPERCALL_RESULT_REG(regs) = -ENOSYS;
> @@ -1488,8 +1490,8 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, 
> register_t *nr,
>   
>       current->hcall_preempted = false;
>   
> -    perfc_incra(hypercalls, *nr);
> -    call = arm_hypercall_table[*nr].fn;
> +    perfc_incra(hypercalls, nr);
> +    call = arm_hypercall_table[nr].fn;
>       if ( call == NULL )
>       {
>           HYPERCALL_RESULT_REG(regs) = -ENOSYS;
> @@ -1502,7 +1504,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, 
> register_t *nr,
>       if ( !current->hcall_preempted )
>       {
>           /* Deliberately corrupt parameter regs used by this hypercall. */
> -        switch ( arm_hypercall_table[*nr].nr_args ) {
> +        switch ( arm_hypercall_table[nr].nr_args ) {
>           case 5: HYPERCALL_ARG5(regs) = 0xDEADBEEF;
>           case 4: HYPERCALL_ARG4(regs) = 0xDEADBEEF;
>           case 3: HYPERCALL_ARG3(regs) = 0xDEADBEEF;
> @@ -1511,7 +1513,7 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, 
> register_t *nr,
>               break;
>           default: BUG();
>           }
> -        *nr = 0xDEADBEEF;
> +        HYPERCALL_NR(regs) = 0xDEADBEEF;
This change is not correct. You don't take into account the fact that on 
32-bit domain, the result register will be r12 and 64-bit domain x16.
>       }
>   #endif
>   
> @@ -2131,7 +2133,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   #endif
>           if ( hsr.iss == 0 )
>               return do_trap_hvc_smccc(regs);
> -        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
> +        do_trap_hypercall(regs, regs->r12, hsr.iss);
IHMO it would be better to avoid modify do_trap_hypercall and just save 
r12 in a temporary variable, use that variable as 2 second argument and 
write-back the variable in r12.
This would make do_trap_hypercall future proof.

>           break;
>   #ifdef CONFIG_ARM_64
>       case HSR_EC_HVC64:
> @@ -2143,7 +2145,7 @@ void do_trap_guest_sync(struct cpu_user_regs *regs)
>   #endif
>           if ( hsr.iss == 0 )
>               return do_trap_hvc_smccc(regs);
> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
> +        do_trap_hypercall(regs, regs->x16, hsr.iss);
>           break;
>       case HSR_EC_SMC64:
>           /*
Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel