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 <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Michal Simek <[email protected]>
Cc: Ley Foon Tan <[email protected]>
Cc: Jonas Bonn <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
--
2.3.6
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 <[email protected]>
Cc: Michal Simek <[email protected]>
---
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
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 <[email protected]>
Cc: Ley Foon Tan <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: Jonas Bonn <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Cc: Chris Zankel <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: [email protected]
---
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
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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Acked-by: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
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 <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Andrew Morton <[email protected]>
---
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
On Fri, Aug 7, 2015 at 8:21 AM, James Hogan <[email protected]> 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 <[email protected]>
-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 <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Michal Simek <[email protected]>
> Cc: Ley Foon Tan <[email protected]>
> Cc: Jonas Bonn <[email protected]>
> Cc: Chris Zankel <[email protected]>
> Cc: Max Filippov <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> --
> 2.3.6
>
--
Kees Cook
Chrome OS Security
On Fri, Aug 7, 2015 at 11:21 PM, James Hogan <[email protected]> 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 <[email protected]>
> Cc: Ley Foon Tan <[email protected]>
> Cc: [email protected]
> ---
> 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 <[email protected]>
From: James Hogan <[email protected]>
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!
From: David Miller <[email protected]>
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
Hi David,
On 10/08/15 23:29, David Miller wrote:
> From: James Hogan <[email protected]>
> 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
On Tue, Aug 11, 2015 at 6:08 AM, David Miller <[email protected]> wrote:
> From: David Miller <[email protected]>
> 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 -- [email protected]
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
From: James Hogan <[email protected]>
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!
From: Geert Uytterhoeven <[email protected]>
Date: Tue, 11 Aug 2015 13:20:53 +0200
> On Tue, Aug 11, 2015 at 6:08 AM, David Miller <[email protected]> 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.