From 4fbfeb8bd684d564bddeff1e3723d3d9f99aa5de Mon Sep 17 00:00:00 2001 From: Hoeun Ryu Date: Sun, 12 Feb 2017 15:13:33 +0900 Subject: usercopy: add testcases to check zeroing on failure During usercopy the destination buffer will be zeroed if copy_from_user() or get_user() fails. This patch adds testcases for it. The destination buffer is set with non-zero value before illegal copy_from_user() or get_user() is executed and the buffer is compared to zero after usercopy is done. Signed-off-by: Hoeun Ryu [kees: clarified commit log, dropped second kmalloc] Signed-off-by: Kees Cook --- lib/test_user_copy.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 0ecef3e..0f86c67 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -69,20 +69,30 @@ static int __init test_user_copy_init(void) "legitimate put_user failed"); /* Invalid usage: none of these should succeed. */ + memset(kmem, 0x5a, PAGE_SIZE); + memset(kmem + PAGE_SIZE, 0, PAGE_SIZE); ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE), PAGE_SIZE), "illegal all-kernel copy_from_user passed"); + ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE), + "zeroing failure for illegal all-kernel copy_from_user"); + memset(bad_usermem, 0x5A, PAGE_SIZE); ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed"); + ret |= test(memcmp(kmem + PAGE_SIZE, bad_usermem, PAGE_SIZE), + "zeroing failure for illegal reversed copy_from_user"); 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"); + memset(kmem, 0x5a, PAGE_SIZE); ret |= test(!get_user(value, (unsigned long __user *)kmem), "illegal get_user passed"); + ret |= test(memcmp(kmem + PAGE_SIZE, kmem, sizeof(value)), + "zeroing failure for illegal get_user"); ret |= test(!put_user(value, (unsigned long __user *)kmem), "illegal put_user passed"); -- cgit v1.1 From f5f893c57e37ca730808cb2eee3820abd05e7507 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Mon, 13 Feb 2017 11:25:26 -0800 Subject: usercopy: Adjust tests to deal with SMAP/PAN Under SMAP/PAN/etc, we cannot write directly to userspace memory, so this rearranges the test bytes to get written through copy_to_user(). Additionally drops the bad copy_from_user() test that would trigger a memcpy() against userspace on failure. Signed-off-by: Kees Cook --- lib/test_user_copy.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 0f86c67..73ff7a6 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -58,7 +58,9 @@ static int __init test_user_copy_init(void) usermem = (char __user *)user_addr; bad_usermem = (char *)user_addr; - /* Legitimate usage: none of these should fail. */ + /* + * Legitimate usage: none of these copies should fail. + */ ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE), "legitimate copy_from_user failed"); ret |= test(copy_to_user(usermem, kmem, PAGE_SIZE), @@ -68,31 +70,45 @@ static int __init test_user_copy_init(void) ret |= test(put_user(value, (unsigned long __user *)usermem), "legitimate put_user failed"); - /* Invalid usage: none of these should succeed. */ + /* + * Invalid usage: none of these copies should succeed. + */ + + /* Prepare kernel memory with check values. */ memset(kmem, 0x5a, PAGE_SIZE); memset(kmem + PAGE_SIZE, 0, PAGE_SIZE); + + /* Reject kernel-to-kernel copies through copy_from_user(). */ ret |= test(!copy_from_user(kmem, (char __user *)(kmem + PAGE_SIZE), PAGE_SIZE), "illegal all-kernel copy_from_user passed"); + + /* Destination half of buffer should have been zeroed. */ ret |= test(memcmp(kmem + PAGE_SIZE, kmem, PAGE_SIZE), "zeroing failure for illegal all-kernel copy_from_user"); - memset(bad_usermem, 0x5A, PAGE_SIZE); + +#if 0 + /* + * When running with SMAP/PAN/etc, this will Oops the kernel + * due to the zeroing of userspace memory on failure. This needs + * to be tested in LKDTM instead, since this test module does not + * expect to explode. + */ ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, PAGE_SIZE), "illegal reversed copy_from_user passed"); - ret |= test(memcmp(kmem + PAGE_SIZE, bad_usermem, PAGE_SIZE), - "zeroing failure for illegal reversed copy_from_user"); +#endif 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"); - memset(kmem, 0x5a, PAGE_SIZE); + + value = 0x5a; ret |= test(!get_user(value, (unsigned long __user *)kmem), "illegal get_user passed"); - ret |= test(memcmp(kmem + PAGE_SIZE, kmem, sizeof(value)), - "zeroing failure for illegal get_user"); + ret |= test(value != 0, "zeroing failure for illegal get_user"); ret |= test(!put_user(value, (unsigned long __user *)kmem), "illegal put_user passed"); -- cgit v1.1 From 4c5d7bc63775b40631b75f6c59a3a3005455262d Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Tue, 14 Feb 2017 12:38:07 -0800 Subject: usercopy: Add tests for all get_user() sizes The existing test was only exercising native unsigned long size get_user(). For completeness, we should check all sizes. But we must skip some 32-bit architectures that don't implement a 64-bit get_user(). These new tests actually uncovered a bug in ARM's 64-bit get_user() zeroing. Signed-off-by: Kees Cook --- lib/test_user_copy.c | 89 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 76 insertions(+), 13 deletions(-) diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 73ff7a6..6f335a3 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -25,6 +25,23 @@ #include #include +/* + * Several 32-bit architectures support 64-bit {get,put}_user() calls. + * As there doesn't appear to be anything that can safely determine + * their capability at compile-time, we just have to opt-out certain archs. + */ +#if BITS_PER_LONG == 64 || (!defined(CONFIG_AVR32) && \ + !defined(CONFIG_BLACKFIN) && \ + !defined(CONFIG_M32R) && \ + !defined(CONFIG_M68K) && \ + !defined(CONFIG_MICROBLAZE) && \ + !defined(CONFIG_MN10300) && \ + !defined(CONFIG_NIOS2) && \ + !defined(CONFIG_PPC32) && \ + !defined(CONFIG_SUPERH)) +# define TEST_U64 +#endif + #define test(condition, msg) \ ({ \ int cond = (condition); \ @@ -40,7 +57,12 @@ static int __init test_user_copy_init(void) char __user *usermem; char *bad_usermem; unsigned long user_addr; - unsigned long value = 0x5A; + u8 val_u8; + u16 val_u16; + u32 val_u32; +#ifdef TEST_U64 + u64 val_u64; +#endif kmem = kmalloc(PAGE_SIZE * 2, GFP_KERNEL); if (!kmem) @@ -61,14 +83,39 @@ static int __init test_user_copy_init(void) /* * Legitimate usage: none of these copies should fail. */ - ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE), - "legitimate copy_from_user failed"); + memset(kmem, 0x3a, PAGE_SIZE * 2); 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"); + memset(kmem, 0x0, PAGE_SIZE); + ret |= test(copy_from_user(kmem, usermem, PAGE_SIZE), + "legitimate copy_from_user failed"); + ret |= test(memcmp(kmem, kmem + PAGE_SIZE, PAGE_SIZE), + "legitimate usercopy failed to copy data"); + +#define test_legit(size, check) \ + do { \ + val_##size = check; \ + ret |= test(put_user(val_##size, (size __user *)usermem), \ + "legitimate put_user (" #size ") failed"); \ + val_##size = 0; \ + ret |= test(get_user(val_##size, (size __user *)usermem), \ + "legitimate get_user (" #size ") failed"); \ + ret |= test(val_##size != check, \ + "legitimate get_user (" #size ") failed to do copy"); \ + if (val_##size != check) { \ + pr_info("0x%llx != 0x%llx\n", \ + (unsigned long long)val_##size, \ + (unsigned long long)check); \ + } \ + } while (0) + + test_legit(u8, 0x5a); + test_legit(u16, 0x5a5b); + test_legit(u32, 0x5a5b5c5d); +#ifdef TEST_U64 + test_legit(u64, 0x5a5b5c5d6a6b6c6d); +#endif +#undef test_legit /* * Invalid usage: none of these copies should succeed. @@ -105,12 +152,28 @@ static int __init test_user_copy_init(void) PAGE_SIZE), "illegal reversed copy_to_user passed"); - value = 0x5a; - ret |= test(!get_user(value, (unsigned long __user *)kmem), - "illegal get_user passed"); - ret |= test(value != 0, "zeroing failure for illegal get_user"); - ret |= test(!put_user(value, (unsigned long __user *)kmem), - "illegal put_user passed"); +#define test_illegal(size, check) \ + do { \ + val_##size = (check); \ + ret |= test(!get_user(val_##size, (size __user *)kmem), \ + "illegal get_user (" #size ") passed"); \ + ret |= test(val_##size != (size)0, \ + "zeroing failure for illegal get_user (" #size ")"); \ + if (val_##size != (size)0) { \ + pr_info("0x%llx != 0\n", \ + (unsigned long long)val_##size); \ + } \ + ret |= test(!put_user(val_##size, (size __user *)kmem), \ + "illegal put_user (" #size ") passed"); \ + } while (0) + + test_illegal(u8, 0x5a); + test_illegal(u16, 0x5a5b); + test_illegal(u32, 0x5a5b5c5d); +#ifdef TEST_U64 + test_illegal(u64, 0x5a5b5c5d6a6b6c6d); +#endif +#undef test_illegal vm_munmap(user_addr, PAGE_SIZE * 2); kfree(kmem); -- cgit v1.1