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(+) (limited to 'lib/test_user_copy.c') diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 0ecef3e4690e..0f86c67d87db 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 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(-) (limited to 'lib/test_user_copy.c') diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 0f86c67d87db..73ff7a628e3a 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 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(-) (limited to 'lib/test_user_copy.c') diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 73ff7a628e3a..6f335a3d4ae2 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 From 4deaa6fd00be2bf408dd06cdf0c40a1b59237879 Mon Sep 17 00:00:00 2001 From: Arnd Bergmann Date: Wed, 22 Feb 2017 11:21:22 -0800 Subject: usercopy: ARM NOMMU has no 64-bit get_user On a NOMMU ARM kernel, we get this link error: ERROR: "__get_user_bad" [lib/test_user_copy.ko] undefined! The problem is that the extended get_user/put_user definitions were only added for the normal (MMU based) case. We could add it for NOMMU as well, but it seems easier to just not call it, since no other code needs it. Fixes: 4c5d7bc63775 ("usercopy: Add tests for all get_user() sizes") Signed-off-by: Arnd Bergmann Signed-off-by: Kees Cook --- lib/test_user_copy.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib/test_user_copy.c') diff --git a/lib/test_user_copy.c b/lib/test_user_copy.c index 6f335a3d4ae2..1a8d71a68531 100644 --- a/lib/test_user_copy.c +++ b/lib/test_user_copy.c @@ -30,7 +30,8 @@ * 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) && \ +#if BITS_PER_LONG == 64 || (!(defined(CONFIG_ARM) && !defined(MMU)) && \ + !defined(CONFIG_AVR32) && \ !defined(CONFIG_BLACKFIN) && \ !defined(CONFIG_M32R) && \ !defined(CONFIG_M68K) && \ -- cgit