[PATCH v2 00/11] test_user_copy improvements

[PATCH v2 00/11] test_user_copy improvements

[James Hogan]

These patches extend the test_user_copy test module to handle lots more
cases of user accessors which architectures can override separately, and
in particular those which are important for checking the MIPS Enhanced
Virtual Addressing (EVA) implementations, which need to handle
overlapping user and kernel address spaces, with special instructions
for accessing user address space from kernel mode.

- Checking that kernel pointers are accepted when user address limit is
  set to KERNEL_DS, as done by the kernel when it internally invokes
  system calls with kernel pointers.
- Checking of the unchecked accessors (which don't call access_ok()).
  Some of the tests are special cased for EVA at the moment which has
  stricter hardware guarantees for bad user accesses than other
  configurations.
- Checking of other sets of user accessors, including the inatomic user
  copies, clear_user, compatibility accessors (copy_in_user and
  _unaligned), the user string accessors, and the user checksum
  functions, all of which need special handling in arch code with EVA.

Tested on MIPS with and without EVA, and on x86_64.

Only build tested for arm, blackfin, metag, microblaze, openrisc,
parisc, powerpc, sh, sparc, tile, i386 & xtensa.

All arches were audited for the appropriate exports, only score is known
to still be missing some.

Changes in v2:
- Add arch exports (patches 1-4).
- Reorder patches slightly.
- Patch 9: Drop strlen_user test. Microblaze doesn't define it, and
  nothing actually uses it. IMO it should be removed, and there's no
  point testing it in the mean time.
- Patch 10: Conditionalise on CONFIG_COMPAT, otherwise it breaks build
  on some 32-bit arches e.g. i386 (kbuild test robot).
- Patch 10: Add testing of _unaligned accessors, which are also
  conditional upon CONFIG_COMPAT.
- Patch 11: Only test csum_partial_copy_from_user #ifndef
  _HAVE_ARCH_COPY_AND_CSUM_FROM_USER, fixing powerpc64 build (Stephen
  Rothwell)

James Hogan (11):
  microblaze: Export __strnlen_user to modules
  nios2: Export strncpy_from_user / strnlen_user to modules
  openrisc: Export __clear_user to modules
  xtensa: Export __strnlen_user to modules
  test_user_copy: Check legit kernel accesses
  test_user_copy: Check unchecked accessors
  test_user_copy: Check __copy_{to,from}_user_inatomic()
  test_user_copy: Check __clear_user()/clear_user()
  test_user_copy: Check user string accessors
  test_user_copy: Check user compatibility accessors
  test_user_copy: Check user checksum functions

 arch/microblaze/kernel/microblaze_ksyms.c |   1 +
 arch/nios2/mm/uaccess.c                   |   2 +
 arch/openrisc/kernel/or32_ksyms.c         |   1 +
 arch/xtensa/kernel/xtensa_ksyms.c         |   1 +
 lib/test_user_copy.c                      | 251 ++++++++++++++++++++++++++++++
 5 files changed, 256 insertions(+)

Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Ley Foon Tan <lftan@altera.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: nios2-dev@lists.rocketboards.org
Cc: linux@lists.openrisc.net
Cc: linux-xtensa@linux-xtensa.org
-- 
2.3.6

[PATCH v2 01/11] microblaze: Export __strnlen_user to modules

[James Hogan]

Update the microblaze architecture code to export __strnlen_user() to
modules, so they can make use of strnlen_user(). For example the
test_user_copy module will soon test it.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Michal Simek <monstr@monstr.eu>
---
 arch/microblaze/kernel/microblaze_ksyms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/microblaze/kernel/microblaze_ksyms.c b/arch/microblaze/kernel/microblaze_ksyms.c
index 9f1d02c4c5cc..baf7a2340d97 100644
--- a/arch/microblaze/kernel/microblaze_ksyms.c
+++ b/arch/microblaze/kernel/microblaze_ksyms.c
@@ -31,6 +31,7 @@ EXPORT_SYMBOL(_mcount);
  */
 EXPORT_SYMBOL(__copy_tofrom_user);
 EXPORT_SYMBOL(__strncpy_user);
+EXPORT_SYMBOL(__strnlen_user);
 
 #ifdef CONFIG_OPT_LIB_ASM
 EXPORT_SYMBOL(memcpy);
-- 
2.3.6

[PATCH v2 02/11] nios2: Export strncpy_from_user / strnlen_user to modules

[James Hogan]

Update the nios2 architecture code to export strncpy_from_user() and
strnlen_user() to modules. The test_user_copy module will soon test
them.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ley Foon Tan <lftan@altera.com>
Cc: nios2-dev@lists.rocketboards.org
---
 arch/nios2/mm/uaccess.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/nios2/mm/uaccess.c b/arch/nios2/mm/uaccess.c
index 7663e156ff4f..ecd8b122948b 100644
--- a/arch/nios2/mm/uaccess.c
+++ b/arch/nios2/mm/uaccess.c
@@ -146,6 +146,7 @@ long strncpy_from_user(char *__to, const char __user *__from, long __len)
 		l--;
 	return l;
 }
+EXPORT_SYMBOL(strncpy_from_user);
 
 long strnlen_user(const char __user *s, long n)
 {
@@ -161,3 +162,4 @@ long strnlen_user(const char __user *s, long n)
 	}
 	return n + 1;
 }
+EXPORT_SYMBOL(strnlen_user);
-- 
2.3.6

Re: [PATCH v2 02/11] nios2: Export strncpy_from_user / strnlen_user to modules

[Ley Foon Tan]

On Fri, Aug 7, 2015 at 11:21 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Update the nios2 architecture code to export strncpy_from_user() and
> strnlen_user() to modules. The test_user_copy module will soon test
> them.
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Ley Foon Tan <lftan@altera.com>
> Cc: nios2-dev@lists.rocketboards.org
> ---
>  arch/nios2/mm/uaccess.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/nios2/mm/uaccess.c b/arch/nios2/mm/uaccess.c
> index 7663e156ff4f..ecd8b122948b 100644
> --- a/arch/nios2/mm/uaccess.c
> +++ b/arch/nios2/mm/uaccess.c
> @@ -146,6 +146,7 @@ long strncpy_from_user(char *__to, const char __user *__from, long __len)
>                 l--;
>         return l;
>  }
> +EXPORT_SYMBOL(strncpy_from_user);
>
>  long strnlen_user(const char __user *s, long n)
>  {
> @@ -161,3 +162,4 @@ long strnlen_user(const char __user *s, long n)
>         }
>         return n + 1;
>  }
> +EXPORT_SYMBOL(strnlen_user);

Acked-by: Ley Foon Tan <lftan@altera.com>

[PATCH v2 03/11] openrisc: Export __clear_user to modules

[James Hogan]

Update the OpenRISC architecture code to export __clear_user() to
modules, so that modules can make use of clear_user(). For example the
test_user_copy module will soon test it.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: linux@lists.openrisc.net
---
 arch/openrisc/kernel/or32_ksyms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/openrisc/kernel/or32_ksyms.c b/arch/openrisc/kernel/or32_ksyms.c
index 83ccf7c0c58d..54eae8b40fda 100644
--- a/arch/openrisc/kernel/or32_ksyms.c
+++ b/arch/openrisc/kernel/or32_ksyms.c
@@ -44,3 +44,4 @@ DECLARE_EXPORT(__ashldi3);
 DECLARE_EXPORT(__lshrdi3);
 
 EXPORT_SYMBOL(__copy_tofrom_user);
+EXPORT_SYMBOL(__clear_user);
-- 
2.3.6

[PATCH v2 04/11] xtensa: Export __strnlen_user to modules

[James Hogan]

Update the Xtensa architecture code to export __strnlen_user() to
modules, so that modules can make use of strnlen_user() and
strlen_user(). For example the test_user_copy module will soon test
them.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
Cc: linux-xtensa@linux-xtensa.org
---
 arch/xtensa/kernel/xtensa_ksyms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/xtensa/kernel/xtensa_ksyms.c b/arch/xtensa/kernel/xtensa_ksyms.c
index 4d2872fd9bb5..1a47bb621816 100644
--- a/arch/xtensa/kernel/xtensa_ksyms.c
+++ b/arch/xtensa/kernel/xtensa_ksyms.c
@@ -42,6 +42,7 @@ EXPORT_SYMBOL(memset);
 EXPORT_SYMBOL(memcpy);
 EXPORT_SYMBOL(memmove);
 EXPORT_SYMBOL(__strncpy_user);
+EXPORT_SYMBOL(__strnlen_user);
 EXPORT_SYMBOL(clear_page);
 EXPORT_SYMBOL(copy_page);
 
-- 
2.3.6

[PATCH v2 05/11] test_user_copy: Check legit kernel accesses

[James Hogan]

Check that the use of the user accessors for accessing kernel memory
succeed as expected after set_fs(get_ds()) is used to increases the
address limit, as used by the kernel to directly invoke system call code
with kernel pointers.

The tests are basically the same as the tests normally expected to be
treated as invalid, but without any user addresses (no reversed copies),
and with the result inverted such that they should succeed instead.

New tests:
- legitimate all-kernel copy_from_user
- legitimate all-kernel copy_to_user
- legitimate kernel get_user
- legitimate kernel put_user

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/test_user_copy.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 0ecef3e4690e..445ca92b0b80 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -41,6 +41,7 @@ static int __init test_user_copy_init(void)
 	char *bad_usermem;
 	unsigned long user_addr;
 	unsigned long value = 0x5A;
+	mm_segment_t fs = get_fs();
 
 	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
 	if (!kmem)
@@ -86,6 +87,28 @@ static int __init test_user_copy_init(void)
 	ret |= test(!put_user(value, (unsigned long __user *)kmem),
 		    "illegal put_user passed");
 
+	/*
+	 * Test access to kernel memory by adjusting address limit.
+	 * This is used by the kernel to invoke system calls with kernel
+	 * pointers.
+	 */
+	set_fs(get_ds());
+
+	/* Legitimate usage: none of these should fail. */
+	ret |= test(copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+				   PAGE_SIZE),
+		    "legitimate all-kernel copy_from_user failed");
+	ret |= test(copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+				 PAGE_SIZE),
+		    "legitimate all-kernel copy_to_user failed");
+	ret |= test(get_user(value, (unsigned long __user *)kmem),
+		    "legitimate kernel get_user failed");
+	ret |= test(put_user(value, (unsigned long __user *)kmem),
+		    "legitimate kernel put_user failed");
+
+	/* Restore previous address limit. */
+	set_fs(fs);
+
 	vm_munmap(user_addr, PAGE_SIZE * 2);
 	kfree(kmem);
 
-- 
2.3.6

[PATCH v2 06/11] test_user_copy: Check unchecked accessors

[James Hogan]

Currently the test_user_copy module only tests the user accessors which
already check the address with access_ok(). Corresponding unchecked
accessors exist however which may be used after access_ok() is checked.
Since the addresses the test uses are known to be valid kernel
addresses, test these unchecked accessors too, as well as access_ok()
itself.

For legitimate user accesses we test the corresponding unchecked macros.
They should all work just as well as the checking versions.

Similarly, for legitimate kernel accesses the unchecked macros are
expected to succeed, and access kernel rather than user memory.

For invalid user accesses we only test the corresponding unchecked
macros in configurations where the behaviour is both known and
important to verify the implementation. Currently this is only enabled
for MIPS with Enhanced Virtual Addressing (EVA) enabled, where user
accesses MUST use the EVA loads/stores, which can only access valid user
mode memory anyway.

New tests:
- legitimate access_ok VERIFY_READ
- legitimate access_ok VERIFY_WRITE
- legitimate __copy_from_user
- legitimate __copy_to_user
- legitimate __get_user
- legitimate __put_user
- illegal all-kernel __copy_from_user
- illegal reversed __copy_from_user
- illegal all-kernel __copy_to_user
- illegal reversed __copy_to_user
- illegal __get_user
- illegal __put_user
- legitimate kernel access_ok VERIFY_READ
- legitimate kernel access_ok VERIFY_WRITE
- legitimate all-kernel __copy_from_user
- legitimate all-kernel __copy_to_user
- legitimate kernel __get_user
- legitimate kernel __put_user

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/test_user_copy.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 445ca92b0b80..23fb9d15f50c 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -69,6 +69,19 @@ static int __init test_user_copy_init(void)
 	ret |= test(put_user(value, (unsigned long __user *)usermem),
 		    "legitimate put_user failed");
 
+	ret |= test(!access_ok(VERIFY_READ, usermem, PAGE_SIZE * 2),
+		    "legitimate access_ok VERIFY_READ failed");
+	ret |= test(!access_ok(VERIFY_WRITE, usermem, PAGE_SIZE * 2),
+		    "legitimate access_ok VERIFY_WRITE failed");
+	ret |= test(__copy_from_user(kmem, usermem, PAGE_SIZE),
+		    "legitimate __copy_from_user failed");
+	ret |= test(__copy_to_user(usermem, kmem, PAGE_SIZE),
+		    "legitimate __copy_to_user failed");
+	ret |= test(__get_user(value, (unsigned long __user *)usermem),
+		    "legitimate __get_user failed");
+	ret |= test(__put_user(value, (unsigned long __user *)usermem),
+		    "legitimate __put_user failed");
+
 	/* Invalid usage: none of these should succeed. */
 	ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
 				    PAGE_SIZE),
@@ -88,6 +101,36 @@ static int __init test_user_copy_init(void)
 		    "illegal put_user passed");
 
 	/*
+	 * If unchecked user accesses (__*) on this architecture cannot access
+	 * kernel mode (i.e. access_ok() is redundant), and usually faults when
+	 * attempted, check this behaviour.
+	 *
+	 * These tests are enabled for:
+	 * - MIPS with Enhanced Virtual Addressing (EVA): user accesses use EVA
+	 *   instructions which can only access user mode accessible memory. It
+	 *   is assumed to be unlikely that user address space mappings will
+	 *   intersect the kernel buffer address.
+	 */
+#if defined(CONFIG_MIPS) && defined(CONFIG_EVA)
+	ret |= test(!__copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+				      PAGE_SIZE),
+		    "illegal all-kernel __copy_from_user passed");
+	ret |= test(!__copy_from_user(bad_usermem, (char __user *)kmem,
+				      PAGE_SIZE),
+		    "illegal reversed __copy_from_user passed");
+	ret |= test(!__copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+				    PAGE_SIZE),
+		    "illegal all-kernel __copy_to_user passed");
+	ret |= test(!__copy_to_user((char __user *)kmem, bad_usermem,
+				    PAGE_SIZE),
+		    "illegal reversed __copy_to_user passed");
+	ret |= test(!__get_user(value, (unsigned long __user *)kmem),
+		    "illegal __get_user passed");
+	ret |= test(!__put_user(value, (unsigned long __user *)kmem),
+		    "illegal __put_user passed");
+#endif
+
+	/*
 	 * Test access to kernel memory by adjusting address limit.
 	 * This is used by the kernel to invoke system calls with kernel
 	 * pointers.
@@ -106,6 +149,22 @@ static int __init test_user_copy_init(void)
 	ret |= test(put_user(value, (unsigned long __user *)kmem),
 		    "legitimate kernel put_user failed");
 
+	ret |= test(!access_ok(VERIFY_READ, (char __user *)kmem, PAGE_SIZE * 2),
+		    "legitimate kernel access_ok VERIFY_READ failed");
+	ret |= test(!access_ok(VERIFY_WRITE, (char __user *)kmem,
+			       PAGE_SIZE * 2),
+		    "legitimate kernel access_ok VERIFY_WRITE failed");
+	ret |= test(__copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+				     PAGE_SIZE),
+		    "legitimate all-kernel __copy_from_user failed");
+	ret |= test(__copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
+				   PAGE_SIZE),
+		    "legitimate all-kernel __copy_to_user failed");
+	ret |= test(__get_user(value, (unsigned long __user *)kmem),
+		    "legitimate kernel __get_user failed");
+	ret |= test(__put_user(value, (unsigned long __user *)kmem),
+		    "legitimate kernel __put_user failed");
+
 	/* Restore previous address limit. */
 	set_fs(fs);
 
-- 
2.3.6

[PATCH v2 07/11] test_user_copy: Check __copy_{to,from}_user_inatomic()

[James Hogan]

Add basic success/failure checking of __copy_to_user_inatomic() and
__copy_from_user_inatomic(). For testing purposes these are similar to
their non-atomic non-checking friends, so the new tests match those for
__copy_to_user() and __copy_from_user().

New tests:
- legitimate __copy_from_user_inatomic
- legitimate __copy_to_user_inatomic
- illegal all-kernel __copy_from_user_inatomic
- illegal reversed __copy_from_user_inatomic
- illegal all-kernel __copy_to_user_inatomic
- illegal reversed __copy_to_user_inatomic
- legitimate all-kernel __copy_from_user_inatomic
- legitimate all-kernel __copy_to_user_inatomic

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/test_user_copy.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 23fb9d15f50c..c002fc1286bd 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -75,8 +75,12 @@ static int __init test_user_copy_init(void)
 		    "legitimate access_ok VERIFY_WRITE failed");
 	ret |= test(__copy_from_user(kmem, usermem, PAGE_SIZE),
 		    "legitimate __copy_from_user failed");
+	ret |= test(__copy_from_user_inatomic(kmem, usermem, PAGE_SIZE),
+		    "legitimate __copy_from_user_inatomic failed");
 	ret |= test(__copy_to_user(usermem, kmem, PAGE_SIZE),
 		    "legitimate __copy_to_user failed");
+	ret |= test(__copy_to_user_inatomic(usermem, kmem, PAGE_SIZE),
+		    "legitimate __copy_to_user_inatomic failed");
 	ret |= test(__get_user(value, (unsigned long __user *)usermem),
 		    "legitimate __get_user failed");
 	ret |= test(__put_user(value, (unsigned long __user *)usermem),
@@ -118,12 +122,25 @@ static int __init test_user_copy_init(void)
 	ret |= test(!__copy_from_user(bad_usermem, (char __user *)kmem,
 				      PAGE_SIZE),
 		    "illegal reversed __copy_from_user passed");
+	ret |= test(!__copy_from_user_inatomic(kmem,
+					(char __user *)(kmem + PAGE_SIZE),
+					PAGE_SIZE),
+		    "illegal all-kernel __copy_from_user_inatomic passed");
+	ret |= test(!__copy_from_user_inatomic(bad_usermem, (char __user *)kmem,
+					       PAGE_SIZE),
+		    "illegal reversed __copy_from_user_inatomic passed");
 	ret |= test(!__copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
 				    PAGE_SIZE),
 		    "illegal all-kernel __copy_to_user passed");
 	ret |= test(!__copy_to_user((char __user *)kmem, bad_usermem,
 				    PAGE_SIZE),
 		    "illegal reversed __copy_to_user passed");
+	ret |= test(!__copy_to_user_inatomic((char __user *)kmem,
+					     kmem + PAGE_SIZE, PAGE_SIZE),
+		    "illegal all-kernel __copy_to_user_inatomic passed");
+	ret |= test(!__copy_to_user_inatomic((char __user *)kmem, bad_usermem,
+					     PAGE_SIZE),
+		    "illegal reversed __copy_to_user_inatomic passed");
 	ret |= test(!__get_user(value, (unsigned long __user *)kmem),
 		    "illegal __get_user passed");
 	ret |= test(!__put_user(value, (unsigned long __user *)kmem),
@@ -157,9 +174,16 @@ static int __init test_user_copy_init(void)
 	ret |= test(__copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
 				     PAGE_SIZE),
 		    "legitimate all-kernel __copy_from_user failed");
+	ret |= test(__copy_from_user_inatomic(kmem,
+					      (char __user *)(kmem + PAGE_SIZE),
+					      PAGE_SIZE),
+		    "legitimate all-kernel __copy_from_user_inatomic failed");
 	ret |= test(__copy_to_user((char __user *)kmem, kmem + PAGE_SIZE,
 				   PAGE_SIZE),
 		    "legitimate all-kernel __copy_to_user failed");
+	ret |= test(__copy_to_user_inatomic((char __user *)kmem,
+					    kmem + PAGE_SIZE, PAGE_SIZE),
+		    "legitimate all-kernel __copy_to_user_inatomic failed");
 	ret |= test(__get_user(value, (unsigned long __user *)kmem),
 		    "legitimate kernel __get_user failed");
 	ret |= test(__put_user(value, (unsigned long __user *)kmem),
-- 
2.3.6

[PATCH v2 08/11] test_user_copy: Check __clear_user()/clear_user()

[James Hogan]

Add basic success/failure checking of __clear_user() and clear_user(),
which zero an area of user or kernel memory and return the number of
bytes left to clear.

This catches a couple of bugs in the MIPS Enhanced Virtual Memory (EVA)
implementation (which have already been fixed):
test_user_copy: legitimate kernel clear_user failed
test_user_copy: legitimate kernel __clear_user failed

Due to neither function checking the user address limit, and both
resorting to user access unconditionally.

New tests:
- legitimate clear_user
- legitimate __clear_user
- illegal kernel clear_user
- illegal kernel __clear_user
- legitimate kernel clear_user
- legitimate kernel __clear_user

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 lib/test_user_copy.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index c002fc1286bd..450b4c379c61 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -68,6 +68,8 @@ static int __init test_user_copy_init(void)
 		    "legitimate get_user failed");
 	ret |= test(put_user(value, (unsigned long __user *)usermem),
 		    "legitimate put_user failed");
+	ret |= test(clear_user(usermem, PAGE_SIZE) != 0,
+		    "legitimate clear_user passed");
 
 	ret |= test(!access_ok(VERIFY_READ, usermem, PAGE_SIZE * 2),
 		    "legitimate access_ok VERIFY_READ failed");
@@ -85,6 +87,8 @@ static int __init test_user_copy_init(void)
 		    "legitimate __get_user failed");
 	ret |= test(__put_user(value, (unsigned long __user *)usermem),
 		    "legitimate __put_user failed");
+	ret |= test(__clear_user(usermem, PAGE_SIZE) != 0,
+		    "legitimate __clear_user passed");
 
 	/* Invalid usage: none of these should succeed. */
 	ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
@@ -103,6 +107,8 @@ static int __init test_user_copy_init(void)
 		    "illegal get_user passed");
 	ret |= test(!put_user(value, (unsigned long __user *)kmem),
 		    "illegal put_user passed");
+	ret |= test(clear_user((char __user *)kmem, PAGE_SIZE) != PAGE_SIZE,
+		    "illegal kernel clear_user passed");
 
 	/*
 	 * If unchecked user accesses (__*) on this architecture cannot access
@@ -145,6 +151,8 @@ static int __init test_user_copy_init(void)
 		    "illegal __get_user passed");
 	ret |= test(!__put_user(value, (unsigned long __user *)kmem),
 		    "illegal __put_user passed");
+	ret |= test(__clear_user((char __user *)kmem, PAGE_SIZE) != PAGE_SIZE,
+		    "illegal kernel __clear_user passed");
 #endif
 
 	/*
@@ -165,6 +173,8 @@ static int __init test_user_copy_init(void)
 		    "legitimate kernel get_user failed");
 	ret |= test(put_user(value, (unsigned long __user *)kmem),
 		    "legitimate kernel put_user failed");
+	ret |= test(clear_user((char __user *)kmem, PAGE_SIZE) != 0,
+		    "legitimate kernel clear_user failed");
 
 	ret |= test(!access_ok(VERIFY_READ, (char __user *)kmem, PAGE_SIZE * 2),
 		    "legitimate kernel access_ok VERIFY_READ failed");
@@ -188,6 +198,8 @@ static int __init test_user_copy_init(void)
 		    "legitimate kernel __get_user failed");
 	ret |= test(__put_user(value, (unsigned long __user *)kmem),
 		    "legitimate kernel __put_user failed");
+	ret |= test(__clear_user((char __user *)kmem, PAGE_SIZE) != 0,
+		    "legitimate kernel __clear_user failed");
 
 	/* Restore previous address limit. */
 	set_fs(fs);
-- 
2.3.6

[PATCH v2 09/11] test_user_copy: Check user string accessors

[James Hogan]

Add basic success/failure checking of the user string functions which
copy or find the length of userland strings.

The following cases are checked:
- strncpy_from_user() with legitimate user to kernel addresses, illegal
  all-kernel and reversed addresses, and legitimate all-kernel
  addresses.
- strnlen_user() with a legitimate user address, an illegal kernel
  address, and a legitimate kernel address.

New tests:
- legitimate strncpy_from_user
- legitimate strnlen_user
- illegal all-kernel strncpy_from_user
- illegal reversed strncpy_from_user
- illegal strnlen_user
- legitimate all-kernel strncpy_from_user
- legitimate kernel strnlen_user

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Changes in v2:
- Drop strlen_user test. Microblaze doesn't define it, and nothing
  actually uses it. IMO it should be removed, and there's no point
  testing it in the mean time.
---
 lib/test_user_copy.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 450b4c379c61..56af2439d2be 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -70,6 +70,10 @@ static int __init test_user_copy_init(void)
 		    "legitimate put_user failed");
 	ret |= test(clear_user(usermem, PAGE_SIZE) != 0,
 		    "legitimate clear_user passed");
+	ret |= test(strncpy_from_user(kmem, usermem, PAGE_SIZE) < 0,
+		    "legitimate strncpy_from_user failed");
+	ret |= test(strnlen_user(usermem, PAGE_SIZE) == 0,
+		    "legitimate strnlen_user failed");
 
 	ret |= test(!access_ok(VERIFY_READ, usermem, PAGE_SIZE * 2),
 		    "legitimate access_ok VERIFY_READ failed");
@@ -109,6 +113,14 @@ static int __init test_user_copy_init(void)
 		    "illegal put_user passed");
 	ret |= test(clear_user((char __user *)kmem, PAGE_SIZE) != PAGE_SIZE,
 		    "illegal kernel clear_user passed");
+	ret |= test(strncpy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+				      PAGE_SIZE) >= 0,
+		    "illegal all-kernel strncpy_from_user passed");
+	ret |= test(strncpy_from_user(bad_usermem, (char __user *)kmem,
+				      PAGE_SIZE) >= 0,
+		    "illegal reversed strncpy_from_user passed");
+	ret |= test(strnlen_user((char __user *)kmem, PAGE_SIZE) != 0,
+		    "illegal strnlen_user passed");
 
 	/*
 	 * If unchecked user accesses (__*) on this architecture cannot access
@@ -175,6 +187,11 @@ static int __init test_user_copy_init(void)
 		    "legitimate kernel put_user failed");
 	ret |= test(clear_user((char __user *)kmem, PAGE_SIZE) != 0,
 		    "legitimate kernel clear_user failed");
+	ret |= test(strncpy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+				      PAGE_SIZE) < 0,
+		    "legitimate all-kernel strncpy_from_user failed");
+	ret |= test(strnlen_user((char __user *)kmem, PAGE_SIZE) == 0,
+		    "legitimate kernel strnlen_user failed");
 
 	ret |= test(!access_ok(VERIFY_READ, (char __user *)kmem, PAGE_SIZE * 2),
 		    "legitimate kernel access_ok VERIFY_READ failed");
-- 
2.3.6

[PATCH v2 10/11] test_user_copy: Check user compatibility accessors

[James Hogan]

Add basic success/failure checking of user accessors used by 32-bit
compatibility code. This includes copy_in_user() which copies data from
userspace to userspace (or kernel to kernel), its unchecking friend
__copy_in_user which assume that access_ok() has already been used,
and __get_user_unaligned() / __put_user_unaligned().

These might not be provided for architectures which don't need 32-bit
compatibility syscalls, so they are tested conditional upon
CONFIG_COMPAT.

The following cases are checked:
- __copy_in_user/copy_in_user from user to user should succeed.
- __copy_in_user/copy_in_user involving 1 or 2 kernel pointers should
  not succeed.
- __copy_in_user/copy_in_user from kernel to kernel should succeed when
  user address limit is set for kernel accesses.
- __get_user_unaligned/__put_user_unaligned with user pointer should
  succeed.
- __get_user_unaligned/__put_user_unaligned with kernel pointer should
  not succeed.
- __get_user_unaligned/__put_user_unaligned with kernel pointer succeed
  when user address limit is set for kernel accesses.

New tests:
- legitimate copy_in_user
- legitimate __copy_in_user
- legitimate __get_user_unaligned
- legitimate __put_user_unaligned
- illegal all-kernel copy_in_user
- illegal copy_in_user to kernel
- illegal copy_in_user from kernel
- illegal all-kernel __copy_in_user
- illegal __copy_in_user to kernel
- illegal __copy_in_user from kernel
- illegal __get_user_unaligned
- illegal __put_user_unaligned
- legitimate all-kernel copy_in_user
- legitimate all-kernel __copy_in_user
- legitimate kernel __get_user_unaligned
- legitimate kernel __put_user_unaligned

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Changes in v2:
- Conditionalise on CONFIG_COMPAT, otherwise it breaks build on some
  32-bit arches e.g. i386 (kbuild test robot).
- Add testing of _unaligned accessors, which are also conditional upon
  CONFIG_COMPAT.
---
 lib/test_user_copy.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index 56af2439d2be..ebaa28d2c8bd 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -74,6 +74,10 @@ static int __init test_user_copy_init(void)
 		    "legitimate strncpy_from_user failed");
 	ret |= test(strnlen_user(usermem, PAGE_SIZE) == 0,
 		    "legitimate strnlen_user failed");
+#ifdef CONFIG_COMPAT
+	ret |= test(copy_in_user(usermem, usermem + PAGE_SIZE, PAGE_SIZE),
+		    "legitimate copy_in_user failed");
+#endif
 
 	ret |= test(!access_ok(VERIFY_READ, usermem, PAGE_SIZE * 2),
 		    "legitimate access_ok VERIFY_READ failed");
@@ -93,6 +97,16 @@ static int __init test_user_copy_init(void)
 		    "legitimate __put_user failed");
 	ret |= test(__clear_user(usermem, PAGE_SIZE) != 0,
 		    "legitimate __clear_user passed");
+#ifdef CONFIG_COMPAT
+	ret |= test(__copy_in_user(usermem, usermem + PAGE_SIZE, PAGE_SIZE),
+		    "legitimate __copy_in_user failed");
+	ret |= test(__get_user_unaligned(value,
+					 (unsigned long __user *)(usermem + 1)),
+		    "legitimate __get_user_unaligned failed");
+	ret |= test(__put_user_unaligned(value,
+					 (unsigned long __user *)(usermem + 1)),
+		    "legitimate __put_user_unaligned failed");
+#endif
 
 	/* Invalid usage: none of these should succeed. */
 	ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
@@ -121,6 +135,17 @@ static int __init test_user_copy_init(void)
 		    "illegal reversed strncpy_from_user passed");
 	ret |= test(strnlen_user((char __user *)kmem, PAGE_SIZE) != 0,
 		    "illegal strnlen_user passed");
+#ifdef CONFIG_COMPAT
+	ret |= test(!copy_in_user((char __user *)kmem,
+				  (char __user *)(kmem + PAGE_SIZE), PAGE_SIZE),
+		    "illegal all-kernel copy_in_user passed");
+	ret |= test(!copy_in_user((char __user *)kmem, usermem,
+				  PAGE_SIZE),
+		    "illegal copy_in_user to kernel passed");
+	ret |= test(!copy_in_user(usermem, (char __user *)kmem,
+				  PAGE_SIZE),
+		    "illegal copy_in_user from kernel passed");
+#endif
 
 	/*
 	 * If unchecked user accesses (__*) on this architecture cannot access
@@ -165,6 +190,24 @@ static int __init test_user_copy_init(void)
 		    "illegal __put_user passed");
 	ret |= test(__clear_user((char __user *)kmem, PAGE_SIZE) != PAGE_SIZE,
 		    "illegal kernel __clear_user passed");
+#ifdef CONFIG_COMPAT
+	ret |= test(!__copy_in_user((char __user *)kmem,
+				    (char __user *)(kmem + PAGE_SIZE),
+				    PAGE_SIZE),
+		    "illegal all-kernel __copy_in_user passed");
+	ret |= test(!__copy_in_user((char __user *)kmem, usermem,
+				    PAGE_SIZE),
+		    "illegal __copy_in_user to kernel passed");
+	ret |= test(!__copy_in_user(usermem, (char __user *)kmem,
+				    PAGE_SIZE),
+		    "illegal __copy_in_user from kernel passed");
+	ret |= test(!__get_user_unaligned(value,
+					  (unsigned long __user *)(kmem + 1)),
+		    "illegal __get_user_unaligned passed");
+	ret |= test(!__put_user_unaligned(value,
+					  (unsigned long __user *)(kmem + 1)),
+		    "illegal __put_user_unaligned passed");
+#endif
 #endif
 
 	/*
@@ -192,6 +235,11 @@ static int __init test_user_copy_init(void)
 		    "legitimate all-kernel strncpy_from_user failed");
 	ret |= test(strnlen_user((char __user *)kmem, PAGE_SIZE) == 0,
 		    "legitimate kernel strnlen_user failed");
+#ifdef CONFIG_COMPAT
+	ret |= test(copy_in_user((char __user *)kmem,
+				 (char __user *)(kmem + PAGE_SIZE), PAGE_SIZE),
+		    "legitimate all-kernel copy_in_user failed");
+#endif
 
 	ret |= test(!access_ok(VERIFY_READ, (char __user *)kmem, PAGE_SIZE * 2),
 		    "legitimate kernel access_ok VERIFY_READ failed");
@@ -217,6 +265,18 @@ static int __init test_user_copy_init(void)
 		    "legitimate kernel __put_user failed");
 	ret |= test(__clear_user((char __user *)kmem, PAGE_SIZE) != 0,
 		    "legitimate kernel __clear_user failed");
+#ifdef CONFIG_COMPAT
+	ret |= test(__copy_in_user((char __user *)kmem,
+				   (char __user *)(kmem + PAGE_SIZE),
+				   PAGE_SIZE),
+		    "legitimate all-kernel __copy_in_user failed");
+	ret |= test(__get_user_unaligned(value,
+					 (unsigned long __user *)(kmem + 1)),
+		    "legitimate kernel __get_user_unaligned failed");
+	ret |= test(__put_user_unaligned(value,
+					 (unsigned long __user *)(kmem + 1)),
+		    "legitimate kernel __put_user_unaligned failed");
+#endif
 
 	/* Restore previous address limit. */
 	set_fs(fs);
-- 
2.3.6

[PATCH v2 11/11] test_user_copy: Check user checksum functions

[James Hogan]

Add basic success/failure checking of the combined user copy and
checksum functions which copy data between user and kernel space while
also checksumming that data. Some architectures have optimised versions
of these which combine both operations into a single pass.

The following cases are checked:
- csum_partial_copy_from_user() with legitimate user to kernel
  addresses, illegal all-kernel and reversed addresses (for
  implementations where this is safe to test, as this function does not
  perform an access_ok() check), and legitimate all-kernel addresses.
- csum_and_copy_from_user() with legitimate user to kernel addresses,
  illegal all-kernel and reversed addresses, and legitimate all-kernel
  addresses.
- csum_partial_copy_from_user() with legitimate kernel to user
  addresses, illegal all-kernel and reversed addresses, and legitimate
  all-kernel addresses.

New tests:
- legitimate csum_and_copy_from_user
- legitimate csum_and_copy_to_user
- legitimate csum_partial_copy_from_user
- illegal all-kernel csum_and_copy_from_user
- illegal reversed csum_and_copy_from_user
- illegal all-kernel csum_and_copy_to_user
- illegal reversed csum_and_copy_to_user
- illegal all-kernel csum_partial_copy_from_user
- illegal reversed csum_partial_copy_from_user
- legitimate kernel csum_and_copy_from_user
- legitimate kernel csum_and_copy_to_user
- legitimate kernel csum_partial_copy_from_user

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Changes in v2:
- Only test csum_partial_copy_from_user #ifndef
  _HAVE_ARCH_COPY_AND_CSUM_FROM_USER, fixing powerpc64 build (Stephen
  Rothwell)
---
 lib/test_user_copy.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c
index ebaa28d2c8bd..b9cf1d5b77ef 100644
--- a/lib/test_user_copy.c
+++ b/lib/test_user_copy.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/vmalloc.h>
+#include <net/checksum.h>
 
 #define test(condition, msg)		\
 ({					\
@@ -41,6 +42,7 @@ static int __init test_user_copy_init(void)
 	char *bad_usermem;
 	unsigned long user_addr;
 	unsigned long value = 0x5A;
+	int err;
 	mm_segment_t fs = get_fs();
 
 	kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL);
@@ -78,6 +80,12 @@ static int __init test_user_copy_init(void)
 	ret |= test(copy_in_user(usermem, usermem + PAGE_SIZE, PAGE_SIZE),
 		    "legitimate copy_in_user failed");
 #endif
+	err = 0;
+	csum_and_copy_from_user(usermem, kmem, PAGE_SIZE, 0, &err);
+	ret |= test(err, "legitimate csum_and_copy_from_user failed");
+	err = 0;
+	csum_and_copy_to_user(kmem, usermem, PAGE_SIZE, 0, &err);
+	ret |= test(err, "legitimate csum_and_copy_to_user failed");
 
 	ret |= test(!access_ok(VERIFY_READ, usermem, PAGE_SIZE * 2),
 		    "legitimate access_ok VERIFY_READ failed");
@@ -107,6 +115,11 @@ static int __init test_user_copy_init(void)
 					 (unsigned long __user *)(usermem + 1)),
 		    "legitimate __put_user_unaligned failed");
 #endif
+#ifndef _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
+	err = 0;
+	csum_partial_copy_from_user(usermem, kmem, PAGE_SIZE, 0, &err);
+	ret |= test(err, "legitimate csum_partial_copy_from_user failed");
+#endif
 
 	/* Invalid usage: none of these should succeed. */
 	ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE),
@@ -146,6 +159,22 @@ static int __init test_user_copy_init(void)
 				  PAGE_SIZE),
 		    "illegal copy_in_user from kernel passed");
 #endif
+	err = 0;
+	csum_and_copy_from_user((char __user *)(kmem + PAGE_SIZE), kmem,
+				PAGE_SIZE, 0, &err);
+	ret |= test(!err, "illegal all-kernel csum_and_copy_from_user passed");
+	err = 0;
+	csum_and_copy_from_user((char __user *)kmem, bad_usermem,
+				PAGE_SIZE, 0, &err);
+	ret |= test(!err, "illegal reversed csum_and_copy_from_user passed");
+	err = 0;
+	csum_and_copy_to_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+			      PAGE_SIZE, 0, &err);
+	ret |= test(!err, "illegal all-kernel csum_and_copy_to_user passed");
+	err = 0;
+	csum_and_copy_to_user(bad_usermem, (char __user *)kmem, PAGE_SIZE, 0,
+			      &err);
+	ret |= test(!err, "illegal reversed csum_and_copy_to_user passed");
 
 	/*
 	 * If unchecked user accesses (__*) on this architecture cannot access
@@ -208,6 +237,18 @@ static int __init test_user_copy_init(void)
 					  (unsigned long __user *)(kmem + 1)),
 		    "illegal __put_user_unaligned passed");
 #endif
+#ifndef _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
+	err = 0;
+	csum_partial_copy_from_user((char __user *)(kmem + PAGE_SIZE), kmem,
+				    PAGE_SIZE, 0, &err);
+	ret |= test(!err,
+		    "illegal all-kernel csum_partial_copy_from_user passed");
+	err = 0;
+	csum_partial_copy_from_user((char __user *)kmem, bad_usermem, PAGE_SIZE,
+				    0, &err);
+	ret |= test(!err,
+		    "illegal reversed csum_partial_copy_from_user passed");
+#endif
 #endif
 
 	/*
@@ -240,6 +281,14 @@ static int __init test_user_copy_init(void)
 				 (char __user *)(kmem + PAGE_SIZE), PAGE_SIZE),
 		    "legitimate all-kernel copy_in_user failed");
 #endif
+	err = 0;
+	csum_and_copy_from_user((char __user *)(kmem + PAGE_SIZE), kmem,
+				PAGE_SIZE, 0, &err);
+	ret |= test(err, "legitimate kernel csum_and_copy_from_user failed");
+	err = 0;
+	csum_and_copy_to_user(kmem, (char __user *)(kmem + PAGE_SIZE),
+			      PAGE_SIZE, 0, &err);
+	ret |= test(err, "legitimate kernel csum_and_copy_to_user failed");
 
 	ret |= test(!access_ok(VERIFY_READ, (char __user *)kmem, PAGE_SIZE * 2),
 		    "legitimate kernel access_ok VERIFY_READ failed");
@@ -277,6 +326,13 @@ static int __init test_user_copy_init(void)
 					 (unsigned long __user *)(kmem + 1)),
 		    "legitimate kernel __put_user_unaligned failed");
 #endif
+#ifndef _HAVE_ARCH_COPY_AND_CSUM_FROM_USER
+	err = 0;
+	csum_partial_copy_from_user((char __user *)(kmem + PAGE_SIZE), kmem,
+				    PAGE_SIZE, 0, &err);
+	ret |= test(err,
+		    "legitimate kernel csum_partial_copy_from_user failed");
+#endif
 
 	/* Restore previous address limit. */
 	set_fs(fs);
-- 
2.3.6

Re: [PATCH v2 00/11] test_user_copy improvements

[Kees Cook]

On Fri, Aug 7, 2015 at 8:21 AM, James Hogan <james.hogan@imgtec.com> wrote:
> These patches extend the test_user_copy test module to handle lots more
> cases of user accessors which architectures can override separately, and
> in particular those which are important for checking the MIPS Enhanced
> Virtual Addressing (EVA) implementations, which need to handle
> overlapping user and kernel address spaces, with special instructions
> for accessing user address space from kernel mode.
>
> - Checking that kernel pointers are accepted when user address limit is
>   set to KERNEL_DS, as done by the kernel when it internally invokes
>   system calls with kernel pointers.
> - Checking of the unchecked accessors (which don't call access_ok()).
>   Some of the tests are special cased for EVA at the moment which has
>   stricter hardware guarantees for bad user accesses than other
>   configurations.
> - Checking of other sets of user accessors, including the inatomic user
>   copies, clear_user, compatibility accessors (copy_in_user and
>   _unaligned), the user string accessors, and the user checksum
>   functions, all of which need special handling in arch code with EVA.
>
> Tested on MIPS with and without EVA, and on x86_64.
>
> Only build tested for arm, blackfin, metag, microblaze, openrisc,
> parisc, powerpc, sh, sparc, tile, i386 & xtensa.
>
> All arches were audited for the appropriate exports, only score is known
> to still be missing some.
>
> Changes in v2:
> - Add arch exports (patches 1-4).
> - Reorder patches slightly.
> - Patch 9: Drop strlen_user test. Microblaze doesn't define it, and
>   nothing actually uses it. IMO it should be removed, and there's no
>   point testing it in the mean time.
> - Patch 10: Conditionalise on CONFIG_COMPAT, otherwise it breaks build
>   on some 32-bit arches e.g. i386 (kbuild test robot).
> - Patch 10: Add testing of _unaligned accessors, which are also
>   conditional upon CONFIG_COMPAT.
> - Patch 11: Only test csum_partial_copy_from_user #ifndef
>   _HAVE_ARCH_COPY_AND_CSUM_FROM_USER, fixing powerpc64 build (Stephen
>   Rothwell)

Thanks for the fixes!

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> James Hogan (11):
>   microblaze: Export __strnlen_user to modules
>   nios2: Export strncpy_from_user / strnlen_user to modules
>   openrisc: Export __clear_user to modules
>   xtensa: Export __strnlen_user to modules
>   test_user_copy: Check legit kernel accesses
>   test_user_copy: Check unchecked accessors
>   test_user_copy: Check __copy_{to,from}_user_inatomic()
>   test_user_copy: Check __clear_user()/clear_user()
>   test_user_copy: Check user string accessors
>   test_user_copy: Check user compatibility accessors
>   test_user_copy: Check user checksum functions
>
>  arch/microblaze/kernel/microblaze_ksyms.c |   1 +
>  arch/nios2/mm/uaccess.c                   |   2 +
>  arch/openrisc/kernel/or32_ksyms.c         |   1 +
>  arch/xtensa/kernel/xtensa_ksyms.c         |   1 +
>  lib/test_user_copy.c                      | 251 ++++++++++++++++++++++++++++++
>  5 files changed, 256 insertions(+)
>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Michal Simek <monstr@monstr.eu>
> Cc: Ley Foon Tan <lftan@altera.com>
> Cc: Jonas Bonn <jonas@southpole.se>
> Cc: Chris Zankel <chris@zankel.net>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> Cc: nios2-dev@lists.rocketboards.org
> Cc: linux@lists.openrisc.net
> Cc: linux-xtensa@linux-xtensa.org
> --
> 2.3.6
>



-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2 00/11] test_user_copy improvements

[David Miller]

From: James Hogan <james.hogan@imgtec.com>
Date: Fri, 7 Aug 2015 16:21:53 +0100

> These patches extend the test_user_copy test module to handle lots more
> cases of user accessors which architectures can override separately, and
> in particular those which are important for checking the MIPS Enhanced
> Virtual Addressing (EVA) implementations, which need to handle
> overlapping user and kernel address spaces, with special instructions
> for accessing user address space from kernel mode.
> 
> - Checking that kernel pointers are accepted when user address limit is
>   set to KERNEL_DS, as done by the kernel when it internally invokes
>   system calls with kernel pointers.
> - Checking of the unchecked accessors (which don't call access_ok()).
>   Some of the tests are special cased for EVA at the moment which has
>   stricter hardware guarantees for bad user accesses than other
>   configurations.
> - Checking of other sets of user accessors, including the inatomic user
>   copies, clear_user, compatibility accessors (copy_in_user and
>   _unaligned), the user string accessors, and the user checksum
>   functions, all of which need special handling in arch code with EVA.
> 
> Tested on MIPS with and without EVA, and on x86_64.
> 
> Only build tested for arm, blackfin, metag, microblaze, openrisc,
> parisc, powerpc, sh, sparc, tile, i386 & xtensa.
> 
> All arches were audited for the appropriate exports, only score is known
> to still be missing some.

James, thanks for doing this work.

If I understand the MIPS EVA facility correctly, it operates exactly like
how sparc64 does.  Wherein user and kernel virtual addresses are fully
segregated, and one must use a specially tagged load or store to access
user addresses.

This actually creates problems for the tests as currently coded on
such systems (this problem existed before your changes).  You might
not be triggering this problem on MIPS EV but it certainly is there.

For example, consider this test:

	ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
				    PAGE_SIZE),
		    "illegal reversed copy_from_user passed");

If the 'kmem' access faults, we will try to zero out PAGE_SIZE bytes
at 'bad_usermem'.  But this is not necessarily going to fail.

The user address 'bad_usermem', on MIPS EV and sparc64, could just as
equally happen to be a legitimate kernel address.  So this clear will
succeed and we will end up clearing memory at an arbitrary kernel
address.

There is no real way to trap this situation as a native load/store
will work just fine on these addresses.

I don't have a good suggestion other than to say that these tests
seem to only be valid in a combined kernel/user address space, ie.
for systems other than MIPS EV and sparc64.

Also, I think the tests you added and protected with MIPS ifdefs could
equally be enabled on sparc64.

Thanks!

Re: [PATCH v2 00/11] test_user_copy improvements

[David Miller]

From: David Miller <davem@redhat.com>
Date: Mon, 10 Aug 2015 15:29:38 -0700 (PDT)

> Also, I think the tests you added and protected with MIPS ifdefs could
> equally be enabled on sparc64.

James, as per this issue, I was thinking we could do something like this
so that the tests don't get messy:

diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index cee5f93..48d20c8 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -1922,6 +1922,7 @@ config CPU_MIPSR6
 
 config EVA
 	bool
+	select ARCH_SPLIT_VA_SPACE
 
 config XPA
 	bool
diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
index 56442d2..4001d04 100644
--- a/arch/sparc/Kconfig
+++ b/arch/sparc/Kconfig
@@ -80,6 +80,7 @@ config SPARC64
 	select NO_BOOTMEM
 	select HAVE_ARCH_AUDITSYSCALL
 	select ARCH_SUPPORTS_ATOMIC_RMW
+	select ARCH_SPLIT_VA_SPACE
 
 config ARCH_DEFCONFIG
 	string
diff --git a/lib/Kconfig b/lib/Kconfig
index 3a2ef67..149eb29 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -62,6 +62,9 @@ config ARCH_USE_CMPXCHG_LOCKREF
 config ARCH_HAS_FAST_MULTIPLIER
 	bool
 
+config ARCH_SPLIT_VA_SPACE
+       def_bool n
+
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help

Re: [PATCH v2 00/11] test_user_copy improvements

[Geert Uytterhoeven]

On Tue, Aug 11, 2015 at 6:08 AM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@redhat.com>
> Date: Mon, 10 Aug 2015 15:29:38 -0700 (PDT)
>
>> Also, I think the tests you added and protected with MIPS ifdefs could
>> equally be enabled on sparc64.
>
> James, as per this issue, I was thinking we could do something like this
> so that the tests don't get messy:
>
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index cee5f93..48d20c8 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -1922,6 +1922,7 @@ config CPU_MIPSR6
>
>  config EVA
>         bool
> +       select ARCH_SPLIT_VA_SPACE
>
>  config XPA
>         bool
> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig
> index 56442d2..4001d04 100644
> --- a/arch/sparc/Kconfig
> +++ b/arch/sparc/Kconfig
> @@ -80,6 +80,7 @@ config SPARC64
>         select NO_BOOTMEM
>         select HAVE_ARCH_AUDITSYSCALL
>         select ARCH_SUPPORTS_ATOMIC_RMW
> +       select ARCH_SPLIT_VA_SPACE
>
>  config ARCH_DEFCONFIG
>         string
> diff --git a/lib/Kconfig b/lib/Kconfig
> index 3a2ef67..149eb29 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -62,6 +62,9 @@ config ARCH_USE_CMPXCHG_LOCKREF
>  config ARCH_HAS_FAST_MULTIPLIER
>         bool
>
> +config ARCH_SPLIT_VA_SPACE
> +       def_bool n
> +
>  config CRC_CCITT
>         tristate "CRC-CCITT functions"
>         help

I think this applies to a few more architectures.

M68k already has a config symbol for his (CPU_HAS_ADDRESS_SPACES),
which is set by classic m68k (not Coldfire) that has the "moves" instruction.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 00/11] test_user_copy improvements

[David Miller]

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Tue, 11 Aug 2015 13:20:53 +0200

> On Tue, Aug 11, 2015 at 6:08 AM, David Miller <davem@davemloft.net> wrote:
 ...
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index 3a2ef67..149eb29 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -62,6 +62,9 @@ config ARCH_USE_CMPXCHG_LOCKREF
>>  config ARCH_HAS_FAST_MULTIPLIER
>>         bool
>>
>> +config ARCH_SPLIT_VA_SPACE
>> +       def_bool n
>> +
>>  config CRC_CCITT
>>         tristate "CRC-CCITT functions"
>>         help
> 
> I think this applies to a few more architectures.
> 
> M68k already has a config symbol for his (CPU_HAS_ADDRESS_SPACES),
> which is set by classic m68k (not Coldfire) that has the "moves" instruction.

Awesome, maybe James can add the M68K case to his patch as well.

Re: [PATCH v2 00/11] test_user_copy improvements

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 5681 bytes --]

Hi David,

On 10/08/15 23:29, David Miller wrote:
> From: James Hogan <james.hogan@imgtec.com>
> Date: Fri, 7 Aug 2015 16:21:53 +0100
> 
>> These patches extend the test_user_copy test module to handle lots more
>> cases of user accessors which architectures can override separately, and
>> in particular those which are important for checking the MIPS Enhanced
>> Virtual Addressing (EVA) implementations, which need to handle
>> overlapping user and kernel address spaces, with special instructions
>> for accessing user address space from kernel mode.
>>
>> - Checking that kernel pointers are accepted when user address limit is
>>   set to KERNEL_DS, as done by the kernel when it internally invokes
>>   system calls with kernel pointers.
>> - Checking of the unchecked accessors (which don't call access_ok()).
>>   Some of the tests are special cased for EVA at the moment which has
>>   stricter hardware guarantees for bad user accesses than other
>>   configurations.
>> - Checking of other sets of user accessors, including the inatomic user
>>   copies, clear_user, compatibility accessors (copy_in_user and
>>   _unaligned), the user string accessors, and the user checksum
>>   functions, all of which need special handling in arch code with EVA.
>>
>> Tested on MIPS with and without EVA, and on x86_64.
>>
>> Only build tested for arm, blackfin, metag, microblaze, openrisc,
>> parisc, powerpc, sh, sparc, tile, i386 & xtensa.
>>
>> All arches were audited for the appropriate exports, only score is known
>> to still be missing some.
> 
> James, thanks for doing this work.
> 
> If I understand the MIPS EVA facility correctly, it operates exactly like
> how sparc64 does.  Wherein user and kernel virtual addresses are fully
> segregated, and one must use a specially tagged load or store to access
> user addresses.

Yes, sort of. Roughly speaking, 6 segments in the MIPS virtual address
space become configurable such that each one may be:
* TLB mapped and accessible to user and kernel (both modes must share
TLB mappings in that segment)
* TLB mapped in user mode, but not-TLB-mapped to kernel mode (a window
into physical memory).
* (and various other combinations)

This allows the kernel virtual address space to be extended down to
overlap the user address space and make more physical memory directly
accessible to the kernel, and potentially for the user virtual address
space to extend upwards too.

So if there is any overlap, the EVA load/store instructions must be used
whenever user memory is accessed by kernel.

> This actually creates problems for the tests as currently coded on
> such systems (this problem existed before your changes).  You might
> not be triggering this problem on MIPS EV but it certainly is there.
> 
> For example, consider this test:
> 
> 	ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem,
> 				    PAGE_SIZE),
> 		    "illegal reversed copy_from_user passed");
> 
> If the 'kmem' access faults, we will try to zero out PAGE_SIZE bytes
> at 'bad_usermem'.  But this is not necessarily going to fail.

Out of interest, is the zeroing a strict requirement for correct use, or
a safety precaution to prevent data leakage in case of bad error checking?

(A quick look reveals that for copy_from_user() when access_ok() fails,
only arm, arm64, frv, m32r, m68k, sparc, tile, x86, and xtensa do this).

> 
> The user address 'bad_usermem', on MIPS EV and sparc64, could just as
> equally happen to be a legitimate kernel address.  So this clear will
> succeed and we will end up clearing memory at an arbitrary kernel
> address.

That's a good point. The reversed tests aren't really safe in that case.
With MIPS EVA the user address is very likely to be a valid
non-TLB-mapped address to kernel mode, and will zero arbitrary memory.
They could also potentially crash the kernel if user memory isn't
normally kernel accessible and the arch doesn't fix up faults for the
kernel accesses (not EVA, but maybe sparc64?).

It is also possible (though less likely) that the kernel address will
have a valid user mapping at the same address, so the reversed
copy_to_user test may well leak arbitrary kernel memory to user memory
without faulting.

> There is no real way to trap this situation as a native load/store
> will work just fine on these addresses.
> 
> I don't have a good suggestion other than to say that these tests
> seem to only be valid in a combined kernel/user address space, ie.
> for systems other than MIPS EV and sparc64.

Yes, although I think the all-kernel ones are still valuable for testing
where it can be assumed that kernel addresses are unlikely to be valid
user addresses (otherwise they may also succeed where they should fail
for similar reasons, but are otherwise harmless).

> Also, I think the tests you added and protected with MIPS ifdefs could
> equally be enabled on sparc64.

Yes, it sounds like it. I'll try the ARCH_SPLIT_VA_SPACE idea.

I have since tested these ones on a different (out of tree) EVA
configuration (legacy segment layout, but EVA instructions enabled) and
it ends up accessing kernel only addresses (not in the overlapping area)
with EVA instructions, which would normally get caught by access_ok() in
the other illegal tests, but are not handled properly by the arch (they
generate address error exception instead of TLB invalid exception). It
sounds like they should just work out of the box on sparc64 though if
they literally use a different ASI (aside from the risk of false positives).

Thanks for the feedback!

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 00/11] test_user_copy improvements

[David Miller]

From: James Hogan <james.hogan@imgtec.com>
Date: Tue, 11 Aug 2015 12:07:20 +0100

> Out of interest, is the zeroing a strict requirement for correct use, or
> a safety precaution to prevent data leakage in case of bad error checking?
> 
> (A quick look reveals that for copy_from_user() when access_ok() fails,
> only arm, arm64, frv, m32r, m68k, sparc, tile, x86, and xtensa do this).

It is required, otherwise the kernel buffer is left partially initialized
which can lead to security bugs.

> That's a good point. The reversed tests aren't really safe in that case.
> With MIPS EVA the user address is very likely to be a valid
> non-TLB-mapped address to kernel mode, and will zero arbitrary memory.
> They could also potentially crash the kernel if user memory isn't
> normally kernel accessible and the arch doesn't fix up faults for the
> kernel accesses (not EVA, but maybe sparc64?).

Sparc64 would fault on an invalid kernel address, but the problem here
is that the addresses are actually valid kernel ones.

> It is also possible (though less likely) that the kernel address will
> have a valid user mapping at the same address, so the reversed
> copy_to_user test may well leak arbitrary kernel memory to user memory
> without faulting.

Yes, this is also a problem.

>> Also, I think the tests you added and protected with MIPS ifdefs could
>> equally be enabled on sparc64.
> 
> Yes, it sounds like it. I'll try the ARCH_SPLIT_VA_SPACE idea.

Great!