[RESEND PATCH 0/8] ARM: spectre-v1.1 mitigations

[RESEND PATCH 0/8] ARM: spectre-v1.1 mitigations

[Julien Thierry]

Hi,

Reposting this series rebased on v4.19-rc1.

The series provides mitigations for spectre-v1.1. It is mostly mirroring
what Russell did [1], but this time for writes to user provided addresses.

* Patches 1-4 ensure user addresses used by __put_user* and
  __copy_to_user functions are sanitized before being used.

* Patches 5-8 replace some __put_user_error calls with __copy_to_user, this
  both reduces the number of time address sanitizing is performed and also
  the number of time PAN needs to be toggled.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/589869.html

Cheers,

Julien

-->

Julien Thierry (8):
  ARM: uaccess: Prevent speculative use of the current addr_limit
  ARM: spectre-v1.1: force address sanitizing for __put_user*()
  ARM: spectre-v1,v1.1: provide helpers for address sanitization
  ARM: spectre-v1.1: harden __copy_to_user
  ARM: signal: copy registers using __copy_to_user()
  ARM: signal: always use __copy_to_user to save iwmmxt context
  ARM: vfp: use __copy_to_user() when saving VFP state
  ARM: oabi-compat: copy oabi events using __copy_to_user()

 arch/arm/include/asm/assembler.h   | 10 ++++++
 arch/arm/include/asm/thread_info.h |  4 +--
 arch/arm/include/asm/uaccess.h     | 48 ++++++++++++++++++++++++++
 arch/arm/kernel/signal.c           | 71 +++++++++++++++++++++-----------------
 arch/arm/kernel/sys_oabi-compat.c  | 10 +++---
 arch/arm/lib/copy_from_user.S      |  6 +---
 arch/arm/lib/copy_to_user.S        |  6 +++-
 arch/arm/lib/uaccess_with_memcpy.c |  3 +-
 arch/arm/vfp/vfpmodule.c           | 20 +++++------
 9 files changed, 119 insertions(+), 59 deletions(-)

--
1.9.1

[RESEND PATCH 1/8] ARM: uaccess: Prevent speculative use of the current addr_limit

[Julien Thierry]

A mispredicted conditional call to set_fs could result in the wrong
addr_limit being forwarded under speculation to a subsequent access_ok
check, potentially forming part of a spectre-v1 attack using uaccess
routines.

This patch prevents this forwarding from taking place, but putting heavy
barriers in set_fs after writing the addr_limit.

Porting commit c2f0ad4fc089cff8 ("arm64: uaccess: Prevent speculative use
of the current addr_limit").

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/include/asm/uaccess.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 5451e1f..d65ef85 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -69,6 +69,14 @@ static inline void uaccess_restore(unsigned int flags)
 static inline void set_fs(mm_segment_t fs)
 {
 	current_thread_info()->addr_limit = fs;
+
+	/*
+	 * Prevent a mispredicted conditional call to set_fs from forwarding
+	 * the wrong address limit to access_ok under speculation.
+	 */
+	dsb(nsh);
+	isb();
+
 	modify_domain(DOMAIN_KERNEL, fs ? DOMAIN_CLIENT : DOMAIN_MANAGER);
 }
 
-- 
1.9.1

[RESEND PATCH 2/8] ARM: spectre-v1.1: force address sanitizing for __put_user*()

[Julien Thierry]

When Spectre mitigation is required, define __put_user() and
__put_user_error() to include check_uaccess, ensuring user address
is sanitized.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/include/asm/uaccess.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index d65ef85..2e18b91 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -370,6 +370,20 @@ static inline void set_fs(mm_segment_t fs)
 	__pu_err;							\
 })
 
+#ifdef CONFIG_CPU_SPECTRE
+/*
+ * When mitigating Spectre variant 1.1, all accessors need to include
+ * verification of the address space.
+ */
+#define __put_user(x, ptr) put_user(x, ptr)
+
+#define __put_user_error(x, ptr, err)					\
+({									\
+	__put_user_switch((x), (ptr), (err), __put_user_check);		\
+	(void) 0;							\
+})
+
+#else
 #define __put_user(x, ptr)						\
 ({									\
 	long __pu_err = 0;						\
@@ -462,6 +476,7 @@ static inline void set_fs(mm_segment_t fs)
 	: "r" (x), "i" (-EFAULT)				\
 	: "cc")
 
+#endif /* !CONFIG_CPU_SPECTRE */
 
 #ifdef CONFIG_MMU
 extern unsigned long __must_check
-- 
1.9.1

[RESEND PATCH 3/8] ARM: spectre-v1, v1.1: provide helpers for address sanitization

[Julien Thierry]

Introduce C and asm helpers to sanitize user address, taking the
address range they target into account.

Use asm helper for existing sanitization in __copy_from_user().

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/include/asm/assembler.h | 10 ++++++++++
 arch/arm/include/asm/uaccess.h   | 25 +++++++++++++++++++++++++
 arch/arm/lib/copy_from_user.S    |  6 +-----
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index b17ee03..63aeb17 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -467,6 +467,16 @@
 #endif
 	.endm
 
+	.macro uaccess_mask_range_ptr, addr:req, size:req, limit:req, tmp:req
+#ifdef CONFIG_CPU_SPECTRE
+	sub	\tmp, \limit, #1
+	subs	\tmp, \tmp, \addr	@ tmp = limit - 1 - addr
+	subhss	\tmp, \tmp, \size	@ if (tmp >= 0) tmp = limit - 1 - (addr + size)
+	movlo	\addr, #0		@ if (tmp < 0) addr = NULL
+	csdb
+#endif
+	.endm
+
 	.macro	uaccess_disable, tmp, isb=1
 #ifdef CONFIG_CPU_SW_DOMAIN_PAN
 	/*
diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 2e18b91..9462b8b 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -100,6 +100,31 @@ static inline void set_fs(mm_segment_t fs)
 	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
 
 /*
+ * Sanitise a uaccess pointer such that it becomes NULL if addr+size
+ * is above the current addr_limit.
+ */
+#define uaccess_mask_range_ptr(ptr, size)			\
+	((__typeof__(ptr))__uaccess_mask_range_ptr(ptr, size))
+static inline void __user *__uaccess_mask_range_ptr(const void __user *ptr,
+						    size_t size)
+{
+	void __user *safe_ptr = (void __user *)ptr;
+	unsigned long tmp;
+
+	asm volatile(
+	"	sub	%1, %3, #1\n"
+	"	subs	%1, %1, %0\n"
+	"	subhss	%1, %1, %2\n"
+	"	movlo	%0, #0\n"
+	: "+r" (safe_ptr), "=&r" (tmp)
+	: "r" (size), "r" (current_thread_info()->addr_limit)
+	: "cc");
+
+	csdb();
+	return safe_ptr;
+}
+
+/*
  * Single-value transfer routines.  They automatically use the right
  * size if we just have the right pointer type.  Note that the functions
  * which read from user space (*get_*) need to take care not to leak
diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index a826df3..6709a8d 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -93,11 +93,7 @@ ENTRY(arm_copy_from_user)
 #ifdef CONFIG_CPU_SPECTRE
 	get_thread_info r3
 	ldr	r3, [r3, #TI_ADDR_LIMIT]
-	adds	ip, r1, r2	@ ip=addr+size
-	sub	r3, r3, #1	@ addr_limit - 1
-	cmpcc	ip, r3		@ if (addr+size > addr_limit - 1)
-	movcs	r1, #0		@ addr = NULL
-	csdb
+	uaccess_mask_range_ptr r1, r2, r3, ip
 #endif
 
 #include "copy_template.S"
-- 
1.9.1

[RESEND PATCH 3/8] ARM: spectre-v1,v1.1: provide helpers for address sanitization

[Russell King - ARM Linux]

Hi Julien,

On Tue, Aug 28, 2018 at 10:08:31AM +0100, Julien Thierry wrote:
> +	.macro uaccess_mask_range_ptr, addr:req, size:req, limit:req, tmp:req
> +#ifdef CONFIG_CPU_SPECTRE
> +	sub	\tmp, \limit, #1
> +	subs	\tmp, \tmp, \addr	@ tmp = limit - 1 - addr
> +	subhss	\tmp, \tmp, \size	@ if (tmp >= 0) tmp = limit - 1 - (addr + size)
> +	movlo	\addr, #0		@ if (tmp < 0) addr = NULL
> +	csdb

I'be been thinking about this and my original code.  This seems to suffer
from a problem in that the last <size> access below the address limit
becomes unaccessible.

Let's take an example.  If limit is 0xbf000000, the 32-bit word at
0xbefffffc should be accessible, so addr=0xbefffffc and size=4.

	sub	\tmp, \limit, #1	tmp = 0xbeffffff
	subs	\tmp, \tmp, \addr	tmp = 0xbeffffff - 0xbefffffc = 3
	subhss	\tmp, \tmp, \size	tmp = 3 - 4 = -1

which means we zero the pointer.  This is obviously incorrect
behaviour, because this word should obviously be accessible - it is
entirely below the limit.

I should point out that the existing code seems to suffer from the
same issue:

	@ r1=addr, r2=size r3=limit
	adds	ip, r1, r2	ip=0xbf000000 C=0
	sub	r3, r3, #1	r3=0xbeffffff
	cmpcc	ip, r3          C=ip >= r3

An obvious solution to your code would be to add the '1' back in, but
realising that fails when addr == 0 and limit=0.

	sub	\tmp, \limit, #1	tmp = 0xffffffff
	subs	\tmp, \tmp, \addr	tmp = 0xffffffff - 0 = 0xffffffff
	add	\tmp, \tmp, #1		tmp = 0
	subhss	\tmp, \tmp, \size	tmp = 0 - 4 = -4

However, that doesn't matter as page 0 should never be mapped, and in
any case, if addr was zero and we then make it zero again, we haven't
changed anything.

> +#endif
> +	.endm
> +
>  	.macro	uaccess_disable, tmp, isb=1
>  #ifdef CONFIG_CPU_SW_DOMAIN_PAN
>  	/*
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index 2e18b91..9462b8b 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -100,6 +100,31 @@ static inline void set_fs(mm_segment_t fs)
>  	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>  
>  /*
> + * Sanitise a uaccess pointer such that it becomes NULL if addr+size
> + * is above the current addr_limit.
> + */
> +#define uaccess_mask_range_ptr(ptr, size)			\
> +	((__typeof__(ptr))__uaccess_mask_range_ptr(ptr, size))
> +static inline void __user *__uaccess_mask_range_ptr(const void __user *ptr,
> +						    size_t size)
> +{
> +	void __user *safe_ptr = (void __user *)ptr;
> +	unsigned long tmp;
> +
> +	asm volatile(
> +	"	sub	%1, %3, #1\n"
> +	"	subs	%1, %1, %0\n"
> +	"	subhss	%1, %1, %2\n"
> +	"	movlo	%0, #0\n"
> +	: "+r" (safe_ptr), "=&r" (tmp)
> +	: "r" (size), "r" (current_thread_info()->addr_limit)
> +	: "cc");
> +
> +	csdb();
> +	return safe_ptr;
> +}
> +
> +/*
>   * Single-value transfer routines.  They automatically use the right
>   * size if we just have the right pointer type.  Note that the functions
>   * which read from user space (*get_*) need to take care not to leak
> diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> index a826df3..6709a8d 100644
> --- a/arch/arm/lib/copy_from_user.S
> +++ b/arch/arm/lib/copy_from_user.S
> @@ -93,11 +93,7 @@ ENTRY(arm_copy_from_user)
>  #ifdef CONFIG_CPU_SPECTRE
>  	get_thread_info r3
>  	ldr	r3, [r3, #TI_ADDR_LIMIT]
> -	adds	ip, r1, r2	@ ip=addr+size
> -	sub	r3, r3, #1	@ addr_limit - 1
> -	cmpcc	ip, r3		@ if (addr+size > addr_limit - 1)
> -	movcs	r1, #0		@ addr = NULL
> -	csdb
> +	uaccess_mask_range_ptr r1, r2, r3, ip
>  #endif
>  
>  #include "copy_template.S"
> -- 
> 1.9.1
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[RESEND PATCH 3/8] ARM: spectre-v1,v1.1: provide helpers for address sanitization

[Julien Thierry]

Hi Russell,

On 06/09/18 13:48, Russell King - ARM Linux wrote:
> Hi Julien,
> 
> On Tue, Aug 28, 2018 at 10:08:31AM +0100, Julien Thierry wrote:
>> +	.macro uaccess_mask_range_ptr, addr:req, size:req, limit:req, tmp:req
>> +#ifdef CONFIG_CPU_SPECTRE
>> +	sub	\tmp, \limit, #1
>> +	subs	\tmp, \tmp, \addr	@ tmp = limit - 1 - addr
>> +	subhss	\tmp, \tmp, \size	@ if (tmp >= 0) tmp = limit - 1 - (addr + size)
>> +	movlo	\addr, #0		@ if (tmp < 0) addr = NULL
>> +	csdb
> 
> I'be been thinking about this and my original code.  This seems to suffer
> from a problem in that the last <size> access below the address limit
> becomes unaccessible.
> 
> Let's take an example.  If limit is 0xbf000000, the 32-bit word at
> 0xbefffffc should be accessible, so addr=0xbefffffc and size=4.
> 
> 	sub	\tmp, \limit, #1	tmp = 0xbeffffff
> 	subs	\tmp, \tmp, \addr	tmp = 0xbeffffff - 0xbefffffc = 3
> 	subhss	\tmp, \tmp, \size	tmp = 3 - 4 = -1
> 
> which means we zero the pointer.  This is obviously incorrect
> behaviour, because this word should obviously be accessible - it is
> entirely below the limit.
> 
You're right, thanks for spotting that!

> I should point out that the existing code seems to suffer from the
> same issue:
> 
> 	@ r1=addr, r2=size r3=limit
> 	adds	ip, r1, r2	ip=0xbf000000 C=0
> 	sub	r3, r3, #1	r3=0xbeffffff
> 	cmpcc	ip, r3          C=ip >= r3
> 
> An obvious solution to your code would be to add the '1' back in, but
> realising that fails when addr == 0 and limit=0.
> 
> 	sub	\tmp, \limit, #1	tmp = 0xffffffff
> 	subs	\tmp, \tmp, \addr	tmp = 0xffffffff - 0 = 0xffffffff
> 	add	\tmp, \tmp, #1		tmp = 0
> 	subhss	\tmp, \tmp, \size	tmp = 0 - 4 = -4
> 
> However, that doesn't matter as page 0 should never be mapped, and in
> any case, if addr was zero and we then make it zero again, we haven't
> changed anything.
> 

True. I can't think of a better solution yet so I think I'll use your 
suggestion.

Thanks,

>> +#endif
>> +	.endm
>> +
>>   	.macro	uaccess_disable, tmp, isb=1
>>   #ifdef CONFIG_CPU_SW_DOMAIN_PAN
>>   	/*
>> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
>> index 2e18b91..9462b8b 100644
>> --- a/arch/arm/include/asm/uaccess.h
>> +++ b/arch/arm/include/asm/uaccess.h
>> @@ -100,6 +100,31 @@ static inline void set_fs(mm_segment_t fs)
>>   	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>>   
>>   /*
>> + * Sanitise a uaccess pointer such that it becomes NULL if addr+size
>> + * is above the current addr_limit.
>> + */
>> +#define uaccess_mask_range_ptr(ptr, size)			\
>> +	((__typeof__(ptr))__uaccess_mask_range_ptr(ptr, size))
>> +static inline void __user *__uaccess_mask_range_ptr(const void __user *ptr,
>> +						    size_t size)
>> +{
>> +	void __user *safe_ptr = (void __user *)ptr;
>> +	unsigned long tmp;
>> +
>> +	asm volatile(
>> +	"	sub	%1, %3, #1\n"
>> +	"	subs	%1, %1, %0\n"
>> +	"	subhss	%1, %1, %2\n"
>> +	"	movlo	%0, #0\n"
>> +	: "+r" (safe_ptr), "=&r" (tmp)
>> +	: "r" (size), "r" (current_thread_info()->addr_limit)
>> +	: "cc");
>> +
>> +	csdb();
>> +	return safe_ptr;
>> +}
>> +
>> +/*
>>    * Single-value transfer routines.  They automatically use the right
>>    * size if we just have the right pointer type.  Note that the functions
>>    * which read from user space (*get_*) need to take care not to leak
>> diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
>> index a826df3..6709a8d 100644
>> --- a/arch/arm/lib/copy_from_user.S
>> +++ b/arch/arm/lib/copy_from_user.S
>> @@ -93,11 +93,7 @@ ENTRY(arm_copy_from_user)
>>   #ifdef CONFIG_CPU_SPECTRE
>>   	get_thread_info r3
>>   	ldr	r3, [r3, #TI_ADDR_LIMIT]
>> -	adds	ip, r1, r2	@ ip=addr+size
>> -	sub	r3, r3, #1	@ addr_limit - 1
>> -	cmpcc	ip, r3		@ if (addr+size > addr_limit - 1)
>> -	movcs	r1, #0		@ addr = NULL
>> -	csdb
>> +	uaccess_mask_range_ptr r1, r2, r3, ip
>>   #endif
>>   
>>   #include "copy_template.S"
>> -- 
>> 1.9.1
>>
> 

-- 
Julien Thierry

[RESEND PATCH 4/8] ARM: spectre-v1.1: harden __copy_to_user

[Julien Thierry]

Sanitize user pointer given to __copy_to_user, both for standard version
and memcopy version of the user accessor.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/lib/copy_to_user.S        | 6 +++++-
 arch/arm/lib/uaccess_with_memcpy.c | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm/lib/copy_to_user.S b/arch/arm/lib/copy_to_user.S
index caf5019..970abe5 100644
--- a/arch/arm/lib/copy_to_user.S
+++ b/arch/arm/lib/copy_to_user.S
@@ -94,6 +94,11 @@
 
 ENTRY(__copy_to_user_std)
 WEAK(arm_copy_to_user)
+#ifdef CONFIG_CPU_SPECTRE
+	get_thread_info r3
+	ldr	r3, [r3, #TI_ADDR_LIMIT]
+	uaccess_mask_range_ptr r0, r2, r3, ip
+#endif
 
 #include "copy_template.S"
 
@@ -108,4 +113,3 @@ ENDPROC(__copy_to_user_std)
 	rsb	r0, r0, r2
 	copy_abort_end
 	.popsection
-
diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c
index 9b4ed17..73dc736 100644
--- a/arch/arm/lib/uaccess_with_memcpy.c
+++ b/arch/arm/lib/uaccess_with_memcpy.c
@@ -152,7 +152,8 @@
 		n = __copy_to_user_std(to, from, n);
 		uaccess_restore(ua_flags);
 	} else {
-		n = __copy_to_user_memcpy(to, from, n);
+		n = __copy_to_user_memcpy(uaccess_mask_range_ptr(to, n),
+					  from, n);
 	}
 	return n;
 }
-- 
1.9.1

[RESEND PATCH 5/8] ARM: signal: copy registers using __copy_to_user()

[Julien Thierry]

When saving the ARM integer registers, use __copy_to_user() to
copy them into user signal frame, rather than __put_user_error().
This has the benefit of disabling/enabling PAN once for the whole copy
intead of once per write.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/kernel/signal.c | 49 ++++++++++++++++++++++++++----------------------
 1 file changed, 27 insertions(+), 22 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index b8f766c..76fe75d 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -288,30 +288,35 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
 {
 	struct aux_sigframe __user *aux;
+	struct sigcontext context;
 	int err = 0;
 
-	__put_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
-	__put_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
-	__put_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
-	__put_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
-	__put_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
-	__put_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
-	__put_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
-	__put_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
-	__put_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
-	__put_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
-	__put_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
-	__put_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
-	__put_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
-	__put_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
-	__put_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
-	__put_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
-	__put_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
-
-	__put_user_error(current->thread.trap_no, &sf->uc.uc_mcontext.trap_no, err);
-	__put_user_error(current->thread.error_code, &sf->uc.uc_mcontext.error_code, err);
-	__put_user_error(current->thread.address, &sf->uc.uc_mcontext.fault_address, err);
-	__put_user_error(set->sig[0], &sf->uc.uc_mcontext.oldmask, err);
+	context = (struct sigcontext) {
+		.arm_r0        = regs->ARM_r0,
+		.arm_r1        = regs->ARM_r1,
+		.arm_r2        = regs->ARM_r2,
+		.arm_r3        = regs->ARM_r3,
+		.arm_r4        = regs->ARM_r4,
+		.arm_r5        = regs->ARM_r5,
+		.arm_r6        = regs->ARM_r6,
+		.arm_r7        = regs->ARM_r7,
+		.arm_r8        = regs->ARM_r8,
+		.arm_r9        = regs->ARM_r9,
+		.arm_r10       = regs->ARM_r10,
+		.arm_fp        = regs->ARM_fp,
+		.arm_ip        = regs->ARM_ip,
+		.arm_sp        = regs->ARM_sp,
+		.arm_lr        = regs->ARM_lr,
+		.arm_pc        = regs->ARM_pc,
+		.arm_cpsr      = regs->ARM_cpsr,
+
+		.trap_no       = current->thread.trap_no,
+		.error_code    = current->thread.error_code,
+		.fault_address = current->thread.address,
+		.oldmask       = set->sig[0],
+	};
+
+	err |= __copy_to_user(&sf->uc.uc_mcontext, &context, sizeof(context));
 
 	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
 
-- 
1.9.1

[RESEND PATCH 5/8] ARM: signal: copy registers using __copy_to_user()

[Russell King - ARM Linux]

Hi Julien,

I would much prefer these to be ordered before the patches changing
__put_user() etc.  That would then allow patch 2 (which would become
the last patch) to get rid of __put_user_error() - __put_user_error()
is utterly pointless with the need to work around Spectre, and that
would reflect what was done in my series for Spectre variant 1.

In any case, removing it from signal handling, vfp and oabi compat
should mean that after your existing series, there are no users of
__put_user_error(), but it still remains.

Thanks.

On Tue, Aug 28, 2018 at 10:08:33AM +0100, Julien Thierry wrote:
> When saving the ARM integer registers, use __copy_to_user() to
> copy them into user signal frame, rather than __put_user_error().
> This has the benefit of disabling/enabling PAN once for the whole copy
> intead of once per write.
> 
> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
> ---
>  arch/arm/kernel/signal.c | 49 ++++++++++++++++++++++++++----------------------
>  1 file changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index b8f766c..76fe75d 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -288,30 +288,35 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
>  setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
>  {
>  	struct aux_sigframe __user *aux;
> +	struct sigcontext context;
>  	int err = 0;
>  
> -	__put_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
> -	__put_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
> -	__put_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
> -	__put_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
> -	__put_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
> -	__put_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
> -	__put_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
> -	__put_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
> -	__put_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
> -	__put_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
> -	__put_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
> -	__put_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
> -	__put_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
> -	__put_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
> -	__put_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
> -	__put_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
> -	__put_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
> -
> -	__put_user_error(current->thread.trap_no, &sf->uc.uc_mcontext.trap_no, err);
> -	__put_user_error(current->thread.error_code, &sf->uc.uc_mcontext.error_code, err);
> -	__put_user_error(current->thread.address, &sf->uc.uc_mcontext.fault_address, err);
> -	__put_user_error(set->sig[0], &sf->uc.uc_mcontext.oldmask, err);
> +	context = (struct sigcontext) {
> +		.arm_r0        = regs->ARM_r0,
> +		.arm_r1        = regs->ARM_r1,
> +		.arm_r2        = regs->ARM_r2,
> +		.arm_r3        = regs->ARM_r3,
> +		.arm_r4        = regs->ARM_r4,
> +		.arm_r5        = regs->ARM_r5,
> +		.arm_r6        = regs->ARM_r6,
> +		.arm_r7        = regs->ARM_r7,
> +		.arm_r8        = regs->ARM_r8,
> +		.arm_r9        = regs->ARM_r9,
> +		.arm_r10       = regs->ARM_r10,
> +		.arm_fp        = regs->ARM_fp,
> +		.arm_ip        = regs->ARM_ip,
> +		.arm_sp        = regs->ARM_sp,
> +		.arm_lr        = regs->ARM_lr,
> +		.arm_pc        = regs->ARM_pc,
> +		.arm_cpsr      = regs->ARM_cpsr,
> +
> +		.trap_no       = current->thread.trap_no,
> +		.error_code    = current->thread.error_code,
> +		.fault_address = current->thread.address,
> +		.oldmask       = set->sig[0],
> +	};
> +
> +	err |= __copy_to_user(&sf->uc.uc_mcontext, &context, sizeof(context));
>  
>  	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
>  
> -- 
> 1.9.1
> 

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

[RESEND PATCH 5/8] ARM: signal: copy registers using __copy_to_user()

[Julien Thierry]

Hi Russell,

On 06/09/18 13:49, Russell King - ARM Linux wrote:
> Hi Julien,
> 
> I would much prefer these to be ordered before the patches changing
> __put_user() etc.  That would then allow patch 2 (which would become
> the last patch) to get rid of __put_user_error() - __put_user_error()
> is utterly pointless with the need to work around Spectre, and that
> would reflect what was done in my series for Spectre variant 1.
> 

Yes, I organized the series this way so the mitigation patches could be 
picked up without the rest but I am happy reordering it.

> In any case, removing it from signal handling, vfp and oabi compat
> should mean that after your existing series, there are no users of
> __put_user_error(), but it still remains.
> 

Yes, I tried reducing the number of uses of __put_user_error but a few 
remained in signal.c where replacing with them with __copy_to_user  was 
less of an optimization since it was only writing unsigned longs.

But since there are just a few remaining calls to it, I'll replace them 
so __put_user_error can go.

I'll send a new version fixing this and the range checking.

Thanks,

> Thanks.
> 
> On Tue, Aug 28, 2018 at 10:08:33AM +0100, Julien Thierry wrote:
>> When saving the ARM integer registers, use __copy_to_user() to
>> copy them into user signal frame, rather than __put_user_error().
>> This has the benefit of disabling/enabling PAN once for the whole copy
>> intead of once per write.
>>
>> Signed-off-by: Julien Thierry <julien.thierry@arm.com>
>> ---
>>   arch/arm/kernel/signal.c | 49 ++++++++++++++++++++++++++----------------------
>>   1 file changed, 27 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
>> index b8f766c..76fe75d 100644
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -288,30 +288,35 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
>>   setup_sigframe(struct sigframe __user *sf, struct pt_regs *regs, sigset_t *set)
>>   {
>>   	struct aux_sigframe __user *aux;
>> +	struct sigcontext context;
>>   	int err = 0;
>>   
>> -	__put_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
>> -	__put_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
>> -	__put_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
>> -	__put_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
>> -	__put_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
>> -	__put_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
>> -	__put_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
>> -	__put_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
>> -	__put_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
>> -	__put_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
>> -	__put_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
>> -	__put_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
>> -	__put_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
>> -	__put_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
>> -	__put_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
>> -	__put_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
>> -	__put_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
>> -
>> -	__put_user_error(current->thread.trap_no, &sf->uc.uc_mcontext.trap_no, err);
>> -	__put_user_error(current->thread.error_code, &sf->uc.uc_mcontext.error_code, err);
>> -	__put_user_error(current->thread.address, &sf->uc.uc_mcontext.fault_address, err);
>> -	__put_user_error(set->sig[0], &sf->uc.uc_mcontext.oldmask, err);
>> +	context = (struct sigcontext) {
>> +		.arm_r0        = regs->ARM_r0,
>> +		.arm_r1        = regs->ARM_r1,
>> +		.arm_r2        = regs->ARM_r2,
>> +		.arm_r3        = regs->ARM_r3,
>> +		.arm_r4        = regs->ARM_r4,
>> +		.arm_r5        = regs->ARM_r5,
>> +		.arm_r6        = regs->ARM_r6,
>> +		.arm_r7        = regs->ARM_r7,
>> +		.arm_r8        = regs->ARM_r8,
>> +		.arm_r9        = regs->ARM_r9,
>> +		.arm_r10       = regs->ARM_r10,
>> +		.arm_fp        = regs->ARM_fp,
>> +		.arm_ip        = regs->ARM_ip,
>> +		.arm_sp        = regs->ARM_sp,
>> +		.arm_lr        = regs->ARM_lr,
>> +		.arm_pc        = regs->ARM_pc,
>> +		.arm_cpsr      = regs->ARM_cpsr,
>> +
>> +		.trap_no       = current->thread.trap_no,
>> +		.error_code    = current->thread.error_code,
>> +		.fault_address = current->thread.address,
>> +		.oldmask       = set->sig[0],
>> +	};
>> +
>> +	err |= __copy_to_user(&sf->uc.uc_mcontext, &context, sizeof(context));
>>   
>>   	err |= __copy_to_user(&sf->uc.uc_sigmask, set, sizeof(*set));
>>   
>> -- 
>> 1.9.1
>>
> 

-- 
Julien Thierry

[RESEND PATCH 6/8] ARM: signal: always use __copy_to_user to save iwmmxt context

[Julien Thierry]

When setting a dummy iwmmxt context, create a local instance and
use __copy_to_user both cases whether iwmmxt is being used or not.
This has the benefit of disabling/enabling PAN once for the whole copy
intead of once per write.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/kernel/signal.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 76fe75d..464393d 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -77,8 +77,6 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe __user *frame)
 		kframe->magic = IWMMXT_MAGIC;
 		kframe->size = IWMMXT_STORAGE_SIZE;
 		iwmmxt_task_copy(current_thread_info(), &kframe->storage);
-
-		err = __copy_to_user(frame, kframe, sizeof(*frame));
 	} else {
 		/*
 		 * For bug-compatibility with older kernels, some space
@@ -86,10 +84,14 @@ static int preserve_iwmmxt_context(struct iwmmxt_sigframe __user *frame)
 		 * Set the magic and size appropriately so that properly
 		 * written userspace can skip it reliably:
 		 */
-		__put_user_error(DUMMY_MAGIC, &frame->magic, err);
-		__put_user_error(IWMMXT_STORAGE_SIZE, &frame->size, err);
+		*kframe = (struct iwmmxt_sigframe) {
+			.magic = DUMMY_MAGIC,
+			.size  = IWMMXT_STORAGE_SIZE,
+		};
 	}
 
+	err = __copy_to_user(frame, kframe, sizeof(*kframe));
+
 	return err;
 }
 
-- 
1.9.1

[RESEND PATCH 7/8] ARM: vfp: use __copy_to_user() when saving VFP state

[Julien Thierry]

Use __copy_to_user() rather than __put_user_error() for individual
members when saving VFP state.
This has the benefit of disabling/enabling PAN once per copied struct
intead of once per write.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/include/asm/thread_info.h |  4 ++--
 arch/arm/kernel/signal.c           | 12 ++++++------
 arch/arm/vfp/vfpmodule.c           | 20 ++++++++------------
 3 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index 9b37b6a..8f55dc5 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -121,8 +121,8 @@ static inline struct thread_info *current_thread_info(void)
 struct user_vfp;
 struct user_vfp_exc;
 
-extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
-					   struct user_vfp_exc __user *);
+extern int vfp_preserve_user_clear_hwstate(struct user_vfp *,
+					   struct user_vfp_exc *);
 extern int vfp_restore_user_hwstate(struct user_vfp *,
 				    struct user_vfp_exc *);
 #endif
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 464393d..baa055d 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -137,17 +137,17 @@ static int restore_iwmmxt_context(char __user **auxp)
 
 static int preserve_vfp_context(struct vfp_sigframe __user *frame)
 {
-	const unsigned long magic = VFP_MAGIC;
-	const unsigned long size = VFP_STORAGE_SIZE;
+	struct vfp_sigframe kframe;
 	int err = 0;
 
-	__put_user_error(magic, &frame->magic, err);
-	__put_user_error(size, &frame->size, err);
+	kframe.magic = VFP_MAGIC;
+	kframe.size = VFP_STORAGE_SIZE;
 
+	err = vfp_preserve_user_clear_hwstate(&kframe.ufp, &kframe.ufp_exc);
 	if (err)
-		return -EFAULT;
+		return err;
 
-	return vfp_preserve_user_clear_hwstate(&frame->ufp, &frame->ufp_exc);
+	return __copy_to_user(frame, &kframe, sizeof(kframe));
 }
 
 static int restore_vfp_context(char __user **auxp)
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index dc7e6b5..2b287d0 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -553,12 +553,11 @@ void vfp_flush_hwstate(struct thread_info *thread)
  * Save the current VFP state into the provided structures and prepare
  * for entry into a new function (signal handler).
  */
-int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
-				    struct user_vfp_exc __user *ufp_exc)
+int vfp_preserve_user_clear_hwstate(struct user_vfp *ufp,
+				    struct user_vfp_exc *ufp_exc)
 {
 	struct thread_info *thread = current_thread_info();
 	struct vfp_hard_struct *hwstate = &thread->vfpstate.hard;
-	int err = 0;
 
 	/* Ensure that the saved hwstate is up-to-date. */
 	vfp_sync_hwstate(thread);
@@ -567,22 +566,19 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
 	 * Copy the floating point registers. There can be unused
 	 * registers see asm/hwcap.h for details.
 	 */
-	err |= __copy_to_user(&ufp->fpregs, &hwstate->fpregs,
-			      sizeof(hwstate->fpregs));
+	memcpy(&ufp->fpregs, &hwstate->fpregs, sizeof(hwstate->fpregs));
+
 	/*
 	 * Copy the status and control register.
 	 */
-	__put_user_error(hwstate->fpscr, &ufp->fpscr, err);
+	ufp->fpscr = hwstate->fpscr;
 
 	/*
 	 * Copy the exception registers.
 	 */
-	__put_user_error(hwstate->fpexc, &ufp_exc->fpexc, err);
-	__put_user_error(hwstate->fpinst, &ufp_exc->fpinst, err);
-	__put_user_error(hwstate->fpinst2, &ufp_exc->fpinst2, err);
-
-	if (err)
-		return -EFAULT;
+	ufp_exc->fpexc = hwstate->fpexc;
+	ufp_exc->fpinst = hwstate->fpinst;
+	ufp_exc->fpinst2 = ufp_exc->fpinst2;
 
 	/* Ensure that VFP is disabled. */
 	vfp_flush_hwstate(thread);
-- 
1.9.1

[RESEND PATCH 8/8] ARM: oabi-compat: copy oabi events using __copy_to_user()

[Julien Thierry]

Copy all events to user using __copy_to_user() rather than copy members
of each event individually with __put_user_error().
This has the benefit of disabling/enabling PAN once for the whole copy
intead of once per write.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
---
 arch/arm/kernel/sys_oabi-compat.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/sys_oabi-compat.c b/arch/arm/kernel/sys_oabi-compat.c
index f0dd4b6..c89d27e 100644
--- a/arch/arm/kernel/sys_oabi-compat.c
+++ b/arch/arm/kernel/sys_oabi-compat.c
@@ -278,7 +278,7 @@ asmlinkage long sys_oabi_epoll_wait(int epfd,
 {
 	struct epoll_event *kbuf;
 	mm_segment_t fs;
-	long ret, err, i;
+	long ret, err;
 
 	if (maxevents <= 0 ||
 			maxevents > (INT_MAX/sizeof(*kbuf)) ||
@@ -294,11 +294,9 @@ asmlinkage long sys_oabi_epoll_wait(int epfd,
 	ret = sys_epoll_wait(epfd, kbuf, maxevents, timeout);
 	set_fs(fs);
 	err = 0;
-	for (i = 0; i < ret; i++) {
-		__put_user_error(kbuf[i].events, &events->events, err);
-		__put_user_error(kbuf[i].data,   &events->data,   err);
-		events++;
-	}
+	if (ret > 0)
+		err = __copy_to_user(events, kbuf, ret * sizeof(*kbuf));
+
 	kfree(kbuf);
 	return err ? -EFAULT : ret;
 }
-- 
1.9.1