* [PATCH] don't pass r12/x16 as reference
@ 2018-01-13 0:07 Stefano Stabellini
2018-01-13 6:33 ` Julien Grall
0 siblings, 1 reply; 3+ messages in thread
From: Stefano Stabellini @ 2018-01-13 0:07 UTC (permalink / raw)
To: julien.grall; +Cc: xen-devel, sstabellini
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@kernel.org>
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;
}
#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 *)®s->r12, hsr.iss);
+ do_trap_hypercall(regs, regs->r12, hsr.iss);
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, ®s->x16, hsr.iss);
+ do_trap_hypercall(regs, regs->x16, hsr.iss);
break;
case HSR_EC_SMC64:
/*
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] don't pass r12/x16 as reference
2018-01-13 0:07 [PATCH] don't pass r12/x16 as reference Stefano Stabellini
@ 2018-01-13 6:33 ` Julien Grall
2018-01-15 18:07 ` Stefano Stabellini
0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2018-01-13 6:33 UTC (permalink / raw)
To: Stefano Stabellini; +Cc: xen-devel
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@kernel.org>
>
> 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 *)®s->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, ®s->x16, hsr.iss);
> + do_trap_hypercall(regs, regs->x16, hsr.iss);
> break;
> case HSR_EC_SMC64:
> /*
>
Cheers,
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] don't pass r12/x16 as reference
2018-01-13 6:33 ` Julien Grall
@ 2018-01-15 18:07 ` Stefano Stabellini
0 siblings, 0 replies; 3+ messages in thread
From: Stefano Stabellini @ 2018-01-15 18:07 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini
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@kernel.org>
> >
> > 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 *)®s->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, ®s->x16, hsr.iss);
> > + do_trap_hypercall(regs, regs->x16, hsr.iss);
> > break;
> > case HSR_EC_SMC64:
> > /*
> >
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-15 18:07 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-13 0:07 [PATCH] don't pass r12/x16 as reference Stefano Stabellini
2018-01-13 6:33 ` Julien Grall
2018-01-15 18:07 ` Stefano Stabellini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.