[PATCH v2 0/9] ARM: spectre-v1.1 mitigations

[PATCH v2 0/9] ARM: spectre-v1.1 mitigations

[Julien Thierry]

Hi,

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

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


Changes since v1[2]:
- Fixed off-by-one error in uaccess_mask_range_ptr, spotted by Russell
- Remove remaining calls to __put_user_error() and get rid of the macro
- Reorder the patches to better reflect the spectre-v1 series

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2018-July/589869.html
[2] https://www.spinics.net/lists/arm-kernel/msg671026.html

Cheers,

Julien

-->

Julien Thierry (9):
  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()
  ARM: signal: replace __put_user_error with __put_user
  ARM: uaccess: Prevent speculative use of the current addr_limit
  ARM: spectre-v1.1: use put_user() for __put_user()
  ARM: spectre-v1,v1.1: provide helpers for address sanitization
  ARM: spectre-v1.1: harden __copy_to_user

 arch/arm/include/asm/assembler.h   | 11 ++++++
 arch/arm/include/asm/thread_info.h |  4 +-
 arch/arm/include/asm/uaccess.h     | 49 ++++++++++++++++++++---
 arch/arm/kernel/signal.c           | 79 +++++++++++++++++++++-----------------
 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(+), 69 deletions(-)

--
1.9.1

[PATCH v2 1/9] 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

[PATCH v2 1/9] ARM: signal: copy registers using __copy_to_user()

[Russell King - ARM Linux]

On Thu, Sep 06, 2018 at 05:38:00PM +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.

Looks fine, but please note:

> +	struct sigcontext context;
>  	int err = 0;
>  
> +	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));

This construct is safe here, because struct sigcontext will have
no padding.

However, in the case of a struct that does contain padding, doing
the above will leave the padding uninitialised, and therefore the
__copy_to_user() will end up leaking information from the kernel
stack into userspace - which is bad news.

So, such constructs are best avoided in generic code (where it's
less known whether padding is present), and only in arch code after
careful review.

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

[PATCH v2 1/9] ARM: signal: copy registers using __copy_to_user()

[Julien Thierry]

Hi Russell,

On 06/09/18 17:50, Russell King - ARM Linux wrote:
> On Thu, Sep 06, 2018 at 05:38:00PM +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.
> 
> Looks fine, but please note:
> 
>> +	struct sigcontext context;
>>   	int err = 0;
>>   
>> +	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));
> 
> This construct is safe here, because struct sigcontext will have
> no padding.
> 
> However, in the case of a struct that does contain padding, doing
> the above will leave the padding uninitialised, and therefore the
> __copy_to_user() will end up leaking information from the kernel
> stack into userspace - which is bad news.
> 

Thanks for pointing this out. I wrongly assumed this would be 
initialized the same way unspecified fields get initialized to 0. But 
you're right, there is no guaranty about the padding getting initialized.

I'll fix the patches affected.

About the ones where there is currently no padding, are 
preserve_iwmmxt_context and setup_sigframe called in performance 
critical paths? (I don't think that is the case but I prefer to be sure)
If not I would prefer adding the memset to those as well just in case 
the corresponding structs get a new field introducing padding. Unless 
you really feel this would be unnecessary.

Thanks,

> So, such constructs are best avoided in generic code (where it's
> less known whether padding is present), and only in arch code after
> careful review.
> 

-- 
Julien Thierry

[PATCH v2 1/9] ARM: signal: copy registers using __copy_to_user()

[Julien Thierry]

On 10/09/18 09:00, Julien Thierry wrote:
> Hi Russell,
> 
> On 06/09/18 17:50, Russell King - ARM Linux wrote:
>> On Thu, Sep 06, 2018 at 05:38:00PM +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.
>>
>> Looks fine, but please note:
>>
>>> +??? struct sigcontext context;
>>> ????? int err = 0;
>>> +??? 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));
>>
>> This construct is safe here, because struct sigcontext will have
>> no padding.
>>
>> However, in the case of a struct that does contain padding, doing
>> the above will leave the padding uninitialised, and therefore the
>> __copy_to_user() will end up leaking information from the kernel
>> stack into userspace - which is bad news.
>>
> 
> Thanks for pointing this out. I wrongly assumed this would be 
> initialized the same way unspecified fields get initialized to 0. But 
> you're right, there is no guaranty about the padding getting initialized.
> 
> I'll fix the patches affected.
> 
> About the ones where there is currently no padding, are 
> preserve_iwmmxt_context and setup_sigframe called in performance 
> critical paths? (I don't think that is the case but I prefer to be sure)
> If not I would prefer adding the memset to those as well just in case 
> the corresponding structs get a new field introducing padding. Unless 
> you really feel this would be unnecessary.
> 

Thinking about this, I might be a bit overzealous. for sigcontext the 
issue would come up only if someone decides to add a member smaller than 
an int (or add some alignment requirement). And for iwmmxt_sigframe 
everything is properly aligned as you stated.

I'll just leave it like this since you said it was fine and fix patch 3 
which definitely needs it.

Thanks,

-- 
Julien Thierry

[PATCH v2 2/9] 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

[PATCH v2 2/9] ARM: signal: always use __copy_to_user to save iwmmxt context

[Russell King - ARM Linux]

On Thu, Sep 06, 2018 at 05:38:01PM +0100, Julien Thierry wrote:
> @@ -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));

Looks safe to me - iwmmxt_sigframe is ultimately made up of
unsigned long's and unsigned int's and the embedded struct
iwmmxt_struct is aligned to an 8 byte boundary anyway, so there
should be no padding here either.

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

[PATCH v2 3/9] 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

[PATCH v2 3/9] ARM: vfp: use __copy_to_user() when saving VFP state

[Russell King - ARM Linux]

On Thu, Sep 06, 2018 at 05:38:02PM +0100, Julien Thierry wrote:
> 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));

This one needs a memset() to ensure that padding is cleared:

/*
 *  8 byte for magic and size, 264 byte for ufp, 12 bytes for ufp_exc,
 *  4 bytes padding.
 */
#define VFP_STORAGE_SIZE        sizeof(struct vfp_sigframe)

so as this patch stands, it results in a leak one word of kernel data
to userspace.

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

[PATCH v2 4/9] 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

[PATCH v2 4/9] ARM: oabi-compat: copy oabi events using __copy_to_user()

[Russell King - ARM Linux]

On Thu, Sep 06, 2018 at 05:38:03PM +0100, Julien Thierry wrote:
> @@ -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));
> +

It's not that simple!

kbuf is of type "struct epoll_event" whereas events is of type
"struct oabi_epoll_event".  They have different layouts - if they
were the same, we wouldn't need the OABI shim.

struct oabi_epoll_event {
        __u32 events;
        __u64 data;
} __attribute__ ((packed,aligned(4)));

vs

struct epoll_event {
        __poll_t events;
        __u64 data;
};

which results in EABI sizeof(struct epoll_event) being 16 and 12 on
OABI.  What's more is that 'data' is at offset 4 in OABI, or 8 in
EABI.

So, what I think needs to happen is that we need to do something
like:

	struct oabi_epoll_event e;

	for (i = 0; i < ret; i++) {
		e.events = kbuf[i].events;
		e.data = kbuf[i].data;
		err = __copy_to_user(events, e, sizeof(e))
		if (err)
			break;
		events++;
	}

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

[PATCH v2 4/9] ARM: oabi-compat: copy oabi events using __copy_to_user()

[Julien Thierry]

Hi Russell,

On 06/09/18 18:04, Russell King - ARM Linux wrote:
> On Thu, Sep 06, 2018 at 05:38:03PM +0100, Julien Thierry wrote:
>> @@ -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));
>> +
> 
> It's not that simple!
> 
> kbuf is of type "struct epoll_event" whereas events is of type
> "struct oabi_epoll_event".  They have different layouts - if they
> were the same, we wouldn't need the OABI shim.
> 

Right, I completely missed the type mismatch... sorry.

I'll fix this.

Thanks,

> struct oabi_epoll_event {
>          __u32 events;
>          __u64 data;
> } __attribute__ ((packed,aligned(4)));
> 
> vs
> 
> struct epoll_event {
>          __poll_t events;
>          __u64 data;
> };
> 
> which results in EABI sizeof(struct epoll_event) being 16 and 12 on
> OABI.  What's more is that 'data' is at offset 4 in OABI, or 8 in
> EABI.
> 
> So, what I think needs to happen is that we need to do something
> like:
> 
> 	struct oabi_epoll_event e;
> 
> 	for (i = 0; i < ret; i++) {
> 		e.events = kbuf[i].events;
> 		e.data = kbuf[i].data;
> 		err = __copy_to_user(events, e, sizeof(e))
> 		if (err)
> 			break;
> 		events++;
> 	}
> 

-- 
Julien Thierry

[PATCH v2 5/9] ARM: signal: replace __put_user_error with __put_user

[Julien Thierry]

With Spectre-v1.1 mitigations, __put_user_error is pointless. In an attempt
to remove it, replace its references in frame setups with __put_user.

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

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index baa055d..61ae58a 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -335,7 +335,7 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 	if (err == 0)
 		err |= preserve_vfp_context(&aux->vfp);
 #endif
-	__put_user_error(0, &aux->end_magic, err);
+	err |= __put_user(0, &aux->end_magic);
 
 	return err;
 }
@@ -498,7 +498,7 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 	/*
 	 * Set uc.uc_flags to a value which sc.trap_no would never have.
 	 */
-	__put_user_error(0x5ac3c35a, &frame->uc.uc_flags, err);
+	err = __put_user(0x5ac3c35a, &frame->uc.uc_flags);
 
 	err |= setup_sigframe(frame, regs, set);
 	if (err == 0)
@@ -518,8 +518,8 @@ asmlinkage int sys_rt_sigreturn(struct pt_regs *regs)
 
 	err |= copy_siginfo_to_user(&frame->info, &ksig->info);
 
-	__put_user_error(0, &frame->sig.uc.uc_flags, err);
-	__put_user_error(NULL, &frame->sig.uc.uc_link, err);
+	err |= __put_user(0, &frame->sig.uc.uc_flags);
+	err |= __put_user(NULL, &frame->sig.uc.uc_link);
 
 	err |= __save_altstack(&frame->sig.uc.uc_stack, regs->ARM_sp);
 	err |= setup_sigframe(&frame->sig, regs, set);
-- 
1.9.1

[PATCH v2 6/9] 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

[PATCH v2 7/9] ARM: spectre-v1.1: use put_user() for __put_user()

[Julien Thierry]

When Spectre mitigation is required, __put_user() needs to include
check_uaccess. This is already the case for put_user(), so just make
__put_user() an alias of put_user().

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

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index d65ef85..1191e7d 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -370,6 +370,14 @@ 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)
+
+#else
 #define __put_user(x, ptr)						\
 ({									\
 	long __pu_err = 0;						\
@@ -377,12 +385,6 @@ static inline void set_fs(mm_segment_t fs)
 	__pu_err;							\
 })
 
-#define __put_user_error(x, ptr, err)					\
-({									\
-	__put_user_switch((x), (ptr), (err), __put_user_nocheck);	\
-	(void) 0;							\
-})
-
 #define __put_user_nocheck(x, __pu_ptr, __err, __size)			\
 	do {								\
 		unsigned long __pu_addr = (unsigned long)__pu_ptr;	\
@@ -462,6 +464,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

[PATCH v2 8/9] 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 | 11 +++++++++++
 arch/arm/include/asm/uaccess.h   | 26 ++++++++++++++++++++++++++
 arch/arm/lib/copy_from_user.S    |  6 +-----
 3 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index b17ee03..88286dd 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -467,6 +467,17 @@
 #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
+	addhs	\tmp, \tmp, #1		@ if (tmp >= 0) {
+	subhss	\tmp, \tmp, \size	@ tmp = limit - (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 1191e7d..c136eef 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -100,6 +100,32 @@ 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"
+	"	addhs	%1, %1, #1\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

[PATCH v2 9/9] 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