| Message ID | 56DDAFA7.4090207@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: > Hi Catalin, > > I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) > to fail. Who to blame is up for discussion. The test is passing a user pointer > as the 'to' field of copy_from_user(), which it expects to fail gracefully: > > lib/test_user_copy.c:75 > > /* Invalid usage: none of these should succeed. */ > [ ... ] > > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, > > PAGE_SIZE), > > "illegal reversed copy_from_user passed"); > > > > access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass > bad_usermem to memset(): > > arch/arm64/include/asm/uaccess.h:279 > > if (access_ok(VERIFY_READ, from, n)) > > n = __copy_from_user(to, from, n); > > else /* security hole - plug it */ > > memset(to, 0, n); > > This (correctly) trips UAO's "Accessing user space memory outside uaccess.h > routines" message, which is a little confusing to debug, and stops the rest of > the module's tests from being run. > > As far as I can see, this would only affect arm64. I can't find an equivalent > memset() for x86_64. I don't think you've looked hard enough. :) arch/x86/lib/usercopy_32.c: unsigned long _copy_from_user(void *to, const void __user *from, unsigned n) { if (access_ok(VERIFY_READ, from, n)) n = __copy_from_user(to, from, n); else memset(to, 0, n); return n; } EXPORT_SYMBOL(_copy_from_user);
(cc'ing Kees) On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: > I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) > to fail. Who to blame is up for discussion. The test is passing a user pointer > as the 'to' field of copy_from_user(), which it expects to fail gracefully: > > lib/test_user_copy.c:75 > > /* Invalid usage: none of these should succeed. */ > [ ... ] > > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, > > PAGE_SIZE), > > "illegal reversed copy_from_user passed"); > > > > access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass > bad_usermem to memset(): > > arch/arm64/include/asm/uaccess.h:279 > > if (access_ok(VERIFY_READ, from, n)) > > n = __copy_from_user(to, from, n); > > else /* security hole - plug it */ > > memset(to, 0, n); > > This (correctly) trips UAO's "Accessing user space memory outside uaccess.h > routines" message, which is a little confusing to debug, and stops the rest of > the module's tests from being run. I suggest we don't do anything on arch/arm64 (or arch/arm), I just consider the test to be broken. The semantics of copy_from_user() don't say anything about the safety checks on the destination pointer, this is supposed to be a valid kernel address. The test assumes that if the source pointer is invalid, the copy_from_user() routine should not touch the destination. A better test would be to use a destination buffer filled with a poison value and checked after the operation.
On 07/03/16 17:23, Russell King - ARM Linux wrote: > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: >> As far as I can see, this would only affect arm64. I can't find an equivalent >> memset() for x86_64. > > I don't think you've looked hard enough. :) Heh, Thanks! I ignored the 32bit code and instead got lost in the maze of underscores and alternative-strings for the 64 bit path. Having seen that path, I've now found: > /* If the destination is a kernel buffer, we always clear the end */ > if (!__addr_ok(to)) > memset(to, 0, len); in arch/x86/lib/usercopy_64.c:copy_user_handle_tail(), which may be the x86_64 equivalent, or I may be lost in the maze again. Thanks! James
On Mon, Mar 07, 2016 at 05:40:23PM +0000, James Morse wrote: > On 07/03/16 17:23, Russell King - ARM Linux wrote: > > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: > >> As far as I can see, this would only affect arm64. I can't find an equivalent > >> memset() for x86_64. > > > > I don't think you've looked hard enough. :) > > Heh, Thanks! I ignored the 32bit code and instead got lost in the maze of > underscores and alternative-strings for the 64 bit path. > > Having seen that path, I've now found: > > /* If the destination is a kernel buffer, we always clear the end */ > > if (!__addr_ok(to)) > > memset(to, 0, len); > > in arch/x86/lib/usercopy_64.c:copy_user_handle_tail(), which may be the x86_64 > equivalent, or I may be lost in the maze again. x86_64 has the function in assembler: /* Standard copy_from_user with segment limit checking */ ENTRY(_copy_from_user) GET_THREAD_INFO(%rax) movq %rsi,%rcx addq %rdx,%rcx jc bad_from_user cmpq TI_addr_limit(%rax),%rcx ja bad_from_user ALTERNATIVE_2 "jmp copy_user_generic_unrolled", \ "jmp copy_user_generic_string", \ X86_FEATURE_REP_GOOD, \ "jmp copy_user_enhanced_fast_string", \ X86_FEATURE_ERMS ENDPROC(_copy_from_user) .section .fixup,"ax" /* must zero dest */ ENTRY(bad_from_user) bad_from_user: movl %edx,%ecx xorl %eax,%eax rep stosb bad_to_user: movl %edx,%eax ret ENDPROC(bad_from_user) The memset() are the 4 instructions after bad_from_user:. This will be triggered if 'from' is an invalid address. However, you're right that we end up in copy_user_handle_tail() if we fault, which will try to copy as much data and then memset() the remainder. Since this is used for both copy_from_user() and copy_to_user() I think the __addr_ok() is just a side-effect of avoiding memsetting for the copy_to_user() case.
On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > (cc'ing Kees) > > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: >> I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) >> to fail. Who to blame is up for discussion. The test is passing a user pointer >> as the 'to' field of copy_from_user(), which it expects to fail gracefully: >> >> lib/test_user_copy.c:75 >> > /* Invalid usage: none of these should succeed. */ >> [ ... ] >> > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, >> > PAGE_SIZE), >> > "illegal reversed copy_from_user passed"); >> > >> >> access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass >> bad_usermem to memset(): >> >> arch/arm64/include/asm/uaccess.h:279 >> > if (access_ok(VERIFY_READ, from, n)) >> > n = __copy_from_user(to, from, n); >> > else /* security hole - plug it */ >> > memset(to, 0, n); >> >> This (correctly) trips UAO's "Accessing user space memory outside uaccess.h >> routines" message, which is a little confusing to debug, and stops the rest of >> the module's tests from being run. > > I suggest we don't do anything on arch/arm64 (or arch/arm), I just > consider the test to be broken. The semantics of copy_from_user() don't > say anything about the safety checks on the destination pointer, this is > supposed to be a valid kernel address. The test assumes that if the > source pointer is invalid, the copy_from_user() routine should not touch > the destination. Hmmm, I'd prefer that copy_*_user was checking both src and destination. That's what these tests are checking for. > A better test would be to use a destination buffer filled with a poison > value and checked after the operation. Well, I could certainly add that check, but I'd prefer that a bogus destination be rejected. -Kees
On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote: > On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > (cc'ing Kees) > > > > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: > >> I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) > >> to fail. Who to blame is up for discussion. The test is passing a user pointer > >> as the 'to' field of copy_from_user(), which it expects to fail gracefully: > >> > >> lib/test_user_copy.c:75 > >> > /* Invalid usage: none of these should succeed. */ > >> [ ... ] > >> > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, > >> > PAGE_SIZE), > >> > "illegal reversed copy_from_user passed"); > >> > > >> > >> access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass > >> bad_usermem to memset(): > >> > >> arch/arm64/include/asm/uaccess.h:279 > >> > if (access_ok(VERIFY_READ, from, n)) > >> > n = __copy_from_user(to, from, n); > >> > else /* security hole - plug it */ > >> > memset(to, 0, n); > >> > >> This (correctly) trips UAO's "Accessing user space memory outside uaccess.h > >> routines" message, which is a little confusing to debug, and stops the rest of > >> the module's tests from being run. > > > > I suggest we don't do anything on arch/arm64 (or arch/arm), I just > > consider the test to be broken. The semantics of copy_from_user() don't > > say anything about the safety checks on the destination pointer, this is > > supposed to be a valid kernel address. The test assumes that if the > > source pointer is invalid, the copy_from_user() routine should not touch > > the destination. > > Hmmm, I'd prefer that copy_*_user was checking both src and > destination. That's what these tests are checking for. If the kernel pointer (source or destination, depending on the function) is not valid, I would rather panic the kernel (maybe as a result of a data abort) than silently returning a non-zero value from copy_from_user() which doesn't even tell whether the source or destination pointer is wrong. The copy_*_user functions are indeed meant to check the validity of user pointer arguments and safely handle aborts (extable annotations) since these values are usually provided by user space. But the kernel pointer arguments are under the control of the kernel, so IMO passing a corrupt value is a serious kernel/driver bug.
On Tue, Mar 8, 2016 at 9:19 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote: >> On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > (cc'ing Kees) >> > >> > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: >> >> I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) >> >> to fail. Who to blame is up for discussion. The test is passing a user pointer >> >> as the 'to' field of copy_from_user(), which it expects to fail gracefully: >> >> >> >> lib/test_user_copy.c:75 >> >> > /* Invalid usage: none of these should succeed. */ >> >> [ ... ] >> >> > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, >> >> > PAGE_SIZE), >> >> > "illegal reversed copy_from_user passed"); >> >> > >> >> >> >> access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass >> >> bad_usermem to memset(): >> >> >> >> arch/arm64/include/asm/uaccess.h:279 >> >> > if (access_ok(VERIFY_READ, from, n)) >> >> > n = __copy_from_user(to, from, n); >> >> > else /* security hole - plug it */ >> >> > memset(to, 0, n); >> >> >> >> This (correctly) trips UAO's "Accessing user space memory outside uaccess.h >> >> routines" message, which is a little confusing to debug, and stops the rest of >> >> the module's tests from being run. >> > >> > I suggest we don't do anything on arch/arm64 (or arch/arm), I just >> > consider the test to be broken. The semantics of copy_from_user() don't >> > say anything about the safety checks on the destination pointer, this is >> > supposed to be a valid kernel address. The test assumes that if the >> > source pointer is invalid, the copy_from_user() routine should not touch >> > the destination. >> >> Hmmm, I'd prefer that copy_*_user was checking both src and >> destination. That's what these tests are checking for. > > If the kernel pointer (source or destination, depending on the function) > is not valid, I would rather panic the kernel (maybe as a result of a > data abort) than silently returning a non-zero value from > copy_from_user() which doesn't even tell whether the source or > destination pointer is wrong. > > The copy_*_user functions are indeed meant to check the validity of user > pointer arguments and safely handle aborts (extable annotations) since > these values are usually provided by user space. But the kernel pointer > arguments are under the control of the kernel, so IMO passing a corrupt > value is a serious kernel/driver bug. I'm totally fine with that, though I suspect Linus would not like this (perhaps just Oops instead of panic). Regardless, if that was changed, we could move this entire test into lkdtm (where system panics are considered a "success"). -Kees
On Tue, Mar 08, 2016 at 09:39:27AM -0800, Kees Cook wrote: > On Tue, Mar 8, 2016 at 9:19 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote: > >> On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > (cc'ing Kees) > >> > > >> > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: > >> >> I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) > >> >> to fail. Who to blame is up for discussion. The test is passing a user pointer > >> >> as the 'to' field of copy_from_user(), which it expects to fail gracefully: > >> >> > >> >> lib/test_user_copy.c:75 > >> >> > /* Invalid usage: none of these should succeed. */ > >> >> [ ... ] > >> >> > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, > >> >> > PAGE_SIZE), > >> >> > "illegal reversed copy_from_user passed"); > >> >> > > >> >> > >> >> access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass > >> >> bad_usermem to memset(): > >> >> > >> >> arch/arm64/include/asm/uaccess.h:279 > >> >> > if (access_ok(VERIFY_READ, from, n)) > >> >> > n = __copy_from_user(to, from, n); > >> >> > else /* security hole - plug it */ > >> >> > memset(to, 0, n); > >> >> > >> >> This (correctly) trips UAO's "Accessing user space memory outside uaccess.h > >> >> routines" message, which is a little confusing to debug, and stops the rest of > >> >> the module's tests from being run. > >> > > >> > I suggest we don't do anything on arch/arm64 (or arch/arm), I just > >> > consider the test to be broken. The semantics of copy_from_user() don't > >> > say anything about the safety checks on the destination pointer, this is > >> > supposed to be a valid kernel address. The test assumes that if the > >> > source pointer is invalid, the copy_from_user() routine should not touch > >> > the destination. > >> > >> Hmmm, I'd prefer that copy_*_user was checking both src and > >> destination. That's what these tests are checking for. > > > > If the kernel pointer (source or destination, depending on the function) > > is not valid, I would rather panic the kernel (maybe as a result of a > > data abort) than silently returning a non-zero value from > > copy_from_user() which doesn't even tell whether the source or > > destination pointer is wrong. > > > > The copy_*_user functions are indeed meant to check the validity of user > > pointer arguments and safely handle aborts (extable annotations) since > > these values are usually provided by user space. But the kernel pointer > > arguments are under the control of the kernel, so IMO passing a corrupt > > value is a serious kernel/driver bug. > > I'm totally fine with that, though I suspect Linus would not like this > (perhaps just Oops instead of panic). That's what we get on arm64 with PAN enabled (oops). > Regardless, if that was changed, we could move this entire test into > lkdtm (where system panics are considered a "success"). The only downside is that not all architectures behave in the same way w.r.t. the copy_*_user() routines, so the test may have some surprises with the oops not being triggered. Anyway, I think the test should also check the invalid source/destination independently (possibly in combination with set_fs(KERNEL_DS)).
On Tue, Mar 8, 2016 at 10:22 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Tue, Mar 08, 2016 at 09:39:27AM -0800, Kees Cook wrote: >> On Tue, Mar 8, 2016 at 9:19 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Mon, Mar 07, 2016 at 12:54:33PM -0800, Kees Cook wrote: >> >> On Mon, Mar 7, 2016 at 9:38 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> > (cc'ing Kees) >> >> > >> >> > On Mon, Mar 07, 2016 at 04:43:19PM +0000, James Morse wrote: >> >> >> I've just spotted UAO causes the test_user_copy module (CONFIG_TEST_USER_COPY) >> >> >> to fail. Who to blame is up for discussion. The test is passing a user pointer >> >> >> as the 'to' field of copy_from_user(), which it expects to fail gracefully: >> >> >> >> >> >> lib/test_user_copy.c:75 >> >> >> > /* Invalid usage: none of these should succeed. */ >> >> >> [ ... ] >> >> >> > ret |= test(!copy_from_user(bad_usermem, (char __user *)kmem, >> >> >> > PAGE_SIZE), >> >> >> > "illegal reversed copy_from_user passed"); >> >> >> > >> >> >> >> >> >> access_ok() catches the "(char __user *)kmem", causing copy_from_user() to pass >> >> >> bad_usermem to memset(): >> >> >> >> >> >> arch/arm64/include/asm/uaccess.h:279 >> >> >> > if (access_ok(VERIFY_READ, from, n)) >> >> >> > n = __copy_from_user(to, from, n); >> >> >> > else /* security hole - plug it */ >> >> >> > memset(to, 0, n); >> >> >> >> >> >> This (correctly) trips UAO's "Accessing user space memory outside uaccess.h >> >> >> routines" message, which is a little confusing to debug, and stops the rest of >> >> >> the module's tests from being run. >> >> > >> >> > I suggest we don't do anything on arch/arm64 (or arch/arm), I just >> >> > consider the test to be broken. The semantics of copy_from_user() don't >> >> > say anything about the safety checks on the destination pointer, this is >> >> > supposed to be a valid kernel address. The test assumes that if the >> >> > source pointer is invalid, the copy_from_user() routine should not touch >> >> > the destination. >> >> >> >> Hmmm, I'd prefer that copy_*_user was checking both src and >> >> destination. That's what these tests are checking for. >> > >> > If the kernel pointer (source or destination, depending on the function) >> > is not valid, I would rather panic the kernel (maybe as a result of a >> > data abort) than silently returning a non-zero value from >> > copy_from_user() which doesn't even tell whether the source or >> > destination pointer is wrong. >> > >> > The copy_*_user functions are indeed meant to check the validity of user >> > pointer arguments and safely handle aborts (extable annotations) since >> > these values are usually provided by user space. But the kernel pointer >> > arguments are under the control of the kernel, so IMO passing a corrupt >> > value is a serious kernel/driver bug. >> >> I'm totally fine with that, though I suspect Linus would not like this >> (perhaps just Oops instead of panic). > > That's what we get on arm64 with PAN enabled (oops). > >> Regardless, if that was changed, we could move this entire test into >> lkdtm (where system panics are considered a "success"). > > The only downside is that not all architectures behave in the same way > w.r.t. the copy_*_user() routines, so the test may have some surprises > with the oops not being triggered. Right. I think if the tests were moved to lkdtm, we could do the checks in a few stages, where lkdtm would generate the Oops if copy_*_user correctly errored/ignored the bad combo without Oopsing on its own. > Anyway, I think the test should also check the invalid > source/destination independently (possibly in combination with > set_fs(KERNEL_DS)). I'm open to whatever you like. :) -Kees
diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h index 0685d74572af..049a82e8dd9e 100644 --- a/arch/arm64/include/asm/uaccess.h +++ b/arch/arm64/include/asm/uaccess.h @@ -278,8 +278,8 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u { if (access_ok(VERIFY_READ, from, n)) n = __copy_from_user(to, from, n); - else /* security hole - plug it */ - memset(to, 0, n); + else if ((unsigned long)to > USER_DS) /* swapped from/to args? */ + memset(to, 0, n); /* security hole - plug it */ return n; }