[PATCH 0/2] arm/arm64: smccc-1.1 fixes

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/2] arm/arm64: smccc-1.1 fixes
@ 2018-08-24 14:08 Marc Zyngier
  2018-08-24 14:08 ` [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long Marc Zyngier
  2018-08-24 14:08 ` [PATCH 2/2] arm/arm64: amccc-1.1: Handle function result as parameters Marc Zyngier
  0 siblings, 2 replies; 4+ messages in thread
From: Marc Zyngier @ 2018-08-24 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

Julien Grall recently reported that our smccc-1.1 helpers suffer from
a couple of glaring problems:

- the return values are not necessarily of the same type as the input,
  and we may end-up truncating them

- the macros are not safe in the presence of function calls passed as
  parameters.

These two patches are trying to handle that. Note that the current use
of these macros in the kernel is safe, so this is not an urgent fix.

Marc Zyngier (2):
  arm/arm64: smccc-1.1: Make return values unsigned long
  arm/arm64: amccc-1.1: Handle function result as parameters

 include/linux/arm-smccc.h | 38 ++++++++++++++++++++++++--------------
 1 file changed, 24 insertions(+), 14 deletions(-)

-- 
2.18.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long
  2018-08-24 14:08 [PATCH 0/2] arm/arm64: smccc-1.1 fixes Marc Zyngier
@ 2018-08-24 14:08 ` Marc Zyngier
  2018-08-29 10:41   ` Will Deacon
  2018-08-24 14:08 ` [PATCH 2/2] arm/arm64: amccc-1.1: Handle function result as parameters Marc Zyngier
  1 sibling, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2018-08-24 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

An unfortunate consequence of having a strong typing for the input
values to the SMC call is that it also affects the type of the
return values, limiting r0 to 32 bits and r{1,2,3} to whatever
was passed as an input.

Let's turn everything into "unsigned long", which satisfies the
requirements of both architectures, and allows for the full
range of return values.

Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/arm-smccc.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index ca1d2cc2cdfa..5a91ff33720b 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -199,31 +199,31 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 
 #define __declare_arg_0(a0, res)					\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
+	register unsigned long r0 asm("r0") = (u32)a0;			\
 	register unsigned long r1 asm("r1");				\
 	register unsigned long r2 asm("r2");				\
 	register unsigned long r3 asm("r3")
 
 #define __declare_arg_1(a0, a1, res)					\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
+	register unsigned long r0 asm("r0") = (u32)a0;			\
+	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)				\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
-	register typeof(a2)    r2 asm("r2") = a2;			\
+	register unsigned long r0 asm("r0") = (u32)a0;			\
+	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)				\
 	struct arm_smccc_res   *___res = res;				\
-	register u32           r0 asm("r0") = a0;			\
-	register typeof(a1)    r1 asm("r1") = a1;			\
-	register typeof(a2)    r2 asm("r2") = a2;			\
-	register typeof(a3)    r3 asm("r3") = a3
+	register unsigned long r0 asm("r0") = (u32)a0;			\
+	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)			\
 	__declare_arg_3(a0, a1, a2, a3, res);				\
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long
  2018-08-24 14:08 ` [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long Marc Zyngier
@ 2018-08-29 10:41   ` Will Deacon
  0 siblings, 0 replies; 4+ messages in thread
From: Will Deacon @ 2018-08-29 10:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Aug 24, 2018 at 03:08:29PM +0100, Marc Zyngier wrote:
> An unfortunate consequence of having a strong typing for the input
> values to the SMC call is that it also affects the type of the
> return values, limiting r0 to 32 bits and r{1,2,3} to whatever
> was passed as an input.
> 
> Let's turn everything into "unsigned long", which satisfies the
> requirements of both architectures, and allows for the full
> range of return values.
> 
> Reported-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  include/linux/arm-smccc.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)

Does this need a Cc stable, or do no current callers need the upper bits of
the return values on arm64?

Will

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH 2/2] arm/arm64: amccc-1.1: Handle function result as parameters
  2018-08-24 14:08 [PATCH 0/2] arm/arm64: smccc-1.1 fixes Marc Zyngier
  2018-08-24 14:08 ` [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long Marc Zyngier
@ 2018-08-24 14:08 ` Marc Zyngier
  1 sibling, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2018-08-24 14:08 UTC (permalink / raw)
  To: linux-arm-kernel

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: Julien Grall <julien.grall@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/arm-smccc.h | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 5a91ff33720b..18863d56273c 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -205,41 +205,51 @@ asmlinkage void __arm_smccc_hvc(unsigned long a0, unsigned long a1,
 	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") = (u32)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") = (u32)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") = (u32)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 typeof(a4) 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 unsigned long 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 unsigned long 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 unsigned long r7 asm("r7") = __a7
 
 #define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
 #define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
-- 
2.18.0

^ permalink raw reply related	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-08-29 10:41 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-08-24 14:08 [PATCH 0/2] arm/arm64: smccc-1.1 fixes Marc Zyngier
2018-08-24 14:08 ` [PATCH 1/2] arm/arm64: smccc-1.1: Make return values unsigned long Marc Zyngier
2018-08-29 10:41   ` Will Deacon
2018-08-24 14:08 ` [PATCH 2/2] arm/arm64: amccc-1.1: Handle function result as parameters Marc Zyngier

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.