Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1202823 > unrolled thread

[PATCH v2 00/11] test_user_copy improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/11] test_user_copy improvements James Hogan <james.hogan@imgtec.com> - 2015-08-07 17:30 +0200
    [PATCH v2 04/11] xtensa: Export __strnlen_user to modules James Hogan <james.hogan@imgtec.com> - 2015-08-07 17:30 +0200
    [PATCH v2 11/11] test_user_copy: Check user checksum functions James Hogan <james.hogan@imgtec.com> - 2015-08-07 17:30 +0200
    Re: [PATCH v2 00/11] test_user_copy improvements Kees Cook <keescook@chromium.org> - 2015-08-08 02:00 +0200
    Re: [PATCH v2 00/11] test_user_copy improvements David Miller <davem@redhat.com> - 2015-08-11 00:30 +0200
      Re: [PATCH v2 00/11] test_user_copy improvements David Miller <davem@davemloft.net> - 2015-08-11 06:10 +0200
        Re: [PATCH v2 00/11] test_user_copy improvements Geert Uytterhoeven <geert@linux-m68k.org> - 2015-08-11 13:30 +0200
          Re: [PATCH v2 00/11] test_user_copy improvements David Miller <davem@davemloft.net> - 2015-08-12 23:40 +0200
      Re: [PATCH v2 00/11] test_user_copy improvements James Hogan <james.hogan@imgtec.com> - 2015-08-11 13:10 +0200
        Re: [PATCH v2 00/11] test_user_copy improvements David Miller <davem@davemloft.net> - 2015-08-11 19:40 +0200

#1202823 — [PATCH v2 00/11] test_user_copy improvements

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203102

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1204609

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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1204738

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205070

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206378

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1205056

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1205322

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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web