diff mbox

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

Message ID 20151210002213.GL8644@n2100.arm.linux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King - ARM Linux Dec. 10, 2015, 12:22 a.m. UTC 
On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> I took both patches for a quick spin (a dozen boots and one hour uptime
> after that for each patch) and no incidents. I have not gathered data,
> but the crash on boot feels like it's quite a bit above 50% when there
> is a problem so this feels good (I used 5 clean reboots when I bisected
> and that worked).
> 
> Reported-by: Peter Rosin <peda@axentia.se>
> Tested-by: Peter Rosin <peda@axentia.se>
> 
> (and please don't forget to cc stable)

I've decided to do a more in-depth fix, so that we also solve the issue
that when we schedule in these down_read()s, we don't leak the permissive
domain register setting into the switched-to context.

Can you test this patch please?  Thanks.

8<====
Subject: [PATCH] ARM: fix uaccess_with_memcpy() with SW_DOMAIN_PAN

The uaccess_with_memcpy() code is currently incompatible with the SW
PAN code: it takes locks within the region that we've changed the DACR,
potentially sleeping as a result.  As we do not save and restore the
DACR across co-operative sleep events, can lead to an incorrect DACR
value later in this code path.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/uaccess.h     |  4 ++++
 arch/arm/lib/uaccess_with_memcpy.c | 29 +++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

Comments

Peter Rosin Dec. 10, 2015, 3:29 p.m. UTC | #1 
Russell King wrote:
> On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > I took both patches for a quick spin (a dozen boots and one hour uptime
> > after that for each patch) and no incidents. I have not gathered data,
> > but the crash on boot feels like it's quite a bit above 50% when there
> > is a problem so this feels good (I used 5 clean reboots when I bisected
> > and that worked).
> > 
> > Reported-by: Peter Rosin <peda@axentia.se>
> > Tested-by: Peter Rosin <peda@axentia.se>
> > 
> > (and please don't forget to cc stable)
> 
> I've decided to do a more in-depth fix, so that we also solve the issue
> that when we schedule in these down_read()s, we don't leak the permissive
> domain register setting into the switched-to context.
> 
> Can you test this patch please?  Thanks.

Still looking good.

Cheers,
Peter
Russell King - ARM Linux Dec. 10, 2015, 4:20 p.m. UTC | #2 
On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> Russell King wrote:
> > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > I took both patches for a quick spin (a dozen boots and one hour uptime
> > > after that for each patch) and no incidents. I have not gathered data,
> > > but the crash on boot feels like it's quite a bit above 50% when there
> > > is a problem so this feels good (I used 5 clean reboots when I bisected
> > > and that worked).
> > > 
> > > Reported-by: Peter Rosin <peda@axentia.se>
> > > Tested-by: Peter Rosin <peda@axentia.se>
> > > 
> > > (and please don't forget to cc stable)
> > 
> > I've decided to do a more in-depth fix, so that we also solve the issue
> > that when we schedule in these down_read()s, we don't leak the permissive
> > domain register setting into the switched-to context.
> > 
> > Can you test this patch please?  Thanks.
> 
> Still looking good.

Does that mean I can add your reported and tested-by to this latest
patch?

Thanks.
Peter Rosin Dec. 10, 2015, 6:32 p.m. UTC | #3 
Russell King wrote:
> On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> > Russell King wrote:
> > > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > > I took both patches for a quick spin (a dozen boots and one hour 
> > > > uptime after that for each patch) and no incidents. I have not 
> > > > gathered data, but the crash on boot feels like it's quite a bit 
> > > > above 50% when there is a problem so this feels good (I used 5 
> > > > clean reboots when I bisected and that worked).
> > > > 
> > > > Reported-by: Peter Rosin <peda@axentia.se>
> > > > Tested-by: Peter Rosin <peda@axentia.se>
> > > > 
> > > > (and please don't forget to cc stable)
> > > 
> > > I've decided to do a more in-depth fix, so that we also solve the 
> > > issue that when we schedule in these down_read()s, we don't leak the 
> > > permissive domain register setting into the switched-to context.
> > > 
> > > Can you test this patch please?  Thanks.
> > 
> > Still looking good.
> 
> Does that mean I can add your reported and tested-by to this latest patch?

Right, I thought that was obvious, sorry for the confusion.

Cheers,
Peter
Peter Rosin Dec. 30, 2015, 4:51 p.m. UTC | #4 
[I repeat myself just in case my last message disappeared. It would be
 a shame if 4.4 was also regressed because of a missing response.]

I wrote:
> Russell King wrote:
> > On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> > > Russell King wrote:
> > > > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > > > I took both patches for a quick spin (a dozen boots and one hour 
> > > > > uptime after that for each patch) and no incidents. I have not 
> > > > > gathered data, but the crash on boot feels like it's quite a bit 
> > > > > above 50% when there is a problem so this feels good (I used 5 
> > > > > clean reboots when I bisected and that worked).
> > > > > 
> > > > > Reported-by: Peter Rosin <peda@axentia.se>
> > > > > Tested-by: Peter Rosin <peda@axentia.se>
> > > > > 
> > > > > (and please don't forget to cc stable)
> > > > 
> > > > I've decided to do a more in-depth fix, so that we also solve the 
> > > > issue that when we schedule in these down_read()s, we don't leak 
> > > > the permissive domain register setting into the switched-to context.
> > > > 
> > > > Can you test this patch please?  Thanks.
> > > 
> > > Still looking good.
> > 
> > Does that mean I can add your reported and tested-by to this latest patch?
> 
> Right, I thought that was obvious, sorry for the confusion.

Reported-by: Peter Rosin <peda@axentia.se>
Tested-by: Peter Rosin <peda@axentia.se>

(and please don't forget to cc stable)

Cheers,
Peter
Peter Rosin Dec. 30, 2015, 4:57 p.m. UTC | #5 
I wrote:
> [I repeat myself just in case my last message disappeared. It would be
>  a shame if 4.4 was also regressed because of a missing response.]

Crap, I should have checked one more time before hitting send. The patch
is already in there! Sorry for the confusion.

Cheers,
Peter
diff mbox

Patch

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 8cc85a4ebec2..35c9db857ebe 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -510,10 +510,14 @@  __copy_to_user_std(void __user *to, const void *from, unsigned long n);
 static inline unsigned long __must_check
 __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+#ifndef CONFIG_UACCESS_WITH_MEMCPY
 	unsigned int __ua_flags = uaccess_save_and_enable();
 	n = arm_copy_to_user(to, from, n);
 	uaccess_restore(__ua_flags);
 	return n;
+#else
+	return arm_copy_to_user(to, from, n);
+#endif
 }
 
 extern unsigned long __must_check
diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c
index d72b90905132..588bbc288396 100644
--- a/arch/arm/lib/uaccess_with_memcpy.c
+++ b/arch/arm/lib/uaccess_with_memcpy.c
@@ -88,6 +88,7 @@  pin_page_for_write(const void __user *_addr, pte_t **ptep, spinlock_t **ptlp)
 static unsigned long noinline
 __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 {
+	unsigned long ua_flags;
 	int atomic;
 
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
@@ -118,7 +119,9 @@  __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		ua_flags = uaccess_save_and_enable();
 		memcpy((void *)to, from, tocopy);
+		uaccess_restore(ua_flags);
 		to += tocopy;
 		from += tocopy;
 		n -= tocopy;
@@ -145,14 +148,21 @@  arm_copy_to_user(void __user *to, const void *from, unsigned long n)
 	 * With frame pointer disabled, tail call optimization kicks in
 	 * as well making this test almost invisible.
 	 */
-	if (n < 64)
-		return __copy_to_user_std(to, from, n);
-	return __copy_to_user_memcpy(to, from, n);
+	if (n < 64) {
+		unsigned long ua_flags = uaccess_save_and_enable();
+		n = __copy_to_user_std(to, from, n);
+		uaccess_restore(ua_flags);
+	} else {
+		n = __copy_to_user_memcpy(to, from, n);
+	}
+	return n;
 }
 	
 static unsigned long noinline
 __clear_user_memset(void __user *addr, unsigned long n)
 {
+	unsigned long ua_flags;
+
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
 		memset((void *)addr, 0, n);
 		return 0;
@@ -175,7 +185,9 @@  __clear_user_memset(void __user *addr, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		ua_flags = uaccess_save_and_enable();
 		memset((void *)addr, 0, tocopy);
+		uaccess_restore(ua_flags);
 		addr += tocopy;
 		n -= tocopy;
 
@@ -193,9 +205,14 @@  __clear_user_memset(void __user *addr, unsigned long n)
 unsigned long arm_clear_user(void __user *addr, unsigned long n)
 {
 	/* See rational for this in __copy_to_user() above. */
-	if (n < 64)
-		return __clear_user_std(addr, n);
-	return __clear_user_memset(addr, n);
+	if (n < 64) {
+		unsigned long ua_flags = uaccess_save_and_enable();
+		n = __clear_user_std(addr, n);
+		uaccess_restore(ua_flags);
+	} else {
+		n = __clear_user_memset(addr, n);
+	}
+	return n;
 }
 
 #if 0