[Xen-devel,2/6] xen/arm: smccc-1.1: Handle function result as parameters

Message ID 20180824165820.32620-3-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: SMCCC fixup and improvement
Related show

Commit Message

Julien Grall Aug. 24, 2018, 4:58 p.m.
From: Marc Zyngier <marc.zyngier@arm.com>

If someone has the silly idea to write something along those lines:

	extern u64 foo(void);

	void bar(struct arm_smccc_res *res)
	{
		arm_smccc_1_1_smc(0xbad, foo(), res);
	}

they are in for a surprise, as this gets compiled as:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d4000003        smc     #0x0
	 5ac:   b4000073        cbz     x19, 5b8 <bar+0x30>
	 5b0:   a9000660        stp     x0, x1, [x19]
	 5b4:   a9010e62        stp     x2, x3, [x19, #16]
	 5b8:   f9400bf3        ldr     x19, [sp, #16]
	 5bc:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c0:   d65f03c0        ret
	 5c4:   d503201f        nop

The call to foo "overwrites" the x0 register for the return value,
and we end up calling the wrong secure service.

A solution is to evaluate all the parameters before assigning
anything to specific registers, leading to the expected result:

	0000000000000588 <bar>:
	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
	 58c:   910003fd        mov     x29, sp
	 590:   f9000bf3        str     x19, [sp, #16]
	 594:   aa0003f3        mov     x19, x0
	 598:   aa1e03e0        mov     x0, x30
	 59c:   94000000        bl      0 <_mcount>
	 5a0:   94000000        bl      0 <foo>
	 5a4:   aa0003e1        mov     x1, x0
	 5a8:   d28175a0        mov     x0, #0xbad
	 5ac:   d4000003        smc     #0x0
	 5b0:   b4000073        cbz     x19, 5bc <bar+0x34>
	 5b4:   a9000660        stp     x0, x1, [x19]
	 5b8:   a9010e62        stp     x2, x3, [x19, #16]
	 5bc:   f9400bf3        ldr     x19, [sp, #16]
	 5c0:   a8c27bfd        ldp     x29, x30, [sp], #32
	 5c4:   d65f03c0        ret

Reported-by: Stefano Stabellini <stefanos@xilinx.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 xen/include/asm-arm/smccc.h | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

Comments

Volodymyr Babchuk Aug. 27, 2018, 2:05 p.m. | #1
Hi,

On 24.08.18 19:58, Julien Grall wrote:
> From: Marc Zyngier <marc.zyngier@arm.com>
> 
> If someone has the silly idea to write something along those lines:
> 
> 	extern u64 foo(void);
> 
> 	void bar(struct arm_smccc_res *res)
> 	{
> 		arm_smccc_1_1_smc(0xbad, foo(), res);
> 	}
> 
> they are in for a surprise, as this gets compiled as:
> 
> 	0000000000000588 <bar>:
> 	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
> 	 58c:   910003fd        mov     x29, sp
> 	 590:   f9000bf3        str     x19, [sp, #16]
> 	 594:   aa0003f3        mov     x19, x0
> 	 598:   aa1e03e0        mov     x0, x30
> 	 59c:   94000000        bl      0 <_mcount>
> 	 5a0:   94000000        bl      0 <foo>
> 	 5a4:   aa0003e1        mov     x1, x0
> 	 5a8:   d4000003        smc     #0x0
> 	 5ac:   b4000073        cbz     x19, 5b8 <bar+0x30>
> 	 5b0:   a9000660        stp     x0, x1, [x19]
> 	 5b4:   a9010e62        stp     x2, x3, [x19, #16]
> 	 5b8:   f9400bf3        ldr     x19, [sp, #16]
> 	 5bc:   a8c27bfd        ldp     x29, x30, [sp], #32
> 	 5c0:   d65f03c0        ret
> 	 5c4:   d503201f        nop
> 
> The call to foo "overwrites" the x0 register for the return value,
> and we end up calling the wrong secure service.
> 
> A solution is to evaluate all the parameters before assigning
> anything to specific registers, leading to the expected result:
> 
> 	0000000000000588 <bar>:
> 	 588:   a9be7bfd        stp     x29, x30, [sp, #-32]!
> 	 58c:   910003fd        mov     x29, sp
> 	 590:   f9000bf3        str     x19, [sp, #16]
> 	 594:   aa0003f3        mov     x19, x0
> 	 598:   aa1e03e0        mov     x0, x30
> 	 59c:   94000000        bl      0 <_mcount>
> 	 5a0:   94000000        bl      0 <foo>
> 	 5a4:   aa0003e1        mov     x1, x0
> 	 5a8:   d28175a0        mov     x0, #0xbad
> 	 5ac:   d4000003        smc     #0x0
> 	 5b0:   b4000073        cbz     x19, 5bc <bar+0x34>
> 	 5b4:   a9000660        stp     x0, x1, [x19]
> 	 5b8:   a9010e62        stp     x2, x3, [x19, #16]
> 	 5bc:   f9400bf3        ldr     x19, [sp, #16]
> 	 5c0:   a8c27bfd        ldp     x29, x30, [sp], #32
> 	 5c4:   d65f03c0        ret
> 
> Reported-by: Stefano Stabellini <stefanos@xilinx.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymr_babchuk@epam.com>
> ---
>   xen/include/asm-arm/smccc.h | 30 ++++++++++++++++++++----------
>   1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index a31d67a1de..648bef28bd 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -125,41 +125,51 @@ struct arm_smccc_res {
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_1(a0, a1, res)                    \
> +    typeof(a1) __a1 = a1;                               \
>       struct arm_smccc_res    *___res = res;              \
>       register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> +    register unsigned long  r1 asm("r1") = __a1;        \
>       register unsigned long  r2 asm("r2");               \
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_2(a0, a1, a2, res)                \
> +    typeof(a1) __a1 = a1;                               \
> +    typeof(a2) __a2 = a2;                               \
>       struct arm_smccc_res    *___res = res;				\
>       register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> -    register unsigned long  r2 asm("r2") = a2;          \
> +    register unsigned long  r1 asm("r1") = __a1;        \
> +    register unsigned long  r2 asm("r2") = __a2;        \
>       register unsigned long  r3 asm("r3")
>   
>   #define __declare_arg_3(a0, a1, a2, a3, res)            \
> +    typeof(a1) __a1 = a1;                               \
> +    typeof(a2) __a2 = a2;                               \
> +    typeof(a3) __a3 = a3;                               \
>       struct arm_smccc_res    *___res = res;              \
>       register unsigned long  r0 asm("r0") = (uint32_t)a0;\
> -    register unsigned long  r1 asm("r1") = a1;          \
> -    register unsigned long  r2 asm("r2") = a2;          \
> -    register unsigned long  r3 asm("r3") = a3
> +    register unsigned long  r1 asm("r1") = __a1;        \
> +    register unsigned long  r2 asm("r2") = __a2;        \
> +    register unsigned long  r3 asm("r3") = __a3
>   
>   #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
> +    typeof(a4) __a4 = a4;                               \
>       __declare_arg_3(a0, a1, a2, a3, res);               \
> -    register unsigned long r4 asm("r4") = a4
> +    register unsigned long r4 asm("r4") = __a4
>   
>   #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
> +    typeof(a5) __a5 = a5;                               \
>       __declare_arg_4(a0, a1, a2, a3, a4, res);           \
> -    register typeof(a5) r5 asm("r5") = a5
> +    register typeof(a5) r5 asm("r5") = __a5
>   
>   #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
> +    typeof(a6) __a6 = a6;                                   \
>       __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
> -    register typeof(a6) r6 asm("r6") = a6
> +    register typeof(a6) r6 asm("r6") = __a6
>   
>   #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
> +    typeof(a7) __a7 = a7;                                       \
>       __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
> -    register typeof(a7) r7 asm("r7") = a7
> +    register typeof(a7) r7 asm("r7") = __a7
>   
>   #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
>   #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
>

Patch

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index a31d67a1de..648bef28bd 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -125,41 +125,51 @@  struct arm_smccc_res {
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_1(a0, a1, res)                    \
+    typeof(a1) __a1 = a1;                               \
     struct arm_smccc_res    *___res = res;              \
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
+    register unsigned long  r1 asm("r1") = __a1;        \
     register unsigned long  r2 asm("r2");               \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_2(a0, a1, a2, res)                \
+    typeof(a1) __a1 = a1;                               \
+    typeof(a2) __a2 = a2;                               \
     struct arm_smccc_res    *___res = res;				\
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
-    register unsigned long  r2 asm("r2") = a2;          \
+    register unsigned long  r1 asm("r1") = __a1;        \
+    register unsigned long  r2 asm("r2") = __a2;        \
     register unsigned long  r3 asm("r3")
 
 #define __declare_arg_3(a0, a1, a2, a3, res)            \
+    typeof(a1) __a1 = a1;                               \
+    typeof(a2) __a2 = a2;                               \
+    typeof(a3) __a3 = a3;                               \
     struct arm_smccc_res    *___res = res;              \
     register unsigned long  r0 asm("r0") = (uint32_t)a0;\
-    register unsigned long  r1 asm("r1") = a1;          \
-    register unsigned long  r2 asm("r2") = a2;          \
-    register unsigned long  r3 asm("r3") = a3
+    register unsigned long  r1 asm("r1") = __a1;        \
+    register unsigned long  r2 asm("r2") = __a2;        \
+    register unsigned long  r3 asm("r3") = __a3
 
 #define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
+    typeof(a4) __a4 = a4;                               \
     __declare_arg_3(a0, a1, a2, a3, res);               \
-    register unsigned long r4 asm("r4") = a4
+    register unsigned long r4 asm("r4") = __a4
 
 #define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
+    typeof(a5) __a5 = a5;                               \
     __declare_arg_4(a0, a1, a2, a3, a4, res);           \
-    register typeof(a5) r5 asm("r5") = a5
+    register typeof(a5) r5 asm("r5") = __a5
 
 #define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
+    typeof(a6) __a6 = a6;                                   \
     __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
-    register typeof(a6) r6 asm("r6") = a6
+    register typeof(a6) r6 asm("r6") = __a6
 
 #define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
+    typeof(a7) __a7 = a7;                                       \
     __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
-    register typeof(a7) r7 asm("r7") = a7
+    register typeof(a7) r7 asm("r7") = __a7
 
 #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
 #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)