[PATCH 0/6] Further spectre variant 1 mitigations

[PATCH 0/6] Further spectre variant 1 mitigations

[Russell King - ARM Linux]

Hi,

This series applies the mitigations for Spectre variant 1 to 32-bit
ARM user accessors.  This series slightly larger by the need to
offset the additional overhead introduced by these checks in various
paths, particularly the signal handling path.

 arch/arm/include/asm/assembler.h   |  4 +++
 arch/arm/include/asm/thread_info.h |  2 +-
 arch/arm/include/asm/uaccess.h     | 26 ++++++++++++-----
 arch/arm/kernel/signal.c           | 58 +++++++++++++++++++-------------------
 arch/arm/kernel/sys_oabi-compat.c  |  8 ++++--
 arch/arm/lib/copy_from_user.S      |  7 +++++
 arch/arm/vfp/vfpmodule.c           | 16 +++++------
 7 files changed, 72 insertions(+), 49 deletions(-)

-- 
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 1/6] ARM: signal: copy registers using __copy_from_user()

[Russell King]

When restoring the ARM integer registers, use __copy_from_user() to
copy them into kernel space before restoring them, rather than
__get_user_error().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/kernel/signal.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index bd8810d4acb3..0ae74207e43e 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -176,6 +176,7 @@ static int restore_vfp_context(char __user **auxp)
 
 static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 {
+	struct sigcontext context;
 	char __user *aux;
 	sigset_t set;
 	int err;
@@ -184,23 +185,26 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
 	if (err == 0)
 		set_current_blocked(&set);
 
-	__get_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
-	__get_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
-	__get_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
-	__get_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
-	__get_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
-	__get_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
-	__get_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
-	__get_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
-	__get_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
-	__get_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
-	__get_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
-	__get_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
-	__get_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
-	__get_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
-	__get_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
-	__get_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
-	__get_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
+	err |= __copy_from_user(&context, &sf->uc.uc_mcontext, sizeof(context));
+	if (err == 0) {
+		regs->ARM_r0 = context.arm_r0;
+		regs->ARM_r1 = context.arm_r1;
+		regs->ARM_r2 = context.arm_r2;
+		regs->ARM_r3 = context.arm_r3;
+		regs->ARM_r4 = context.arm_r4;
+		regs->ARM_r5 = context.arm_r5;
+		regs->ARM_r6 = context.arm_r6;
+		regs->ARM_r7 = context.arm_r7;
+		regs->ARM_r8 = context.arm_r8;
+		regs->ARM_r9 = context.arm_r9;
+		regs->ARM_r10 = context.arm_r10;
+		regs->ARM_fp = context.arm_fp;
+		regs->ARM_ip = context.arm_ip;
+		regs->ARM_sp = context.arm_sp;
+		regs->ARM_lr = context.arm_lr;
+		regs->ARM_pc = context.arm_pc;
+		regs->ARM_cpsr = context.arm_cpsr;
+	}
 
 	err |= !valid_user_regs(regs);
 
-- 
2.7.4

[PATCH 1/6] ARM: signal: copy registers using __copy_from_user()

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:13:47PM +0100, Russell King wrote:
> When restoring the ARM integer registers, use __copy_from_user() to
> copy them into kernel space before restoring them, rather than
> __get_user_error().

I assume that this is so that we only have to pay the cost of sanitizing
the user pointer once in __copy_from_user(), rather than in each
__get_user_error()?

Can we mention that in the commit message? e.g. something like:

  In subsequent patches, each uaccess will sanitize the __user pointer
  to prevent speculative accesses to kernel memory.
  
  To amortize the cost of this sanitization when restoring the integer
  registers, we can use __copy_from_user() rather than a sequence of
  __get_user_error() calls.
 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/kernel/signal.c | 38 +++++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index bd8810d4acb3..0ae74207e43e 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -176,6 +176,7 @@ static int restore_vfp_context(char __user **auxp)
>  
>  static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
>  {
> +	struct sigcontext context;
>  	char __user *aux;
>  	sigset_t set;
>  	int err;
> @@ -184,23 +185,26 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
>  	if (err == 0)
>  		set_current_blocked(&set);
>  
> -	__get_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
> -	__get_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
> -	__get_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
> -	__get_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
> -	__get_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
> -	__get_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
> -	__get_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
> -	__get_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
> -	__get_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
> -	__get_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
> -	__get_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
> -	__get_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
> -	__get_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
> -	__get_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
> -	__get_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
> -	__get_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
> -	__get_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
> +	err |= __copy_from_user(&context, &sf->uc.uc_mcontext, sizeof(context));

IIUC, this means that we'll also copy the (unused) fault_address field,
but I assume that's ok.

Thanks,
Mark.

> +	if (err == 0) {
> +		regs->ARM_r0 = context.arm_r0;
> +		regs->ARM_r1 = context.arm_r1;
> +		regs->ARM_r2 = context.arm_r2;
> +		regs->ARM_r3 = context.arm_r3;
> +		regs->ARM_r4 = context.arm_r4;
> +		regs->ARM_r5 = context.arm_r5;
> +		regs->ARM_r6 = context.arm_r6;
> +		regs->ARM_r7 = context.arm_r7;
> +		regs->ARM_r8 = context.arm_r8;
> +		regs->ARM_r9 = context.arm_r9;
> +		regs->ARM_r10 = context.arm_r10;
> +		regs->ARM_fp = context.arm_fp;
> +		regs->ARM_ip = context.arm_ip;
> +		regs->ARM_sp = context.arm_sp;
> +		regs->ARM_lr = context.arm_lr;
> +		regs->ARM_pc = context.arm_pc;
> +		regs->ARM_cpsr = context.arm_cpsr;
> +	}
>  
>  	err |= !valid_user_regs(regs);
>  
> -- 
> 2.7.4
> 

[PATCH 1/6] ARM: signal: copy registers using __copy_from_user()

[Russell King - ARM Linux]

On Thu, Jul 26, 2018 at 01:23:50PM +0100, Mark Rutland wrote:
> On Tue, Jul 10, 2018 at 03:13:47PM +0100, Russell King wrote:
> > When restoring the ARM integer registers, use __copy_from_user() to
> > copy them into kernel space before restoring them, rather than
> > __get_user_error().
> 
> I assume that this is so that we only have to pay the cost of sanitizing
> the user pointer once in __copy_from_user(), rather than in each
> __get_user_error()?
> 
> Can we mention that in the commit message? e.g. something like:
> 
>   In subsequent patches, each uaccess will sanitize the __user pointer
>   to prevent speculative accesses to kernel memory.
>   
>   To amortize the cost of this sanitization when restoring the integer
>   registers, we can use __copy_from_user() rather than a sequence of
>   __get_user_error() calls.

It's not only that, it's also the overhead of the software PAN support
as well, and it's a change I've been thinking has been worth doing
for some time.

>  
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  arch/arm/kernel/signal.c | 38 +++++++++++++++++++++-----------------
> >  1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > index bd8810d4acb3..0ae74207e43e 100644
> > --- a/arch/arm/kernel/signal.c
> > +++ b/arch/arm/kernel/signal.c
> > @@ -176,6 +176,7 @@ static int restore_vfp_context(char __user **auxp)
> >  
> >  static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
> >  {
> > +	struct sigcontext context;
> >  	char __user *aux;
> >  	sigset_t set;
> >  	int err;
> > @@ -184,23 +185,26 @@ static int restore_sigframe(struct pt_regs *regs, struct sigframe __user *sf)
> >  	if (err == 0)
> >  		set_current_blocked(&set);
> >  
> > -	__get_user_error(regs->ARM_r0, &sf->uc.uc_mcontext.arm_r0, err);
> > -	__get_user_error(regs->ARM_r1, &sf->uc.uc_mcontext.arm_r1, err);
> > -	__get_user_error(regs->ARM_r2, &sf->uc.uc_mcontext.arm_r2, err);
> > -	__get_user_error(regs->ARM_r3, &sf->uc.uc_mcontext.arm_r3, err);
> > -	__get_user_error(regs->ARM_r4, &sf->uc.uc_mcontext.arm_r4, err);
> > -	__get_user_error(regs->ARM_r5, &sf->uc.uc_mcontext.arm_r5, err);
> > -	__get_user_error(regs->ARM_r6, &sf->uc.uc_mcontext.arm_r6, err);
> > -	__get_user_error(regs->ARM_r7, &sf->uc.uc_mcontext.arm_r7, err);
> > -	__get_user_error(regs->ARM_r8, &sf->uc.uc_mcontext.arm_r8, err);
> > -	__get_user_error(regs->ARM_r9, &sf->uc.uc_mcontext.arm_r9, err);
> > -	__get_user_error(regs->ARM_r10, &sf->uc.uc_mcontext.arm_r10, err);
> > -	__get_user_error(regs->ARM_fp, &sf->uc.uc_mcontext.arm_fp, err);
> > -	__get_user_error(regs->ARM_ip, &sf->uc.uc_mcontext.arm_ip, err);
> > -	__get_user_error(regs->ARM_sp, &sf->uc.uc_mcontext.arm_sp, err);
> > -	__get_user_error(regs->ARM_lr, &sf->uc.uc_mcontext.arm_lr, err);
> > -	__get_user_error(regs->ARM_pc, &sf->uc.uc_mcontext.arm_pc, err);
> > -	__get_user_error(regs->ARM_cpsr, &sf->uc.uc_mcontext.arm_cpsr, err);
> > +	err |= __copy_from_user(&context, &sf->uc.uc_mcontext, sizeof(context));
> 
> IIUC, this means that we'll also copy the (unused) fault_address field,
> but I assume that's ok.

It is completely fine, because the stack contains everything in
sf->uc.uc_mcontext - this is a member in the middle of the parent
struct sigframe, and we access both before and after that member.

-- 
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 1/6] ARM: signal: copy registers using __copy_from_user()

[Mark Rutland]

On Thu, Jul 26, 2018 at 02:56:32PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 26, 2018 at 01:23:50PM +0100, Mark Rutland wrote:
> > On Tue, Jul 10, 2018 at 03:13:47PM +0100, Russell King wrote:
> > > When restoring the ARM integer registers, use __copy_from_user() to
> > > copy them into kernel space before restoring them, rather than
> > > __get_user_error().
> > 
> > I assume that this is so that we only have to pay the cost of sanitizing
> > the user pointer once in __copy_from_user(), rather than in each
> > __get_user_error()?
> > 
> > Can we mention that in the commit message? e.g. something like:
> > 
> >   In subsequent patches, each uaccess will sanitize the __user pointer
> >   to prevent speculative accesses to kernel memory.
> >   
> >   To amortize the cost of this sanitization when restoring the integer
> >   registers, we can use __copy_from_user() rather than a sequence of
> >   __get_user_error() calls.
> 
> It's not only that, it's also the overhead of the software PAN support
> as well, and it's a change I've been thinking has been worth doing
> for some time.

Makes sense.

If you can put something in the commit message to cover all of that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

... applying to patches 2 and 3, too.

Mark.

[PATCH 2/6] ARM: vfp: use __copy_from_user() when restoring VFP state

[Russell King]

Use __copy_from_user() rather than __get_user_err() for individual
members when restoring VFP state.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/thread_info.h |  2 +-
 arch/arm/kernel/signal.c           | 20 ++++++++------------
 arch/arm/vfp/vfpmodule.c           | 16 +++++++---------
 3 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index e71cc35de163..341f6eb9b8be 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -123,7 +123,7 @@ struct user_vfp_exc;
 
 extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
 					   struct user_vfp_exc __user *);
-extern int vfp_restore_user_hwstate(struct user_vfp __user *,
+extern int vfp_restore_user_hwstate(struct user_vfp *,
 				    struct user_vfp_exc __user *);
 #endif
 
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 0ae74207e43e..db62c51250ad 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -150,22 +150,18 @@ static int preserve_vfp_context(struct vfp_sigframe __user *frame)
 
 static int restore_vfp_context(char __user **auxp)
 {
-	struct vfp_sigframe __user *frame =
-		(struct vfp_sigframe __user *)*auxp;
-	unsigned long magic;
-	unsigned long size;
-	int err = 0;
-
-	__get_user_error(magic, &frame->magic, err);
-	__get_user_error(size, &frame->size, err);
+	struct vfp_sigframe frame;
+	int err;
 
+	err = __copy_from_user(&frame, *auxp, sizeof(frame));
 	if (err)
-		return -EFAULT;
-	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
+		return err;
+
+	if (frame.magic != VFP_MAGIC || frame.size != VFP_STORAGE_SIZE)
 		return -EINVAL;
 
-	*auxp += size;
-	return vfp_restore_user_hwstate(&frame->ufp, &frame->ufp_exc);
+	*auxp += sizeof(frame);
+	return vfp_restore_user_hwstate(&frame.ufp, &frame.ufp_exc);
 }
 
 #endif
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index 4c375e11ae95..9929f79a9e87 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -597,13 +597,12 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
 }
 
 /* Sanitise and restore the current VFP state from the provided structures. */
-int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
+int vfp_restore_user_hwstate(struct user_vfp *ufp,
 			     struct user_vfp_exc __user *ufp_exc)
 {
 	struct thread_info *thread = current_thread_info();
 	struct vfp_hard_struct *hwstate = &thread->vfpstate.hard;
 	unsigned long fpexc;
-	int err = 0;
 
 	/* Disable VFP to avoid corrupting the new thread state. */
 	vfp_flush_hwstate(thread);
@@ -612,17 +611,16 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	 * Copy the floating point registers. There can be unused
 	 * registers see asm/hwcap.h for details.
 	 */
-	err |= __copy_from_user(&hwstate->fpregs, &ufp->fpregs,
-				sizeof(hwstate->fpregs));
+	memcpy(&hwstate->fpregs, &ufp->fpregs, sizeof(hwstate->fpregs));
 	/*
 	 * Copy the status and control register.
 	 */
-	__get_user_error(hwstate->fpscr, &ufp->fpscr, err);
+	hwstate->fpscr = ufp->fpscr;
 
 	/*
 	 * Sanitise and restore the exception registers.
 	 */
-	__get_user_error(fpexc, &ufp_exc->fpexc, err);
+	fpexc = ufp_exc->fpexc;
 
 	/* Ensure the VFP is enabled. */
 	fpexc |= FPEXC_EN;
@@ -631,10 +629,10 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
 	fpexc &= ~(FPEXC_EX | FPEXC_FP2V);
 	hwstate->fpexc = fpexc;
 
-	__get_user_error(hwstate->fpinst, &ufp_exc->fpinst, err);
-	__get_user_error(hwstate->fpinst2, &ufp_exc->fpinst2, err);
+	hwstate->fpinst = ufp_exc->fpinst;
+	hwstate->fpinst2 = ufp_exc->fpinst2;
 
-	return err ? -EFAULT : 0;
+	return 0;
 }
 
 /*
-- 
2.7.4

[PATCH 2/6] ARM: vfp: use __copy_from_user() when restoring VFP state

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:13:52PM +0100, Russell King wrote:
> Use __copy_from_user() rather than __get_user_err() for individual
> members when restoring VFP state.

Same comment as for patch 1: I assume this is to amortize the cost of
the __user pointer santiziation, and if so it'd be good to mention that
in the commit message.

> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/include/asm/thread_info.h |  2 +-
>  arch/arm/kernel/signal.c           | 20 ++++++++------------
>  arch/arm/vfp/vfpmodule.c           | 16 +++++++---------
>  3 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index e71cc35de163..341f6eb9b8be 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -123,7 +123,7 @@ struct user_vfp_exc;
>  
>  extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
>  					   struct user_vfp_exc __user *);
> -extern int vfp_restore_user_hwstate(struct user_vfp __user *,
> +extern int vfp_restore_user_hwstate(struct user_vfp *,
>  				    struct user_vfp_exc __user *);
>  #endif
>  
> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> index 0ae74207e43e..db62c51250ad 100644
> --- a/arch/arm/kernel/signal.c
> +++ b/arch/arm/kernel/signal.c
> @@ -150,22 +150,18 @@ static int preserve_vfp_context(struct vfp_sigframe __user *frame)
>  
>  static int restore_vfp_context(char __user **auxp)
>  {
> -	struct vfp_sigframe __user *frame =
> -		(struct vfp_sigframe __user *)*auxp;
> -	unsigned long magic;
> -	unsigned long size;
> -	int err = 0;
> -
> -	__get_user_error(magic, &frame->magic, err);
> -	__get_user_error(size, &frame->size, err);
> +	struct vfp_sigframe frame;
> +	int err;
>  
> +	err = __copy_from_user(&frame, *auxp, sizeof(frame));
>  	if (err)
> -		return -EFAULT;
> -	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
> +		return err;
> +
> +	if (frame.magic != VFP_MAGIC || frame.size != VFP_STORAGE_SIZE)
>  		return -EINVAL;

I think that in a few cases, trying to load the whole vfp_sigframe size
first means that we can now fault and return -EFAULT when previously
we'd have returned -EINVAL.

However, as the return value is only consumed as a boolean by all
callers, I think that's ok. Might be worth calling that out in the
commit message.

Otherwise, this looks sane to me.

Thanks,
Mark.

>  
> -	*auxp += size;
> -	return vfp_restore_user_hwstate(&frame->ufp, &frame->ufp_exc);
> +	*auxp += sizeof(frame);
> +	return vfp_restore_user_hwstate(&frame.ufp, &frame.ufp_exc);
>  }
>  
>  #endif
> diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
> index 4c375e11ae95..9929f79a9e87 100644
> --- a/arch/arm/vfp/vfpmodule.c
> +++ b/arch/arm/vfp/vfpmodule.c
> @@ -597,13 +597,12 @@ int vfp_preserve_user_clear_hwstate(struct user_vfp __user *ufp,
>  }
>  
>  /* Sanitise and restore the current VFP state from the provided structures. */
> -int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
> +int vfp_restore_user_hwstate(struct user_vfp *ufp,
>  			     struct user_vfp_exc __user *ufp_exc)
>  {
>  	struct thread_info *thread = current_thread_info();
>  	struct vfp_hard_struct *hwstate = &thread->vfpstate.hard;
>  	unsigned long fpexc;
> -	int err = 0;
>  
>  	/* Disable VFP to avoid corrupting the new thread state. */
>  	vfp_flush_hwstate(thread);
> @@ -612,17 +611,16 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
>  	 * Copy the floating point registers. There can be unused
>  	 * registers see asm/hwcap.h for details.
>  	 */
> -	err |= __copy_from_user(&hwstate->fpregs, &ufp->fpregs,
> -				sizeof(hwstate->fpregs));
> +	memcpy(&hwstate->fpregs, &ufp->fpregs, sizeof(hwstate->fpregs));
>  	/*
>  	 * Copy the status and control register.
>  	 */
> -	__get_user_error(hwstate->fpscr, &ufp->fpscr, err);
> +	hwstate->fpscr = ufp->fpscr;
>  
>  	/*
>  	 * Sanitise and restore the exception registers.
>  	 */
> -	__get_user_error(fpexc, &ufp_exc->fpexc, err);
> +	fpexc = ufp_exc->fpexc;
>  
>  	/* Ensure the VFP is enabled. */
>  	fpexc |= FPEXC_EN;
> @@ -631,10 +629,10 @@ int vfp_restore_user_hwstate(struct user_vfp __user *ufp,
>  	fpexc &= ~(FPEXC_EX | FPEXC_FP2V);
>  	hwstate->fpexc = fpexc;
>  
> -	__get_user_error(hwstate->fpinst, &ufp_exc->fpinst, err);
> -	__get_user_error(hwstate->fpinst2, &ufp_exc->fpinst2, err);
> +	hwstate->fpinst = ufp_exc->fpinst;
> +	hwstate->fpinst2 = ufp_exc->fpinst2;
>  
> -	return err ? -EFAULT : 0;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.7.4
> 

[PATCH 2/6] ARM: vfp: use __copy_from_user() when restoring VFP state

[Russell King - ARM Linux]

On Thu, Jul 26, 2018 at 01:32:44PM +0100, Mark Rutland wrote:
> On Tue, Jul 10, 2018 at 03:13:52PM +0100, Russell King wrote:
> > Use __copy_from_user() rather than __get_user_err() for individual
> > members when restoring VFP state.
> 
> Same comment as for patch 1: I assume this is to amortize the cost of
> the __user pointer santiziation, and if so it'd be good to mention that
> in the commit message.

It's also to do with the software PAN stuff as well.

> 
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  arch/arm/include/asm/thread_info.h |  2 +-
> >  arch/arm/kernel/signal.c           | 20 ++++++++------------
> >  arch/arm/vfp/vfpmodule.c           | 16 +++++++---------
> >  3 files changed, 16 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> > index e71cc35de163..341f6eb9b8be 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -123,7 +123,7 @@ struct user_vfp_exc;
> >  
> >  extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
> >  					   struct user_vfp_exc __user *);
> > -extern int vfp_restore_user_hwstate(struct user_vfp __user *,
> > +extern int vfp_restore_user_hwstate(struct user_vfp *,
> >  				    struct user_vfp_exc __user *);
> >  #endif
> >  
> > diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
> > index 0ae74207e43e..db62c51250ad 100644
> > --- a/arch/arm/kernel/signal.c
> > +++ b/arch/arm/kernel/signal.c
> > @@ -150,22 +150,18 @@ static int preserve_vfp_context(struct vfp_sigframe __user *frame)
> >  
> >  static int restore_vfp_context(char __user **auxp)
> >  {
> > -	struct vfp_sigframe __user *frame =
> > -		(struct vfp_sigframe __user *)*auxp;
> > -	unsigned long magic;
> > -	unsigned long size;
> > -	int err = 0;
> > -
> > -	__get_user_error(magic, &frame->magic, err);
> > -	__get_user_error(size, &frame->size, err);
> > +	struct vfp_sigframe frame;
> > +	int err;
> >  
> > +	err = __copy_from_user(&frame, *auxp, sizeof(frame));
> >  	if (err)
> > -		return -EFAULT;
> > -	if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
> > +		return err;
> > +
> > +	if (frame.magic != VFP_MAGIC || frame.size != VFP_STORAGE_SIZE)
> >  		return -EINVAL;
> 
> I think that in a few cases, trying to load the whole vfp_sigframe size
> first means that we can now fault and return -EFAULT when previously
> we'd have returned -EINVAL.
> 
> However, as the return value is only consumed as a boolean by all
> callers, I think that's ok. Might be worth calling that out in the
> commit message.

The signal stack is created by the kernel in a certain format - the
tagged nature of it is to deal with all the combinations of different
hardwares that we have, so that userspace can parse the stack
irrespective of the configuration of the kernel or hardwares present.

The layout is ultimately defined by struct aux_sigframe, and the
entire frame as defined by that structure must be present and valid
when restoring.

Hence, if we expect there to be a VFP structure present, then it
quite simply must be present.

Since this is one of the basics of the signal handling code, it
doesn't warrant commenting in the commit message.

-- 
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 2/6] ARM: vfp: use __copy_from_user() when restoring VFP state

[Kees Cook]

On Thu, Jul 26, 2018 at 5:32 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Jul 10, 2018 at 03:13:52PM +0100, Russell King wrote:
>> Use __copy_from_user() rather than __get_user_err() for individual
>> members when restoring VFP state.
>
> Same comment as for patch 1: I assume this is to amortize the cost of
> the __user pointer santiziation, and if so it'd be good to mention that
> in the commit message.
>
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> ---
>>  arch/arm/include/asm/thread_info.h |  2 +-
>>  arch/arm/kernel/signal.c           | 20 ++++++++------------
>>  arch/arm/vfp/vfpmodule.c           | 16 +++++++---------
>>  3 files changed, 16 insertions(+), 22 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
>> index e71cc35de163..341f6eb9b8be 100644
>> --- a/arch/arm/include/asm/thread_info.h
>> +++ b/arch/arm/include/asm/thread_info.h
>> @@ -123,7 +123,7 @@ struct user_vfp_exc;
>>
>>  extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
>>                                          struct user_vfp_exc __user *);
>> -extern int vfp_restore_user_hwstate(struct user_vfp __user *,
>> +extern int vfp_restore_user_hwstate(struct user_vfp *,
>>                                   struct user_vfp_exc __user *);
>>  #endif
>>
>> diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
>> index 0ae74207e43e..db62c51250ad 100644
>> --- a/arch/arm/kernel/signal.c
>> +++ b/arch/arm/kernel/signal.c
>> @@ -150,22 +150,18 @@ static int preserve_vfp_context(struct vfp_sigframe __user *frame)
>>
>>  static int restore_vfp_context(char __user **auxp)
>>  {
>> -     struct vfp_sigframe __user *frame =
>> -             (struct vfp_sigframe __user *)*auxp;
>> -     unsigned long magic;
>> -     unsigned long size;
>> -     int err = 0;
>> -
>> -     __get_user_error(magic, &frame->magic, err);
>> -     __get_user_error(size, &frame->size, err);
>> +     struct vfp_sigframe frame;
>> +     int err;
>>
>> +     err = __copy_from_user(&frame, *auxp, sizeof(frame));
>>       if (err)
>> -             return -EFAULT;
>> -     if (magic != VFP_MAGIC || size != VFP_STORAGE_SIZE)
>> +             return err;

While this appears not to have a behavior change (everything is
checking non-zero), why return "err" here instead of -EFAULT?
__copy_from_user() is returning a count, not a negative return code.
Seems weird to mix that with the -EINVAL below...

>> +
>> +     if (frame.magic != VFP_MAGIC || frame.size != VFP_STORAGE_SIZE)
>>               return -EINVAL;
>
> I think that in a few cases, trying to load the whole vfp_sigframe size
> first means that we can now fault and return -EFAULT when previously
> we'd have returned -EINVAL.

-Kees

-- 
Kees Cook
Pixel Security

[PATCH 2/6] ARM: vfp: use __copy_from_user() when restoring VFP state

[Julien Thierry]

Hi Russell,

On 10/07/18 15:13, Russell King wrote:
> Use __copy_from_user() rather than __get_user_err() for individual
> members when restoring VFP state.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   arch/arm/include/asm/thread_info.h |  2 +-
>   arch/arm/kernel/signal.c           | 20 ++++++++------------
>   arch/arm/vfp/vfpmodule.c           | 16 +++++++---------
>   3 files changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index e71cc35de163..341f6eb9b8be 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -123,7 +123,7 @@ struct user_vfp_exc;
>   
>   extern int vfp_preserve_user_clear_hwstate(struct user_vfp __user *,
>   					   struct user_vfp_exc __user *);
> -extern int vfp_restore_user_hwstate(struct user_vfp __user *,
> +extern int vfp_restore_user_hwstate(struct user_vfp *,
>   				    struct user_vfp_exc __user *);


Just a small nit. With this patch, shouldn't the struct user_vfp_exc 
argument lose the __user tag for this function?

Cheers,

-- 
Julien Thierry

[PATCH 3/6] ARM: oabi-compat: copy semops using __copy_from_user()

[Russell King]

Rather than using __get_user_error(), copy each semops element using
__copy_from_user().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/kernel/sys_oabi-compat.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/sys_oabi-compat.c b/arch/arm/kernel/sys_oabi-compat.c
index b9786f491873..4abe4909417f 100644
--- a/arch/arm/kernel/sys_oabi-compat.c
+++ b/arch/arm/kernel/sys_oabi-compat.c
@@ -329,9 +329,11 @@ asmlinkage long sys_oabi_semtimedop(int semid,
 		return -ENOMEM;
 	err = 0;
 	for (i = 0; i < nsops; i++) {
-		__get_user_error(sops[i].sem_num, &tsops->sem_num, err);
-		__get_user_error(sops[i].sem_op,  &tsops->sem_op,  err);
-		__get_user_error(sops[i].sem_flg, &tsops->sem_flg, err);
+		struct oabi_sembuf osb;
+		err |= __copy_from_user(&osb, tsops, sizeof(osb));
+		sops[i].sem_num = osb.sem_num;
+		sops[i].sem_op = osb.sem_op;
+		sops[i].sem_flg = osb.sem_flg;
 		tsops++;
 	}
 	if (timeout) {
-- 
2.7.4

[PATCH 3/6] ARM: oabi-compat: copy semops using __copy_from_user()

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:13:57PM +0100, Russell King wrote:
> Rather than using __get_user_error(), copy each semops element using
> __copy_from_user().
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

As per patch one and two, it would be good to explain why.

Otherwise, this looks sane to me.

Thanks,
Mark.

> ---
>  arch/arm/kernel/sys_oabi-compat.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kernel/sys_oabi-compat.c b/arch/arm/kernel/sys_oabi-compat.c
> index b9786f491873..4abe4909417f 100644
> --- a/arch/arm/kernel/sys_oabi-compat.c
> +++ b/arch/arm/kernel/sys_oabi-compat.c
> @@ -329,9 +329,11 @@ asmlinkage long sys_oabi_semtimedop(int semid,
>  		return -ENOMEM;
>  	err = 0;
>  	for (i = 0; i < nsops; i++) {
> -		__get_user_error(sops[i].sem_num, &tsops->sem_num, err);
> -		__get_user_error(sops[i].sem_op,  &tsops->sem_op,  err);
> -		__get_user_error(sops[i].sem_flg, &tsops->sem_flg, err);
> +		struct oabi_sembuf osb;
> +		err |= __copy_from_user(&osb, tsops, sizeof(osb));
> +		sops[i].sem_num = osb.sem_num;
> +		sops[i].sem_op = osb.sem_op;
> +		sops[i].sem_flg = osb.sem_flg;
>  		tsops++;
>  	}
>  	if (timeout) {
> -- 
> 2.7.4
> 

[PATCH 4/6] ARM: use __inttype() in get_user()

[Russell King]

Borrow the x86 implementation of __inttype() to use in get_user() to
select an integer type suitable to temporarily hold the result value.
This is necessary to avoid propagating the volatile nature of the
result argument, which can cause the following warning:

lib/iov_iter.c:413:5: warning: optimization may eliminate reads and/or writes to register variables [-Wvolatile-register-var]

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/uaccess.h | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 0bf2347495f1..29fa6f3ea25a 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -85,6 +85,13 @@ static inline void set_fs(mm_segment_t fs)
 	flag; })
 
 /*
+ * This is a type: either unsigned long, if the argument fits into
+ * that type, or otherwise unsigned long long.
+ */
+#define __inttype(x) \
+	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
+
+/*
  * 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
@@ -153,7 +160,7 @@ extern int __get_user_64t_4(void *);
 	({								\
 		unsigned long __limit = current_thread_info()->addr_limit - 1; \
 		register const typeof(*(p)) __user *__p asm("r0") = (p);\
-		register typeof(x) __r2 asm("r2");			\
+		register __inttype(x) __r2 asm("r2");			\
 		register unsigned long __l asm("r1") = __limit;		\
 		register int __e asm("r0");				\
 		unsigned int __ua_flags = uaccess_save_and_enable();	\
-- 
2.7.4

[PATCH 4/6] ARM: use __inttype() in get_user()

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:14:02PM +0100, Russell King wrote:
> Borrow the x86 implementation of __inttype() to use in get_user() to
> select an integer type suitable to temporarily hold the result value.
> This is necessary to avoid propagating the volatile nature of the
> result argument, which can cause the following warning:
> 
> lib/iov_iter.c:413:5: warning: optimization may eliminate reads and/or writes to register variables [-Wvolatile-register-var]
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm/include/asm/uaccess.h | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index 0bf2347495f1..29fa6f3ea25a 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -85,6 +85,13 @@ static inline void set_fs(mm_segment_t fs)
>  	flag; })
>  
>  /*
> + * This is a type: either unsigned long, if the argument fits into
> + * that type, or otherwise unsigned long long.
> + */
> +#define __inttype(x) \
> +	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
> +
> +/*
>   * 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
> @@ -153,7 +160,7 @@ extern int __get_user_64t_4(void *);
>  	({								\
>  		unsigned long __limit = current_thread_info()->addr_limit - 1; \
>  		register const typeof(*(p)) __user *__p asm("r0") = (p);\
> -		register typeof(x) __r2 asm("r2");			\
> +		register __inttype(x) __r2 asm("r2");			\
>  		register unsigned long __l asm("r1") = __limit;		\
>  		register int __e asm("r0");				\
>  		unsigned int __ua_flags = uaccess_save_and_enable();	\
> -- 
> 2.7.4
> 

[PATCH 5/6] ARM: spectre-v1: use get_user() for __get_user()

[Russell King]

Fixing __get_user() for spectre variant 1 is not sane: we would have to
add address space bounds checking in order to validate that the location
should be accessed, and then zero the address if found to be invalid.

Since __get_user() is supposed to avoid the bounds check, and this is
exactly what get_user() does, there's no point having two different
implementations that are doing the same thing.  So, when the Spectre
workarounds are required, make __get_user() an alias of get_user().

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/uaccess.h | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 29fa6f3ea25a..4140be431087 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -250,6 +250,16 @@ static inline void set_fs(mm_segment_t fs)
 #define user_addr_max() \
 	(uaccess_kernel() ? ~0UL : get_fs())
 
+#ifdef CONFIG_CPU_SPECTRE
+/*
+ * When mitigating Spectre variant 1, it is not worth fixing the non-
+ * verifying accessors, because we need to add verification of the
+ * address space there.  Force these to use the standard get_user()
+ * version instead.
+ */
+#define __get_user(x, ptr) get_user(x, ptr)
+#else
+
 /*
  * The "__xxx" versions of the user access functions do not verify the
  * address space - it must have been done previously with a separate
@@ -266,12 +276,6 @@ static inline void set_fs(mm_segment_t fs)
 	__gu_err;							\
 })
 
-#define __get_user_error(x, ptr, err)					\
-({									\
-	__get_user_err((x), (ptr), err);				\
-	(void) 0;							\
-})
-
 #define __get_user_err(x, ptr, err)					\
 do {									\
 	unsigned long __gu_addr = (unsigned long)(ptr);			\
@@ -331,6 +335,7 @@ do {									\
 
 #define __get_user_asm_word(x, addr, err)			\
 	__get_user_asm(x, addr, err, ldr)
+#endif
 
 
 #define __put_user_switch(x, ptr, __err, __fn)				\
-- 
2.7.4

[PATCH 5/6] ARM: spectre-v1: use get_user() for __get_user()

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:14:07PM +0100, Russell King wrote:
> Fixing __get_user() for spectre variant 1 is not sane: we would have to
> add address space bounds checking in order to validate that the location
> should be accessed, and then zero the address if found to be invalid.
> 
> Since __get_user() is supposed to avoid the bounds check, and this is
> exactly what get_user() does, there's no point having two different
> implementations that are doing the same thing.  So, when the Spectre
> workarounds are required, make __get_user() an alias of get_user().
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

I believe that we need to do likewise for __put_user(), given
spectre-v1.1.

Thanks,
Mark.

> ---
>  arch/arm/include/asm/uaccess.h | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
> index 29fa6f3ea25a..4140be431087 100644
> --- a/arch/arm/include/asm/uaccess.h
> +++ b/arch/arm/include/asm/uaccess.h
> @@ -250,6 +250,16 @@ static inline void set_fs(mm_segment_t fs)
>  #define user_addr_max() \
>  	(uaccess_kernel() ? ~0UL : get_fs())
>  
> +#ifdef CONFIG_CPU_SPECTRE
> +/*
> + * When mitigating Spectre variant 1, it is not worth fixing the non-
> + * verifying accessors, because we need to add verification of the
> + * address space there.  Force these to use the standard get_user()
> + * version instead.
> + */
> +#define __get_user(x, ptr) get_user(x, ptr)
> +#else
> +
>  /*
>   * The "__xxx" versions of the user access functions do not verify the
>   * address space - it must have been done previously with a separate
> @@ -266,12 +276,6 @@ static inline void set_fs(mm_segment_t fs)
>  	__gu_err;							\
>  })
>  
> -#define __get_user_error(x, ptr, err)					\
> -({									\
> -	__get_user_err((x), (ptr), err);				\
> -	(void) 0;							\
> -})
> -
>  #define __get_user_err(x, ptr, err)					\
>  do {									\
>  	unsigned long __gu_addr = (unsigned long)(ptr);			\
> @@ -331,6 +335,7 @@ do {									\
>  
>  #define __get_user_asm_word(x, addr, err)			\
>  	__get_user_asm(x, addr, err, ldr)
> +#endif
>  
>  
>  #define __put_user_switch(x, ptr, __err, __fn)				\
> -- 
> 2.7.4
> 

[PATCH 5/6] ARM: spectre-v1: use get_user() for __get_user()

[Russell King - ARM Linux]

On Thu, Jul 26, 2018 at 01:44:01PM +0100, Mark Rutland wrote:
> On Tue, Jul 10, 2018 at 03:14:07PM +0100, Russell King wrote:
> > Fixing __get_user() for spectre variant 1 is not sane: we would have to
> > add address space bounds checking in order to validate that the location
> > should be accessed, and then zero the address if found to be invalid.
> > 
> > Since __get_user() is supposed to avoid the bounds check, and this is
> > exactly what get_user() does, there's no point having two different
> > implementations that are doing the same thing.  So, when the Spectre
> > workarounds are required, make __get_user() an alias of get_user().
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> I believe that we need to do likewise for __put_user(), given
> spectre-v1.1.

Spectre v1.1 is not covered by this patch series.

-- 
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 5/6] ARM: spectre-v1: use get_user() for __get_user()

[Mark Rutland]

On Thu, Jul 26, 2018 at 02:19:37PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 26, 2018 at 01:44:01PM +0100, Mark Rutland wrote:
> > On Tue, Jul 10, 2018 at 03:14:07PM +0100, Russell King wrote:
> > > Fixing __get_user() for spectre variant 1 is not sane: we would have to
> > > add address space bounds checking in order to validate that the location
> > > should be accessed, and then zero the address if found to be invalid.
> > > 
> > > Since __get_user() is supposed to avoid the bounds check, and this is
> > > exactly what get_user() does, there's no point having two different
> > > implementations that are doing the same thing.  So, when the Spectre
> > > workarounds are required, make __get_user() an alias of get_user().
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > 
> > I believe that we need to do likewise for __put_user(), given
> > spectre-v1.1.
> 
> Spectre v1.1 is not covered by this patch series.

Ok; given that:

Acked-by: Mark Rutland <mark.rutland@arm.com>

As an aside, on arm64 we always do the full checks in __*_user() since
this also hardens against missing/bad access_ok() checks when PAN is not
in use.

As a future thing, it might be worth having the option to always perform
the full checks.

Mark.

[PATCH 6/6] ARM: spectre-v1: mitigate user accesses

[Russell King]

Spectre variant 1 attacks are about this sequence of pseudo-code:

	index = load(user-manipulated pointer);
	access(base + index * stride);

In order for the cache side-channel to work, the access() must me made
to memory which userspace can detect whether cache lines have been
loaded.  On 32-bit ARM, this must be either user accessible memory, or
a kernel mapping of that same user accessible memory.

The problem occurs when the load() speculatively loads privileged data,
and the subsequent access() is made to user accessible memory.

Any load() which makes use of a user-maniplated pointer is a potential
problem if the data it has loaded is used in a subsequent access.  This
also applies for the access() if the data loaded by that access is used
by a subsequent access.

Harden the get_user() accessors against Spectre attaacks by forcing out
of bounds addresses to a NULL pointer.  This prevents get_user() being
used as the load() step above.  As a side effect, put_user() will also
be affected even though it isn't implicated.

Also harden copy_from_user() by redoing the bounds check within the
arm_copy_from_user() code, and NULLing the pointer if out of bounds.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/assembler.h | 4 ++++
 arch/arm/lib/copy_from_user.S    | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index ef1386b1af9b..f0515f60cff5 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -460,6 +460,10 @@ THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
 	adds	\tmp, \addr, #\size - 1
 	sbcccs	\tmp, \tmp, \limit
 	bcs	\bad
+#ifdef CONFIG_CPU_SPECTRE
+	movcs	\addr, #0
+	csdb
+#endif
 #endif
 	.endm
 
diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
index 7a4b06049001..ebf292e9478f 100644
--- a/arch/arm/lib/copy_from_user.S
+++ b/arch/arm/lib/copy_from_user.S
@@ -90,6 +90,13 @@
 	.text
 
 ENTRY(arm_copy_from_user)
+	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
 
 #include "copy_template.S"
 
-- 
2.7.4

[PATCH 6/6] ARM: spectre-v1: mitigate user accesses

[Mark Rutland]

On Tue, Jul 10, 2018 at 03:14:12PM +0100, Russell King wrote:
> Spectre variant 1 attacks are about this sequence of pseudo-code:
> 
> 	index = load(user-manipulated pointer);
> 	access(base + index * stride);
> 
> In order for the cache side-channel to work, the access() must me made
> to memory which userspace can detect whether cache lines have been
> loaded.  On 32-bit ARM, this must be either user accessible memory, or
> a kernel mapping of that same user accessible memory.
> 
> The problem occurs when the load() speculatively loads privileged data,
> and the subsequent access() is made to user accessible memory.
> 
> Any load() which makes use of a user-maniplated pointer is a potential
> problem if the data it has loaded is used in a subsequent access.  This
> also applies for the access() if the data loaded by that access is used
> by a subsequent access.
> 
> Harden the get_user() accessors against Spectre attaacks by forcing out
> of bounds addresses to a NULL pointer.  This prevents get_user() being
> used as the load() step above.  As a side effect, put_user() will also
> be affected even though it isn't implicated.
> 
> Also harden copy_from_user() by redoing the bounds check within the
> arm_copy_from_user() code, and NULLing the pointer if out of bounds.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  arch/arm/include/asm/assembler.h | 4 ++++
>  arch/arm/lib/copy_from_user.S    | 7 +++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index ef1386b1af9b..f0515f60cff5 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -460,6 +460,10 @@ THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
>  	adds	\tmp, \addr, #\size - 1
>  	sbcccs	\tmp, \tmp, \limit
>  	bcs	\bad
> +#ifdef CONFIG_CPU_SPECTRE
> +	movcs	\addr, #0
> +	csdb
> +#endif
>  #endif
>  	.endm
>  
> diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> index 7a4b06049001..ebf292e9478f 100644
> --- a/arch/arm/lib/copy_from_user.S
> +++ b/arch/arm/lib/copy_from_user.S
> @@ -90,6 +90,13 @@
>  	.text
>  
>  ENTRY(arm_copy_from_user)
> +	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

Given spectre-v1.1, I believe we need to do the same for
arm_copy_to_user().

I'm a little worried that we might need to do something for
__copy_{to,from}_user_memcpy(), since we could mis-speculate the
uaccess_kernel() check (or other branches), and go straight to the
unsanitized memcpy().

I suspect we want to pull out the pointer sanitization into a reusable
helper like arm64's uaccess_mask_ptr().

Thanks,
Mark.

[PATCH 6/6] ARM: spectre-v1: mitigate user accesses

[Russell King - ARM Linux]

On Thu, Jul 26, 2018 at 01:49:00PM +0100, Mark Rutland wrote:
> On Tue, Jul 10, 2018 at 03:14:12PM +0100, Russell King wrote:
> > Spectre variant 1 attacks are about this sequence of pseudo-code:
> > 
> > 	index = load(user-manipulated pointer);
> > 	access(base + index * stride);
> > 
> > In order for the cache side-channel to work, the access() must me made
> > to memory which userspace can detect whether cache lines have been
> > loaded.  On 32-bit ARM, this must be either user accessible memory, or
> > a kernel mapping of that same user accessible memory.
> > 
> > The problem occurs when the load() speculatively loads privileged data,
> > and the subsequent access() is made to user accessible memory.
> > 
> > Any load() which makes use of a user-maniplated pointer is a potential
> > problem if the data it has loaded is used in a subsequent access.  This
> > also applies for the access() if the data loaded by that access is used
> > by a subsequent access.
> > 
> > Harden the get_user() accessors against Spectre attaacks by forcing out
> > of bounds addresses to a NULL pointer.  This prevents get_user() being
> > used as the load() step above.  As a side effect, put_user() will also
> > be affected even though it isn't implicated.
> > 
> > Also harden copy_from_user() by redoing the bounds check within the
> > arm_copy_from_user() code, and NULLing the pointer if out of bounds.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  arch/arm/include/asm/assembler.h | 4 ++++
> >  arch/arm/lib/copy_from_user.S    | 7 +++++++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> > index ef1386b1af9b..f0515f60cff5 100644
> > --- a/arch/arm/include/asm/assembler.h
> > +++ b/arch/arm/include/asm/assembler.h
> > @@ -460,6 +460,10 @@ THUMB(	orr	\reg , \reg , #PSR_T_BIT	)
> >  	adds	\tmp, \addr, #\size - 1
> >  	sbcccs	\tmp, \tmp, \limit
> >  	bcs	\bad
> > +#ifdef CONFIG_CPU_SPECTRE
> > +	movcs	\addr, #0
> > +	csdb
> > +#endif
> >  #endif
> >  	.endm
> >  
> > diff --git a/arch/arm/lib/copy_from_user.S b/arch/arm/lib/copy_from_user.S
> > index 7a4b06049001..ebf292e9478f 100644
> > --- a/arch/arm/lib/copy_from_user.S
> > +++ b/arch/arm/lib/copy_from_user.S
> > @@ -90,6 +90,13 @@
> >  	.text
> >  
> >  ENTRY(arm_copy_from_user)
> > +	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
> 
> Given spectre-v1.1, I believe we need to do the same for
> arm_copy_to_user().

Spectre v1.1 is not covered by this patch series and is a subject for
future work.

-- 
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 6/6] ARM: spectre-v1: mitigate user accesses

[Robert Jarzmik]

Russell King - ARM Linux <linux@armlinux.org.uk> writes:

> On Thu, Jul 26, 2018 at 01:49:00PM +0100, Mark Rutland wrote:
>> On Tue, Jul 10, 2018 at 03:14:12PM +0100, Russell King wrote:

>> > Harden the get_user() accessors against Spectre attaacks by forcing out
Small typo: s/attaacks/attacks/

Cheers.

-- 
Robert

[PATCH 6/6] ARM: spectre-v1: mitigate user accesses

[Russell King - ARM Linux]

On Thu, Jul 26, 2018 at 01:49:00PM +0100, Mark Rutland wrote:
> I'm a little worried that we might need to do something for
> __copy_{to,from}_user_memcpy(), since we could mis-speculate the
> uaccess_kernel() check (or other branches), and go straight to the
> unsanitized memcpy().

There is no __copy_from_user_memcpy function:

$ git grep copy_from_user_memcpy
$

Since this patch series is only addressing Spectre Variant 1 as
published in January, __copy_to_user_memcpy() is not relevant to it.

__copy_to_user_memcpy() is relevent for a future patch series
addressing more recently released CVEs.

-- 
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 6/6] ARM: spectre-v1: mitigate user accesses

[Mark Rutland]

On Thu, Jul 26, 2018 at 03:12:05PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 26, 2018 at 01:49:00PM +0100, Mark Rutland wrote:
> > I'm a little worried that we might need to do something for
> > __copy_{to,from}_user_memcpy(), since we could mis-speculate the
> > uaccess_kernel() check (or other branches), and go straight to the
> > unsanitized memcpy().
> 
> There is no __copy_from_user_memcpy function:
> 
> $ git grep copy_from_user_memcpy
> $

My bad. I had seen __copy_to_user_memcpy and assumed that there was a
__copy_from_user_memcpy.

Sorry for the noise.

Othewise:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

[PATCH 0/6] Further spectre variant 1 mitigations

[Russell King - ARM Linux]

I don't have any public feedback on this series... so, I guess no one
is really interested in this?

On Tue, Jul 10, 2018 at 03:13:22PM +0100, Russell King - ARM Linux wrote:
> Hi,
> 
> This series applies the mitigations for Spectre variant 1 to 32-bit
> ARM user accessors.  This series slightly larger by the need to
> offset the additional overhead introduced by these checks in various
> paths, particularly the signal handling path.
> 
>  arch/arm/include/asm/assembler.h   |  4 +++
>  arch/arm/include/asm/thread_info.h |  2 +-
>  arch/arm/include/asm/uaccess.h     | 26 ++++++++++++-----
>  arch/arm/kernel/signal.c           | 58 +++++++++++++++++++-------------------
>  arch/arm/kernel/sys_oabi-compat.c  |  8 ++++--
>  arch/arm/lib/copy_from_user.S      |  7 +++++
>  arch/arm/vfp/vfpmodule.c           | 16 +++++------
>  7 files changed, 72 insertions(+), 49 deletions(-)
> 
> -- 
> 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

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