Re: [git pull] vfs.git pile 11 - iov_iter/hardening
On Fri, Jul 7, 2017 at 5:29 PM, Al Virowrote: > > Trivial conflicts with libnvdimm; this stuff will get some > followups, but again, that's for another series. Gaah. Yeah, I guess I could have done the trivial ugly merge that just took the new copy_from_iter_flushcache() as-is, and didn't match it up with all the new iov_iter hardening. Except I decided I don't want that, and want to do a proper merge instead. Which made the trivial merge something that actually changed that copy_from_iter_flushcache() logic, and maybe I screwed it up in the process. It builds, and it looks right to me, but you and Dan should really check out the end result. Particularly so for the CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE logic, which is where this differs from the other cases, and which I changed to make the #ifdef less noticeable. I did try both a i386 and a x86-64 build of the iov_iter code, since that should test both of those ARCH_HAS cases, but that was purely a build test. Also, the new __must_check warnings do trigger. At least for lustre. I couldn't be arsed to try to fix those, since .. lustre. But I'm also adding a couple of lustre people to the cc, just to make them aware of it.. I'd really like the allmodconfig build to be clean. Linus
Re: [git pull] vfs.git pile 11 - iov_iter/hardening
On Fri, Jul 7, 2017 at 5:29 PM, Al Viro wrote: > > Trivial conflicts with libnvdimm; this stuff will get some > followups, but again, that's for another series. Gaah. Yeah, I guess I could have done the trivial ugly merge that just took the new copy_from_iter_flushcache() as-is, and didn't match it up with all the new iov_iter hardening. Except I decided I don't want that, and want to do a proper merge instead. Which made the trivial merge something that actually changed that copy_from_iter_flushcache() logic, and maybe I screwed it up in the process. It builds, and it looks right to me, but you and Dan should really check out the end result. Particularly so for the CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE logic, which is where this differs from the other cases, and which I changed to make the #ifdef less noticeable. I did try both a i386 and a x86-64 build of the iov_iter code, since that should test both of those ARCH_HAS cases, but that was purely a build test. Also, the new __must_check warnings do trigger. At least for lustre. I couldn't be arsed to try to fix those, since .. lustre. But I'm also adding a couple of lustre people to the cc, just to make them aware of it.. I'd really like the allmodconfig build to be clean. Linus
[git pull] vfs.git pile 11 - iov_iter/hardening
iov_iter/uaccess/hardening pile. For one thing, it trims the inline part of copy_to_user/copy_from_user to the minimum that *does* need to be inlined - object size checks, basically. For another, it sanitizes the checks for iov_iter primitives. There are 4 groups of checks: access_ok(), might_fault(), object size and KASAN. * access_ok() had been verified by whoever had set the iov_iter up. However, that has happened in a function far away, so proving that there's no path to actual copying bypassing those checks is hard and proving that iov_iter has not been buggered in the meanwhile is also not pleasant. So we want those redone in actual copyin/copyout. * might_fault() is better off consolidated - we know whether it needs to be checked as soon as we enter iov_iter primitive and observe the iov_iter flavour. No need to wait until the copyin/copyout. The call chains are short enough to make sure we won't miss anything - in fact, it's more robust that way, since there are cases where we do e.g. forced fault-in before getting to copyin/copyout. It's not quite what we need to check (in particular, combination of iovec-backed and set_fs(KERNEL_DS) is almost certainly a bug, not a cause to skip checks), but that's for later series. For now let's keep might_fault(). * KASAN checks belong in copyin/copyout - at the same level where other iov_iter flavours would've hit them in memcpy(). * object size checks should apply to *all* iov_iter flavours, not just iovec-backed ones. There are two groups of primitives - one gets the kernel object described as pointer + size (copy_to_iter(), etc.) while another gets it as page + offset + size (copy_page_to_iter(), etc.) For the first group the checks are best done where we actually have a chance to find the object size. In other words, those belong in inline wrappers in uio.h, before calling into iov_iter.c. Same kind as we have for inlined part of copy_to_user(). For the second group there is no object to look at - offset in page is just a number, it bears no type information. So we do them in the common helper called by iov_iter.c primitives of that kind. All it currently does is checking that we are not trying to access outside of the compound page; eventually we might want to add some sanity checks on the page involved. So the things we need in copyin/copyout part of iov_iter.c do not quite match anything in uaccess.h (we want no zeroing, we *do* want access_ok() and KASAN and we want no might_fault() or object size checks done on that level). OTOH, these needs are simple enough to provide a couple of helpers (static in iov_iter.c) doing just what we need... Trivial conflicts with libnvdimm; this stuff will get some followups, but again, that's for another series. The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess-work.iov_iter for you to fetch changes up to 09fc68dc66f7597bdc8898c991609a48f061bed5: iov_iter: saner checks on copyin/copyout (2017-07-07 05:18:09 -0400) Al Viro (5): copy_{from,to}_user(): move kasan checks and might_fault() out-of-line copy_{to,from}_user(): consolidate object size checks iov_iter/hardening: move object size checks to inlined part iov_iter: sanity checks for copy to/from page primitives iov_iter: saner checks on copyin/copyout include/linux/thread_info.h | 27 + include/linux/uaccess.h | 44 + include/linux/uio.h | 58 +--- lib/iov_iter.c | 94 - lib/usercopy.c | 10 - 5 files changed, 165 insertions(+), 68 deletions(-)
[git pull] vfs.git pile 11 - iov_iter/hardening
iov_iter/uaccess/hardening pile. For one thing, it trims the inline part of copy_to_user/copy_from_user to the minimum that *does* need to be inlined - object size checks, basically. For another, it sanitizes the checks for iov_iter primitives. There are 4 groups of checks: access_ok(), might_fault(), object size and KASAN. * access_ok() had been verified by whoever had set the iov_iter up. However, that has happened in a function far away, so proving that there's no path to actual copying bypassing those checks is hard and proving that iov_iter has not been buggered in the meanwhile is also not pleasant. So we want those redone in actual copyin/copyout. * might_fault() is better off consolidated - we know whether it needs to be checked as soon as we enter iov_iter primitive and observe the iov_iter flavour. No need to wait until the copyin/copyout. The call chains are short enough to make sure we won't miss anything - in fact, it's more robust that way, since there are cases where we do e.g. forced fault-in before getting to copyin/copyout. It's not quite what we need to check (in particular, combination of iovec-backed and set_fs(KERNEL_DS) is almost certainly a bug, not a cause to skip checks), but that's for later series. For now let's keep might_fault(). * KASAN checks belong in copyin/copyout - at the same level where other iov_iter flavours would've hit them in memcpy(). * object size checks should apply to *all* iov_iter flavours, not just iovec-backed ones. There are two groups of primitives - one gets the kernel object described as pointer + size (copy_to_iter(), etc.) while another gets it as page + offset + size (copy_page_to_iter(), etc.) For the first group the checks are best done where we actually have a chance to find the object size. In other words, those belong in inline wrappers in uio.h, before calling into iov_iter.c. Same kind as we have for inlined part of copy_to_user(). For the second group there is no object to look at - offset in page is just a number, it bears no type information. So we do them in the common helper called by iov_iter.c primitives of that kind. All it currently does is checking that we are not trying to access outside of the compound page; eventually we might want to add some sanity checks on the page involved. So the things we need in copyin/copyout part of iov_iter.c do not quite match anything in uaccess.h (we want no zeroing, we *do* want access_ok() and KASAN and we want no might_fault() or object size checks done on that level). OTOH, these needs are simple enough to provide a couple of helpers (static in iov_iter.c) doing just what we need... Trivial conflicts with libnvdimm; this stuff will get some followups, but again, that's for another series. The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess-work.iov_iter for you to fetch changes up to 09fc68dc66f7597bdc8898c991609a48f061bed5: iov_iter: saner checks on copyin/copyout (2017-07-07 05:18:09 -0400) Al Viro (5): copy_{from,to}_user(): move kasan checks and might_fault() out-of-line copy_{to,from}_user(): consolidate object size checks iov_iter/hardening: move object size checks to inlined part iov_iter: sanity checks for copy to/from page primitives iov_iter: saner checks on copyin/copyout include/linux/thread_info.h | 27 + include/linux/uaccess.h | 44 + include/linux/uio.h | 58 +--- lib/iov_iter.c | 94 - lib/usercopy.c | 10 - 5 files changed, 165 insertions(+), 68 deletions(-)
Re: [git pull] vfs.git pile 11
On Thu, Jul 6, 2017 at 2:20 PM, Al Virowrote: > > Linus, could you hold that one back until tomorrow? I want to tweak the > last commit in there a bit, but I want to give it a local beating first... Ok, dropping this one. All your other branches are merged now. Linus
Re: [git pull] vfs.git pile 11
On Thu, Jul 6, 2017 at 2:20 PM, Al Viro wrote: > > Linus, could you hold that one back until tomorrow? I want to tweak the > last commit in there a bit, but I want to give it a local beating first... Ok, dropping this one. All your other branches are merged now. Linus
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 09:29:27PM +0100, Al Viro wrote: > On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote: > > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > > > > > + if (unlikely(!check_copy_size(addr, bytes, false))) > > > + return false; > > > + else > > > + return _copy_from_iter_full(addr, bytes, i); > > > > > > Can these be rewritten to avoid the double-negative? > > > > Matter of taste - I've no strong preferences here. > > > > > + might_fault(); > > > > > > Should this be might_sleep()? Just from reading the patch it looked > > > like you were adding might_sleep()s in the other cases. > > > > D'oh - shouldn't have written that pull request message before the > > first cup of coffee... might_sleep() it is, of course. > > Hrm... Said that, might_sleep() doesn't check one thing might_fault() > does - the > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) > if (current->mm) > might_lock_read(>mm->mmap_sem); > #endif > thing. Let me think a bit... FWIW, * with iovec-backed, any of those primitives under pagefault_disable() is seriously wrong. To the point where we probably want to complain when called that way. That, of course, needs to be checked at the outermost level - the primitives might do pagefault_disable() internally; that's fine. Outside caller doing that under pagefault_disable() isn't. * uaccess_kernel() (== set_fs(KERNEL_DS)) has nothing to do with it. If anything, we should not do copyin/copyout on iovec-backed ones with that present. * telling lockdep that we might end up grabbing ->mm->mmap_sem, OTOH, is the right thing to do. In addition to might_sleep(). Linus, could you hold that one back until tomorrow? I want to tweak the last commit in there a bit, but I want to give it a local beating first...
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 09:29:27PM +0100, Al Viro wrote: > On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote: > > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > > > > > + if (unlikely(!check_copy_size(addr, bytes, false))) > > > + return false; > > > + else > > > + return _copy_from_iter_full(addr, bytes, i); > > > > > > Can these be rewritten to avoid the double-negative? > > > > Matter of taste - I've no strong preferences here. > > > > > + might_fault(); > > > > > > Should this be might_sleep()? Just from reading the patch it looked > > > like you were adding might_sleep()s in the other cases. > > > > D'oh - shouldn't have written that pull request message before the > > first cup of coffee... might_sleep() it is, of course. > > Hrm... Said that, might_sleep() doesn't check one thing might_fault() > does - the > #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) > if (current->mm) > might_lock_read(>mm->mmap_sem); > #endif > thing. Let me think a bit... FWIW, * with iovec-backed, any of those primitives under pagefault_disable() is seriously wrong. To the point where we probably want to complain when called that way. That, of course, needs to be checked at the outermost level - the primitives might do pagefault_disable() internally; that's fine. Outside caller doing that under pagefault_disable() isn't. * uaccess_kernel() (== set_fs(KERNEL_DS)) has nothing to do with it. If anything, we should not do copyin/copyout on iovec-backed ones with that present. * telling lockdep that we might end up grabbing ->mm->mmap_sem, OTOH, is the right thing to do. In addition to might_sleep(). Linus, could you hold that one back until tomorrow? I want to tweak the last commit in there a bit, but I want to give it a local beating first...
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote: > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > > > + if (unlikely(!check_copy_size(addr, bytes, false))) > > + return false; > > + else > > + return _copy_from_iter_full(addr, bytes, i); > > > > Can these be rewritten to avoid the double-negative? > > Matter of taste - I've no strong preferences here. > > > + might_fault(); > > > > Should this be might_sleep()? Just from reading the patch it looked > > like you were adding might_sleep()s in the other cases. > > D'oh - shouldn't have written that pull request message before the > first cup of coffee... might_sleep() it is, of course. Hrm... Said that, might_sleep() doesn't check one thing might_fault() does - the #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) if (current->mm) might_lock_read(>mm->mmap_sem); #endif thing. Let me think a bit...
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 09:18:26PM +0100, Al Viro wrote: > On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > > > + if (unlikely(!check_copy_size(addr, bytes, false))) > > + return false; > > + else > > + return _copy_from_iter_full(addr, bytes, i); > > > > Can these be rewritten to avoid the double-negative? > > Matter of taste - I've no strong preferences here. > > > + might_fault(); > > > > Should this be might_sleep()? Just from reading the patch it looked > > like you were adding might_sleep()s in the other cases. > > D'oh - shouldn't have written that pull request message before the > first cup of coffee... might_sleep() it is, of course. Hrm... Said that, might_sleep() doesn't check one thing might_fault() does - the #if defined(CONFIG_DEBUG_ATOMIC_SLEEP) if (current->mm) might_lock_read(>mm->mmap_sem); #endif thing. Let me think a bit...
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > + if (unlikely(!check_copy_size(addr, bytes, false))) > + return false; > + else > + return _copy_from_iter_full(addr, bytes, i); > > Can these be rewritten to avoid the double-negative? Matter of taste - I've no strong preferences here. > + might_fault(); > > Should this be might_sleep()? Just from reading the patch it looked > like you were adding might_sleep()s in the other cases. D'oh - shouldn't have written that pull request message before the first cup of coffee... might_sleep() it is, of course.
Re: [git pull] vfs.git pile 11
On Thu, Jul 06, 2017 at 12:45:36PM -0700, Kees Cook wrote: > > + if (unlikely(!check_copy_size(addr, bytes, false))) > + return false; > + else > + return _copy_from_iter_full(addr, bytes, i); > > Can these be rewritten to avoid the double-negative? Matter of taste - I've no strong preferences here. > + might_fault(); > > Should this be might_sleep()? Just from reading the patch it looked > like you were adding might_sleep()s in the other cases. D'oh - shouldn't have written that pull request message before the first cup of coffee... might_sleep() it is, of course.
Re: [git pull] vfs.git pile 11
On Thu, Jul 6, 2017 at 2:12 AM, Al Virowrote: > iov_iter/uaccess/hardening pile. For one thing, it trims the > inline part of copy_to_user/copy_from_user to the minimum that *does* > need to be inlined - object size checks, basically. For another, > it sanitizes the checks for iov_iter primitives. There are 4 groups > of checks: access_ok(), might_fault(), object size and KASAN. > * access_ok() had been verified by whoever had set the iov_iter > up. However, that has happened in a function far away, so proving that > there's no path to actual copying bypassing those checks is hard and > proving that iov_iter has not been buggered in the meanwhile is also > not pleasant. So we want those redone in actual copyin/copyout. > * might_fault() is better off consolidated - we know whether > it needs to be checked as soon as we enter iov_iter primitive and > observe the iov_iter flavour. No need to wait until the copyin/copyout. > The call chains are short enough to make sure we won't miss anything - > in fact, it's more robust that way, since there are cases where we do > e.g. forced fault-in before getting to copyin/copyout. > * KASAN checks belong in copyin/copyout - at the same level > where other iov_iter flavours would've hit them in memcpy(). > * object size checks should apply to *all* iov_iter flavours, > not just iovec-backed ones. > There are two groups of primitives - one gets the kernel object > described as pointer + size (copy_to_iter(), etc.) while another gets > it as page + offset + size (copy_page_to_iter(), etc.) > For the first group the checks are best done where we actually > have a chance to find the object size. In other words, those belong in > inline wrappers in uio.h, before calling into iov_iter.c. Same kind > as we have for inlined part of copy_to_user(). > For the second group there is no object to look at - offset in > page is just a number, it bears no type information. So we do them > in the common helper called by iov_iter.c primitives of that kind. > All it currently does is checking that we are not trying to access > outside of the compound page; eventually we might want to add some > sanity checks on the page involved. > > So the things we need in copyin/copyout part of iov_iter.c > do not quite match anything in uaccess.h (we want no zeroing, we *do* > want access_ok() and KASAN and we want no might_fault() or object size > checks done on that level). OTOH, these needs are simple enough to > provide a couple of helpers (static in iov_iter.c) doing just what > we need... > > The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: > > Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git > uaccess-work.iov_iter > > for you to fetch changes up to ea93a426af164d346a0b4fe0836143bf32177330: > > iov_iter: saner checks on copyin/copyout (2017-06-29 22:29:36 -0400) > > > Al Viro (5): > copy_{from,to}_user(): move kasan checks and might_fault() out-of-line > copy_{to,from}_user(): consolidate object size checks We still need to fix the missed-zeroing-on-overflow corner-case: https://patchwork.kernel.org/patch/9826959/ > iov_iter/hardening: move object size checks to inlined part + if (unlikely(!check_copy_size(addr, bytes, false))) + return false; + else + return _copy_from_iter_full(addr, bytes, i); Can these be rewritten to avoid the double-negative? > iov_iter: sanity checks for copy to/from page primitives Nice to see these! > iov_iter: saner checks on copyin/copyout + might_fault(); Should this be might_sleep()? Just from reading the patch it looked like you were adding might_sleep()s in the other cases. > > include/linux/thread_info.h | 27 + > include/linux/uaccess.h | 44 + > include/linux/uio.h | 58 --- > lib/iov_iter.c | 96 > - > lib/usercopy.c | 10 - > 5 files changed, 167 insertions(+), 68 deletions(-) -Kees -- Kees Cook Pixel Security
Re: [git pull] vfs.git pile 11
On Thu, Jul 6, 2017 at 2:12 AM, Al Viro wrote: > iov_iter/uaccess/hardening pile. For one thing, it trims the > inline part of copy_to_user/copy_from_user to the minimum that *does* > need to be inlined - object size checks, basically. For another, > it sanitizes the checks for iov_iter primitives. There are 4 groups > of checks: access_ok(), might_fault(), object size and KASAN. > * access_ok() had been verified by whoever had set the iov_iter > up. However, that has happened in a function far away, so proving that > there's no path to actual copying bypassing those checks is hard and > proving that iov_iter has not been buggered in the meanwhile is also > not pleasant. So we want those redone in actual copyin/copyout. > * might_fault() is better off consolidated - we know whether > it needs to be checked as soon as we enter iov_iter primitive and > observe the iov_iter flavour. No need to wait until the copyin/copyout. > The call chains are short enough to make sure we won't miss anything - > in fact, it's more robust that way, since there are cases where we do > e.g. forced fault-in before getting to copyin/copyout. > * KASAN checks belong in copyin/copyout - at the same level > where other iov_iter flavours would've hit them in memcpy(). > * object size checks should apply to *all* iov_iter flavours, > not just iovec-backed ones. > There are two groups of primitives - one gets the kernel object > described as pointer + size (copy_to_iter(), etc.) while another gets > it as page + offset + size (copy_page_to_iter(), etc.) > For the first group the checks are best done where we actually > have a chance to find the object size. In other words, those belong in > inline wrappers in uio.h, before calling into iov_iter.c. Same kind > as we have for inlined part of copy_to_user(). > For the second group there is no object to look at - offset in > page is just a number, it bears no type information. So we do them > in the common helper called by iov_iter.c primitives of that kind. > All it currently does is checking that we are not trying to access > outside of the compound page; eventually we might want to add some > sanity checks on the page involved. > > So the things we need in copyin/copyout part of iov_iter.c > do not quite match anything in uaccess.h (we want no zeroing, we *do* > want access_ok() and KASAN and we want no might_fault() or object size > checks done on that level). OTOH, these needs are simple enough to > provide a couple of helpers (static in iov_iter.c) doing just what > we need... > > The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: > > Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) > > are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git > uaccess-work.iov_iter > > for you to fetch changes up to ea93a426af164d346a0b4fe0836143bf32177330: > > iov_iter: saner checks on copyin/copyout (2017-06-29 22:29:36 -0400) > > > Al Viro (5): > copy_{from,to}_user(): move kasan checks and might_fault() out-of-line > copy_{to,from}_user(): consolidate object size checks We still need to fix the missed-zeroing-on-overflow corner-case: https://patchwork.kernel.org/patch/9826959/ > iov_iter/hardening: move object size checks to inlined part + if (unlikely(!check_copy_size(addr, bytes, false))) + return false; + else + return _copy_from_iter_full(addr, bytes, i); Can these be rewritten to avoid the double-negative? > iov_iter: sanity checks for copy to/from page primitives Nice to see these! > iov_iter: saner checks on copyin/copyout + might_fault(); Should this be might_sleep()? Just from reading the patch it looked like you were adding might_sleep()s in the other cases. > > include/linux/thread_info.h | 27 + > include/linux/uaccess.h | 44 + > include/linux/uio.h | 58 --- > lib/iov_iter.c | 96 > - > lib/usercopy.c | 10 - > 5 files changed, 167 insertions(+), 68 deletions(-) -Kees -- Kees Cook Pixel Security
[git pull] vfs.git pile 11
iov_iter/uaccess/hardening pile. For one thing, it trims the inline part of copy_to_user/copy_from_user to the minimum that *does* need to be inlined - object size checks, basically. For another, it sanitizes the checks for iov_iter primitives. There are 4 groups of checks: access_ok(), might_fault(), object size and KASAN. * access_ok() had been verified by whoever had set the iov_iter up. However, that has happened in a function far away, so proving that there's no path to actual copying bypassing those checks is hard and proving that iov_iter has not been buggered in the meanwhile is also not pleasant. So we want those redone in actual copyin/copyout. * might_fault() is better off consolidated - we know whether it needs to be checked as soon as we enter iov_iter primitive and observe the iov_iter flavour. No need to wait until the copyin/copyout. The call chains are short enough to make sure we won't miss anything - in fact, it's more robust that way, since there are cases where we do e.g. forced fault-in before getting to copyin/copyout. * KASAN checks belong in copyin/copyout - at the same level where other iov_iter flavours would've hit them in memcpy(). * object size checks should apply to *all* iov_iter flavours, not just iovec-backed ones. There are two groups of primitives - one gets the kernel object described as pointer + size (copy_to_iter(), etc.) while another gets it as page + offset + size (copy_page_to_iter(), etc.) For the first group the checks are best done where we actually have a chance to find the object size. In other words, those belong in inline wrappers in uio.h, before calling into iov_iter.c. Same kind as we have for inlined part of copy_to_user(). For the second group there is no object to look at - offset in page is just a number, it bears no type information. So we do them in the common helper called by iov_iter.c primitives of that kind. All it currently does is checking that we are not trying to access outside of the compound page; eventually we might want to add some sanity checks on the page involved. So the things we need in copyin/copyout part of iov_iter.c do not quite match anything in uaccess.h (we want no zeroing, we *do* want access_ok() and KASAN and we want no might_fault() or object size checks done on that level). OTOH, these needs are simple enough to provide a couple of helpers (static in iov_iter.c) doing just what we need... The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess-work.iov_iter for you to fetch changes up to ea93a426af164d346a0b4fe0836143bf32177330: iov_iter: saner checks on copyin/copyout (2017-06-29 22:29:36 -0400) Al Viro (5): copy_{from,to}_user(): move kasan checks and might_fault() out-of-line copy_{to,from}_user(): consolidate object size checks iov_iter/hardening: move object size checks to inlined part iov_iter: sanity checks for copy to/from page primitives iov_iter: saner checks on copyin/copyout include/linux/thread_info.h | 27 + include/linux/uaccess.h | 44 + include/linux/uio.h | 58 --- lib/iov_iter.c | 96 - lib/usercopy.c | 10 - 5 files changed, 167 insertions(+), 68 deletions(-)
[git pull] vfs.git pile 11
iov_iter/uaccess/hardening pile. For one thing, it trims the inline part of copy_to_user/copy_from_user to the minimum that *does* need to be inlined - object size checks, basically. For another, it sanitizes the checks for iov_iter primitives. There are 4 groups of checks: access_ok(), might_fault(), object size and KASAN. * access_ok() had been verified by whoever had set the iov_iter up. However, that has happened in a function far away, so proving that there's no path to actual copying bypassing those checks is hard and proving that iov_iter has not been buggered in the meanwhile is also not pleasant. So we want those redone in actual copyin/copyout. * might_fault() is better off consolidated - we know whether it needs to be checked as soon as we enter iov_iter primitive and observe the iov_iter flavour. No need to wait until the copyin/copyout. The call chains are short enough to make sure we won't miss anything - in fact, it's more robust that way, since there are cases where we do e.g. forced fault-in before getting to copyin/copyout. * KASAN checks belong in copyin/copyout - at the same level where other iov_iter flavours would've hit them in memcpy(). * object size checks should apply to *all* iov_iter flavours, not just iovec-backed ones. There are two groups of primitives - one gets the kernel object described as pointer + size (copy_to_iter(), etc.) while another gets it as page + offset + size (copy_page_to_iter(), etc.) For the first group the checks are best done where we actually have a chance to find the object size. In other words, those belong in inline wrappers in uio.h, before calling into iov_iter.c. Same kind as we have for inlined part of copy_to_user(). For the second group there is no object to look at - offset in page is just a number, it bears no type information. So we do them in the common helper called by iov_iter.c primitives of that kind. All it currently does is checking that we are not trying to access outside of the compound page; eventually we might want to add some sanity checks on the page involved. So the things we need in copyin/copyout part of iov_iter.c do not quite match anything in uaccess.h (we want no zeroing, we *do* want access_ok() and KASAN and we want no might_fault() or object size checks done on that level). OTOH, these needs are simple enough to provide a couple of helpers (static in iov_iter.c) doing just what we need... The following changes since commit 2ea659a9ef488125eb46da6eb571de5eae5c43f6: Linux 4.12-rc1 (2017-05-13 13:19:49 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git uaccess-work.iov_iter for you to fetch changes up to ea93a426af164d346a0b4fe0836143bf32177330: iov_iter: saner checks on copyin/copyout (2017-06-29 22:29:36 -0400) Al Viro (5): copy_{from,to}_user(): move kasan checks and might_fault() out-of-line copy_{to,from}_user(): consolidate object size checks iov_iter/hardening: move object size checks to inlined part iov_iter: sanity checks for copy to/from page primitives iov_iter: saner checks on copyin/copyout include/linux/thread_info.h | 27 + include/linux/uaccess.h | 44 + include/linux/uio.h | 58 --- lib/iov_iter.c | 96 - lib/usercopy.c | 10 - 5 files changed, 167 insertions(+), 68 deletions(-)