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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sat, 13 Jan 2018, Julien Grall wrote:
> 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.

I defined HYPERCALL_NR differently depending on CONFIG_ARM_64, but you
are right, that's not right, because it will misbehave for 32-bit guests
on a 64-bit hypervisor.


> >       }
> >   #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.

Sounds good.


> >           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